713087 - Port |Bug 534956 - Sync add-ons|

Status: RESOLVED FIXED

(SeaMonkey :: Sync UI, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch [Checkin: Comment 6]

Bug 534956 introduced a new Sync engine for add-ons. This bug is to port the UI parts from FF.

Comment 1

[[e-mail]]

Comment on attachment 583913 [details] [diff] [review]
patch [Checkin: Comment 6]

[access keys don't seem to work in listitems]

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #1)
> [access keys don't seem to work in listitems]

Right, found that, too (obviously, the richlistbox FF is using doesn't have that problem). The question is whether we should just remove them hence.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Before I forget: Since this is just making the functionality that is already on Aurora available through UI but cannot land on Aurora due to l10n impact, I wonder whether we can/should provide an add-on (maybe even a restartless one) for 2.8. The Quota part could be ignored (the code seems to support a fallback since FF forget to change syncQuota.properties) so only the Preferences panel and Sync Setup wizard are left (plus the pref change).

Comment 4

[[e-mail]]

(In reply to Jens Hatlak from comment #3)
> Before I forget: Since this is just making the functionality that is already
> on Aurora available through UI but cannot land on Aurora due to l10n impact,
> I wonder whether we can/should provide an add-on (maybe even a restartless
> one) for 2.8.
Wouldn't the add-on try to synchronise itself? Would that be a problem?

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #4)
> Wouldn't the add-on try to synchronise itself? Would that be a problem?

I don't think so, as long as we set min/maxVersion correctly and enable strictCompatibility for that add-on. After all, Add-on Sync does not mess with compatibility checks (I hope). Maybe you could run into issues with the ACR installed, though.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 583913 [details] [diff] [review]
patch [Checkin: Comment 6]

http://hg.mozilla.org/comm-central/rev/3b30e81cd037

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: restartless add-on for SM 2.8

As promised, here is the restartless add-on for SM 2.8. :-)

I only did the Preferences integration (ignoring the Sync Setup part) and didn't include any l10n. On the upside I was able to make sure it only ever gets enabled for SM 2.8, and that a Preferences window that is already open is taken care of.

If you wonder why I don't remove the <preference> element that I add: It's because the following line triggered the following error:

syncPrefs.removeChild(syncPrefs.firstChild);

->

Error: this.preferences is null
Source File: chrome://global/content/bindings/preferences.xml
Line: 122

That's the XBL destructor for preference elements which tries to access this.preferences which resolves to this.parentNode which is apparently already null when the destructor is called. Since I found no way to work around the issue (for the adding case I used createElementNS instead of cloneNode which triggered a similar error in the XBL constructor) I chose to leave the <preference> in (it does no real harm) but make sure it's only added once per Preferences window invocation.

Neil, if you find no other issues, I propose to officially ship this add-on on addons.mozilla.org from our SeaMonkey Council account once 2.8 hits Aurora (January 31). Otherwise I'll upload it using my personal account.

Comment 8

[[e-mail]]

Comment on attachment 590391 [details]
restartless add-on for SM 2.8

(In theory resetting the pref won't suffice; if the prefwindow is open then it will save the pref when you click OK. The workaround is to set readonly="true" on the element, and remove it again when the addon is re-enabled of course!)

>    if (el.id == "engine.addons")
>      return; // already there, don't add again
Why not use document.getElementByID?
Note that in the other prefpanes, we keep the id equal to the name.
I wonder whether we have any other mismatches. Might be worth a test ;-)

>  let newPref = aWin.document.createElementNS(XUL_NS, "preference");
[XUL_NS is the default for that document.]

>    if (syncPane.firstChild) // pane is set up
>      addToSyncPane(aWin);
>    else
>      syncPane.addEventListener("paneload", function() {
>        syncPane.removeEventListener("paneload", arguments.callee, false);
I was going to say "arguments.callee is deprecated"...

>  if (syncPane && syncPane.firstChild) { // pane is set up
>    let syncEnginesList = aWin.document.getElementById("syncEnginesList");
>    syncEnginesList.removeChild(syncEnginesList.firstChild);
>  }
... but the paneload listener will still exist here. Oops.

>    if (aWin.windowtype && aWin.windowtype != prefWindowType)
>      return;
Won't this try to listen to windows without a windowtype?

>    let domWindow = aWin.QueryInterface(Ci.nsIInterfaceRequestor)
>                        .getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);
[Did this change during the 2.8 cycle?]

>  if (!isSM28(aData))
[Doesn't strict compatibility suffice? Should the addon try to disable itself?]

>  Services.prefs.setCharPref("services.sync.registerEngines",
>                             "Bookmarks,Form,History,Password,Prefs,Tab,Addons");
[Hmm, why is Addons last, but first in the UI?]

  // Load into existing Preferences window
>  let wm = Services.wm;
Hardly seems worth it...

>    let win = enumerator.getNext().QueryInterface(Ci.nsIDOMWindow);
Nit: DOM Windows have class info, no need to QI

Too many quibbles on it for me to be able to give you f+.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Attachment: restartless add-on for SM 2.8 v1a

(In reply to [e-mail] from comment #8)
> Note that in the other prefpanes, we keep the id equal to the name.
> I wonder whether we have any other mismatches. Might be worth a test ;-)

I think in this case it's for FF compatibility so let's not unnecessarily mess with it. We could look for a general test with exceptions like this, though.

> >    if (syncPane.firstChild) // pane is set up
> >      addToSyncPane(aWin);
> >    else
> >      syncPane.addEventListener("paneload", function() {
> >        syncPane.removeEventListener("paneload", arguments.callee, false);
> I was going to say "arguments.callee is deprecated"...
> 
> >  if (syncPane && syncPane.firstChild) { // pane is set up
> >    let syncEnginesList = aWin.document.getElementById("syncEnginesList");
> >    syncEnginesList.removeChild(syncEnginesList.firstChild);
> >  }
> ... but the paneload listener will still exist here. Oops.

Umm, so do I need to change anything there?

> >    if (aWin.windowtype && aWin.windowtype != prefWindowType)
> >      return;
> Won't this try to listen to windows without a windowtype?

Wow, this is embarrassing. Not only did I get the check wrong (which is why it kind of worked), I also checked a non-existent property! :-o One actually needs to use getAttribute(). I was confused by MDN's window (XUL) docs. Next time I check MXR first.

> >    let domWindow = aWin.QueryInterface(Ci.nsIInterfaceRequestor)
> >                        .getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);
> [Did this change during the 2.8 cycle?]

C&p mistake from some sample code which included this for downward compatibility. nsIDOMWindowInternal is deprecated since Gecko 8 (SM 2.5). Removed.

> >  if (!isSM28(aData))
> [Doesn't strict compatibility suffice? Should the addon try to disable
> itself?]

Normally, strict compat should suffice, but I don't want to get caught off guard by ACR, Nightly Tester Tools, changed prefs etc. Better safe than sorry! :-)

> >  Services.prefs.setCharPref("services.sync.registerEngines",
> >                             "Bookmarks,Form,History,Password,Prefs,Tab,Addons");
> [Hmm, why is Addons last, but first in the UI?]

Because it was added last I guess. Probably my mistake, but that's how it is on trunk. rs=me if you want to change the order before it transitions to branches. ;-)

Thanks for looking at this!

Comment 10

[[e-mail]]

(In reply to Jens Hatlak from comment #9)
> (In reply to comment #8)
> > Note that in the other prefpanes, we keep the id equal to the name.
> > I wonder whether we have any other mismatches. Might be worth a test ;-)
> I think in this case it's for FF compatibility so let's not unnecessarily
> mess with it. We could look for a general test with exceptions like this,
> though.
Hmm, well, our prefwindow isn't that compatible with Fx, is it? I suppose this pane is a bit of an exception.

> > >    if (syncPane.firstChild) // pane is set up
> > >      addToSyncPane(aWin);
> > >    else
> > >      syncPane.addEventListener("paneload", function() {
> > >        syncPane.removeEventListener("paneload", arguments.callee, false);
> > I was going to say "arguments.callee is deprecated"...
> > 
> > >  if (syncPane && syncPane.firstChild) { // pane is set up
> > >    let syncEnginesList = aWin.document.getElementById("syncEnginesList");
> > >    syncEnginesList.removeChild(syncEnginesList.firstChild);
> > >  }
> > ... but the paneload listener will still exist here. Oops.
> Umm, so do I need to change anything there?
Well, by my inspection, if you disable the addon while the prefwindow is open but before you look at the Sync pane, then you will still get the checkbox when you do open the Sync pane.

> > >    let domWindow = aWin.QueryInterface(Ci.nsIInterfaceRequestor)
> > >                        .getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);
> > [Did this change during the 2.8 cycle?]
> C&p mistake from some sample code which included this for downward
> compatibility.
[It's annoying that nsXULWindow required the Internal interface...]

> > >  if (!isSM28(aData))
> > [Doesn't strict compatibility suffice? Should the addon try to disable
> > itself?]
> Normally, strict compat should suffice, but I don't want to get caught off
> guard by ACR, Nightly Tester Tools, changed prefs etc. Better safe than
> sorry! :-)
Fair enough.

> > >  Services.prefs.setCharPref("services.sync.registerEngines",
> > >                             "Bookmarks,Form,History,Password,Prefs,Tab,Addons");
> > [Hmm, why is Addons last, but first in the UI?]
> Because it was added last I guess.
That and Firefox uses the same order...

Comment 11

[[e-mail]]

Comment on attachment 590554 [details]
restartless add-on for SM 2.8 v1a

As well as the disabling while prefwindow open issue still open from above, just some minor queries I noticed while rereading the code:

>  syncEnginesList.insertBefore(newEngine, syncEnginesList.firstChild);
Does this update the checkbox automatically, even when repeatedly disabling and enabling the add-on with the sync pane open?

>  if (!aWin)
>    return;
[Don't see how aWin can be null here. Or again below.]

>  if (appInfo.ID != SEAMONKEY_ID ||
>      versionChecker.compare(appInfo.version, "2.8a") < 0 ||
>      versionChecker.compare(appInfo.version, "2.9a") >= 0) {
>    Components.utils.reportError(aData.id + " requires SeaMonkey 2.8; aborting.");
>    return false;
>  }
> 
>  return true;
Very slightly nicer way to write this:
if (appInfo.ID == SEAMONKEY_ID &&
    versionChecker.compare(appInfo.version, "2.9a") < 0 &&
    versionChecker.compare(appInfo.version, "2.8a") >= 0)
  return true;

Components.utils.reportError(aData.id + " requires SeaMonkey 2.8; aborting.");
return false;

>      if (!windowtype || windowtype != prefWindowType)
>        return;
>      loadIntoWindow(domWindow);
Could be just
if (windowtype == prefWindowType)
  loadIntoWindow(domWindow);

>  // Set prefs
>  Services.prefs.setCharPref("services.sync.registerEngines",
>                             "Bookmarks,Form,History,Password,Prefs,Tab,Addons");
Are bootstrapped addons supposed to set user or default pref values?

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Attachment: restartless add-on for SM 2.8 v1b

Callek, can you please have a quick look at the included install.rdf and check whether the information included (author etc.) is OK from a Council POV? Thanks!

(In reply to [e-mail] from comment #10)
> > > I was going to say "arguments.callee is deprecated"...

Replaced, following the approach in syncUI.js.

> Well, by my inspection, if you disable the addon while the prefwindow is
> open but before you look at the Sync pane, then you will still get the
> checkbox when you do open the Sync pane.

Oh, right. Fixed.

(In reply to [e-mail] from comment #11)
> >  syncEnginesList.insertBefore(newEngine, syncEnginesList.firstChild);
> Does this update the checkbox automatically, even when repeatedly disabling
> and enabling the add-on with the sync pane open?

Hmm, no. Fixed by explicitly setting the checked attribute according to the pref (which always exists BTW, probably because the platform sets it somewhere - after all, the whole point of this add-on is that the SM 2.8 back-end already provides everything we need!).

> >  if (!aWin)
> >    return;
> [Don't see how aWin can be null here. Or again below.]

Can't harm either. Better safe than sorry.

> Are bootstrapped addons supposed to set user or default pref values?

Comments suggest that you can do either but explicitly removing/resetting default prefs is non-trivial (they'll disappear at app shutdown anyway, though):

* http://starkravingfinkle.org/blog/2011/01/restartless-add-ons-%E2%80%93-default-preferences/
* bug 564675

But I see you've been involved in both discussions already so you probably know better than me. ;-)

Comment 13

[Justin Wood (:Callek)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #12)
> Created attachment 590593 [details]
> restartless add-on for SM 2.8 v1b
> 
> Callek, can you please have a quick look at the included install.rdf and
> check whether the information included (author etc.) is OK from a Council
> POV? Thanks!
> 

Don't suppose you can make my life easier and point at a user repo for this file, or upload the install.rdf here for me?

Comment 14

[[e-mail]]

(In reply to Justin Wood from comment #13)
> (In reply to Jens Hatlak from comment #12)
> > Callek, can you please have a quick look at the included install.rdf
> Don't suppose you can make my life easier and point at a user repo for this
> file, or upload the install.rdf here for me?
jar:https://bug713087.bugzilla.mozilla.org/attachment.cgi?id=590593!/install.rdf

But you might have to set the network.jar.open-unsafe-types pref, as Jens has been attaching them as application/octet-stream instead of (say) /x-jar.

Comment 15

[[e-mail]]

Comment on attachment 590593 [details]
restartless add-on for SM 2.8 v1b

>  // Reset prefs
>  Services.prefs.setCharPref
[Any reason not to clear that one too?]

Comment 16

[Justin Wood (:Callek)]

Comment on attachment 590593 [details]
restartless add-on for SM 2.8 v1b

Looks good to me as well

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #15)
> >  Services.prefs.setCharPref
> [Any reason not to clear that one too?]

No, will do.

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

The restartless add-on can now be found here, creator SeaMonkey Council:
https://addons.mozilla.org/addon/add-ons-sync-prefs/

I slightly renamed it to "Add-ons Sync Prefs" before uploading (where I first ran into bug 720322). I put the add-on up for full review (can take up to 10 days), included English and German descriptions and some tags.

Possible improvements regarding AMO:
* add icon and/or screen shot
* tune other settings that affect the listing
* add more l10n

Since this was my first restartless add-on and I learned from it, I'll also create a blog post soon (but not today anymore). :-)

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Fully update packaging wrt |Bug 534956 - Sync add-ons| [Moved to bug 774265]

(Too bad for 2.9.)

Comment 20

[Justin Wood (:Callek)]

Comment on attachment 627177 [details] [diff] [review]
(Cv1) Fully update packaging wrt |Bug 534956 - Sync add-ons|
[Moved to bug 774265]

...maybe not needed [and prefer a new bug for this one]

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to Justin Wood (:Callek) from comment #20)
> ...maybe not needed [and prefer a new bug for this one]

Bug 774265 Submitted