566870 - Make green arrows add more data to the page instead of replacing it

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, enhancement)

Description

[Jason Orendorff [:jorendorff]]

I love TBPL, *looove* it, but I think the "pagination" is unhelpful.

Loading is slow enough that my mind wanders and I start doing other work. When I come back, sometimes I'm a little confused about what exactly I'm seeing. Is my Try Server build before this stuff or after? Did I click once or twice?

Or suppose I'm looking for a transition, say, from orange to green. What I really want to do is (a) find the transition; (b) click the before and after previews just to be sure. The pagination makes part b unnecessarily slow.

So I propose that the down arrow be changed from "Go back 12 hours" to "Add 12 more hours of data".

Comment 1

[(no longer active)]

Markus, is there a reason for the way things are right now?  I always thought this has had something to do with the large data chunk that we need to load...

Comment 2

[Markus Stange [:mstange]]

No reason, it's just the way Arpad initially implemented it. I'd love this bug to get fixed.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

The suckiness of our build automation data is endless.

Here's the patch to make this possible.  I couldn't get it to work completely, because loading the non-cached json data just times out, but I'm fairly confident in the correctness of this patch.  The same problem happens when trying to go back on the tests.themasta.com instance.

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 450522 [details] [diff] [review]
Patch (v1)

Yeah, this should do it.

It would probably also be good if TBPL could combine data from smaller ranges, which might load a little faster individually, so when going back in time you wouldn't have to load data that you already have.

Comment 5

[Phil Ringnalda (:philor)]

Not that I think my crappy connections should entirely drive tbpl design, but, I tried downloading m-c's &hours=48&json=1 over my second-best connection earlier today, and that 15MB took something like 17 minutes, so this would probably mean that I could never use back at all. I'd expect that after the first couple of weeks after a repo reset, probably nobody at all would be able to go back in MozillaTry, either.

Comment 6

[(no longer active)]

This clearly depends on the work in bug 553549, even though that bug might make this issue obsolete.

Comment 7

[Arpad Borsos [:Swatinem]]

Attachment: 1: refactor html generation

Comment 8

[Arpad Borsos [:Swatinem]]

Attachment: 2: don’t regenerate the whole dom when changing timezone

Comment 9

[Arpad Borsos [:Swatinem]]

Attachment: 3: seal most of Data

Comment 10

[Arpad Borsos [:Swatinem]]

Attachment: 4: keep track of running and orphan runs

As far as my testing goes, every part is in itself stable.

I need maybe 2-3 more patches to get this done, finally.

Comment 11

[Arpad Borsos [:Swatinem]]

Attachment: 5: UserInterface takes care of sorting the pushes list

--- a/js/TinderboxJSONUser.js
+++ b/js/TinderboxJSONUser.js-      try {
-        if (!tinderbox_data) throw "tinderbox_data is invalid";
+      if (!tinderbox_data)
+        failCallback("tinderbox_data is invalid");
+      else
         loadCallback(self.parseTinderbox(tree, tinderbox_data));
-      } catch (e) {
-        window.tinderboxException = e;
-        failCallback(e);
-      }

Its just annoying that exceptions from Data._combineResults are caught and not displayed in the error console with file and line number.

Comment 12

[Arpad Borsos [:Swatinem]]

Attachment: 6: Data takes care of the machines

Comment 13

[Arpad Borsos [:Swatinem]]

Attachment: 7: add history to the existing pushes

Thats it for now, finally :)

This makes it straightforward to use pending/running.json later.
Also loads less tinderbox data so adding history should fail less often and be faster :)

Comment 14

[Arpad Borsos [:Swatinem]]

Attachment: 6: Data takes care of the machines

Only successful runs contribute to the averagetime, as it was before.

Comment 15

[Arpad Borsos [:Swatinem]]

Attachment: 7: add history to the existing pushes

running builds that are finished are correctly cleared on refreshes.

Comment 16

[Arpad Borsos [:Swatinem]]

Attachment: 7: add history to the existing pushes

Now we add new comments to already finished runs on update.
Sorry for the bugspam :P

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 477565 [details] [diff] [review]
2: don’t regenerate the whole dom when changing timezone

>diff --git a/js/UserInterface.js b/js/UserInterface.js

>+      var from = timeOffset ? new Date((timeOffset - 12 * 3600) * 1000) :

>+      var and = timeOffset ? new Date(timeOffset * 1000) : new Date();

Please call the startDate and endDate.

Comment 18

[Arpad Borsos [:Swatinem]]

(In reply to comment #17)
> Comment on attachment 477565 [details] [diff] [review]
> 2: don’t regenerate the whole dom when changing timezone
> 
> >diff --git a/js/UserInterface.js b/js/UserInterface.js
> 
> >+      var from = timeOffset ? new Date((timeOffset - 12 * 3600) * 1000) :
> 
> >+      var and = timeOffset ? new Date(timeOffset * 1000) : new Date();
> 
> Please call the startDate and endDate.

These are being dropped completely in patch 7. So if you don’t mind I would like to not need any unnecessary rebases here.
Each patch is self contained to ease the review burden, bud I’d like to land them all together.

Comment 19

[Markus Stange [:mstange]]

Oh I see. I haven't gotten that far yet (need to apply them so my tired eyes can benefit from syntax highlighting... :) )

Comment 20

[Markus Stange [:mstange]]

Some final comments:
 - CSS styles for the Up arrow can be removed
 - if (~["building", "pending"].indexOf(result.state)) is too demanding on
   the reader
 - this._orphanResults = {}; needs a comment like // results that can't be
   associated to any push

Comment 21

[Markus Stange [:mstange]]

Oh, another thing: I think the message "There are no pushes to be displayed" shouldn't be visible at the beginning. It should only be added after the first pushlog load, if there are no pushes.

Comment 22

[Arpad Borsos [:Swatinem]]

Attachment: 8: adress review comments

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 478279 [details] [diff] [review]
8: adress review comments

r=me except for the typos ;)

Comment 24

[Arpad Borsos [:Swatinem]]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/pushloghtml?changeset=90b45cf65dda

Whee :)