rpgtkoolmv / corescript

http://www.rpgmakerweb.com/products/programs/rpg-maker-mv
MIT License
311 stars 74 forks source link

fix doesn't work on iOS8 Safari #31

Closed krmbn0576 closed 7 years ago

krmbn0576 commented 7 years ago

In iOS8 Safari, performance isn't defined so TypeError may always be happened. I changed SceneManager._getTimeInMs simply returns undefined because it's no used in MobileSafari.

ghost commented 7 years ago

IMO It's not robust code. (ex. only iOS8 have no performance.now() ? )

It works, but You had better polyfill it.

krmbn0576 commented 7 years ago

Of course it would be better to implement polyfill because SceneManager._getTimeInMs should have a responsibility for returning time on any device. But as a matter of fact, performance isn't defined only on iOS8 (See Can I use http://caniuse.com/#feat=high-resolution-time) and it's not used effectively on MobileSafari, so polyfill is overkill.

ghost commented 7 years ago

it's not used effectively on MobileSafari

No matter how to be used, Your Implementation breaks _getTimeInMs's semantics.

And Intention is not fully written. To understand the code, the reader must know all of browser's implementation.

Check existence of performance. It's good way to detect browser's feature.

krmbn0576 commented 7 years ago

I still have a few suggestions because it is strange to write a polyfill that is not used at all at the moment.

If neither is accepted, I'll give up and write polyfill. 😇

ghost commented 7 years ago

Simply, why not write ponyfill(not polyfill)? Like this.

if(window.performance) return performance.now();
else Date.now() - this._startTime;

Thinking about your suggestion, Not to modify _getTimeInMs. You'd better guard SceneManager: 31.

I think your suggetion means 'It's caller's prolbem'. It's clear intention, more preferable.

krmbn0576 commented 7 years ago

OK, I finally rename _getTimeInMs to _getTimeInMsWithoutMobileSafari, and guard SceneManager: 31.