571897 - port Sync 1.4 UI for Firefox to mozilla-central

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mike Connor [:mconnor]]

We've split out the UI for Firefox in current hg tip.  There's some pending changes to follow the Fx4 design direction (removal of statusbar UI being the primary) and better support the iPhone client, so this isn't 100% ready, but we're almost there.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Theme Patch v0.1

I kept all the theme stuff together so it would be easier to look at. There is a lot of duplication & I think I might still be missing a style or 2. All images are there. They have been renamed from their counterparts in the fx-sync repo.

I know of at least 1 followup for theme related stuff - create a shaded sync icon & update Options.png with the 2 versions. It isn't filed, but I haven't forgotten about it.

I haven't stripped color profiles or any of that jazz. I'm not familiar with the process, and I'm not even sure if it needs to be done.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Everything Else Patch v0.1

Everything else... I've got it separated locally into "sequential" chunks (about:sync-tabs, pref pane, setup wizard, et al.), but it's easier for me to request feedback this way. For real review I'll break it up

I left in all of my XXXzpao comments. Some of them are just "delete this because we don't need it". Others are other thoughts I had as I went through the code.

There are a few missing #ifdef MOZ_SERVICES_SYNC - I know about those.

A bit of this is just copy + paste + make work. Other parts went through a big refactor.

I plan on going through all of it again, but I wanted to get some eyeballs on this ASAP. I'll be able to make edits through Summit, but after that I'm out of the country for 2 weeks, so won't be available. I'd like to have major kinks worked out by then & be able to put r? on this. That way if somebody else needs to pick it up, the work is minimal.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I should also note that my build hasn't been updated in over a week, so there may be some fun merge action.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 455808 [details] [diff] [review]
Theme Patch v0.1

None of the -moz-bindings belong in themes.

>+#tabs-display, #tabsList {

\n after comma (throughout the patch)

>+#tabsList {
>+ width: 100%;

indentation is off

You hardcoded a bunch of hex values which should probably platform colors instead.

Comment 5

[Dão Gottwald [:dao]]

The largest glitch I've seen when skimming the patch was _handleNoScript. We need to provide hooks for extensions to do their work, not the other way around.

Comment 6

[Mike Connor [:mconnor]]

I think that's a fine opinion, and if/when NoScript issues an update that doesn't break registration, great.  In the meantime, breaking anyone running NoScript probably isn't a win for us.

Comment 7

[Dão Gottwald [:dao]]

Letting NoScript break a built-in feature (as opposed to an add-on) is not an issue. It means that NoScript needs an update to work with Firefox 4. That's business as usual.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 1: Theme (v0.2)

Theme sans -moz-binding stuff

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 2: Prefs (v0.1)

The prefs

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 3: General hookup (v0.1)

Some of the hookups in browser.js

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 4: about:sync-tabs (v0.1)

everything for about:sync-tabs

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 5: Setup wizard (v0.1)

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 6: Pref pane (v0.1)

Comment 14

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 7: Generic change dialog (v0.1)

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 8: Tools menu (v0.1)

Also has the all-tabs menu entry for about:sync-tabs... I'm thinking the other menu entry that is currently in HistoryMenu (part 4) might just belong with these, though all other things in the history menu are in there, so...

Comment 16

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 3: General hookup (v0.1)

(posted wrong patch)

Dao - can you take a look at these? Not sure if you're the right person for all of this, but if you could kick it off and look at these patches, that would be really helpful. Thanks!

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 458095 [details] [diff] [review]
Part 3: General hookup (v0.1)

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+XPCOMUtils.defineLazyGetter(this, "Weave", function() {

Shouldn't this be "Sync"?

>@@ -1489,16 +1497,36 @@ function delayedStartup(isLoadingBlank, 

>+#ifdef MOZ_SERVICES_SYNC
>+  // Log in to Weave. Weave.Service.delayedAutoConnect checks for logged in as
>+  // well, but check here so we don't do unneccessary enumeration.
>+  if (!Weave.Service._loggedIn) {
>+    let wait = 5; // delay in seconds
>+    //XXXzpao is this going to have any effect if this is the first window?
>+    //        alternatively we can just set services.sync.autoconnectDelay
>+    //XXXzpao Perhaps we should avoid using Weave.Svc.* & replace those (at least
>+    //        in /browser) with Services.*
>+    // Add one second delay for each busy tab in every window
>+    let enum = Weave.Svc.WinMediator.getEnumerator("navigator:browser");

use Services.wm

>+    while (enum.hasMoreElements()) {
>+      Array.forEach(enum.getNext().gBrowser.mTabs, function(tab) {

use gBrowser.tabs

>+        wait += tab.hasAttribute("busy");
>+      });
>+    }
>+    Weave.Service.delayedAutoConnect(wait);

The whole heuristic seems a little fragile if not plain wrong for multiple windows...

Comment 18

[Dão Gottwald [:dao]]

You shouldn't add stuff to the status bar, we want to do away with it asap.

Comment 19

[Philipp von Weitershausen [:philikon]]

(In reply to comment #17)
> >+XPCOMUtils.defineLazyGetter(this, "Weave", function() {
> 
> Shouldn't this be "Sync"?

Well, the object exposed by resource://services-sync/service.js is called Weave. Of course it doesn't have to be called the same way in the XUL namespace, but it might be a bit confusing perhaps.

> >+    while (enum.hasMoreElements()) {
> >+      Array.forEach(enum.getNext().gBrowser.mTabs, function(tab) {
> 
> use gBrowser.tabs

Also note that this particular spelling of this "algorithm" accesses gBrowser too early in some cases, breaking XUL overlays from extensions that add buttons to the toolbar. See bug 577349.

> >+        wait += tab.hasAttribute("busy");
> >+      });
> >+    }
> >+    Weave.Service.delayedAutoConnect(wait);
> 
> The whole heuristic seems a little fragile if not plain wrong for multiple
> windows...

Admitted it's a bit crude (though how is it wrong for multiple windows?). The intent is obviously not to have Sync spend CPU cycles and cause traffic while the browser is still restoring.

Comment 20

[Mike Connor [:mconnor]]

(In reply to comment #18)
> You shouldn't add stuff to the status bar, we want to do away with it asap.

Is there a new place to display error indicators yet?  We can happily add in there, but the 1.4 release demonstrated that we need some form of primary UI indicator for problems with Sync.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #19)
> Admitted it's a bit crude (though how is it wrong for multiple windows?).

Because you might get not all the windows depending on how the session restore internals work. I'm pretty sure you won't get /all loading tabs/ in all windows, otherwise you wouldn't have bug 577349.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #20)
> (In reply to comment #18)
> > You shouldn't add stuff to the status bar, we want to do away with it asap.
> 
> Is there a new place to display error indicators yet?  We can happily add in
> there, but the 1.4 release demonstrated that we need some form of primary UI
> indicator for problems with Sync.

How about the alert service?

Comment 23

[Robert Kaiser]

(In reply to comment #22)
> How about the alert service?

The alert service is good for anything that can go away after a few seconds and can then be ignored. A connection loss of Sync may not count in that space.

Comment 24

[Philipp von Weitershausen [:philikon]]

(In reply to comment #21)
> (In reply to comment #19)
> > Admitted it's a bit crude (though how is it wrong for multiple windows?).
> 
> Because you might get not all the windows depending on how the session restore
> internals work. I'm pretty sure you won't get /all loading tabs/ in all
> windows, otherwise you wouldn't have bug 577349.

Indeed, that's why we should at least be waiting a bit before looking through all tabs in all windows. As said, it's a bit crude and I wouldn't mind scrapping that bit of code at all, as long as we can ensure not to make session restore slower.

Comment 25

[Johnathan Nightingale [:johnath]]

mconnor - how many of these can you give a once-over to, to expedite reviews and give Paul a deep queue of review comments to follow up on when he gets back?

Comment 26

[Axel Hecht [:Pike]]

Once we switch on services/sync in the build, we actually need to make sure that the strings are shown in the face of l10n, which bug 579175 is gonna do.

Comment 27

[Johnathan Nightingale [:johnath]]

Moving this to Firefox since the patch is against mozilla-central, and since it lets me mark this as a blocker.

Comment 28

[u60234]

Comment on attachment 458087 [details] [diff] [review]
Part 6: Pref pane (v0.1)

Some localization comments:

>--- a/browser/locales/en-US/chrome/browser/preferences/preferences.dtd
>+++ b/browser/locales/en-US/chrome/browser/preferences/preferences.dtd
>+
>+#ifdef MOZ_SERVICES_SYNC
>+<!-- This should match syncBrand.shortName.label in ../syncBrand.dtd -->
>+<!ENTITY  paneSync.title          "Sync">
>+#endif
>\ No newline at end of file

Preprocessor macros are usually unwanted in l10n files according to https://developer.mozilla.org/en/Writing_localizable_code#Guidelines
Do you really need to use it here? Same thing in preferences.properties and browser.dtd.

>--- a/browser/locales/en-US/chrome/browser/preferences/preferences.properties
>+++ b/browser/locales/en-US/chrome/browser/preferences/preferences.properties

>+additionalClients.label = and %S additional devices
>+
>+bookmarkCount.label = %S bookmarks
>+historyCount.label  = %S days of history
>+passwordCount.label = %S passwords

Some languages needs additional plural forms here. See https://developer.mozilla.org/en/Localization_and_Plurals
Also, a note about the context in which the additionalClients.label is used would be nice to have.
Same thing in syncSetup.properties

Comment 29

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #28)
> Preprocessor macros are usually unwanted in l10n files according to
> https://developer.mozilla.org/en/Writing_localizable_code#Guidelines
> Do you really need to use it here? Same thing in preferences.properties and
> browser.dtd.

Most of the preprocessor uses are there because sync is a build option (for now anyway). I'm not sure if that's important at all for l10n (there shouldn't be entity collision, so it's not likely to matter except for l10n file sizes)

> Some languages needs additional plural forms here. See
> https://developer.mozilla.org/en/Localization_and_Plurals

I had forgotten about that. I had just pulled most of the l10n from the add-on verbatim, but things like this should be fixed.

> Also, a note about the context in which the additionalClients.label is used
> would be nice to have.
> Same thing in syncSetup.properties

Good idea.

Comment 30

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I'm also using #include in 2 places so that a sizable chunk of shared entities are only written once. The alternative is a larger dtd for some sync stuff or confusing uses of string bundles. I thought my way was "cleaner" but perhaps doesn't follow those guidelines.

Comment 31

[Axel Hecht [:Pike]]

Preprocessor markup will be rejected by our l10n infrastructure, we shouldn't use that.

Also, re-using strings isn't always safe, it'd require that the space considerations and the context are the same in all use cases of a string. Quite generally, even though the en-US strings are the same, they may not be in localizations, due to different context. Either how they're used, or how bad the localizer needs to compromise on visual space.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 458095 [details] [diff] [review]
Part 3: General hookup (v0.1)

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+    //XXXzpao Perhaps we should avoid using Weave.Svc.* & replace those (at least
>+    //        in /browser) with Services.*

Yes, we should. All of the Weave.Svc.* stuff should be ripped out, IMO (along with all other "utility" code that already has a suitable alternative in mozilla-central).

>+    while (enum.hasMoreElements()) {
>+      Array.forEach(enum.getNext().gBrowser.mTabs, function(tab) {
>+        wait += tab.hasAttribute("busy");
>+      });
>+    }

wait += Array.filter(gBrowser.tabs, function (t) t.hasAttribute("busy")).length;

Wouldn't this code be better placed in a sessionstore-windows-restored observer, rather than in delayed-startup? In fact as it is it seems like we'd end up queuing multiple sign-ins in the case where we're restoring multiple windows during startup (not sure if the weave code handles that sanely).

Comment 33

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #32)
> Wouldn't this code be better placed in a sessionstore-windows-restored
> observer, rather than in delayed-startup?

That thought crossed my mind yesterday actually. I think it's a better idea.

However, the imported sync module will look at a sync pref the first time it's imported and autoconnect on a delay if it's set (sync.autoconnectDelay I think). It's not set by default, but could cause some issues if it is (though I don't think it should be too hard to find out, can always just look at the pref)

> In fact as it is it seems like we'd
> end up queuing multiple sign-ins in the case where we're restoring multiple
> windows during startup (not sure if the weave code handles that sanely).

It seems like it should handle it sanely (or at least mostly) but even so I agree that it would be best to avoid doing so.

Comment 34

[Justin Dolske [:Dolske]]

(In reply to comment #32)

> wait += Array.filter(gBrowser.tabs, function (t)
> t.hasAttribute("busy")).length;

Or just assume that since this is running right at startup, that all tabs are busy and so don't bother with the attribute check.

Comment 35

[Justin Dolske [:Dolske]]

Comment on attachment 458083 [details] [diff] [review]
Part 2: Prefs (v0.1)


>+// Preferences to be synced by default

How does Weave resolve pref sync conflicts? EG, If I change pref.foo.bar on both my mobile and desktop and then sync, which wins?

Does Weave only sync the prefs that have been changed from their default values? EG, what happens when we change the default value for a new release?

>+pref("services.sync.prefs.sync.browser.history_expire_days", true);
>+pref("services.sync.prefs.sync.browser.history_expire_days_min", true);

These are dead now.

>+pref("services.sync.prefs.sync.browser.tabs.tabMaxWidth", true);
>+pref("services.sync.prefs.sync.browser.tabs.tabMinWidth", true);

I sort of wonder if we should be syncing prefs not exposed in the UI...

>+pref("services.sync.prefs.sync.extensions.personas.current", true);

This pref is dead, the value in my profile seems to be stuck from the last time I used the Personas extension.

>+pref("services.sync.prefs.sync.security.enable_java", true);

Also dead (bug 506985, bug 513979)

Comment 36

[Dão Gottwald [:dao]]

(In reply to comment #35)
> >+pref("services.sync.prefs.sync.browser.tabs.tabMaxWidth", true);
> >+pref("services.sync.prefs.sync.browser.tabs.tabMinWidth", true);
> 
> I sort of wonder if we should be syncing prefs not exposed in the UI...

These are dead too.

Comment 37

[Philipp von Weitershausen [:philikon]]

(In reply to comment #34)
> > wait += Array.filter(gBrowser.tabs, function (t)
> > t.hasAttribute("busy")).length;
> 
> Or just assume that since this is running right at startup, that all tabs are
> busy and so don't bother with the attribute check.

It actually didn't run at startup but after a delay. This was taken out in Sync 1.4 by mistake (bug 570573) but added back in 1.4.2 (bug 577349) since it caused gBrowser being accessed too early.

(In reply to comment #32)
> Wouldn't this code be better placed in a sessionstore-windows-restored
> observer, rather than in delayed-startup? In fact as it is it seems like we'd
> end up queuing multiple sign-ins in the case where we're restoring multiple
> windows during startup (not sure if the weave code handles that sanely).

It does, but a sessionstore-windows-restored observer seems saner indeed, especially if that would make sure not accessing gBrowser too early (provided we stick with the tab delay heuristics).

(In reply to comment #35)
> How does Weave resolve pref sync conflicts? EG, If I change pref.foo.bar on
> both my mobile and desktop and then sync, which wins?

Sync's conflict resolution will pick the later one.

> Does Weave only sync the prefs that have been changed from their default
> values? EG, what happens when we change the default value for a new release?

It doesn't right now, but this has been raised before and I think it should. I have filed bug 582536 for this.

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 458095 [details] [diff] [review]
Part 3: General hookup (v0.1)

See comments 17-34, this isn't quite there yet.

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 458082 [details] [diff] [review]
Part 1: Theme (v0.2)

There's a bunch of seemingly unused images. What's going on with these?

Comment 40

[Dão Gottwald [:dao]]

(In reply to comment #19)
> (In reply to comment #17)
> > >+XPCOMUtils.defineLazyGetter(this, "Weave", function() {
> > 
> > Shouldn't this be "Sync"?
> 
> Well, the object exposed by resource://services-sync/service.js is called
> Weave.

Can we change this?

Comment 41

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #39)
> Comment on attachment 458082 [details] [diff] [review]
> Part 1: Theme (v0.2)
> 
> There's a bunch of seemingly unused images. What's going on with these?

I'm pretty sure I went through and added only images being used. Many aren't used directly in CSS but as <image>s.

Comment 42

[Philipp von Weitershausen [:philikon]]

(In reply to comment #40)
> > Well, the object exposed by resource://services-sync/service.js is called
> > Weave.
> 
> Can we change this?

Apart from the fact that it would be a pretty big and (unless we also continue to export it as "Weave") backwards-incompatible change for other add-ons and Fennec, I wonder whether it's necessary. It's not like there aren't other code names (places, satchel, ...) in the codebase.

Comment 43

[Dão Gottwald [:dao]]

Comment on attachment 458085 [details] [diff] [review]
Part 4: about:sync-tabs (v0.1)

>+++ b/browser/base/content/aboutSyncTabs.js

>+    Weave.Svc.Obs.add("weave:service:login:finish", this);
>+    Weave.Svc.Obs.add("weave:engine:sync:finish",   this);

>+    let uri = Weave.Utils.makeURI(item.getAttribute("url"));

>+      let lastFetch = Weave.Svc.Prefs.get("lastTabFetch", 0);

>+    Weave.Svc.Prefs.set("lastTabFetch", Math.floor(Date.now() / 1000));

You should use Services.jsm here as well.

>+    //XXXzpao Seems like this should be easier...
>+    let win = getTopWin();
>+    let tabs = win.gBrowser.mTabs;
>+    for (let i = 0; i < tabs.length; i++) {
>+      if (tabs[i].linkedBrowser.currentURI.spec == "about:sync-tabs")
>+        win.gBrowser.setIcon(tabs[i], "chrome://browser/skin/sync-16.png");

<html:link rel="icon" .../>?

>+      client.tabs.forEach(function({title, urlHistory, icon}) {
...
>+          let clientEnt = RemoteTabViewer.createItem(attrs);
...
>+        let tab = RemoteTabViewer.createItem(attrs);
>+        list.appendChild(tab);
>+      });

Make this client.tabs.forEach(..., this); and use this instead of RemoteTabViewer.

>+            //XXXzpao Would it be better to just do this via CSS?
>+            icon: Weave.Clients.isMobile(client.id) ? "chrome://browser/skin/sync-mobileIcon.png"
>+                                                    : "chrome://browser/skin/sync-desktopIcon.png",
>+          };

Yes, set a class or attribute or something and let the the CSS assign the image.

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
[...]
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
>+                                         Ci.nsISupportsWeakReference]),

I think you can get rid of this.

>+++ b/browser/base/content/aboutSyncTabs.xul

>+    <hbox align="center" style="background-color: transparent;">

?

>+      <hbox id="headers" align="center">
>+        <image src="chrome://browser/skin/sync-32.png"
>+               height="32" width="32"/>

CSS

Comment 44

[Dão Gottwald [:dao]]

(In reply to comment #41)
> Many aren't used directly in CSS but as <image>s.

That's probably a bug in most cases.

(In reply to comment #42)
> > > Well, the object exposed by resource://services-sync/service.js is called
> > > Weave.
> > 
> > Can we change this?
> 
> Apart from the fact that it would be a pretty big and (unless we also continue
> to export it as "Weave") backwards-incompatible change for other add-ons and
> Fennec, I wonder whether it's necessary. It's not like there aren't other code
> names (places, satchel, ...) in the codebase.

It's not that we couldn't have code names, but Sync itself is a code name too. It seems that we should clean this up now rather than dragging it along forever. We haven't shipped this in Firefox yet, so I'm not really concerned about add-on compatibility.

Comment 45

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 3: General hookup (v0.2)

now hooks into _onBrowserStartup in BrowserGlue (sooo in an observer for "sessionstore-windows-restored"

Comment 46

[Justin Dolske [:Dolske]]

Comment on attachment 458089 [details] [diff] [review]
Part 8: Tools menu (v0.1)

r- mostly just for volume of nits, but I'd like to make one more pass to verify that your sync.js --> browser-syncui.js rewrite didn't miss/break anything.

>+++ b/browser/base/content/browser-menubar.inc
...
>+#ifdef MOZ_SERVICES_SYNC
>+              <!-- only one of sync-menu or sync-setup will be showing at once -->
>+              <!-- XXXzpao probably don't want menu-iconic & image... -->

Agreed. Windows may end up icons in the Firefox button, but we'll let UX sort that out once the updated menu structure is done.

Remove this XXX comment and the other commented out attributes here.

Might want to move the sync-setup to before sync-menu? The comment + shorter sync-setup menu makes looking at the whole group a bit easier. Not a big deal though.

>+                <menupopup id="sync-menu-popup"
>+                           onpopupshowing="if (event.target == this) gSyncUI.doPopup(event);">

This seems to be the only place doPopup is called, a less generic name would be preferable. updateMenuItems() or something like that.

All of the added <menuitems> should have accesskeys, as well as the parent <menu>. (Other than sync-lastsyncitem, since it's just informational).


>+++ b/browser/base/content/browser-syncui.js
...
>+# The Initial Developer of the Original Code is Mozilla.

"the Mozilla Foundation." (here and anywhere else)


>+    let addRem = function(add) {
>+      obs.forEach(function([topic, func]) {
>+        Weave.Svc.Obs[add ? "add" : "remove"](topic, self[func], self);
>+      });
>+    };

That would be a little less obfuscated as:
          if (add)
            Weave.Svc.Obs.add(topic, self[func], self);
          else
            Weave.Svc.Obs.remove(topic, self[func], self);

>+    //XXXzpao this could be moved into an uninit function...
>+    window.addEventListener("unload", function() addRem(false), false);

Eh, fine either way. Remove the comment and either ignore it, do it, or file a followup. :)

>+    // Find the alltabs-popup, only if there is a gBrowser
>+    if (gBrowser) {

Hmm, why |if (gBrowser)| here, but |window.location.href == getBrowserURL()| above? They're testing for the same thing, no?

>+      popup.addEventListener("popupshowing", function() {
>+        self.alltabsPopupShowing();
>+      }, true);

tabbrowser populates these upon popupshowing as well, theoretically we're racing it but I'm not sure there's a better way to do this (or if a race can actually be triggered).


>+  //XXXzpao these strings should be moved from /services to /browser... but for now just make it work

File a followup bug. Should make this changes ASAP, or the localizers will be grumpy.

>+  get _stringBundle() {

XPCOMUtils.defineLazyGetter ?


>+  // getters for elements
>+  _els: {},
>+  _el: function(id) {
>+    if (!this._els[id]) {
>+      this._els.__defineGetter__(id, function() {
>+        return document.getElementById(id);
>+      });
>+    }
>+    return this._els[id];
>+  },

Ick. This code make me cringe for about 4 different reasons. Nuke this and just use "document.getElementById" where needed.

Or is the caching needed for some performance reason I'm missing?

>+  updateMenus: function SUI_updateMenus() {
>+    let needsSetup = this._needsSetup();
>+    // document.getElementById("sync-setup").hidden = !needsSetup;
>+    // document.getElementById("sync-menu").hidden = needsSetup;
>+    this._el("sync-setup").hidden = !needsSetup;
>+    this._el("sync-menu").hidden = needsSetup;

2 of these lines can be deleted!


>+  alltabsPopupShowing: function(event) {
>+    // We need to create the menuitem & seperator manually

Nit: This comment could just be nuked, I don't think it contributes anything.

>+    let uri = Services.io.newURI("about:sync-tabs", null, null);
>+    if (gBrowser.browsers.some(function(b) b.currentURI.equals(uri)))
>+      return;

It seems odd for this UI to not show up if you happen to have about:sync-tabs open somewhere else in the window. Users are likely to think it's broken and only appears randomly. Unless this somehow breaks sync, this check should go away.

>+    let menuitem = this.el("menuitem");

That's just confusing,  due to the caching.

>+    menuitem.setAttribute("id", "sync-tabs-menuitem");
>+    menuitem.setAttribute("label", label);
>+    menuitem.setAttribute("class", "menuitem-iconic alltabs-item");
>+    menuitem.setAttribute("image", "chrome://browser/content/sync-16.png");

Don't think we want the icons here either. Especially since everything else in the menu has an icon (favicon), which makes this look like a tab.

>+    // Fake the tab object on the menu entries, so that we don't have to worry
>+    // about removing them ourselves. They will just get cleaned up by popup
>+    // binding. This also makes sure the statusbar updates with the URL.
>+    menuitem.tab = { "linkedBrowser": { "currentURI": { "spec": label } } };
>+    sep.tab = { "linkedBrowser": { "currentURI": { "spec": " " } } };

Hmm. File a followup to avoid this hack, we should:
  - have tabbrowser.xml's _createTabMenuItem() set the URL directly as a property on the menuitem (well, more likely use a getter for performance)
  - have this code set the URL directly
  - have tabbrowser.xml's DOMMenuItemActive use said property instead of tab.linkedBrowser


>+  // Commands
>+  doPopup: function SUI_doPoup(event) {
>+    this._updateLastSyncItem();
>+
>+    // let loginItem = document.getElementById("sync-loginitem");
>+    // let logoutItem = document.getElementById("sync-logoutitem");
>+    // let syncItem = document.getElementById("sync-syncnowitem");
>+    let loginItem = this._el("sync-loginitem");
>+    let logoutItem = this._el("sync-logoutitem");
>+    let syncItem = this._el("sync-syncnowitem");

3 of these lines can be deleted!


>+  //XXXzpao should be part of syncCommon.js - which we might want to make a module...

File followup, remove comment.


>+  // Helpers
>+  _updateLastSyncItem: function SUI__updateLastSyncItem() {
...
>+    //XXXzpao why wouldn't this exist?
>+    let lastSyncItem = this._el("sync-lastsyncitem");
>+    // if (!lastSyncItem)
>+    //   return;

It should, remove commented-out code.


>+  _onSyncEnd: function SUI__onSyncEnd(success) {
...
>+      if (!Weave.Status.enforceBackoff) {

The Weave extension has code right above this to check for the version being outdated, why isn't that here too? [It opens the EM, but we'd want this for the case of the app being way out of date / unsupported.]


>+++ b/browser/base/content/browser.js
...
>+#ifdef MOZ_SERVICES_SYNC
>+#include browser-syncui.js
>+#endif

Sigh. Would be nice to kill all these #includes some day, refactor browser.js, and obtain a pony.


>     Weave.Service.delayedAutoConnect(wait);
>   }
> #endif
> 
>+#ifdef MOZ_SERVICES_SYNC
>+  // initialize the sync UI
>+  gSyncUI.init();
>+#endif

I think you're already right below a MOZ_SERVICES_SYNC block here, merge them.

Worth a followup bug to move gSyncUI.* into Weave.* somewhere?


>+++ b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY logInItem.label      "Connect">
>+<!ENTITY logOutItem.label     "Disconnect">
>+<!ENTITY syncNowItem.label    "Sync Now">
>+<!ENTITY openPrefsItem.label  "Preferencesâ¦">
>+<!ENTITY openLogItem.label    "Activity Log">
>+<!ENTITY status.offline.label "Offline">

Worth renaming these to syncFoo.label? Probably obvious from the context, though.

But do change status.offline.label changed to syncStatus.offline.label, since it's overly generic.

Comment 47

[Ed Lee :Mardak]

(In reply to comment #46)
> >+++ b/browser/locales/en-US/chrome/browser/browser.dtd
> >+<!ENTITY openLogItem.label    "Activity Log">
This string is probably unused !

Comment 48

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #46)
> >+      popup.addEventListener("popupshowing", function() {
> >+        self.alltabsPopupShowing();
> >+      }, true);
> 
> tabbrowser populates these upon popupshowing as well, theoretically we're
> racing it but I'm not sure there's a better way to do this (or if a race can
> actually be triggered).

So I guess it could race, but at least this is inserting before the first child whereas tabbrowser is appending children. I looked into hooking this into the binding in tabbrowser, but it didn't seem quite right.

> >+  //XXXzpao these strings should be moved from /services to /browser... but for now just make it work
> 
> File a followup bug. Should make this changes ASAP, or the localizers will be
> grumpy.

Yea. It's not a quick change though (unless we just put the strings in both places, which we might need to do anyway). Weave is not loosely coupled to its strings. It's pretty messy (overly clever at times - I'm looking at you Mardak).

> >+  // getters for elements
> >+  _els: {},
> >+  _el: function(id) {
> >+    if (!this._els[id]) {
> >+      this._els.__defineGetter__(id, function() {
> >+        return document.getElementById(id);
> >+      });
> >+    }
> >+    return this._els[id];
> >+  },
> 
> Ick. This code make me cringe for about 4 different reasons. Nuke this and just
> use "document.getElementById" where needed.
> Or is the caching needed for some performance reason I'm missing?

Yea, I thought it was a good idea (mostly because the all tabs popup could be shown frequently) but it's hacky & not likely worth it.

> >+    let uri = Services.io.newURI("about:sync-tabs", null, null);
> >+    if (gBrowser.browsers.some(function(b) b.currentURI.equals(uri)))
> >+      return;
> 
> It seems odd for this UI to not show up if you happen to have about:sync-tabs
> open somewhere else in the window. Users are likely to think it's broken and
> only appears randomly. Unless this somehow breaks sync, this check should go
> away.

So in that case about:sync-tabs shows up in popup normally as a tab. So instead of having it at the top & then again (potentially immediately) below with the other tabs, it's only shown once.

> >+    // Fake the tab object on the menu entries, so that we don't have to worry
> >+    // about removing them ourselves. They will just get cleaned up by popup
> >+    // binding. This also makes sure the statusbar updates with the URL.
> >+    menuitem.tab = { "linkedBrowser": { "currentURI": { "spec": label } } };
> >+    sep.tab = { "linkedBrowser": { "currentURI": { "spec": " " } } };
> 
> Hmm. File a followup to avoid this hack, we should:
>   - have tabbrowser.xml's _createTabMenuItem() set the URL directly as a
> property on the menuitem (well, more likely use a getter for performance)
>   - have this code set the URL directly
>   - have tabbrowser.xml's DOMMenuItemActive use said property instead of
> tab.linkedBrowser

Something can be done to make this better for sure. I did it this way to get it working as quickly as possible. Removing our items manually was a pain. I had some tweaks to tabbrowser to make it better, so I'll make it less hacky in a followup.

> >+  _onSyncEnd: function SUI__onSyncEnd(success) {
> ...
> >+      if (!Weave.Status.enforceBackoff) {
> 
> The Weave extension has code right above this to check for the version being
> outdated, why isn't that here too? [It opens the EM, but we'd want this for the
> case of the app being way out of date / unsupported.]

I hadn't thought of that case. I took that out because I just assumed that would be part of "you need to update Firefox because..."

> >     Weave.Service.delayedAutoConnect(wait);
> >   }
> > #endif
> > 
> >+#ifdef MOZ_SERVICES_SYNC
> >+  // initialize the sync UI
> >+  gSyncUI.init();
> >+#endif
> 
> I think you're already right below a MOZ_SERVICES_SYNC block here, merge them.

I was, but that block is gone now (as of attachment 461082 [details] [diff] [review])

> Worth a followup bug to move gSyncUI.* into Weave.* somewhere?

Worth at least looking into.

> >+++ b/browser/locales/en-US/chrome/browser/browser.dtd
> 
> >+<!ENTITY logInItem.label      "Connect">
> >+<!ENTITY logOutItem.label     "Disconnect">
> >+<!ENTITY syncNowItem.label    "Sync Now">
> >+<!ENTITY openPrefsItem.label  "Preferencesâ¦">
> >+<!ENTITY openLogItem.label    "Activity Log">
> >+<!ENTITY status.offline.label "Offline">
> 
> Worth renaming these to syncFoo.label? Probably obvious from the context,
> though.

I was trying to avoid changing the entity names from what they are in the extension (to keep it relatively easy to stay in sync), but I suppose that's fine - it does make them more obvious.

Comment 49

[Axel Hecht [:Pike]]

Re
//XXXzpao these strings should be moved from /services to /browser... but for now just make it work

I thought those would be string that are shared between different apps using sync, i.e., Firefox, Fennec, etc. Is that not true?

Comment 50

[Robert Kaiser]

(In reply to comment #48)
> So in that case about:sync-tabs shows up in popup normally as a tab. So instead
> of having it at the top & then again (potentially immediately) below with the
> other tabs, it's only shown once.

We have a function ensuring that about:addons is only shown once and brought to the top if already open, perhaps we should have a generalized function for that and use it here as well (I guess we'll need it more often as other things move to tabs anyhow)? Might be worth a followup, at least.

Comment 51

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #49)
> Re
> //XXXzpao these strings should be moved from /services to /browser... but for
> now just make it work
> 
> I thought those would be string that are shared between different apps using
> sync, i.e., Firefox, Fennec, etc. Is that not true?

Many of them. I didn't check if Fennec was using them, but they were only ever
used in UI stuff, so in that case it would make sense for them to be in
/browser & wherever fennec puts its UI strings. I'm pretty sure anyway. I'd
need to look again though to be 100% sure

(In reply to comment #50)
> We have a function ensuring that about:addons is only shown once and brought to
> the top if already open, perhaps we should have a generalized function for that
> and use it here as well (I guess we'll need it more often as other things move
> to tabs anyhow)? Might be worth a followup, at least.

Not a bad idea. I'll file a followup to at least look into it.

Comment 52

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 461082 [details] [diff] [review]
Part 3: General hookup (v0.2)

>+    const DELAY_PER_TAB = 5;
>+    const MAX_DELAY = 300;
>+    let syncTemp = {};
>+    Cu.import("resource://services-sync/service.js", syncTemp);
>+    let delay = 0;
>+    let enum = Services.wm.getEnumerator("navigator:browser");
>+    while (enum.hasMoreElements()) {
>+      delay += enum.getNext().gBrowser.tabs.length * DELAY_PER_TAB;
>+    }

I realize this was NOT how the delay works in the extension. It has a base delay (3 seconds) & then 1 second was added for each (busy) tab. That's probably better (it's annoying me while testing...). I'm going to do that & get rid of the MAX_DELAY here unless there are objections.

Comment 53

[Justin Dolske [:Dolske]]

Comment on attachment 458088 [details] [diff] [review]
Part 7: Generic change dialog (v0.1)

r- just due to number of small issues...

>+++ b/browser/base/content/syncGenericChange.js

>+__defineGetter__("Weave", function() {
>+  delete this.Weave;
>+  Components.utils.import("resource://services-sync/service.js");
>+  return this.Weave;
>+});

This and a number of |Change| getters could just use XPCOMUtils.defineLazyGetter...

Although, frankly, I think it would be simpler to just init them all in one pass in onLoad.

>+  //XXXzpao _stringBundle is only used by _str, so we can make this better, set it up in onLoad.

Yeah, please do. Also, just use JS string for the bundlename, and nuke the <stringbundle> and <stringbundleset>.

>+  get _title() {
>+    return document.getElementById("change-dialog").getAttribute("title");
>+  },

|document.title| instead?

>+  onLoad: function Change_onLoad() {
...
>+        this._dialog.setAttribute(
>+          "ondialogaccept",
>+          "return Change.doChangePassphrase();"
>+        );

Eww. addEventListener would be preferable, but since (almost) all the cases want to do this, I'd just add an ondialogaccept in the XUL, and have the function it calls demux as needed.

>+          introText.innerHTML = this._str("new.passphrase.introText");

We don't need (or want) to be injecting real HTML. s/innerHTML/textContent/. Here and elsewhere.

>+          this._dialog.getButton("accept")
>+              .setAttribute("label", this._str("new.password.acceptButton"));

getButton().label = foo; should work.


>+  doChangePassphrase: function Change_doChangePassphrase() {
...
>+      if (Weave.Service.login()) {
>+        this._updateStatus("change.passphrase.success", "success");
>+        Weave.Service.persistLogin();
>+      }
>+      else
>+        this._updateStatus("new.passphrase.status.incorrect", "error");

else block should be braced. A few other places too (where the if block is but the else isn't).


>+++ b/browser/base/content/syncGenericChange.xul

>+<dialog xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
...
>+        style="width: 40em"

dialog { width: 40em; }

>+        ondialogcancel="return true;"

O_o


>+  <script type="application/x-javascript"
>+          src="chrome://browser/content/syncGenericChange.js"/>
>+  <script type="application/x-javascript"
>+          src="chrome://browser/content/syncUtils.js"/>

application/javascript


>+  <stringbundleset id="stringbundleset">
>+    <stringbundle id="syncGenericChangeBundle"
>+                  src="chrome://browser/locale/syncGenericChange.properties"/>
>+  </stringbundleset>

Burn with fire.

>+  <hbox align="top">
>+    <image src="chrome://browser/skin/sync-32.png"/>
>+    <spacer style="width: 1em"/>
>+    <description flex="1">
>+      <html:p style="margin-top: 2px" id="introText"/>

The style here should all be CSS (<image> src too, via list-style-image)

>+      <columns>
>+        <column align="right"/>
>+        <column/>
>+      </columns>

Urk. This is could have some RTL issues, not sure sure how we've handled elsewhere. Followup fodder.


>+++ b/browser/locales/en-US/chrome/browser/syncGenericChange.properties
...
>+new.password.title            = Update Password

Probably worth a localizer note here to (briefly) explain what the difference between new/change/update is.

>\ No newline at end of file

nit.

Comment 54

[Justin Dolske [:Dolske]]

Comment on attachment 458087 [details] [diff] [review]
Part 6: Pref pane (v0.1)

Again, r- just for a bunch of small stuff.

>+++ b/browser/components/preferences/sync.js
...
>+__defineGetter__("Weave", function() {
>+  delete this.Weave;
>+  Components.utils.import("resource://services-sync/service.js");
>+  return this.Weave;
>+});

This gets used immediately, so no point in putting in a getter.


>+let gWeavePane = {

So much bouncing between Weave and Sync. :/

>+  get bundle() {
>+    delete this.bundle;
>+    return this.bundle = document.getElementById("bundlePreferences");
>+  },

No point in this being a getter either, just assign it in init().

>+  get prefArray() {
>+    return ["engine.bookmarks", "engine.passwords", "engine.prefs",
>+      "engine.tabs", "engine.history"];
>+  },

Not sure why this is a getter instead of a property...

>+  onLoginStart: function () {
>+    if (this.page == 0)
>+      return;

Add some global |const PAGE_FOO = 0;| stuff so that we're not doing mysterious integer checks like this. :(

>+  init: function () {
...
>+    let addRem = function(add) obs.forEach(function([topic, func])
>+      Weave.Svc.Obs[add ? "add" : "remove"](topic, weavePrefs[func], weavePrefs));

Same comment as other patch in this series... Deobfuscate via |if (add) {} else {}|.


>+  updateWeavePrefs: function () {
>+    if (Weave.Status.service == Weave.CLIENT_NOT_CONFIGURED ||
>+        Weave.Svc.Prefs.get("firstSync", "") == "notReady")

I suspect we want to replace all of these Weave.Svc shortcuts with Services.* at some point. Followup?

>+  updateSetupButtons: function () {
>+    let elems = [ "manageAccountExpander"];
>+    let pbEnabled = Weave.Svc.Private.privateBrowsingEnabled;
>+    for (let i = 0;i < elems.length;i++)
>+      document.getElementById(elems[i]).disabled = pbEnabled;

Unless this list of elements is likely to grow in the near future, kind of silly to iterate over a 1-element array. :)


>+  handleExpanderClick: function () {
>+    //XXXzpao shit like this is bad. Look into it
>+    // ok, this is pretty evil, and likely fragile if the prefwindow
>+    // binding changes, but that won't happen in 3.6 *fingers crossed*

Sigh. I think (some of) this really wants to live in a resizePane() in preferences.xul, could be useful in other cases too. Followup bug?


>+  //XXXzpao this should probably go into gWeaveUtils
>+  openSetup: function (resetSync) {

Do or file?

>+    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                       .getService(Components.interfaces.nsIWindowMediator);
>+    var win = wm.getMostRecentWindow("Weave:AccountSetup");

Services.wm



>+++ b/browser/components/preferences/sync.xul
...
>+          <description flex="1" style="padding: 0 12em">
>+            &weaveDesc.label;
>+          </description>

Similar to previous comment... The style= and <image src=> around here should be in CSS.


>+                <!-- XXXzpao We should make this behave like the "details" view in CRH.
>+                             do in followup -->

Yes. File? Also, that might take care of this, but having the expander button move to to the bottom of the expanded list seems broken. It should stay in the same place, and have the list expand below it.


>+++ b/browser/locales/en-US/chrome/browser/preferences/preferences.dtd
>+<!-- This should match syncBrand.shortName.label in ../syncBrand.dtd -->
>+<!ENTITY  paneSync.title          "Sync">

# LOCALIZATION NOTE (paneSync.title): This should match...

>\ No newline at end of file

Nit.



>+++ b/browser/locales/en-US/chrome/browser/preferences/preferences.properties

A number of these strings look unused. Are used in another of these patches, or just dead?

>+additionalClients.label = and %S additional devices

This looks like an L10N bug, assuming a certain sentence structure.

>\ No newline at end of file

Nit.


>+++ b/browser/locales/en-US/chrome/browser/preferences/sync.dtd
>@@ -0,0 +1,30 @@
>+<!-- The page shown when not logged in... -->

These comments should become LOCALIZATION NOTEs, or just removed for cases where they're not really adding value.

>+<!-- Manage Account -->
>+<!ENTITY manageAccount.label          "Manage Account">

...like that. :)

>+<!-- XXXzpao these are the same as "setup.*.label" from syncSetup.dtd... combine? -->

Yeah.

>+<!-- Include items that are common to multple sync dialogs -->
>+#include ../syncCommon.dtd

Maybe there! :)

>\ No newline at end of file

Nit.

>+++ b/browser/locales/jar.mn
>-    locale/browser/preferences/preferences.dtd        (%chrome/browser/preferences/preferences.dtd)
>-    locale/browser/preferences/preferences.properties (%chrome/browser/preferences/preferences.properties)
>+*   locale/browser/preferences/preferences.dtd        (%chrome/browser/preferences/preferences.dtd)
>+*   locale/browser/preferences/preferences.properties (%chrome/browser/preferences/preferences.properties)

Oh, right, as previously mentioned in this bug shouldn't be conditionally preprocessing l10n bits.

Comment 55

[Justin Dolske [:Dolske]]

Comment on attachment 461082 [details] [diff] [review]
Part 3: General hookup (v0.2)

>+#ifdef MOZ_SERVICES_SYNC
>+    // Assume that a non-zero value for services.sync.autoconnectDelay should override
>+    if (Services.prefs.prefHasUserValue("services.sync.autoconnectDelay")) {
>+      let prefDelay = Services.prefs.getIntPref("services.sync.autoconnectDelay");
>+
>+      if (prefDelay > 0)
>+        return;
>+    }

I wonder if it would be simpler to just always have this code call delayedAutoConnect, using the pref value obtained here (when set). That way Sync's start time is controlled from a single function in the code.

Comment 56

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 2: Prefs (v1.0)

Prefs after removing dead ones. Carrying over review

Comment 57

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #55)
> Comment on attachment 461082 [details] [diff] [review]
> Part 3: General hookup (v0.2)
> 
> >+#ifdef MOZ_SERVICES_SYNC
> >+    // Assume that a non-zero value for services.sync.autoconnectDelay should override
> >+    if (Services.prefs.prefHasUserValue("services.sync.autoconnectDelay")) {
> >+      let prefDelay = Services.prefs.getIntPref("services.sync.autoconnectDelay");
> >+
> >+      if (prefDelay > 0)
> >+        return;
> >+    }
> 
> I wonder if it would be simpler to just always have this code call
> delayedAutoConnect, using the pref value obtained here (when set). That way
> Sync's start time is controlled from a single function in the code.

I agree, but followup? The service currently checks that pref the first time it gets imported, so we'd have to make that change there.

Comment 58

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 3: General hookup (v1.0)

got rid of the 5s delay per tab, kept max delay after talking with dolske

Comment 59

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #57)
> > I wonder if it would be simpler to just always have this code call
> > delayedAutoConnect, using the pref value obtained here (when set). That way
> > Sync's start time is controlled from a single function in the code.
> 
> I agree, but followup? The service currently checks that pref the first time it
> gets imported, so we'd have to make that change there.

Filed bug 583350 to move the pref checking out of the service.

Comment 60

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 8: Tools menu (v0.2)

Comment 61

[Justin Dolske [:Dolske]]

Comment on attachment 458085 [details] [diff] [review]
Part 4: about:sync-tabs (v0.1)

>+++ b/browser/base/content/aboutSyncTabs-bindings.xml
>+<?xml version="1.0"?>

Need a MPL block here.


>+++ b/browser/base/content/aboutSyncTabs.js

Mostly a light review of this file, since it's straight Weave cut'n'paste.

>+Cu.import("resource://services-sync/service.js");
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource:///modules/PlacesUIUtils.jsm");
>+
>+__defineGetter__("Weave", function() {
>+  delete this.Weave;
>+  Cu.import("resource://services-sync/service.js");
>+  return this.Weave;
>+});

Seems like the getter isn't helping much here either, but meh.

>+let RemoteTabViewer = {
>+  get _tabsList() document.getElementById("tabsList"),

Missing braces make Tiny Tim cry.

But it seems this is a static element (the <richlistbox>), just make it a static property set in init().

>+    //XXXzpao Seems like this should be easier...
>+    let win = getTopWin();
>+    let tabs = win.gBrowser.mTabs;
>+    for (let i = 0; i < tabs.length; i++) {
>+      if (tabs[i].linkedBrowser.currentURI.spec == "about:sync-tabs")
>+        win.gBrowser.setIcon(tabs[i], "chrome://browser/skin/sync-16.png");
>+    }

<link rel>, as Dao noted. A release or two back that only worked with data: URIs (about:robots), but I'm pretty sure it's since been fixed.


>+  buildList: function(force) {
...
>+    //XXXzpao We should say something about not being logged in & not having data
>+    //        or tell the appropriate condition

File followup!

>+            //XXXzpao Would it be better to just do this via CSS?
>+            icon: Weave.Clients.isMobile(client.id) ? "chrome://browser/skin/sync-mobileIcon.png"
>+                                                    : "chrome://browser/skin/sync-desktopIcon.png",

Probably, set an iconClass.

>+  adjustContextMenu: function(event) {
...
>+    let menu = document.getElementById("tabListContext");
>+    let el = menu.firstChild;
>+    while (el) {
>+      let sf = el.getAttribute("showFor");
>+      if (sf)
>+        el.hidden = sf != mode && sf != "all";
>+
>+      el = el.nextSibling;

The two-letter variable names really ought to be expanded. :/


>+++ b/browser/base/content/aboutSyncTabs.xul
...
>+    <popup id="tabListContext">
>+      <menuitem label="&tabs.context.openTab.label;"
>+                oncommand="RemoteTabViewer.openSelected()"
>+                showFor="single"/>

Need accesskeys on these <menuitems>.


>+  <richlistbox context="tabListContext" id="tabsList" seltype="multiple"
>+               align="center" flex="1"
>+               onclick="RemoteTabViewer.handleClick(event)"
>+               oncontextmenu="RemoteTabViewer.adjustContextMenu(event)">
>+    <hbox align="center" style="background-color: transparent;">
>+      <hbox id="headers" align="center">
>+        <image src="chrome://browser/skin/sync-32.png"
>+               height="32" width="32"/>

CSS here, please.


>+++ b/browser/base/content/browser-menubar.inc
...
>+#ifdef MOZ_SERVICES_SYNC
>+# XXXzpao I went ahead an got rid of the icon, other menuitems here don't do that

Agreed.

>+#         I also just show the menuitem by default instead of hiding/showing

Seems like it should be hidden if the user isn't using Sync, though? (ie, not signed up?). (Can we even easily determine that state?)

>+# XXXzpao name this menuitem consistently?

???

>+++ b/browser/components/about/AboutRedirector.cpp
...
>+#ifdef MOZ_SERVICES_SYNC
>+  { "sync-tabs", "chrome://browser/content/aboutSyncTabs.xul",
>+    nsIAboutModule::ALLOW_SCRIPT },
>+#endif

Technically this should have a security review, I wonder if that already happened in the original Weave security review?

>+++ b/browser/locales/en-US/chrome/browser/browser.dtd
...
>\ No newline at end of file

Nit.

>diff --git a/browser/locales/jar.mn b/browser/locales/jar.mn

Comment 62

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 461695 [details] [diff] [review]
Part 8: Tools menu (v0.2)


>+++ b/browser/locales/en-US/chrome/browser/browser.dtd
>@@ -504,9 +504,24 @@ just addresses the organization to follo
> 
> <!ENTITY allTabs.filter.emptyText "Search Tabs">
> <!-- Name for the tabs toolbar as spoken by screen readers.
>      The word "toolbar" is appended automatically and should not be contained below! -->
> <!ENTITY tabsToolbar.label "Browser tabs">
> 
> #ifdef MOZ_SERVICES_SYNC
> <!ENTITY syncTabsMenu.label "Tabs From Other Computers">
>-#endif
>\ No newline at end of file
>+
>+<!ENTITY syncBrand.shortName.label  "Sync">
>+
>+<!ENTITY syncMenu.label               "&syncBrand.shortName.label;">
>+<!-- LOCALIZATION NOTE (sync.menu.accesskey): This is part of the tools menu, so
>+                                              don't use a conflicting access key -->
>+<!ENTITY syncMenu.accesskey           "Y">
>+<!ENTITY syncSetup.label              "Set Up &syncBrand.shortName.label;â¦">
>+<!ENTITY syncSetup.accesskey          "Y">
>+<!ENTITY syncLogInItem.label          "Connect">
>+<!ENTITY syncLogInItem.accesskey      "C">
>+<!ENTITY syncLogOutItem.label         "Disconnect">
>+<!ENTITY syncLogOutItem.accesskey     "D">
>+<!ENTITY syncSyncNowItem.label        "Sync Now">
>+<!ENTITY syncSyncNowItem.accesskey    "S">
>+#endif

Just pretend for a minute that there is no #ifdef'ing because that's how it really is

Comment 63

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #54)
> Comment on attachment 458087 [details] [diff] [review]
> Part 6: Pref pane (v0.1)
> >+let gWeavePane = {
> 
> So much bouncing between Weave and Sync. :/

Changed it to gSyncPane (and subsequently it's changed in parts 5 & 7)

> >+  updateSetupButtons: function () {
> >+    let elems = [ "manageAccountExpander"];
> >+    let pbEnabled = Weave.Svc.Private.privateBrowsingEnabled;
> >+    for (let i = 0;i < elems.length;i++)
> >+      document.getElementById(elems[i]).disabled = pbEnabled;
> 
> Unless this list of elements is likely to grow in the near future, kind of
> silly to iterate over a 1-element array. :)

Gone anyway due to bug 580158.

> >+  handleExpanderClick: function () {
> >+    //XXXzpao shit like this is bad. Look into it
> >+    // ok, this is pretty evil, and likely fragile if the prefwindow
> >+    // binding changes, but that won't happen in 3.6 *fingers crossed*
> 
> Sigh. I think (some of) this really wants to live in a resizePane() in
> preferences.xul, could be useful in other cases too. Followup bug?

Filed bug 583441 to fix the behavior which should maybe make this alllllllll better. Replaced with a comment that doesn't use "shit"

> >+  //XXXzpao this should probably go into gWeaveUtils
> >+  openSetup: function (resetSync) {
> 
> Do or file?

Bug 583366

> A number of these strings look unused. Are used in another of these patches, or
> just dead?

Dead (and now gone)

> >+additionalClients.label = and %S additional devices
> 
> This looks like an L10N bug, assuming a certain sentence structure.

It probably is. Luckily it's gone & we don't have to worry about it until the wizard. You'll see why it's like that. I'm open to ideas & I _think_ it still allows for localizations to make it work.

> >+++ b/browser/locales/en-US/chrome/browser/preferences/sync.dtd
> >@@ -0,0 +1,30 @@
> >+<!-- The page shown when not logged in... -->
> 
> These comments should become LOCALIZATION NOTEs, or just removed for cases
> where they're not really adding value.

I was just trying to bring some order to a l10n file as opposed to the usual clusterfuck they are.

Are LOCALIZATION NOTES ok for multiple lines? It wouldn't be tied to a particular entity...

> >+<!-- Manage Account -->
> >+<!ENTITY manageAccount.label          "Manage Account">
> 
> ...like that. :)

...yea. But it felt appropriate to do all of the sections as opposed to most.

> >+<!-- XXXzpao these are the same as "setup.*.label" from syncSetup.dtd... combine? -->
> 
> Yeah.

Not happening. Per the rest of your comment & comment 31 (reusing strings isn't always safe). Just going to leave them duplicated.

Comment 64

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 6: Pref pane (v0.2)

I _think_ I got everything (sans some of the l10n notes)

Comment 65

[Vlado Valastiak [:wladow]]

(In reply to comment #54)
> >+<!-- XXXzpao these are the same as "setup.*.label" from syncSetup.dtd... combine? -->
> 
> Yeah.

I asked to add those in bug 564637, please do not combine them.

Comment 66

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 461744 [details] [diff] [review]
Part 6: Pref pane (v0.2)

>+let gWeavePane = {

Also pretend I changed this to gSyncPane.

Comment 67

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #53)
> >+  get _title() {
> >+    return document.getElementById("change-dialog").getAttribute("title");
> >+  },
> 
> |document.title| instead?

Changed & it seems to be working as expected (hard to tell on OSX without using DOMi). Also got rid of the getter/setter, just setting directly.

> >+          introText.innerHTML = this._str("new.passphrase.introText");
> 
> We don't need (or want) to be injecting real HTML. s/innerHTML/textContent/.
> Here and elsewhere.

Done. Now I'm wondering why those are <html:p> nodes at all... Connor? Mardak?

> >+          this._dialog.getButton("accept")
> >+              .setAttribute("label", this._str("new.password.acceptButton"));
> 
> getButton().label = foo; should work.

I might have mentioned this before, but I'm just pushing this off to a followup (bug 583405).

> >+++ b/browser/base/content/syncGenericChange.xul
> >+        ondialogcancel="return true;"
> 
> O_o

I don't know... got rid of it

> >+      <columns>
> >+        <column align="right"/>
> >+        <column/>
> >+      </columns>
> 
> Urk. This is could have some RTL issues, not sure sure how we've handled
> elsewhere. Followup fodder.

Filed bug 583509

Comment 68

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 7: Generic change dialog (v0.2)

Comment 69

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 1: Theme (v0.3)

Theme as is. It already includes the theme changes from about:sync-tabs. Will probably change a little bit more post-wizard-review

Comment 70

[Justin Dolske [:Dolske]]

Comment on attachment 458086 [details] [diff] [review]
Part 5: Setup wizard (v0.1)

Note: I'm giving syncSetup.[js,xul] light reviews, and mostly just rs+ing with a few nits. They're well-contained and already shipped with Weave, and I don't see anything boneheadedly dumb from skimming through them.


>+++ b/browser/base/content/syncSetup.js

>+  status: { username: false, password: false, email: false, server: false},

Nit: line breaks.

>+  get bundle() {
>+    delete this.bundle;
>+    return this.bundle = document.getElementById("syncSetupBundle");
>+  },

Burn with fire, replace with normal XPCOM stringbundle goop.


>+  get wizard() {
>+    delete this.wizard;
>+    return this.wizard = document.getElementById("accountSetup");
>+  },

Just set property in init, it's already called from there anyway.

>+  init: function () {
...
>+    // Add the observers now and remove them on unload
>+    let weavePrefs = this;

weavePrefs?

>+    let addRem = function(add) obs.forEach(function([topic, func])
>+      Weave.Svc.Obs[add ? "add" : "remove"](topic, weavePrefs[func], weavePrefs));

Nit: as before, deobfuscate.


>+  onResetPP: function () {
>+    document.getElementById("existingPassphrase").value = Weave.Service.passphrase;
>+    this.wizard.advance();
>+  },

Nit: onResetPassphrase

>+  onEmailChange: function () {
>+    let re = /^(([^<>()[\]\\.,;:\s@\"]+(\.[^<>()[\]\\.,;:\s@\"]+)*)|(\".+\"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/;

Yuck. Email regex checking is something people almost always get wrong.

I'm _hoping_ this came from some definitive source as wasn't just invented ad hoc. Should have a comment here noting it's provenance. Followup for investigating this?


>+  _handleNoScript: function (addExceptions) {

Oh my. I'm a bit wary that some (vocal minority) might freak out about this, but I'm please to see this was very responsibly handled in bug 508112 (where this was added). Add a comment to the bug number here?


>+  startThrobber: function (start) {
>+    // FIXME: stubbed
>+  },

Sigh. Followup?

Looks like this is only used when creating an account, but there really ought to some UI "we're busy" feedback if the servers are slow.

>+  QueryInterface: function (aIID) {
>+    if (aIID.equals(Ci.nsIWebProgressListener) ||
>+        aIID.equals(Ci.nsISupportsWeakReference) ||
>+        aIID.equals(Ci.nsISupports))
>+      return this;
>+    throw Cr.NS_NOINTERFACE;
>+  },

XPCOMUtils ftw.


>+++ b/browser/base/content/syncSetup.xul
...

Multiple spots here: as noted in other patches, please move the |style=| to CSS.


>+++ b/browser/base/content/syncUtils.js
...
>+  // opens in a new window if we're in a modal prefwindow world, in a new tab otherwise
>+  _openLink: function (url) {
>+    if (document.documentElement.id == "accountSetup" &&
>+        window.opener &&
>+        window.opener.document.documentElement.id == "BrowserPreferences" &&
>+        !window.opener.document.documentElement.instantApply)
>+      openUILinkIn(url, "window");
>+    else if (document.documentElement.id == "BrowserPreferences" &&
>+             !document.documentElement.instantApply)
>+      openUILinkIn(url, "window");
>+    else
>+      openUILinkIn(url, "tab");
>+  },

This would benefit from adding a |thisDocEl| and |openerDocEl|.


>+  openPP: function () {
>+    this._openLink(Weave.Svc.Prefs.get("privacyURL"));
>+  },

openPassphrase. owait, "PP" means PrivayPolicy here! This is why 2 letter abbreviations suck. :) So openPrivacyPolicy.

>+  // xxxmpc - fix domain before 1.3 final
>+  _baseURL: "http://www.mozilla.com/firefox/sync/",

And how's that working out for for 1.3 final? Sigh. Followup?

>+  _validate: function (el1, el2, isPassword) {
>+    let valid = false;
>+    let val1 = el1.value;
>+    let val2 = el2 ? el2.value : "";

Is this ever actually called with just one input?

It's a little weird, because if that happens the only validity checking is the min-length, and I'm not sure why you'd even want to be checking that...


>+++ b/browser/locales/en-US/chrome/browser/syncBrand.dtd
>@@ -0,0 +1,2 @@
>+<!ENTITY syncBrand.shortName.label  "Sync">
>+<!ENTITY syncBrand.fullName.label   "Firefox Sync">

Hmm. So, normally "Firefox" strings should live over under the offical-branding, but I think it's ok in this case because it's the name of the _webservice_, not the app. Nightlies shouldn't be "Minefield Sync", nor alphas "National Park Sync". [EG, we ask the user "Have you used &syncBrand.fullName.label; before?", so changing the name would be rather confusing. That's true even if you're running IceWeasel, and if someone wants to default the app to using a a different server instance they should be changing these strings anyway.

>\ No newline at end of file

Nit.


>+++ b/browser/locales/en-US/chrome/browser/syncSetup.dtd
...
>+<!-- New Account Page 4: Captcha -->
>+<!ENTITY setup.captchaPage.title.label     "Please confirm you're not a robot ;)">

<3

Comment 71

[Justin Dolske [:Dolske]]

Comment on attachment 461695 [details] [diff] [review]
Part 8: Tools menu (v0.2)

>+++ b/browser/base/content/browser-menubar.inc
...
>+                  <menuitem id="sync-loginitem"
>+                            label="&syncLogInItem.label;"
>+                            accesskey="&syncLogInItem.accesskey;"
>+                            oncommand="gSyncUI.doLogin();"/>
>+                  <menuitem id="sync-logoutitem"
>+                            label="&syncLogOutItem.label;"
>+                            accesskey="&syncLogOutItem.accesskey;"
>+                            oncommand="gSyncUI.doLogout();"/>

Hrm, these strings (entities) don't seem to actually be defined any of the attached patches. Is there a DTD not being included by Hg, or am I just missing it?


>+++ b/browser/base/content/browser-syncui.js
...
>+  alltabsPopupShowing: function(event) {
...
>+    // Fake the tab object on the menu entries, so that we don't have to worry
>+    // about removing them ourselves. They will just get cleaned up by popup
>+    // binding. This also makes sure the statusbar updates with the URL.
>+    menuitem.tab = { "linkedBrowser": { "currentURI": { "spec": label } } };
>+    sep.tab = { "linkedBrowser": { "currentURI": { "spec": " " } } };

Need a followup bug for eliminating this hack (comment 46).

Comment 72

[Justin Dolske [:Dolske]]

Comment on attachment 461744 [details] [diff] [review]
Part 6: Pref pane (v0.2)

>+++ b/browser/components/preferences/sync.js

>+  init: function () {
...
>+    this.bundle = document.getElementById("bundlePreferences");

This should go away, replace with XPCOM/Services.strings.

>+++ b/browser/locales/en-US/chrome/browser/preferences/sync.dtd
...
>+<!-- Footer stuff -->
>+<!-- XXXzpao these are the same as "setup.*.label" from syncSetup.dtd... combine? -->
>+<!ENTITY prefs.tosLink.label        "Terms of Service">
>+<!ENTITY prefs.ppLink.label         "Privacy Policy">

Nuke XXX.

Comment 73

[Vlado Valastiak [:wladow]]

Comment on attachment 458086 [details] [diff] [review]
Part 5: Setup wizard (v0.1)

>+++ b/browser/locales/en-US/chrome/browser/syncCommon.dtd
>@@ -0,0 +1,26 @@
>+# syncCommon.dtd contains items that are common to multiple XUL files used by
>+# sync. Namely, the pref pane and the setup dialog
>+

This is not a valid comment in DTDs. Wrap a <!-- --> around. 


>+++ b/browser/locales/en-US/chrome/browser/syncSetup.properties
>@@ -0,0 +1,14 @@
>+
>+/* Several other strings are used (via Weave.Status.login), but they come from
>+   /services/sync */

This doesn't seem to be valid either.

>+additionalClients.label     = and %S additional devices
>+bookmarkCount.label         = %S bookmarks
>+historyCount.label          = %S days of history
>+passwordCount.label         = %S passwords

Just a friedly reminder: a pluralFrom here please

Comment 74

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #71)
> Comment on attachment 461695 [details] [diff] [review]
> Part 8: Tools menu (v0.2)
> 
> >+++ b/browser/base/content/browser-menubar.inc
> ...
> >+                  <menuitem id="sync-loginitem"
> >+                            label="&syncLogInItem.label;"
> >+                            accesskey="&syncLogInItem.accesskey;"
> >+                            oncommand="gSyncUI.doLogin();"/>
> >+                  <menuitem id="sync-logoutitem"
> >+                            label="&syncLogOutItem.label;"
> >+                            accesskey="&syncLogOutItem.accesskey;"
> >+                            oncommand="gSyncUI.doLogout();"/>
> 
> Hrm, these strings (entities) don't seem to actually be defined any of the
> attached patches. Is there a DTD not being included by Hg, or am I just missing
> it?

They are in this patch, in browser.dtd (the very bottom of the patch).

> >+++ b/browser/base/content/browser-syncui.js
> ...
> >+  alltabsPopupShowing: function(event) {
> ...
> >+    // Fake the tab object on the menu entries, so that we don't have to worry
> >+    // about removing them ourselves. They will just get cleaned up by popup
> >+    // binding. This also makes sure the statusbar updates with the URL.
> >+    menuitem.tab = { "linkedBrowser": { "currentURI": { "spec": label } } };
> >+    sep.tab = { "linkedBrowser": { "currentURI": { "spec": " " } } };
> 
> Need a followup bug for eliminating this hack (comment 46).

I forgot to add the comment, but I filed bug 583348 for that.

Comment 75

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #61)
> Comment on attachment 458085 [details] [diff] [review]
> Part 4: about:sync-tabs (v0.1)

> >+Cu.import("resource://services-sync/service.js");
> >+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >+Cu.import("resource:///modules/PlacesUIUtils.jsm");
> >+
> >+__defineGetter__("Weave", function() {
> >+  delete this.Weave;
> >+  Cu.import("resource://services-sync/service.js");
> >+  return this.Weave;
> >+});
> 
> Seems like the getter isn't helping much here either, but meh.

Already got rid of it.

> >+  buildList: function(force) {
> ...
> >+    //XXXzpao We should say something about not being logged in & not having data
> >+    //        or tell the appropriate condition
> 
> File followup!

filed bug 583344, updated comment with bug number

> >+#         I also just show the menuitem by default instead of hiding/showing
> 
> Seems like it should be hidden if the user isn't using Sync, though? (ie, not
> signed up?). (Can we even easily determine that state?)

Yea. The addon does that. I took the liberty of not though. I don't think there are other places where we hide menuitems... but I guess this is something completely different since we've never incorporated a web service.

Made it as you suggested.

> >+# XXXzpao name this menuitem consistently?
> 
> ???

Most of the other history menuitems are "history____". Not all though so meh.

> >+++ b/browser/components/about/AboutRedirector.cpp
> ...
> >+#ifdef MOZ_SERVICES_SYNC
> >+  { "sync-tabs", "chrome://browser/content/aboutSyncTabs.xul",
> >+    nsIAboutModule::ALLOW_SCRIPT },
> >+#endif
> 
> Technically this should have a security review, I wonder if that already
> happened in the original Weave security review?

Ohhh right. I saw that when I started & completely forgot.

Comment 76

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 4: about:sync-tabs (v0.2)

Fixed comments from Dao & Dolske

Comment 77

[Jim Jeffery not reading bug-mail 1/2/11]

> >+#         I also just show the menuitem by default instead of hiding/showing
> 
> Seems like it should be hidden if the user isn't using Sync, though? (ie, not
> signed up?). (Can we even easily determine that state?)

Yea. The addon does that. I took the liberty of not though. I don't think there
are other places where we hide menuitems... but I guess this is something
completely different since we've never incorporated a web service.

Made it as you suggested.


I'm probably not reading this right - but are you saying the Sync Menu Item will be hidden if the user has never signed up or does not use the service ?  To me, if its hidden, discover-ability will be absolutely nil.

Chrome has in their Options settings a statement indicating you have not signed up..  the button there shows "Set up Sync"

Comment 78

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #77)
> I'm probably not reading this right - but are you saying the Sync Menu Item
> will be hidden if the user has never signed up or does not use the service ? 
> To me, if its hidden, discover-ability will be absolutely nil.
> 
> Chrome has in their Options settings a statement indicating you have not signed
> up..  the button there shows "Set up Sync"

So there are 2 places that menu items show up. The one we're talking about here is the "tabs from other computers" entry (in the history menu). We'll hide that one if no account is set up.

The other menu entry is the primary menu & includes actions for sync now & disconnect (this will show up in the tools menu). When you don't have an account, then the item reads "set up sync".

tldr; You weren't reading it right :) Or rather, were reading too much into a very specific comment.

Comment 79

[Justin Dolske [:Dolske]]

Comment on attachment 461923 [details] [diff] [review]
Part 4: about:sync-tabs (v0.2)

>+++ b/browser/base/content/browser-menubar.inc
>+# XXXzpao I went ahead an got rid of the icon, other menuitems here don't do that
>+#         I also just show the menuitem by default instead of hiding/showing
>+# XXXzpao name this menuitem consistently?

These two comments can go away.

Comment 80

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 1: Theme (v0.4)

Now with wizard changes

Comment 81

[Justin Dolske [:Dolske]]

Comment on attachment 461960 [details] [diff] [review]
Part 1: Theme (v0.4)

Again doing a fairly light review on the CSS, I don't see anything obnoxious, so I'm largely rs+ing it. Zpao said he was pretty sure he checked for unused images when first porting the Weave stuff over to m-c, good enough for me.

>diff --git a/browser/themes/gnomestripe/browser/sync-16-throbber.apng b/browser/themes/gnomestripe/browser/sync-16-throbber.apng

s/apng/png/, please.

Comment 82

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #70)
> >+  startThrobber: function (start) {
> >+    // FIXME: stubbed
> >+  },
> 
> Sigh. Followup?
> 
> Looks like this is only used when creating an account, but there really ought
> to some UI "we're busy" feedback if the servers are slow.

bug 583653

> >+  // xxxmpc - fix domain before 1.3 final
> >+  _baseURL: "http://www.mozilla.com/firefox/sync/",
> 
> And how's that working out for for 1.3 final? Sigh. Followup?

So I think it did change (was services.m.c) but I filed bug 583652.

> >+  _validate: function (el1, el2, isPassword) {
> >+    let valid = false;
> >+    let val1 = el1.value;
> >+    let val2 = el2 ? el2.value : "";
> 
> Is this ever actually called with just one input?

Yes. Through validatePassword and validatePassphrase, which will get called with just one arg for the change dialog - the case where your credentials were changed on a difference computer so login fails & you're asked to input your new creds.

> It's a little weird, because if that happens the only validity checking is the
> min-length, and I'm not sure why you'd even want to be checking that...

Yea, but it's the only check you can make. It'll at the very least give minimal feedback that you couldn't possibly have a password/phrase that short. meh?

> >+++ b/browser/locales/en-US/chrome/browser/syncBrand.dtd
> >@@ -0,0 +1,2 @@
> >+<!ENTITY syncBrand.shortName.label  "Sync">
> >+<!ENTITY syncBrand.fullName.label   "Firefox Sync">
> 
> Hmm. So, normally "Firefox" strings should live over under the
> offical-branding, but I think it's ok in this case because it's the name of the
> _webservice_, not the app. Nightlies shouldn't be "Minefield Sync", nor alphas
> "National Park Sync". [EG, we ask the user "Have you used
> &syncBrand.fullName.label; before?", so changing the name would be rather
> confusing. That's true even if you're running IceWeasel, and if someone wants
> to default the app to using a a different server instance they should be
> changing these strings anyway.

Yea I tried to some fancy stuff to make "Iceweasel Sync" possible but then came to the same conclusion.

Comment 83

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 5: Setup wizard (v0.2)

my brain hurts. i think i got it all. I'll be going through the r+'s and posting the polished versions later.

Comment 84

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #73)
> >+additionalClients.label     = and %S additional devices
> >+bookmarkCount.label         = %S bookmarks
> >+historyCount.label          = %S days of history
> >+passwordCount.label         = %S passwords
> 
> Just a friedly reminder: a pluralFrom here please

Good catch, but this missed due to faulty brain cells. However it is the first followup on the list (bug 583661). I'm going to go through again and see if there were any other strings that need pluralForm.

Comment 85

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 8: Tools menu (and status bar) (v1.0)

Comment 86

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 6: Pref pane (v1.0)

Comment 87

[Justin Dolske [:Dolske]]

Comment on attachment 461983 [details] [diff] [review]
Part 5: Setup wizard (v0.2)

>+++ b/browser/base/content/syncUtils.js
...
>+  _openLink: function (url) {
>+    let thisDocEl = document.documentElement,
>+        openerDocEl = window.opener.document.documentElement;

Seems window.opener may be null here, so you'll need to conditionally set this.

>+    if (thisDocEl.id == "accountSetup" && window.opener &&
>+        openerDocEl == "BrowserPreferences" && !openerDocEl.instantApply)

openerDocEl.id

r+ with that!

Comment 88

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 7: Generic change dialog (v1.0)

Comment 89

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 4: about:sync-tabs (v1.0)

Comment 90

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 5: Setup wizard (v1.0)

Comment 91

[Robert Kaiser]

(In reply to comment #70)
> Hmm. So, normally "Firefox" strings should live over under the
> offical-branding, but I think it's ok in this case because it's the name of the
> _webservice_, not the app.

Still, when shipping this is the product, and even in those versions that are _not_ legally cleared for trademark use, do we need and OK from legal on this trademark use?

Comment 92

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Landed. Parts 1-8:
http://hg.mozilla.org/mozilla-central/rev/72fbbb3310e7
http://hg.mozilla.org/mozilla-central/rev/924e4f41d023
http://hg.mozilla.org/mozilla-central/rev/29177a927440
http://hg.mozilla.org/mozilla-central/rev/48da9f6a8ad5
http://hg.mozilla.org/mozilla-central/rev/e25f6e0000c0
http://hg.mozilla.org/mozilla-central/rev/6fbf47730e48
http://hg.mozilla.org/mozilla-central/rev/eaf2e61d63e3
http://hg.mozilla.org/mozilla-central/rev/f2659f5555fe

NOTE: It still isn't turned on. That is bug 583339.

Any followup issues should be done in a different bug. I have a list so far: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=[sync-ui-followup] - please tag bugs you create that are UI followup issue with the same whiteboard status. Also, please CC me.

Comment 93

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #91)
> Still, when shipping this is the product, and even in those versions that are
> _not_ legally cleared for trademark use, do we need and OK from legal on this
> trademark use?

That's a good question. Hopefully Ragavan can answer that or get the right people involved. If any changes need to be made, new bug :)

Comment 94

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 1: Theme (v1.0)

Comment 95

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Part 8: Tools menu (and status bar) (v1.1)

Comment 96

[Axel Hecht [:Pike]]

Just to give an overview on the impact on beta3 1l0n:

This is a 174 strings, with no ad-hoc reuse of the work on weave-l10n.