Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extending the prototype #8074

Open
dhoko opened this issue Jun 9, 2015 · 6 comments
Open

Extending the prototype #8074

dhoko opened this issue Jun 9, 2015 · 6 comments
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.

Comments

@dhoko
Copy link

dhoko commented Jun 9, 2015

Hi,

Inside Piwik.js there is Date.prototype.getTimeAlias = Date.prototype.getTime; (L 5381) why ?

We can only find one call to this alias (L641):

       if (expireDateTime) {
                // the things we do for backwards compatibility...
                // in ECMA-262 5th ed., we could simply use:
                //     while (Date.now() < expireDateTime) { }
                do {
                    now = new Date();
                } while (now.getTimeAlias() < expireDateTime);
            }

Can we remove it and use getTime and keep the prototype clean ?

Edit:

We can find getTime :)

setHeartBeatTimer: function (minimumVisitLength, heartBeatDelay) {
    var now = new Date();

    configMinimumVisitTime = now.getTime() + minimumVisitLength * 1000;
    configHeartBeatTimer = heartBeatDelay * 1000;
},

lastActivityTime = now.getTime();
setTimeout(function heartBeat() {
    var requestPing;
    now = new Date();

    // there was activity during the heart beat period;
    // on average, this is going to overstate the visitDuration by configHeartBeatTimer/2
    if ((lastActivityTime + configHeartBeatTimer) > now.getTime()) {
        // send ping if minimum visit time has elapsed
        if (configMinimumVisitTime < now.getTime()) {
            requestPing = getRequest('ping=1', customData, 'ping');

            sendRequest(requestPing, configTrackerPause);
        }

 if (String(orderId).length) {
     request += '&ec_id=' + encodeWrapper(orderId);
     // Record date of order in the visitor cookie
     lastEcommerceOrderTs = Math.round(now.getTime() / 1000);
 }

...

@tsteur
Copy link
Member

tsteur commented Jun 9, 2015

The blame says: "fixes #2929 - workaround Chrome suckage"

From http://forums.thedailywtf.com/forums/p/24374/253916.aspx#253916

I fiddled around a bit, it's really quite targeted. A while() loop alone doesn't trigger the behavior, nor does calling getTime() alone. But if you call getTime() in the condition or body of a while() loop, bam.

Can we remove it and use getTime and keep the prototype clean ?

Does it cause any problems for you or just in general?

@mattab
Copy link
Member

mattab commented Jun 10, 2015

@diosmosis is working on #2041 which is related in the sense, that maybe the Ping/heatbeat timer code will be cleaned up (ie. removed!) from the piwik.js. if this code can be completely removed then maybe it would be best way to solve the issue

@dhoko
Copy link
Author

dhoko commented Jun 10, 2015

Hum, it works as expected today with Chrome, (webkit too).

var s = e = new Date(); 

for( var i = 0; e.getTime() - s.getTime() < 500; i++) 
{ 
    e = new Date(); 
}
Wed Jun 10 2015 08:33:49 GMT+0200 (CEST)
var s = e = new Date(); 

while (e.getTime() - s.getTime() < 1) 
{ 
    e = new Date(); 
}
Wed Jun 10 2015 08:34:00 GMT+0200 (CEST)
var s = e = new Date(); 

while (e.getTime() - s.getTime() < 500) 
{ 
    e = new Date(); 
}
Wed Jun 10 2015 08:34:08 GMT+0200 (CEST)

@tsteur The promblem ? Date.prototype.getTimeAlias...
@mattab ok

@tsteur
Copy link
Member

tsteur commented Jun 10, 2015

We'd need to figure out which Chrome versions are effected by that but since Chrome updates automatically does versions shouldn't be used anymore. I reckon we could also do something like

function getCurrentTime() {
    var now = new Date();
    now.getTimeAlias = now.getTime;
   return now.getTimeAlias();
}

do {
   // could be also done here instead of in getCurrentTime
} while (getCurrentTime() < expireDateTime);

@mattab
Copy link
Member

mattab commented Jul 15, 2015

Feel free to submit a pull request 👍

@mattab mattab added this to the Mid term milestone Jul 15, 2015
@mattab mattab added the Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. label Jul 15, 2015
@dhoko
Copy link
Author

dhoko commented Aug 25, 2015

Maybe we can follow the comment:

if (expireDateTime) {
    // the things we do for backwards compatibility...
    // in ECMA-262 5th ed., we could simply use:
    //     while (Date.now() < expireDateTime) { }
    do {
        now = new Date();
    } while (now.getTimeAlias() < expireDateTime);
}

And use Date.now cf Date.now compat table with a custom polyfill for IE8 ? ( Might not be the best idea if you need to support IE8 ).

@mattab mattab modified the milestones: Long term, Mid term Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

3 participants