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

xabolcs · 2012-06-06T03:40:22Z

Please note the following:

~~it's a draft!~~
it is based on that addon in [force-compatibility] Detect the application version automatically and setup the right compatibility pref #52.
it introduces a new feature: restart on demand - inspired by that addon. :)

…ations

whimboo · 2012-07-02T13:34:09Z

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"].


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.

- renamed 'os' to 'obs' - moved observer code to an object - shortened Components.* - formatted foreign code - renamed preference constants - moved version()

xabolcs · 2012-08-14T00:18:58Z

Added commit 392a9b8 - Addressing comments, round 1.

xabolcs · 2012-08-14T00:28:04Z

extension/components/nttAddonCompatibilityService.js

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


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

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

whimboo · 2013-03-07T20:25:09Z

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 · 2013-03-07T20:56:18Z

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 · 2013-03-08T21:03:40Z

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

whimboo · 2013-03-12T09:04:05Z

extension/components/nttAddonCompatibilityService.js

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


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

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.

- JSONed observer data - version is now a property - space nits - Songbird exclusion note - filtering addons - not inlining long name function yet

- rework setCompatPrefsHelper

xabolcs · 2013-03-14T04:23:48Z

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

xabolcs · 2013-03-14T04:31:04Z

@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.

whimboo · 2013-03-14T08:52:17Z

extension/chrome/content/nightly.js

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


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

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!

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.

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.

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.

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?

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. :(

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.

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

Renamed, but not tested.

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 :)

whimboo · 2013-03-14T08:53:47Z

extension/components/nttAddonCompatibilityService.js

-  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)) {


nit: a blank between if and the opening bracket.

- space nits - inline pending filter

xabolcs · 2013-03-21T00:29:56Z

Pull updated for the nits.
The nightly ↔ nttAddonCompatibilityService private communication is still unclear to me.

- "nttACS" -> "_nttACS"

whimboo · 2013-03-26T10:56:01Z

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 · 2013-03-26T14:39:51Z

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 · 2013-03-27T04:38:40Z

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

- not starting a boolean in isPendingAddon() - space nits

- not starting a boolean in isPendingAddon(), really

xabolcs · 2013-03-27T22:32:08Z

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

- space nit

xabolcs · 2013-04-09T01:02:41Z

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

- indentation

xabolcs · 2013-04-09T16:34:31Z

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

whimboo · 2013-04-10T10:34:35Z

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);


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.

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

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

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

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

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

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

Filed #128.

whimboo · 2013-04-10T10:37:03Z

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

- obs + restartObserver is relevant only in nightlyApp.openNotification case - nttAddonCompatibilityService space nits

xabolcs · 2013-04-13T13:00:18Z

Added commit 4bf06c6:

move obs, restartObserver where they belong
space nits

I hope you agree with that move in

xabolcs · 2013-04-13T13:01:22Z

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

whimboo · 2013-04-15T09:25:18Z

Looks fine to me. Lets get it landed.

xabolcs · 2013-04-16T00:16:01Z

Landed as ae9184f.

xabolcs added 3 commits June 6, 2012 03:00

Issue mozilla#52 - Part 1: updated setCompatPrefs()

c755e07

Issue mozilla#52 - Part 2: on demand restart

f301158

Issue mozilla#52 - Part 3: no need to get application startup notific…

95d4f08

…ations

whimboo reviewed Jul 2, 2012
View reviewed changes

xabolcs mentioned this pull request Jul 13, 2012

aboutNightly component does not implement nsIObserver #57

Closed

#Issue 52 - addressing comments, round 1

392a9b8

- renamed 'os' to 'obs' - moved observer code to an object - shortened Components.* - formatted foreign code - renamed preference constants - moved version()

xabolcs reviewed Aug 14, 2012
View reviewed changes

xabolcs mentioned this pull request Oct 11, 2012

Prepare to release version 3.4. Helps to fix Issue #95. #97

Closed

Merge in v3.4 changes

92e1120

whimboo reviewed Mar 12, 2013
View reviewed changes

xabolcs added 2 commits March 14, 2013 04:14

#Issue 52 - addressing comments, round 2, part 1

f6fef70

- JSONed observer data - version is now a property - space nits - Songbird exclusion note - filtering addons - not inlining long name function yet

#Issue 52 - addressing comments, round 2, part 2

d24465f

- rework setCompatPrefsHelper

whimboo reviewed Mar 14, 2013
View reviewed changes

#Issue 52 - addressing comments, round 3, part 1

593d77f

- space nits - inline pending filter

Issue mozilla#52 - addressing comments, round 3, part 2

e616eda

- "nttACS" -> "_nttACS"

xabolcs added 2 commits March 27, 2013 22:45

Issue mozilla#52 - addressing comments, round 4, part 1

c4eb549

- not starting a boolean in isPendingAddon() - space nits

Issue mozilla#52 - addressing comments, round 4, part 2

0605f66

- not starting a boolean in isPendingAddon(), really

Issue mozilla#52 - addressing comments, round 4, part 3

62388ad

- space nit

Issue mozilla#52 - addressing comments, round 5

94e0e83

- indentation

whimboo reviewed Apr 10, 2013
View reviewed changes

Issue mozilla#52 - addressing comments, round 6

4bf06c6

- obs + restartObserver is relevant only in nightlyApp.openNotification case - nttAddonCompatibilityService space nits

xabolcs mentioned this pull request Apr 13, 2013

Anonymize nightly and nightlyApp implementations to allow Components.* (Cc, Cu, ...) shortcuts be used #128

Closed

xabolcs closed this Apr 16, 2013

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

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

Conversation

xabolcs commented Jun 6, 2012

Choose a reason for hiding this comment

xabolcs commented Aug 14, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whimboo commented Mar 7, 2013

xabolcs commented Mar 7, 2013

xabolcs commented Mar 8, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabolcs commented Mar 14, 2013

xabolcs commented Mar 14, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabolcs commented Mar 21, 2013

whimboo commented Mar 26, 2013

xabolcs commented Mar 26, 2013

tonymec commented Mar 27, 2013

xabolcs commented Mar 27, 2013

xabolcs commented Apr 9, 2013

xabolcs commented Apr 9, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whimboo commented Apr 10, 2013

xabolcs commented Apr 13, 2013

xabolcs commented Apr 13, 2013

whimboo commented Apr 15, 2013

xabolcs commented Apr 16, 2013