Move push filtering into Data.

Request pushes first, and then the results for the received pushes. Resurrection of bug 611436, with this comment addressed: (In reply to bug 611436 comment #1) > In that case, we don’t know what the time difference between earliestPushDate > and newestPushDate will be, which may lead to gaps in the loaded tb data

Comment on attachment 498104 [details] [diff] [review] part 1, v1 As said in the other bug: I don’t think we need sorting any more.

Comment on attachment 498106 [details] [diff] [review] part 2, v1 >+ self._loadTinderboxDataForPushes(updatedPushes, loadTracker, updatedPushCallback); >+ tinderboxLoadEstimation.cancel(); So inside the loadtinderbox we add new loads and then remove the potentially added ones (why 2 as a default?) As long as this process happens fast enough the update doesn’t arrive in the finally rendered page (jumping numbers) so I guess this is fine. >+ var maxResultDelayAfterPush = 6 * 60 * 60 * 1000; // 6 hours >+ var latestPushDate = pushes[pushes.length - 1].date; >+ var requestEnd = new Date(Math.min(latestPushDate.getTime() + maxResultDelayAfterPush, now)); For now this is fine because we go back in time in constant steps. I guess that will change, which means we will only look 6 hours in the future for the results. I’m not sure this is enough if there are larger time gaps in the pushes.

And yes, sorting the pushes all makes sense now :)

(In reply to comment #4) > Comment on attachment 498106 [details] [diff] [review] > part 2, v1 > > >+ self._loadTinderboxDataForPushes(updatedPushes, loadTracker, updatedPushCallback); > >+ tinderboxLoadEstimation.cancel(); > > So inside the loadtinderbox we add new loads and then remove the potentially > added ones (why 2 as a default?) I was already thinking in terms of push count pagination, where we really have no idea at all how many Tinderbox loads we'll have to do... With the current 12-hour pagination, 1 would probably make more sense. > As long as this process happens fast enough the update doesn’t arrive in the > finally rendered page (jumping numbers) so I guess this is fine. Most browsers guarantee that no page paints happen during JavaScript execution (as long as you don't reenter an event loop, which we don't). > >+ var maxResultDelayAfterPush = 6 * 60 * 60 * 1000; // 6 hours > >+ var latestPushDate = pushes[pushes.length - 1].date; > >+ var requestEnd = new Date(Math.min(latestPushDate.getTime() + maxResultDelayAfterPush, now)); > > For now this is fine because we go back in time in constant steps. I guess that > will change, which means we will only look 6 hours in the future for the > results. I’m not sure this is enough if there are larger time gaps in the > pushes. Hmm... 12 hours, then?

Move away from timeOffset towards push ranges. Here's the model: 1. Range parameters - At the beginning, we have one of these range parameters: - no params: use pushlog default, which is the 10 most recent pushes - "startdate" & "enddate": time range - "fromchange" & "tochange": revision range - "rev": single push - They are piped through to pushlog. - As soon as we get a result from pushlog, our perspective changes, and the visible range is transformed into the form (toPushID, numPushes). - From here on, history requests to pushlog will be made using the (startID, endID) form. 2. Requesting data - We only ever request pushes from pushlog that we don't know about yet. - Pushes are requested: 1. At the beginning 2. Whenever the history arrow is clicked 3. During periodic refreshes if we are in trackingTip mode - trackingTip mode is only active if the initial request was made without any parameters, i.e. the default 10 most recent pushes.

(In reply to comment #6) > > For now this is fine because we go back in time in constant steps. I guess that > > will change, which means we will only look 6 hours in the future for the > > results. I’m not sure this is enough if there are larger time gaps in the > > pushes. > > Hmm... 12 hours, then? Maybe even more, I’m not sure. We also need a mechanism to not load data that is already loaded. So not only cap requestEnd to "now" but also to the oldest push in case we load history.

Comment on attachment 498121 [details] [diff] [review] part 3, v1 Just that I understand pushlog correctly: startID is the lower bound and endID the upper (as in most recent) one? >- if ("tree" in params) { >- if (!Config.repoNames[params.tree]) >- throw "wrongtree"; // er, hm. >- this.treeName = params.tree; >- } >- >- var pusher = null; >- if ("pusher" in params) { >- pusher = params.pusher; >- } >+ this.treeName = (("tree" in params) && params.tree) || "Firefox"; >+ var pusher = ("pusher" in params) && params.pusher; Nice! >+ extendPushRange: function Controller_extendPushRange(num) { >+ var currentRange = this._requestedRange || this._data.getLoadedPushRange(); >+ if (!currentRange) >+ return; >+ >+ var requestedRange; >+ if (num >= 0) { >+ requestedRange = { >+ toPushID: currentRange.toPushID, >+ numPushes: currentRange.numPushes + num, >+ }; >+ } else { >+ requestedRange = { >+ toPushID: currentRange.toPushID + num, >+ numPushes: currentRange.numPushes + Math.abs(num), >+ }; >+ } As far as I read the code, there is no negatve num (yet?) Also: >+ var currentRange = this._requestedRange || this._data.getLoadedPushRange(); […] >+ requestedRange = { >+ toPushID: currentRange.toPushID, >+ numPushes: currentRange.numPushes + num, >+ }; […] >+ this._requestedRange = requestedRange; Am I missing something or is toPushID staying the same and numPushes increasing all the time? Shouldn’t numPushes just be num, as in Config.goBackPushes? Otherwise the naming of the var is wrong. Also, toPushID is the lower or upper bound? It is used as both in getPushlogParamsForRange. Please use better naming and make it consistent. >+ getLoadedPushRange: function Data_getLoadedPushRange() { >+ if (!this._mostRecentPush) >+ return null; >+ return { >+ toPushID: this._mostRecentPush.id, >+ numPushes: Controller.valuesFromObject(this._pushes).length >+ }; >+ }, So numPushes means the number of pushes we already have? Why would we need that? >+ >+ loadPushRange: function Data_loadPushRange(pushRange, loadPendingRunning, loadTracker, updatedPushCallback, infraStatsCallback, initialPushlogLoadCallback) { > var self = this; >- var tinderboxLoadEstimation = loadTracker.addMorePotentialLoads(2); >+ var pushlogRequestParams = this._getPushlogParamsForRange(pushRange, false); That function does not take a second parameter. >+ // 1. Above loaded range: >+ if (pushRange.toPushID > this._mostRecentPush.id) { >+ params.push({ startID: this._mostRecentPush.id, endID: pushRange.toPushID }); >+ } >+ // 2. Below loaded range: >+ if ("numPushes" in pushRange) { >+ var wantPushCount = pushRange.numPushes; As noted above, the naming numPushes is wrong, it is definitely not the number of pushes we want to load. >+ var currentPushCount = Controller.valuesFromObject(this._pushes).length; >+ var leastRecentPushID = this._mostRecentPush.id - currentPushCount + 1; >+ params.push({ >+ startID: pushRange.toPushID - wantPushCount - 1, >+ endID: leastRecentPushID - 1, >+ }); >+ } >+ return params; + 1, - 1? As you only use the var in that spot you can drop that unnecessary increment/decrement. >+ var tinderboxLoadEstimation = loadTracker.addMorePotentialLoads(30); Why suddenly 30? >+ if ("startdate" in params && "enddate" in params) >+ return [{ startdate: params.startdate, enddate: params.enddate }, false]; […] >- _getLogUrl: function PushlogJSONParser__getLogUrl(repoName, timeOffset) { >- var startDate = timeOffset ? this._formattedDate(new Date((timeOffset - Config.goBackHours * 3600) * 1000)) : '12+hours+ago'; >- var endDate = timeOffset ? this._formattedDate(new Date(timeOffset * 1000)) : 'now'; >- return "http://hg.mozilla.org/" + repoName + "/json-pushes?full=1&startdate=" + startDate + "&enddate=" + endDate; >+ _getLogUrl: function PushlogJSONParser__getLogUrl(repoName, params) { >+ var url = "http://hg.mozilla.org/" + repoName + "/json-pushes?full=1"; >+ for (var paramName in params) { >+ url += "&" + escape(paramName) + "=" + escape(params[paramName]); >+ } >+ return url; As we do no input processing and thus require the user to pass in correctly formatted pushlog dates, you can as well remove all the code that has to do with date formatting. Or you can move the processing to Controller or Data and convert a user provided UTC timestamp to pushlogs pst timestamps. I really don’t care which solution you choose, but I expect the users not to know what format/timezone the times need to be in. Please clean up the variable naming, it is confusing and leads to errors.

(In reply to comment #9) > Comment on attachment 498121 [details] [diff] [review] > part 3, v1 > > Just that I understand pushlog correctly: startID is the lower bound and endID > the upper (as in most recent) one? You're almost right: startID + 1 is the lower bound. In other words, the pushes returned by a request with startID=s&endID=e all have s < push.id <= e. This definitely needs to be documented somewhere in the code. > >+ extendPushRange: function Controller_extendPushRange(num) { > >+ var currentRange = this._requestedRange || this._data.getLoadedPushRange(); > >+ if (!currentRange) > >+ return; > >+ > >+ var requestedRange; > >+ if (num >= 0) { > >+ requestedRange = { > >+ toPushID: currentRange.toPushID, > >+ numPushes: currentRange.numPushes + num, > >+ }; > >+ } else { > >+ requestedRange = { > >+ toPushID: currentRange.toPushID + num, > >+ numPushes: currentRange.numPushes + Math.abs(num), > >+ }; > >+ } > > As far as I read the code, there is no negatve num (yet?) Correct. But now that the initial push range can be in the past, we might want to add an up arrow, too, to go forward in time. That would use a negative num. Hmm, maybe it would make more sense if going forward was positive and backward negative. > > Also: > > >+ var currentRange = this._requestedRange || this._data.getLoadedPushRange(); > […] > >+ requestedRange = { > >+ toPushID: currentRange.toPushID, > >+ numPushes: currentRange.numPushes + num, > >+ }; > […] > >+ this._requestedRange = requestedRange; > > Am I missing something or is toPushID staying the same and numPushes increasing > all the time? Correct, you're not missing anything. > Shouldn’t numPushes just be num, as in Config.goBackPushes? No. What we're calculating here is the new view we want to have on the data: we want to extend the range of pushes that we display. We don't want to move our window, we want to keep what we already have. So we need to increase the number of pushes we want to display. What we actually have to load is a different question. That's handled in data._getPushlogParamsForRange, which compares the range that's already loaded to the requested range and only loads those pushes that we don't have yet. > Otherwise the > naming of the var is wrong. How so? We could rename "num" to "extendByHowMany". > Also, toPushID is the lower or upper bound? Upper. > It is used as both in getPushlogParamsForRange. I'm confused. > >+ getLoadedPushRange: function Data_getLoadedPushRange() { > >+ if (!this._mostRecentPush) > >+ return null; > >+ return { > >+ toPushID: this._mostRecentPush.id, > >+ numPushes: Controller.valuesFromObject(this._pushes).length > >+ }; > >+ }, > > So numPushes means the number of pushes we already have? Why would we need > that? That's what getLoadedPushRange is all about: "Tell me what's already loaded." > >+ loadPushRange: function Data_loadPushRange(pushRange, loadPendingRunning, loadTracker, updatedPushCallback, infraStatsCallback, initialPushlogLoadCallback) { > > var self = this; > >- var tinderboxLoadEstimation = loadTracker.addMorePotentialLoads(2); > >+ var pushlogRequestParams = this._getPushlogParamsForRange(pushRange, false); > > That function does not take a second parameter. Oops. > >+ // 1. Above loaded range: > >+ if (pushRange.toPushID > this._mostRecentPush.id) { > >+ params.push({ startID: this._mostRecentPush.id, endID: pushRange.toPushID }); > >+ } > >+ // 2. Below loaded range: > >+ if ("numPushes" in pushRange) { > >+ var wantPushCount = pushRange.numPushes; > > As noted above, the naming numPushes is wrong, it is definitely not the number > of pushes we want to load. Right, it's the total number of pushes we want to have loaded after we we're done loading the remaining ones. > >+ var currentPushCount = Controller.valuesFromObject(this._pushes).length; > >+ var leastRecentPushID = this._mostRecentPush.id - currentPushCount + 1; > >+ params.push({ > >+ startID: pushRange.toPushID - wantPushCount - 1, > >+ endID: leastRecentPushID - 1, > >+ }); > >+ } > >+ return params; > > + 1, - 1? As you only use the var in that spot you can drop that unnecessary > increment/decrement. But then the variable name is no longer correct. > >+ var tinderboxLoadEstimation = loadTracker.addMorePotentialLoads(30); > > Why suddenly 30? Just to ensure that the load percentage never decreases. Now that the initial push range can span arbitrarily many days, we really don't know what to expect. Maybe our loadTracker needs an undetermined mode instead, during which we don't display any percentages. > >+ if ("startdate" in params && "enddate" in params) > >+ return [{ startdate: params.startdate, enddate: params.enddate }, false]; > […] > >- _getLogUrl: function PushlogJSONParser__getLogUrl(repoName, timeOffset) { > >- var startDate = timeOffset ? this._formattedDate(new Date((timeOffset - Config.goBackHours * 3600) * 1000)) : '12+hours+ago'; > >- var endDate = timeOffset ? this._formattedDate(new Date(timeOffset * 1000)) : 'now'; > >- return "http://hg.mozilla.org/" + repoName + "/json-pushes?full=1&startdate=" + startDate + "&enddate=" + endDate; > >+ _getLogUrl: function PushlogJSONParser__getLogUrl(repoName, params) { > >+ var url = "http://hg.mozilla.org/" + repoName + "/json-pushes?full=1"; > >+ for (var paramName in params) { > >+ url += "&" + escape(paramName) + "=" + escape(params[paramName]); > >+ } > >+ return url; > > As we do no input processing and thus require the user to pass in correctly > formatted pushlog dates, you can as well remove all the code that has to do > with date formatting. Oh, good! > Or you can move the processing to Controller or Data and > convert a user provided UTC timestamp to pushlogs pst timestamps. I really > don’t care which solution you choose, but I expect the users not to know what > format/timezone the times need to be in. Hmm. I don't see anything wrong with accepting the same input as pushlog. And as far as I know, pushlog even honors the timezone offset you give in the timestamp.

(In reply to comment #10) > (In reply to comment #9) > > Just that I understand pushlog correctly: startID is the lower bound and endID > > the upper (as in most recent) one? > > You're almost right: startID + 1 is the lower bound. In other words, the pushes > returned by a request with startID=s&endID=e all have s < push.id <= e. This > definitely needs to be documented somewhere in the code. Adding a comment will definitely help. > Correct. But now that the initial push range can be in the past, we might want > to add an up arrow, too, to go forward in time. That would use a negative num. > > Hmm, maybe it would make more sense if going forward was positive and backward > negative. > > > Shouldn’t numPushes just be num, as in Config.goBackPushes? > > No. > What we're calculating here is the new view we want to have on the data: we > want to extend the range of pushes that we display. We don't want to move our > window, we want to keep what we already have. So we need to increase the number > of pushes we want to display. > > What we actually have to load is a different question. That's handled in > data._getPushlogParamsForRange, which compares the range that's already loaded > to the requested range and only loads those pushes that we don't have yet. > > > Otherwise the > > naming of the var is wrong. > > How so? > We could rename "num" to "extendByHowMany". > > > Also, toPushID is the lower or upper bound? > > Upper. Ah I see, it is much clearer now. But can we change the naming to startWindow/endWindow? That would be much clearer and consistent with the pushlog naming, meaning it is the lower/upper bound of the window of pushes we already have. Depending on the sgn of num, we could just add to the upper bound or substract from the lower bound. When we work with ranges it also becomes easier to know what we need to load: The difference between the new range and the old one. > > Why suddenly 30? > > Just to ensure that the load percentage never decreases. Now that the initial > push range can span arbitrarily many days, we really don't know what to expect. > Maybe our loadTracker needs an undetermined mode instead, during which we don't > display any percentages. You are right, I have considered the backwards jumping percentage as well. But I don’t have any good idea what to do about it. > > Or you can move the processing to Controller or Data and > > convert a user provided UTC timestamp to pushlogs pst timestamps. I really > > don’t care which solution you choose, but I expect the users not to know what > > format/timezone the times need to be in. > > Hmm. I don't see anything wrong with accepting the same input as pushlog. And > as far as I know, pushlog even honors the timezone offset you give in the > timestamp. So you can include the timezone in the string you pass to pushlog? Why haven’t we been using it then instead of http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/bc58c4a26094/js/PushlogJSONParser.js#l85 ? Probably because I wrote that code and never heard of passing the timezone inside the string to pushlog. :-D

I think this addresses all of the comments. I went for startID/endID instead of startWindow/endWindow to be consistent with pushlog and because "window" is synonymous with "range" and thus redundant.

(In reply to comment #11) > So you can include the timezone in the string you pass to pushlog? Why haven’t > we been using it then instead of > http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/bc58c4a26094/js/PushlogJSONParser.js#l85 > ? Good question ;-)

Link pusher to pusher-filtered view and push time to single-push view.

Comment on attachment 498319 [details] [diff] [review] part 3, v2 Much better :-)

Comment on attachment 498321 [details] [diff] [review] part 4, v1 >+ _pusher: null, >+ _params: {}, […] >- var pusher = ("pusher" in params) && params.pusher; >+ this._pusher = ("pusher" in params) && params.pusher; […] >- this._data = new Data(this.treeName, noIgnore, Config, pusher); >+ this._data = new Data(this.treeName, noIgnore, Config, this._pusher); As far as I see, there is no reason to make the var a member. >+ var params = $.extend({}, this._params); // fancy way of cloning an object Nice!

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/d54cb0b7cce2 http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/1ad2d86e03d5 http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/414ce4ac170c http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/a256bd89f2dc