Revamp NTT's force-compatiblity feature. Fixes #52. #82

Closed
wants to merge 14 commits into
from

Conversation

Projects
None yet
3 participants
@xabolcs
Collaborator

xabolcs commented Jun 6, 2012

Please note the following:

  • it's a draft!
  • it is based on that addon in #52.
  • it introduces a new feature: restart on demand - inspired by that addon. :)
extension/chrome/content/nightly.js
@@ -493,14 +493,22 @@ openPushlog: function() {
toggleCompatibility: function() {
var forceCompat = nightly.preferences.getBoolPref("disableCheckCompatibility");
- nightly.preferences.setBoolPref("disableCheckCompatibility", !forceCompat);
+ var os = Components.classes["@mozilla.org/observer-service;1"].

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Jul 2, 2012

Contributor

It has been named obs across the Firefox codebase. I would propose we do the same here, which makes it easier for us later to replace the instances with Services.obs.

@whimboo

whimboo Jul 2, 2012

Contributor

It has been named obs across the Firefox codebase. I would propose we do the same here, which makes it easier for us later to replace the instances with Services.obs.

extension/chrome/content/nightly.js
- nightly.getString("nightly.restart.label"),
- nightly.getString("nightly.restart.accesskey"),
- function() { Application.restart(); });
+ os.addObserver({observe: restartObserver}, "nttACS", false);

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Jul 2, 2012

Contributor

I would propose we better define an object with the observe method contained.

@whimboo

whimboo Jul 2, 2012

Contributor

I would propose we better define an object with the observe method contained.

+ this.prefService.addObserver(PREF, this, false);
+
+ this.appinfo = Components.classes["@mozilla.org/xre/app-info;1"]
+ .getService(Components.interfaces.nsIXULAppInfo);

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Jul 2, 2012

Contributor

Please shorten to Cc and Ci and align the second row with C from Cc.

@whimboo

whimboo Jul 2, 2012

Contributor

Please shorten to Cc and Ci and align the second row with C from Cc.

@@ -37,17 +37,30 @@
Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+const PREF = "nightly.disableCheckCompatibility";
+const PREFIX = "extensions.checkCompatibility.";
+const PREF_NIGHTLY = "nightly";

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Jul 2, 2012

Contributor

Please use better names for those constants so it's easier later in the code what we are referring to.

@whimboo

whimboo Jul 2, 2012

Contributor

Please use better names for those constants so it's easier later in the code what we are referring to.

this.setCompatPrefs();
}
nttAddonCompatibilityService.prototype = {
+ get version() this.appinfo.version.replace(/^([^\.]+\.[0-9]+[a-z]*).*/i, "$1"),

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Jul 2, 2012

Contributor

Please move this down below all the components registration code and also put brackets around the function body.

@whimboo

whimboo Jul 2, 2012

Contributor

Please move this down below all the components registration code and also put brackets around the function body.

+ } else {
+ self.obs.notifyObservers(null, "nttACS", null);
+ }
+ });

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Jul 2, 2012

Contributor

This looks like a 1:1 copy from the source. Please make sure to format the code accordingly to our styles. Also please separate out getAddonsByTypes so it can be read and understood easier.

@whimboo

whimboo Jul 2, 2012

Contributor

This looks like a 1:1 copy from the source. Please make sure to format the code accordingly to our styles. Also please separate out getAddonsByTypes so it can be read and understood easier.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Aug 14, 2012

Collaborator

whimboo commented on commit

...
Also please separate out getAddonsByTypes so it can be read and understood easier.

I had no idea how to do this nicely without #90. :(
After checking 392a9b8 please advice me how to separate out getAddonByTypes more!
I gave it a try in the commit above, but any comments are welcome!

@xabolcs

xabolcs Aug 14, 2012

Collaborator

whimboo commented on commit

...
Also please separate out getAddonsByTypes so it can be read and understood easier.

I had no idea how to do this nicely without #90. :(
After checking 392a9b8 please advice me how to separate out getAddonByTypes more!
I gave it a try in the commit above, but any comments are welcome!

+ );
+ self.obs.notifyObservers(null, "nttACS", "restartNeeded");
+ } else {
+ self.obs.notifyObservers(null, "nttACS", null);

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Jul 2, 2012

Contributor

why do we have to notify with null?

@whimboo

whimboo Jul 2, 2012

Contributor

why do we have to notify with null?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Aug 14, 2012

Collaborator

You should read the code more carefully! :)

Definitely notifying with "pleaseDeRegisterYourself" message would be more understandable:
in nightly.toggleCompatibility an observer is registered in every menuItem click ... that should be unregistered! Always.

Of course there are other ways of implementing "Restart on demand". :)

@xabolcs

xabolcs Aug 14, 2012

Collaborator

You should read the code more carefully! :)

Definitely notifying with "pleaseDeRegisterYourself" message would be more understandable:
in nightly.toggleCompatibility an observer is registered in every menuItem click ... that should be unregistered! Always.

Of course there are other ways of implementing "Restart on demand". :)

+ },
+
+
+ setCompatPrefs : function() {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Jul 2, 2012

Contributor

Why a separate function? Move this code directly into the else case of manageCompatPrefsSetting.

@whimboo

whimboo Jul 2, 2012

Contributor

Why a separate function? Move this code directly into the else case of manageCompatPrefsSetting.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Aug 14, 2012

Collaborator

Again, You should read the code more carefully! :)
Or I missed something.

manageCompatPrefsSetting setCompatPrefsHelper does the following:
IF there is AddonsManager then:

  1. do something
  2. call setCompatPrefs
  3. do other stuffs
    ELSE
  4. call setCompatPrefs
  5. other stuff

Please clarify Your question!

@xabolcs

xabolcs Aug 14, 2012

Collaborator

Again, You should read the code more carefully! :)
Or I missed something.

manageCompatPrefsSetting setCompatPrefsHelper does the following:
IF there is AddonsManager then:

  1. do something
  2. call setCompatPrefs
  3. do other stuffs
    ELSE
  4. call setCompatPrefs
  5. other stuff

Please clarify Your question!

#Issue 52 - addressing comments, round 1
- renamed 'os' to 'obs'
- moved observer code to an object
- shortened Components.*
- formatted foreign code
- renamed preference constants
- moved version()
@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Aug 14, 2012

Collaborator

Added commit 392a9b8 - Addressing comments, round 1.

Collaborator

xabolcs commented Aug 14, 2012

Added commit 392a9b8 - Addressing comments, round 1.

+ self.obs.notifyObservers(null, "nttACS", null);
+ }
+ }
+ AddonManager.getAddonsByTypes(null, countPendingAddonsAndNotifyToRestartCallback);

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Aug 14, 2012

Collaborator

The number of characters in function name countPendingAddonsAndNotifyToRestartCallback is too damn high.

@xabolcs

xabolcs Aug 14, 2012

Collaborator

The number of characters in function name countPendingAddonsAndNotifyToRestartCallback is too damn high.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 12, 2013

Contributor

I would suggest we move the callback inline. There is no reason to have it separate given that it is only used once.

@whimboo

whimboo Mar 12, 2013

Contributor

I would suggest we move the callback inline. There is no reason to have it separate given that it is only used once.

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 7, 2013

Contributor

Would you mind to update the pull for the latest version of the NTT code? We really missed that in the 3.4 release. :/

Contributor

whimboo commented Mar 7, 2013

Would you mind to update the pull for the latest version of the NTT code? We really missed that in the 3.4 release. :/

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 7, 2013

Collaborator

whimboo wrote:

Would you mind to update the pull for the latest version of the NTT code? We really missed that in the 3.4 release. :/

Sure! :)
Thank you for picking up.

Collaborator

xabolcs commented Mar 7, 2013

whimboo wrote:

Would you mind to update the pull for the latest version of the NTT code? We really missed that in the 3.4 release. :/

Sure! :)
Thank you for picking up.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 8, 2013

Collaborator

This pull request can be automatically merged. Again. :)

Collaborator

xabolcs commented Mar 8, 2013

This pull request can be automatically merged. Again. :)

+ let startCount = count();
+ self.setCompatPrefs();
+ if (startCount != count()) {
+ self.obs.notifyObservers(null, "nttACS", "restartNeeded");

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 12, 2013

Contributor

I would suggest we send an object like { restart: true } which will make it easier to extend the observer data later if needed.

@whimboo

whimboo Mar 12, 2013

Contributor

I would suggest we send an object like { restart: true } which will make it easier to extend the observer data later if needed.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 14, 2013

Collaborator

I would suggest we send an object ...

Are you sure? Please check notifyObservers() documentation again! ;)

void notifyObservers(
  in nsISupports aSubject,
  in string aTopic,
  in wstring someData 
);

I could use JSON.parse() here on the other side, if you really want to pass an object.

@xabolcs

xabolcs Mar 14, 2013

Collaborator

I would suggest we send an object ...

Are you sure? Please check notifyObservers() documentation again! ;)

void notifyObservers(
  in nsISupports aSubject,
  in string aTopic,
  in wstring someData 
);

I could use JSON.parse() here on the other side, if you really want to pass an object.

+ let self = this;
+ function countPendingAddonsAndNotifyToRestartCallback(aAddons) {
+ function count() {
+ return aAddons.reduce(function (aAccumulator, aAddon) {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 12, 2013

Contributor

Instead of reduce I would run a filter for the addon state and take the count from the final array length.

@whimboo

whimboo Mar 12, 2013

Contributor

Instead of reduce I would run a filter for the addon state and take the count from the final array length.

+ },
+
+
+ setCompatPrefs : function() {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 12, 2013

Contributor

nit: space before the rounded opening bracket

@whimboo

whimboo Mar 12, 2013

Contributor

nit: space before the rounded opening bracket

+ setCompatPrefs : function() {
+ var prefs = [];
+
+ switch(this.appinfo.name) {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 12, 2013

Contributor

nit: same here

@whimboo

whimboo Mar 12, 2013

Contributor

nit: same here

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 12, 2013

Contributor

Also please add a comment why we are excluding Songbird here.

@whimboo

whimboo Mar 12, 2013

Contributor

Also please add a comment why we are excluding Songbird here.

- "extensions.checkCompatibility.nightly"];
+
+ get version() {
+ return this.appinfo.version.replace(/^([^\.]+\.[0-9]+[a-z]*).*/i, "$1");

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 12, 2013

Contributor

We don't need a getter here. Call this once in the constructor and store it. The version will never change during a Firefox session.

@whimboo

whimboo Mar 12, 2013

Contributor

We don't need a getter here. Call this once in the constructor and store it. The version will never change during a Firefox session.

xabolcs added some commits Mar 14, 2013

#Issue 52 - addressing comments, round 2, part 1
- JSONed observer data
- version is now a property
- space nits
- Songbird exclusion note
- filtering addons
- not inlining long name function yet
@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 14, 2013

Collaborator

@whimboo
All your comments addressed.
While in-lining the long named function I restructured a little bit.

Collaborator

xabolcs commented Mar 14, 2013

@whimboo
All your comments addressed.
While in-lining the long named function I restructured a little bit.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 14, 2013

Collaborator

@whimboo
You could review now, but please don't land it for a while.
Let me consult Gerv about crediting Kris for his original work before landing.

Collaborator

xabolcs commented Mar 14, 2013

@whimboo
You could review now, but please don't land it for a while.
Let me consult Gerv about crediting Kris for his original work before landing.

extension/chrome/content/nightly.js
+ obs.removeObserver(restartObserver, "nttACS");
+ var parsedData = JSON.parse(data);
+ if (parsedData && parsedData.restart) {
+ nightlyApp.openNotification("nightly-compatibility-restart",

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 14, 2013

Contributor

Before we had if (nightlyApp.openNotification). Isn't that necessary anymore?

@whimboo

whimboo Mar 14, 2013

Contributor

Before we had if (nightlyApp.openNotification). Isn't that necessary anymore?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 14, 2013

Collaborator

Before we had if (nightlyApp.openNotification). Isn't that necessary anymore?
It's still around here! :)

The restartObserver above is activated only if nightlyApp.openNotification exists. See L491!

@xabolcs

xabolcs Mar 14, 2013

Collaborator

Before we had if (nightlyApp.openNotification). Isn't that necessary anymore?
It's still around here! :)

The restartObserver above is activated only if nightlyApp.openNotification exists. See L491!

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 14, 2013

Contributor

I would move this if in here, so that we always send an observer notification. Other extensions may want to rely on the notification and then could benefit from.

@whimboo

whimboo Mar 14, 2013

Contributor

I would move this if in here, so that we always send an observer notification. Other extensions may want to rely on the notification and then could benefit from.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 14, 2013

Collaborator

I would move this if in here, so that we always send an observer notification. ...

?!
The nttAddonCompatibilityService sends the notification in all case. See the other, sendNotification(), notifyObject related threads near at line L54 and L73/L83!

The restartObserver here isn't always activated.
For example Songbird doesn't have nightlyApp.openNotification implemented yet,
therefore nightly.toggleCompatibility doesn't register for the restart notification.
It simply discards. :)
But in Firefox case it will register the observer, and the PopupNotification/NotificationBox could open if restart would be needed after pref change.

@xabolcs

xabolcs Mar 14, 2013

Collaborator

I would move this if in here, so that we always send an observer notification. ...

?!
The nttAddonCompatibilityService sends the notification in all case. See the other, sendNotification(), notifyObject related threads near at line L54 and L73/L83!

The restartObserver here isn't always activated.
For example Songbird doesn't have nightlyApp.openNotification implemented yet,
therefore nightly.toggleCompatibility doesn't register for the restart notification.
It simply discards. :)
But in Firefox case it will register the observer, and the PopupNotification/NotificationBox could open if restart would be needed after pref change.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 14, 2013

Contributor

It doesn't matter which application has openNotification available or not. If a restart is necessary we should fire the observer anyway. If the notification cannot be handled in observe because the above method is not available, we can always skip it. But we should not stop sending the observer notification due to that reason. As I have mentioned above others may be very well interested in our notification too.

@whimboo

whimboo Mar 14, 2013

Contributor

It doesn't matter which application has openNotification available or not. If a restart is necessary we should fire the observer anyway. If the notification cannot be handled in observe because the above method is not available, we can always skip it. But we should not stop sending the observer notification due to that reason. As I have mentioned above others may be very well interested in our notification too.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 20, 2013

Contributor

I don't think we should over-engineer it at this point. When we really only want to have it available internally, why can't we call it directly through it's object/instance? If the prompt has to happen async we can use setTimeout(fun, 0) to trigger it, right?

@whimboo

whimboo Mar 20, 2013

Contributor

I don't think we should over-engineer it at this point. When we really only want to have it available internally, why can't we call it directly through it's object/instance? If the prompt has to happen async we can use setTimeout(fun, 0) to trigger it, right?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 21, 2013

Collaborator

In this case the component should find the most recent Tb, SM, Fx, ... window - if any - then it could call nightly.openNotification.

But I'm still voting for drop this demand stuff from this pull, and let it born as a follow-up issue. Or simply leave it as is - a public interface.
And I still can't imagine what do you think how should I implement this interface privately. :(

@xabolcs

xabolcs Mar 21, 2013

Collaborator

In this case the component should find the most recent Tb, SM, Fx, ... window - if any - then it could call nightly.openNotification.

But I'm still voting for drop this demand stuff from this pull, and let it born as a follow-up issue. Or simply leave it as is - a public interface.
And I still can't imagine what do you think how should I implement this interface privately. :(

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 22, 2013

Contributor

Ok, so lets rename the observer topic to _nttACS which should make it clear it's somewhat private. Please file a follow-up where we can discuss another (better) solution. Thanks.

@whimboo

whimboo Mar 22, 2013

Contributor

Ok, so lets rename the observer topic to _nttACS which should make it clear it's somewhat private. Please file a follow-up where we can discuss another (better) solution. Thanks.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 25, 2013

Collaborator

Ok, so lets rename the observer topic to _nttACS ...

Renamed, but not tested.

@xabolcs

xabolcs Mar 25, 2013

Collaborator

Ok, so lets rename the observer topic to _nttACS ...

Renamed, but not tested.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 13, 2013

Collaborator

Until this private interface doesn't get implemented (should file a follow-up issue) I move this obs and restartObserver into if (nightlyApp.openNotification) section:

  • to avoid confusion, because they belongs there
  • due to performance :)
@xabolcs

xabolcs Apr 13, 2013

Collaborator

Until this private interface doesn't get implemented (should file a follow-up issue) I move this obs and restartObserver into if (nightlyApp.openNotification) section:

  • to avoid confusion, because they belongs there
  • due to performance :)
- if(this.prefService.getBoolPref("nightly.disableCheckCompatibility"))
+ this.version = this.appinfo.version.replace(/^([^\.]+\.[0-9]+[a-z]*).*/i, "$1");
+
+ if(this.prefService.getBoolPref(PREF_FORCE_COMPAT)) {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 14, 2013

Contributor

nit: a blank between if and the opening bracket.

@whimboo

whimboo Mar 14, 2013

Contributor

nit: a blank between if and the opening bracket.

// nsIObserver
- observe : function(subject, topic, data) {
+ observe : function (subject, topic, data) {
if (topic == "nsPref:changed") {
switch(data) {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 14, 2013

Contributor

nit: blank after switch please.

@whimboo

whimboo Mar 14, 2013

Contributor

nit: blank after switch please.

+ try{
+ Cu.import("resource://gre/modules/AddonManager.jsm");
+ AddonManager.getAddonsByTypes(null, function (aAddons) {
+ function isPendingAddon(aAddon) {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 14, 2013

Contributor

I still don't see the reason why it has to be a separate function. Make it an inline function for filter() please which let us better follow the flow.

@whimboo

whimboo Mar 14, 2013

Contributor

I still don't see the reason why it has to be a separate function. Make it an inline function for filter() please which let us better follow the flow.

+ // but now it's always returning a different number
+ return counter++;
+ }
+ sendNotification({count: count});

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 14, 2013

Contributor

So we always send a notification, right? We may want to condense both calls into a single one and put it after the try/catch.

@whimboo

whimboo Mar 14, 2013

Contributor

So we always send a notification, right? We may want to condense both calls into a single one and put it after the try/catch.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 14, 2013

Collaborator

So we always send a notification, right?

Nope.
The first sendNotification is called async, the second is called in sync.
The common stuff - sendNotification - is already put before the try/catch.

@xabolcs

xabolcs Mar 14, 2013

Collaborator

So we always send a notification, right?

Nope.
The first sendNotification is called async, the second is called in sync.
The common stuff - sendNotification - is already put before the try/catch.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 14, 2013

Contributor

Ah, you are right. So that's fine then. :)

@whimboo

whimboo Mar 14, 2013

Contributor

Ah, you are right. So that's fine then. :)

+ /**
+ * Excluding Songbird because it is based on Gecko 1.9.2
+ * and "extensions.checkCompatibility.nightly" preference
+ * introduced in Gecko 7.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 14, 2013

Contributor

What about Firefox 3.6 you still want to support? It would have the same issue. Shouldn't we set the compatibility for 1.9.2 instead? There may be extensions which are disabled and the user still can't force to be enabled.

@whimboo

whimboo Mar 14, 2013

Contributor

What about Firefox 3.6 you still want to support? It would have the same issue. Shouldn't we set the compatibility for 1.9.2 instead? There may be extensions which are disabled and the user still can't force to be enabled.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 14, 2013

Contributor

Oh wait. The line is below. Forget about it.

@whimboo

whimboo Mar 14, 2013

Contributor

Oh wait. The line is below. Forget about it.

#Issue 52 - addressing comments, round 3, part 1
- space nits
- inline pending filter
@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 21, 2013

Collaborator

Pull updated for the nits.
The nightlynttAddonCompatibilityService private communication is still unclear to me.

Collaborator

xabolcs commented Mar 21, 2013

Pull updated for the nits.
The nightlynttAddonCompatibilityService private communication is still unclear to me.

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Mar 26, 2013

Contributor

Looks fine to me. @xabolcs can we get this tested? Do you have the time for? If not could you, @tonymec do it? I would kinda appreciate that given the lack of time on my side this week. :/

Contributor

whimboo commented Mar 26, 2013

Looks fine to me. @xabolcs can we get this tested? Do you have the time for? If not could you, @tonymec do it? I would kinda appreciate that given the lack of time on my side this week. :/

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 26, 2013

Collaborator

Looks fine to me. @xabolcs can we get this tested?

Grrr!
Testing with an old Nighlty (Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121229 Firefox/20.0 ID:20121229030919 CSet: 0bb4773db082) showed me that commit 593d77f introduced a regression in the restart-notification. :(

Seems like the inline transformation of isPendingAddon() wasn't succesful.

Collaborator

xabolcs commented Mar 26, 2013

Looks fine to me. @xabolcs can we get this tested?

Grrr!
Testing with an old Nighlty (Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121229 Firefox/20.0 ID:20121229030919 CSet: 0bb4773db082) showed me that commit 593d77f introduced a regression in the restart-notification. :(

Seems like the inline transformation of isPendingAddon() wasn't succesful.

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec Mar 27, 2013

Contributor

I'll have extremely little free time until at least Easter Monday. The two weeks after that I might have a little more slack.

Contributor

tonymec commented Mar 27, 2013

I'll have extremely little free time until at least Easter Monday. The two weeks after that I might have a little more slack.

xabolcs added some commits Mar 27, 2013

Issue #52 - addressing comments, round 4, part 1
- not starting a boolean in isPendingAddon()
- space nits
Issue #52 - addressing comments, round 4, part 2
- not starting a boolean in isPendingAddon(), really
@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 27, 2013

Collaborator

Seems like the inline transformation of isPendingAddon() wasn't succesful.

Bug hunted down.
Tested with

  • Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130327 Firefox/22.0 ID:20130327031035 CSet: 178a4a770bb1
  • Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121229 Firefox/20.0 ID:20121229030919 CSet: 0bb4773db082
  • Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 ID:20130116073211 CSet: eecd28b7ba09
Collaborator

xabolcs commented Mar 27, 2013

Seems like the inline transformation of isPendingAddon() wasn't succesful.

Bug hunted down.
Tested with

  • Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130327 Firefox/22.0 ID:20130327031035 CSet: 178a4a770bb1
  • Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121229 Firefox/20.0 ID:20121229030919 CSet: 0bb4773db082
  • Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 ID:20130116073211 CSet: eecd28b7ba09
@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Mar 27, 2013

Owner

This returns undefined instead the given result.

This returns undefined instead the given result.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 9, 2013

Collaborator

@whimboo
Addressing comments is done for now, feel free to review again!

Collaborator

xabolcs commented Apr 9, 2013

@whimboo
Addressing comments is done for now, feel free to review again!

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 9, 2013

So what was the problem with those lines? You didn't gave an answer on the changes. And I also don't see anything here. So I'm interested in what fixed it.

nit: Can you please correct the indentation of the second line, so that both brackets start in the same column?

So what was the problem with those lines? You didn't gave an answer on the changes. And I also don't see anything here. So I'm interested in what fixed it.

nit: Can you please correct the indentation of the second line, so that both brackets start in the same column?

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 9, 2013

Owner

So what was the problem with those lines? You didn't gave an answer on the changes. And I also don't see anything here. So I'm interested in what fixed it.

Try this gist!
The problem is that if return finds nothing but whitespace to EOL then it thinks there is nothing there, and returns undefined.
I had to start the expression in the same line where the return is.

nit: Can you please correct the indentation of the second line, so that both brackets start in the same column?

Sure!

Owner

xabolcs commented on 0605f66 Apr 9, 2013

So what was the problem with those lines? You didn't gave an answer on the changes. And I also don't see anything here. So I'm interested in what fixed it.

Try this gist!
The problem is that if return finds nothing but whitespace to EOL then it thinks there is nothing there, and returns undefined.
I had to start the expression in the same line where the return is.

nit: Can you please correct the indentation of the second line, so that both brackets start in the same column?

Sure!

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 9, 2013

Hah, I have never seen that and expected such a smart thing from the JS interpreter. Good to know! Thanks

Hah, I have never seen that and expected such a smart thing from the JS interpreter. Good to know! Thanks

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 9, 2013

Collaborator

@whimboo
Commit 94e0e83 is added, now it's your turn.

Collaborator

xabolcs commented Apr 9, 2013

@whimboo
Commit 94e0e83 is added, now it's your turn.

extension/chrome/content/nightly.js
@@ -473,14 +473,25 @@ openPushlogSinceCurrentBuild: function() {
toggleCompatibility: function() {
var forceCompat = nightly.preferences.getBoolPref("disableCheckCompatibility");
- nightly.preferences.setBoolPref("disableCheckCompatibility", !forceCompat);
+ var obs = Components.classes["@mozilla.org/observer-service;1"].
+ getService(Components.interfaces.nsIObserverService);

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 10, 2013

Contributor

We have an inconsistency here how we use Cc and Ci regarding their short and long names. If you feel fine please make it shorter here.

@whimboo

whimboo Apr 10, 2013

Contributor

We have an inconsistency here how we use Cc and Ci regarding their short and long names. If you feel fine please make it shorter here.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 10, 2013

Collaborator

I see no inconsistency here reagrding to Cx shortcuts.
Please note that nightly.js doesn't have those shortcuts!

@xabolcs

xabolcs Apr 10, 2013

Collaborator

I see no inconsistency here reagrding to Cx shortcuts.
Please note that nightly.js doesn't have those shortcuts!

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 10, 2013

Contributor

So please file a new issue which we can use to sync up all the files.

@whimboo

whimboo Apr 10, 2013

Contributor

So please file a new issue which we can use to sync up all the files.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 11, 2013

Collaborator

We have #87 already for styling. Should I file a new for code consistency?

@xabolcs

xabolcs Apr 11, 2013

Collaborator

We have #87 already for styling. Should I file a new for code consistency?

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 11, 2013

Contributor

Whatever works best for you. Is something else remaining now or can we can get this pull merged?

@whimboo

whimboo Apr 11, 2013

Contributor

Whatever works best for you. Is something else remaining now or can we can get this pull merged?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 11, 2013

Collaborator

I have to fix the two nits, so the pull request is not ready yet.

@xabolcs

xabolcs Apr 11, 2013

Collaborator

I have to fix the two nits, so the pull request is not ready yet.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 13, 2013

Collaborator

So please file a new issue which we can use to sync up all the files.

Filed #128.

@xabolcs

xabolcs Apr 13, 2013

Collaborator

So please file a new issue which we can use to sync up all the files.

Filed #128.

+ let { count } = options;
+ let startCount = count();
+ self.setCompatPrefs();
+ notifyObject.restart = startCount != count();

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 10, 2013

Contributor

I believe you want to have a triple operator here?

@whimboo

whimboo Apr 10, 2013

Contributor

I believe you want to have a triple operator here?

+ }
+ sendNotification({count: count});
+ });
+ } catch(e) {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 10, 2013

Contributor

nit: space before the rounded brackets

@whimboo

whimboo Apr 10, 2013

Contributor

nit: space before the rounded brackets

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 10, 2013

Contributor

Except the two nits we are very close to get this landed!

Contributor

whimboo commented Apr 10, 2013

Except the two nits we are very close to get this landed!

Issue #52 - addressing comments, round 6
- obs + restartObserver is relevant only in nightlyApp.openNotification case
- nttAddonCompatibilityService space nits
@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 13, 2013

Collaborator

Added commit 4bf06c6:

  • move obs, restartObserver where they belong
  • space nits

I hope you agree with that move in

Collaborator

xabolcs commented Apr 13, 2013

Added commit 4bf06c6:

  • move obs, restartObserver where they belong
  • space nits

I hope you agree with that move in

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 13, 2013

Collaborator

... nightly.js and we are still very close to get this landed! :)

Collaborator

xabolcs commented Apr 13, 2013

... nightly.js and we are still very close to get this landed! :)

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 15, 2013

Contributor

Looks fine to me. Lets get it landed.

Contributor

whimboo commented Apr 15, 2013

Looks fine to me. Lets get it landed.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 16, 2013

Collaborator

Landed as ae9184f.

Collaborator

xabolcs commented Apr 16, 2013

Landed as ae9184f.

@xabolcs xabolcs closed this Apr 16, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment