650126 - Show pushlog feature no longer displayed in Nightly Tester Tools menu, since lack of "pre" string causes isTrunk() to fail

Status: RESOLVED FIXED

(Other Applications Graveyard :: Nightly Tester Tools, defect)

Description

[Ed Morley [:emorley]]

Using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110414 Firefox/6.0a1 ID:20110414030535 and NTT 3.1.3, the "show pushlog" link in the NTT menu no longer appears.

This is due to /chrome/content/nightly.js line 87 (https://github.com/ed2010/nightlytt/blob/master/chrome/content/nightly.js#L87), which is currently:
> && nightly.variables.version.indexOf("pre") != -1;

This fails now that pre isn't in appinfo.version, meaning isTrunk() returns false; resulting in prevChangeset/currChangeset not being updated and the pushlog menu item not being shown.

The line now needs to read:
> && (nightly.variables.version.indexOf("a") != -1 || nightly.variables.version.indexOf("pre") != -1);

(Presuming you want to maintain support for older nightlies).

This should support Aurora nightlies as well as "Nightly" nightlies (!), since it will catch the "X.0a1" of Nightly and the "X.0a2" of Aurora and automatically use the correct URL due to nightly.getRepo().

I guess if you wanted to avoid 3.7a1 false positives, you could also check that appinfo.version >=5.0

Comment 1

[Ed Morley [:emorley]]

Submitted git pull request containing fix:
https://github.com/mozautomation/nightlytt/pull/5/files

It fixes isTrunk() by adding support for X.0a1 (Nightly) and X.0a2 (Aurora). Avoids false positives with 3.7aX by checking for the presence of ".0a" rather than just "a".

> --- a/chrome/content/nightly.js
> +++ b/chrome/content/nightly.js
> @@ -84,7 +84,8 @@ preferences: null,
>  
>  isTrunk: function() { 
>    return nightly.getRepo().indexOf(nightlyApp.repository) != -1
> -    && nightly.variables.version.indexOf("pre") != -1;
> +    && (nightly.variables.version.indexOf("pre") != -1 || 
> +        nightly.variables.version.indexOf(".0a") != -1);
>  },
>  
>  showAlert: function(id, args) {

Comment 2

[Heather Arthur [:harth]]

Thanks so much for the fix!

master:
https://github.com/mozautomation/nightlytt/commit/320799b5d69f029c62390dd50f86df2e69c0c086

Comment 3

[Szabolcs Hubai (:xabolcs)]

Hi!


I didn't want to file a new bug, but the isTrunk() is still failing for Aurora.
And not because nightly.version but the nightlyApp.repository:
https://github.com/mozautomation/nightlytt/blob/master/chrome/content/nightly.js#L86

nightlyApp.repository is at https://github.com/mozautomation/nightlytt/blob/master/chrome/content/browser.js#L40 and defined to be 'mozilla-central' which is valid only for Nightly, not the others.

Could you fix this (to work with Aurora)?

I happily fix this and send a pull request if you specify or write a recommendation how should I hack the nightlyApp.repository to contain both 'mozilla-central' and 'mozilla-aurora'.
Or if you have lot of time, then I could play with the source requiring lot of review. :)

Comment 4

[Ed Morley [:emorley]]

Ok, I've looked into this and...

1) As before, isTrunk() is being called and the result used to determine whether to show the pushlog entry and also whether to update the record of current changeset vs last changeset.

2) isTrunk looks at both the version string (which was what was fixed in comment 1) and also the repo that the current app was built from - using the values from application.ini. This is why in comment 0 that I thought this should work for aurora as is. 

3) However, the repo found in application.ini is cross-checked against that stored in browser.js/suite.js/messenger.js - and if it doesn't match, causes isTrunk to fail; which is a shame since up until this point, the code would work irrespective of repo, due to using the appropriate value in application.ini.

Now seeing that moving forwards, the repo names are going to remain consistent (ie mozilla-central, mozilla-aurora, mozilla-beta), I think the easiest thing is just to make isTrunk not bother to double check the repo name - and just use the value from application.ini as-is. In the past, if alpha/beta builds branched into their own repo, it might break the pushlog function, so I can see why the check was needed - but now it isn't. 

The only problem I can foresee is if someone uses the same profile for Nightly and Aurora - and regularly switches between the two - since it will mess up the values for current vs last changeset. I guess these could be stored as currChangesetNightly currChangesetAurora - or else having a lastChannelUsed (and if it differs, scrap existing changeset values); but unless Heather thinks this will be a common use-case, perhaps it can just be ignored for now.

Comment 5

[Ed Morley [:emorley]]

Meant to add, making the change I proposed in comment 4 would have the added benefit of making NTT pushlog work with other repos eg Tracemonkey (albeit again for users that regularly switch between builds/repos using the same profile, it would mess up the current vs last changeset values).

Comment 6

[Szabolcs Hubai (:xabolcs)]

Your proposed change is
check only that the current XUL App's branch in one of the Nightly / Aurora branch (mozilla-central, comm-central, mozilla-aurora, comm-miramar, ...) defined in browser.js/suite.js/messenger.js?
For example:
"return nightly.getBranch() in nightlyApp.repositories;"

and in browser.js:
"
var nightlyApp = {

repositories: {mozilla-central: 1, mozilla-aurora: 1 },
...
"

Am I right?


Compatibility with other repos would be good. :)