576970 - Port Sync UI to SeaMonkey trunk

Status: RESOLVED FIXED

(SeaMonkey :: Sync UI, defect)

Description

[Robert Kaiser]

As bug 571902 has landed the Sync 1.4 backend on mozilla-central, the UI parts are moved into the various app repository spaces, bug 571898 for Fennec in mobile-browser, bug 571897 for Firefox in mozilla-central.
SeaMonkey should do the same and port the applicable UI parts into our own repository space on comm-central, I'd propose to put things into suite/sync or suite/common/sync.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

I think this is too important to let it slip for 2.1, and big enough to justify keeping an extra eye on. Maybe the Sync extension will be discontinued. Requesting 2.1 flags. I'd suggest requiring this for the next alpha/beta or something like that.

Comment 2

[Ian Neal]

Should try and get this into a beta, not blocking a3 on it though.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Intermediate result: Have everything except Preferences, Setup and Generic Change dialog, and MailNews status bar integration running locally (excluding Modern, which I'll make a follow-up in any case). Patch size so far: 160 KB (21 files modified, 21 new, 11 of which are images). Review will be fun... ;-)

First part will be build system / installer changes, will probably ask Callek to review that. I have a busy weekend in front of me, though, so this'll have to wait. Just wanted to give you an update. It's alive!

BTW: I chose suite/common/sync (and themes/classic/communicator/sync).

Comment 4

[Robert Kaiser]

(In reply to comment #3)
> Have everything except Preferences, Setup and Generic
> Change dialog, and MailNews status bar integration running locally

kewl!
And yes, could easily be a not-too-small patch ;-)

> BTW: I chose suite/common/sync (and themes/classic/communicator/sync).

Sounds good to me.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

From bug 571902 comments 86 and 87:
>> Are there any plans (bug) to remove MOZ_SERVICES_SYNC?
> At this time, there are no plans to remove that, as I don't expect all apps to
build it. It's up to you for the preprocessing, but I also don't know what
you're optimizing for there.

AFAIK the SM reviewers don't like adding preprocessing to UI files, so I'll assume sync will always be built for SM and drop the preprocessing on the UI side.

Comment 6

[Robert Kaiser]

(In reply to comment #5)
> AFAIK the SM reviewers don't like adding preprocessing to UI files, so I'll
> assume sync will always be built for SM and drop the preprocessing on the UI
> side.

Sounds good to me. We might add a configure check that fails out if this this is not set, but if you don't feel like coding it up, I think that wouldn't be worse than with other options we take for granted to be set.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Intermediate result: Have everything except Preferences running locally (excluding Modern and Mac-specific changes). Patch size so far: 266 KB (23 files modified, 32 new, 11 of which are images).

Upside: The Sync icon now also updates correctly on the MailNews window, which it doesn't with the extension. :-)

BTW: Firefox Sync is using http://www.mozilla.com/firefox/sync/ as the base URL for the pages that are shown after the first client has been configured (firstrun.html) or one has been added (secondrun.html). We should create our own pages since theirs are using the Firefox design and wording. I'll eventually file a follow-up bug, but also added a comment to the code in case I forget or we use their base URL for starters.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

So, final patch size 300 KB, which I'll attach in nine parts. While I'll attach all at once, please refrain from reviewing/commenting on multiple ones in detail before other reviews are finished because otherwise I'll go mad. If need be we can split this bug. General comments are welcome, though. For example it's debatable whether we want to call this feature Sync everywhere or SeaMonkey Sync in some places (as with my initial patches). And no, Firefox Sync is not an option, even though we probably won't provide alternatives for the Terms of Service and Privacy Policy pages (which refer to this term) in order to stay in sync <PI>. ;-)

Things I'd like to delay / move to follow-ups include:
- Modern theme changes
- Mac-specific changes (I cannot test those anyway)
- Theme and UI changes beyond what current trunk Firefox Sync provides
- Creating any web pages (cf. comment 7)
- Fixing the Email my Sync Key functionality (unless it's very easy to fix)
- Help changes
- Any minor MailNews/integration issues, unless they can be resolved easily.

I didn't make any brace style changes at all yet to allow for some comparability with FF trunk for the time being (aid review). However I'll apply any style (including whitespace etc.) that is requested by reviewers; no need to nit-pick each and every detail, one example per file is enough.

BTW: While working on this I found and filed bug 595548 to fix a bug in FF's Sync pref panel. The same is already fixed in the patches that I'll attach here.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Attachment: build v1

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Attachment: prefs v1

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Attachment: theming v1

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Attachment: statusbar & menu v1

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Attachment: prefpanel v1

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Attachment: locales v1

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Attachment: about:sync-tabs v1

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dialogs v1

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

Attachment: common v1

Comment 18

[Justin Wood (:Callek)]

(In reply to comment #6)
> (In reply to comment #5)
> > AFAIK the SM reviewers don't like adding preprocessing to UI files, so I'll
> > assume sync will always be built for SM and drop the preprocessing on the UI
> > side.
> 
> Sounds good to me. We might add a configure check that fails out if this this
> is not set, but if you don't feel like coding it up, I think that wouldn't be
> worse than with other options we take for granted to be set.

If we take it for "granted" that sync is enabled, I do want configure to error on this, its ok for a followup though, and as for how much I care, you don't have to do it (as long as a bug is on file for it)

(In reply to comment #8)
> - Fixing the Email my Sync Key functionality (unless it's very easy to fix)

I'm curious what this functionality/issue is. (no general objection to the splitting off to a new bug)

Comment 19

[Justin Wood (:Callek)]

Comment on attachment 474420 [details] [diff] [review]
build v1

Actually when we don't have a configure _option_ for it, and set stuff in confvars.sh we don't need (nor do *I* want) a configure error on this.  That said, the current iteration of this patch can _only_ land when the rest is ready.

If you want to land this earlier (to help sanity in bitrot potential) I'm ok with that, without the confvars.sh change, and with ifdef's in the package-manifest file for the new stuff.


>+@BINPATH@/components/nsAboutSyncTabs.js

Nit: "Huh, where is this file?"  (If its part of a future patch here, I'd prefer if you just included it in that patch)

Comment 20

[Justin Wood (:Callek)]

Comment on attachment 474421 [details] [diff] [review]
prefs v1

This seems a copy/paste of the Firefox pref list... my thoughts, neil has final say.

>+pref("services.sync.prefs.sync.app.update.mode", true);

I'm not a fan of having app.update.mode synced between different installs. If I don't want SeaMonkey on my dev builds to auto-update but I do want SeaMonkey to on my main install, this won't be great.

>+pref("services.sync.prefs.sync.browser.offline-apps.notify", true);

Not used in SeaMonkey (unsure if it should be)

>+pref("services.sync.prefs.sync.browser.safebrowsing.enabled", true);
>+pref("services.sync.prefs.sync.browser.safebrowsing.malware.enabled", true);

We don't have safeBrowsing.

I am also thinking about the possible bug-acting when we sync between SeaMonkey and Firefox, and each app has a pref set on default branch (as in no user value) I wonder if Firefox will get set with the seamonkey default, vice-versa, or neither will get synced because of that. [I'm also not sure which is right, just a thought]

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #19)
> That said, the current iteration of this patch can _only_ land when the
> rest is ready.

That's fine, see below.

> >+@BINPATH@/components/nsAboutSyncTabs.js
> 
> Nit: "Huh, where is this file?"  (If its part of a future patch here, I'd
> prefer if you just included it in that patch)

It's in the "about:sync-tabs" patch. None (or just a few) of the patches here are self-contained. For example, the changes to common/jar.mn are all contained in the "common" patch. Given the size of this bug, any other approach (like hg queues or whatever) might aid the reviewer a bit but hurt my sanity considerably. That said, I intend to land everything at once in any case (each patch as one changeset with its own check-in comment, together as one push).

Thanks so far, that was quick! :-)

(In reply to comment #20)
> Comment on attachment 474421 [details] [diff] [review]
> prefs v1
> 
> This seems a copy/paste of the Firefox pref list...

Correct. No changes made there. I didn't think about pref default values yet and will probably take whatever is suggested/requested/required. Thus I'm leaving your thoughts for Neil or others to consider.

> >+pref("services.sync.prefs.sync.app.update.mode", true);
> 
> I'm not a fan of having app.update.mode synced between different installs. If I
> don't want SeaMonkey on my dev builds to auto-update but I do want SeaMonkey to
> on my main install, this won't be great.

Please keep in mind that you can have multiple sync profiles (I think they call it accounts) and you can change this pref. The question is what our "standard" users might want, and I'm not sure they'll use a dev build. They will however probably use multiple stable builds on different computers (or OSs, think dual-boot, VM).

> >+pref("services.sync.prefs.sync.browser.safebrowsing.enabled", true);
> >+pref("services.sync.prefs.sync.browser.safebrowsing.malware.enabled", true);
> 
> We don't have safeBrowsing.

True. I haven't checked whether anything breaks if we just leave these out, though.

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #18)
> (In reply to comment #8)
> > - Fixing the Email my Sync Key functionality (unless it's very easy to fix)
> 
> I'm curious what this functionality/issue is. (no general objection to the
> splitting off to a new bug)

Sync asks for a password and a passphrase (called Sync Key in the UI). This key has now been made readable on purpose (cf. bug 590805 comment 14 and bug 589980 attachment 468606 [details]), with the options to mail, print or save it to disk. The "Email" option brought up a dialog for me asking for a mailto: handler application (SeaMonkey preselected) for me instead of invoking a compose window.

I just found that there's more work waiting for me on bug 590805, CC'ing myself now (which obviously didn't happen automatically when I commented from the attachment details screen, grr)...

Comment 23

[Jens Hatlak (:InvisibleSmiley)]

Attachment: statusbar & menu v1a

I forgot to port the menupopup onpopupshowing attribute from browser-menubar.inc. Now the Tools menu updates correctly in MailNews, too.

This patch contains the only changes specific to MailNews, and the other changes are either the same for the browser (statusbar) or affect both (menu).

Comment 24

[[e-mail]]

Comment on attachment 474556 [details] [diff] [review]
statusbar & menu v1a

>+                    image="chrome://communicator/skin/sync/sync-16.png"
This needs to be specified in theme CSS, not as an attribute.

>+                    onmousedown="event.preventDefault();">
What does this achieve?

>+    <separator class="thin"/>
I doubt this is going to look right in a statusbar...

>+    <panel id="sync-notifications-panel" position="before_end">
>+       <notificationbox id="sync-notifications-box"/>
>+    </panel>
Doesn't this belong in a popupset?

>+        <menupopup id="taskPopup"
>+                   onpopupshowing="gSyncUI.updateUI();">
Unfortunately we have other overlays trying to set this attribute.

Comment 25

[Jens Hatlak (:InvisibleSmiley)]

Hmm, while looking at bug 595603 I saw I forgot to apply the changes made to syncSetup.css here. Fixed locally, including the image removals from bug 595603.

Comment 26

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #24)
> Comment on attachment 474556 [details] [diff] [review]
> statusbar & menu v1a
> 
> >+                    image="chrome://communicator/skin/sync/sync-16.png"
> This needs to be specified in theme CSS, not as an attribute.
> 
> >+                    onmousedown="event.preventDefault();">
> What does this achieve?
> 
> >+    <separator class="thin"/>
> I doubt this is going to look right in a statusbar...

Bug 594488 just moved the status bar UI to an optional toolbar button, and follow-up Bug 594663 introduced two broadcasters and changed the menu. I'll port these within the next days which will address the above three review comments. I already integrated the changes from the other bugs I added to the Depends On list locally (luckily I haven't requested review for any of the corresponding patches yet; I'll probably have to update all of them except the about:sync-tabs one).

> >+    <panel id="sync-notifications-panel" position="before_end">
> >+       <notificationbox id="sync-notifications-box"/>
> >+    </panel>
> Doesn't this belong in a popupset?

No idea, just ported. Please don't assume I have the background to tell which XUL elements exist and might apply somewhere.

> >+        <menupopup id="taskPopup"
> >+                   onpopupshowing="gSyncUI.updateUI();">
> Unfortunately we have other overlays trying to set this attribute.

Example?

Comment 27

[Jens Hatlak (:InvisibleSmiley)]

I just read in bug 595810 comment 6 that the 2.1 feature freeze is scheduled for the end of September. I doubt this bug will be ready by then (two weeks from now!), no matter how fast I port changes or react to review comments, simply because this one is complex and our review process (review + reacting on it) is known to take ages (read: from weeks to months). I'd deeply regret if we let this one slip, though. Can we agree on an exemption or something? I'm going to take a few days off from the 24th to the 28th, if that helps.

If the above leads to a longer discussion, let's take it to the newsgroups.

Comment 28

[Robert Kaiser]

Let's see how far we can get. There's no formal beta or feature freeze yet, we just said that we are targeting the end of September. Let's discuss that in the regular IRC meeting on Tuesday (I hope you have time to be there).

Comment 29

[[e-mail]]

Comment on attachment 474421 [details] [diff] [review]
prefs v1

>+pref("services.sync.prefs.sync.accessibility.typeaheadfind", true);
We don't actually use this pref. Instead we use .typeaheadfind.autostart

>+pref("services.sync.prefs.sync.accessibility.typeaheadfind.linksonly", true);
What about the other typeaheadfind preferences? You might want to add .usefindbar for instance.

>+pref("services.sync.prefs.sync.browser.download.manager.showWhenStarting", true);
Not sure we use this one either. And we have our own .behaviour pref of course.

>+pref("services.sync.prefs.sync.browser.link.open_newwindow", true);
.open_external too perhaps?

>+pref("services.sync.prefs.sync.browser.search.selectedEngine", true);
>+pref("services.sync.prefs.sync.browser.search.update", true);
I guess this is in readiness for the switch to OPenSerach?

>+pref("services.sync.prefs.sync.browser.startup.homepage", true);
Ah, now this is where it gets tricky, since that's only one page for us.

>+pref("services.sync.prefs.sync.browser.tabs.warnOnClose", true);
But not .warnOnCloseOther?

>+pref("services.sync.prefs.sync.browser.urlbar.maxRichResults", true);
One we're unlikely to have in the near future ;-)

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

>+pref("services.sync.prefs.sync.pref.advanced.images.disable_button.view_image", true);
>+pref("services.sync.prefs.sync.pref.advanced.javascript.disable_button.advanced", true);
>+pref("services.sync.prefs.sync.pref.downloads.disable_button.edit_actions", true);
>+pref("services.sync.prefs.sync.pref.privacy.disable_button.cookie_exceptions", true);
These make no sense.

>+pref("services.sync.prefs.sync.privacy.clearOnShutdown.cache", true);
>+pref("services.sync.prefs.sync.privacy.clearOnShutdown.cookies", true);
>+pref("services.sync.prefs.sync.privacy.clearOnShutdown.downloads", true);
>+pref("services.sync.prefs.sync.privacy.clearOnShutdown.formdata", true);
>+pref("services.sync.prefs.sync.privacy.clearOnShutdown.history", true);
>+pref("services.sync.prefs.sync.privacy.clearOnShutdown.offlineApps", true);
>+pref("services.sync.prefs.sync.privacy.clearOnShutdown.passwords", true);
>+pref("services.sync.prefs.sync.privacy.clearOnShutdown.sessions", true);
s/clearOnShutdown/item/

>+pref("services.sync.prefs.sync.privacy.clearOnShutdown.siteSettings", true);
We don't have this, but we do have .item.urlbar

>+pref("services.sync.prefs.sync.security.OCSP.disable_button.managecrl", true);
Doesn't make sense.

Comment 30

[Jens Hatlak (:InvisibleSmiley)]

Attachment: prefs v2

(In reply to comment #29)
> >+pref("services.sync.prefs.sync.browser.search.selectedEngine", true);
> >+pref("services.sync.prefs.sync.browser.search.update", true);
> I guess this is in readiness for the switch to OPenSerach?

Hmm, we currently have browser.search.selectedEngine but it'll be removed through OpenSearch bug 410613. FF doesn't seem to have it. Removed it. I kept browser.search.update, which will be introduced through bug 410613.

> >+pref("services.sync.prefs.sync.browser.startup.homepage", true);
> Ah, now this is where it gets tricky, since that's only one page for us.

Something I need to address, or can I, or possible follow-up?

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

Neither do I; removed.

BTW: There are some prefs in mozilla/services/sync/services-sync.js which I seem not to be able to override in browser-prefs.js. I think we need something (what? Suggestions welcome!) for the following ones (or introduce our own prefs in the other patches of this bug) so that we can brand it accordingly (s/Firefox/SeaMonkey/):
pref("services.sync.termsURL", "https://services.mozilla.com/tos/");
pref("services.sync.privacyURL", "https://services.mozilla.com/privacy-policy/");
pref("services.sync.statusURL", "https://services.mozilla.com/status/");
pref("services.sync.syncKeyHelpURL", "https://services.mozilla.com/help/synckey");
Note that the last one has been introduced only recently; the page doesn't exist yet.

I'll post updated patches for the rest soon now that I have everything to date ported and working.

Comment 31

[Jens Hatlak (:InvisibleSmiley)]

Attachment: about:sync-tabs v2

Comment 32

[Jens Hatlak (:InvisibleSmiley)]

Attachment: common v2

Comment 33

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dialogs v2

Comment 34

[Jens Hatlak (:InvisibleSmiley)]

Attachment: integration v1

Comment 35

[Jens Hatlak (:InvisibleSmiley)]

Attachment: locales v2

Comment 36

[Jens Hatlak (:InvisibleSmiley)]

Attachment: prefpanel v2

Comment 37

[Jens Hatlak (:InvisibleSmiley)]

Attachment: theming v2

Comment 38

[Jens Hatlak (:InvisibleSmiley)]

Attachment: build v1a

Just unbitrot, carrying over r+.

Comment 39

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 478571 [details] [diff] [review]
integration v1

So, shall we? :-) I kept everything in sync (yeah I know, no further pun notes in comments!) with the changes made to m-c/FF, except the move from statusbar to add-on bar. I won't go into detail in general, but the changes that apply to this patch (which is incomplete as-is, you need to check the other patches, too!) are:

- The former statusbar icon is now a customizable toolbar button, available in both the browser and MailNews.
- The sub menu is gone, only two entries are left. Ideally only one should show at any time, but there may be a delay due to sync UI init taking some time. If anyone has a problem with that, please let us take that to a follow-up.
- Sync notifications (like failure to sync) appear as a notification bar above the statusbar.

The onpopupshowing issue is not solved yet, mainly because I only got a very vague hint from Neil and no further explanation, despite my questioning.

BTW: I took the liberty to rename browser-syncui.js (original name in m-c) to syncui.js since it's hardly browser-specific code.

Comment 40

[Axel Hecht [:Pike]]

Filed bug 600141 on a problem in the fx ui, fyi.

Comment 41

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #40)
> Filed bug 600141 on a problem in the fx ui, fyi.

Thanks, but I'd like to keep the Depends list here clean, meaning only listing bugs that I have incorporated into the attached patches. I'm adding bugs once they have landed on m-c *and* I have ported them. Comments like this are welcome, though. :-)

Comment 42

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dialogs v3

Bah, missed the (relatively new) syncQuota files.

mcsmurf, can you push to Try again with this update, please? Thanks!

Comment 43

[Jens Hatlak (:InvisibleSmiley)]

Attachment: locales v2a

Comment 44

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dialogs v3a

With locales v2a and dialogs v3a, bug 600141 is now incorporated as well.

I also found that I had removed a closing brace in syncSetup.js, method checkAccount, by mistake, which I fixed in dialogs v3a, too.

mcsmurf, one final round please? :)

Comment 45

[[e-mail]]

(In reply to comment #26)
>(In reply to comment #24)
>>(From update of attachment 474556 [details] [diff] [review])
>>>+    <panel id="sync-notifications-panel" position="before_end">
>>>+       <notificationbox id="sync-notifications-box"/>
>>>+    </panel>
>>Doesn't this belong in a popupset?
>No idea, just ported. Please don't assume I have the background to tell which
>XUL elements exist and might apply somewhere.
We moved all of our other popups and tooltips into a popupset for some reason, which I don't remember offhand.

>>>+        <menupopup id="taskPopup"
>>>+                   onpopupshowing="gSyncUI.updateUI();">
>>Unfortunately we have other overlays trying to set this attribute.
>Example?
suite/mailnews/mailWindowOverlay.xul:  <menupopup id="taskPopup" onpopupshowing="document.commandDispatcher.updateCommands('create-menu-tasks')">

Comment 46

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 478568 [details] [diff] [review]
about:sync-tabs v2

I'll probably be AFK from now until Saturday evening, and addressing the Integration patch review comments will take me a while anyway, so let's start review on the rest in the meantime. Also if someone of the usual suspects (IanN et al.) feels like asking Neil about stealing this review from him or reviewing another part, just give it a try. :-)

Also I see mcsmurf just pushed another round of patches to the Try server (thanks!). If all builds succeed this time we'll have something to ease testing and reviews (e.g. the locales part) under http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/?C=M;O=D (look for mcsmurf and 30-Sep-2010) in a couple of hours.

Comment 47

[[e-mail]]

Comment on attachment 478568 [details] [diff] [review]
about:sync-tabs v2

>+    <!-- Sync -->
>+    <broadcaster id="sync-setup-state"/>
>+    <broadcaster id="sync-syncnow-state"/>
What do these do?

>+      menuitem.setAttribute("hidden", true);
Nit: just use .hidden = true/false

>+    menuitem.setAttribute("disabled", !enabled);
[But don't change this]

>+      <xul:hbox flex="1">
This box appears to be superfluous.

>+        <xul:vbox pack="start">
Nit: pack="start" is the default.

>+      <xul:hbox pack="start" align="center" onfocus="event.target.blur();" onselect="return false;">
How does this ever get focus or select events?

>+      item.label = attrs.title != "" ? attrs.title : attrs.url;
attrs.title || attrs.url

>+    for (let i = 0;i < items.length;i++) {
Nit: spaces after ;s

>+      getTopWin().gBrowser.loadTabs(urls);
Nit: getBrowser()

>+    if (event.target.getAttribute("type") != "tab")
>+      return;
>+
>+    if (event.button == 1) {
Nit: should use same style for both tests i.e. either both early return or combine them with &&.

>+  <richlistbox context="tabListContext" id="tabsList" seltype="multiple"
>+               align="center" flex="1"
>+               onclick="RemoteTabViewer.handleClick(event);"
>+               oncontextmenu="RemoteTabViewer.adjustContextMenu(event);">
>+    <hbox id="headers" align="center">
So, the headers scroll with the content?

Comment 48

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #47)
> Comment on attachment 478568 [details] [diff] [review]
> about:sync-tabs v2
> 
> >+    <!-- Sync -->
> >+    <broadcaster id="sync-setup-state"/>
> >+    <broadcaster id="sync-syncnow-state"/>
> What do these do?

They are used to toggle the entry in the Tools menu (which reads Setup Sync or Sync Now, depending on whether Sync has been set up already or not). The logic is in syncui.js ("common" patch), the observers are in tasksOverlay.xul ("integration" patch).

> >+  <richlistbox context="tabListContext" id="tabsList" seltype="multiple"
> >+               align="center" flex="1"
> >+               onclick="RemoteTabViewer.handleClick(event);"
> >+               oncontextmenu="RemoteTabViewer.adjustContextMenu(event);">
> >+    <hbox id="headers" align="center">
> So, the headers scroll with the content?

The header line consists of the Sync logo, the string "Tabs From Other Computers", and a search field. It starts the page contents, i.e. it appears at the top of the page and is followed by the individual computers' tab data, and moves with it (left/right when resizing due to the centering, up/down when scrolling).

Regarding those:

> >+      <xul:hbox flex="1">
> This box appears to be superfluous.

> >+      <xul:hbox pack="start" align="center" onfocus="event.target.blur();" onselect="return false;">
> How does this ever get focus or select events?

No idea, really. And you better get used to that answer. This is all not my invention after all, and I'm not a XUL expert either. I'll try to answer your questions where possible, but I really appreciate the support of anyone who has some knowledge in the field and likes to give one of these builds a try:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/
-> [e-mail]-769c95c15554/tryserver-win32/ for Windows
-> [e-mail]-8742b1486b62/ for Linux and Mac

Comment 49

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #45)
> suite/mailnews/mailWindowOverlay.xul:  <menupopup id="taskPopup"
> onpopupshowing="document.commandDispatcher.updateCommands('create-menu-tasks')">

And here's the other one:
suite/common/permissions/permissionsNavigatorOverlay.xul:  <menupopup id="taskPopup" onpopupshowing="CheckForVisibility()">

So I think we have three of possibilities:
a) prepend gSyncUI.updateUI(); to the onpopupshowing attribute value of the above two overlays. This is the simplest approach, but obviously leaves room for improvement.
b) like a, but instead call a custom function (e.g. "onTasksMenuPopupShowing") to be defined (and used) in tasksOverlay.xul which for the moment would just call gSyncUI.updateUI()
c) use an addEventListener call to react to the popupshowing event. This could be done for either the main overlay or the other two. I wouldn't know where to best put the JS and whether there could be initialization issues.

Personally I'd go for option b. Opinions?

Comment 50

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #45)
> (In reply to comment #26)
> >(In reply to comment #24)
> >>(From update of attachment 474556 [details] [diff] [review])
> >>>+    <panel id="sync-notifications-panel" position="before_end">
> >>>+       <notificationbox id="sync-notifications-box"/>
> >>>+    </panel>
> >>Doesn't this belong in a popupset?
> >No idea, just ported. Please don't assume I have the background to tell which
> >XUL elements exist and might apply somewhere.
> We moved all of our other popups and tooltips into a popupset for some reason,
> which I don't remember offhand.

FYI: After the latest refactorings, the direct XUL is now gone, and with it the panel element. The notificationbox is now created on the fly using JS (function initNotifications, syncui.js, "common" patch) and inserted before the statusbar. I'm really not sure whether this (popupset question) is still an issue in that light, but let's discuss that when review on the "common" patch starts.

Comment 51

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #47)
> Comment on attachment 478568 [details] [diff] [review]
> about:sync-tabs v2
> (...)
> >+      <xul:hbox flex="1">
> This box appears to be superfluous.
> (...)
> >+      <xul:hbox pack="start" align="center" onfocus="event.target.blur();" onselect="return false;">
> How does this ever get focus or select events?

Both were introduced by mconnor in bug 539056.

As for the hbox: I don't know. Let's just keep it for the sake of compatibility (it doesn't harm either), OK?

About the events question: Mardak asked something related to that in bug 539056 comment 6. In comment 7, mconnor said he'd file a follow-up bug, but he didn't actually do it. I can only guess that the intention is to skip those client rows when going through the content rows using the arrow keys, i.e. only focus tab rows.

Comment 52

[[e-mail]]

(In reply to comment #49)
> So I think we have three of possibilities:
> a) prepend gSyncUI.updateUI(); to the onpopupshowing attribute value of the
> above two overlays. This is the simplest approach, but obviously leaves room
> for improvement.
> b) like a, but instead call a custom function (e.g. "onTasksMenuPopupShowing")
> to be defined (and used) in tasksOverlay.xul which for the moment would just
> call gSyncUI.updateUI()
Both a) and b) depend on the order in which the overlays get applied. I'm not 100% sure (which doesn't help) but I think tasksOverlay gets applied last.

> c) use an addEventListener call to react to the popupshowing event. This could
> be done for either the main overlay or the other two. I wouldn't know where to
> best put the JS and whether there could be initialization issues.
You would presumably do it in a similar way to viewZoomOverlay's registerZoomManager which adds a popupshowing listener when the window loads.

Comment 53

[Philip Chee]

Jens, Suggestion:

in browser-syncui.js
at the end of gSyncUI::initUI()
just before |this.updateUI();|
add this:

var tasks = document.getElementById("taskPopup");
if (tasks)
  tasks.addEventListener("popupshowing", gSyncUI.updateUI, false);

Comment 54

[Philip Chee]

Comment on attachment 478571 [details] [diff] [review]
integration v1

> +++ b/suite/browser/navigator.css
> +
> +#sync-notifications {
> +  -moz-binding: url("chrome://communicator/content/sync/syncNotification.xml#notificationbox");
> +  overflow-y: visible !important;
> +}
> +
> +#sync-notifications notification {
> +  -moz-binding: url("chrome://communicator/content/sync/syncNotification.xml#notification");
> +}
> 
> +++ b/suite/mailnews/messenger.css
> +#sync-notifications {
> +  -moz-binding: url("chrome://communicator/content/sync/syncNotification.xml#notificationbox");
> +  overflow-y: visible !important;
> +}
> +
> +#sync-notifications notification {
> +  -moz-binding: url("chrome://communicator/content/sync/syncNotification.xml#notification");
> +}

Centralize these in suite/common/communicator.css so that you don't have to duplicate the bindings.

> +++ b/suite/browser/navigator.xul
> +      <toolbarbutton id="sync-button"
> +                     class="toolbarbutton-1 chromeclass-toolbar-additional"
> +                     label="&syncToolbarButton.label;"
> +                     oncommand="gSyncUI.handleToolbarButton();"/>
> 
> +++ b/suite/mailnews/mailWindowOverlay.xul
> +    <toolbarbutton id="sync-button"
> +                   class="toolbarbutton-1 chromeclass-toolbar-additional"
> +                   label="&syncToolbarButton.label;"
> +                   oncommand="gSyncUI.handleToolbarButton();"/>

Centralize these in common/tasksOverlay.xul so that you don't have to duplicate. Also put the entities in tasksOverlay.dtd.

> -        <menupopup id="taskPopup">
> +        <menupopup id="taskPopup"
> +                   onpopupshowing="gSyncUI.updateUI();">

in browser-syncui.js
at the end of gSyncUI::initUI()
just before |this.updateUI();|
add this:

var tasks = document.getElementById("taskPopup");
if (tasks)
  tasks.addEventListener("popupshowing", gSyncUI.updateUI, false);

> +++ b/suite/mailnews/mailWindowOverlay.xul
>  <broadcasterset id="mailBroadcasters">
> +  <!-- Sync -->
> +  <broadcaster id="sync-setup-state"/>
> +  <broadcaster id="sync-syncnow-state"/>
>  </broadcasterset>

> +++ b/suite/browser/navigatorOverlay.xul
> +    <!-- Sync -->
> +    <broadcaster id="sync-setup-state"/>
> +    <broadcaster id="sync-syncnow-state"/>

In tasksOverlay.xul do something like:

<broadcasterset id="mainBroadcasterSet">
  <!-- Sync -->
  <broadcaster id="sync-setup-state"/>
  <broadcaster id="sync-syncnow-state"/>
</broadcasterset>

In mailWindowOverlay.xul:
add a stub <broadcasterset id="mainBroadcasterSet"/>

Comment 55

[Philip Chee]

Drive by comment:

> +++ b/suite/common/utilityOverlay.js
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyGetter(this, "Weave", function() {
> +  let tmp = {};
> +  Components.utils.import("resource://services-sync/main.js", tmp);
> +  return tmp.Weave;
> +});
> +#include sync/syncui.js

Can we use mozIJSSubScriptLoader here to avoid pre processing?

Comment 56

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #53)
> Jens, Suggestion:
> 
> in browser-syncui.js
> at the end of gSyncUI::initUI()
> just before |this.updateUI();|
> add this:
> 
> var tasks = document.getElementById("taskPopup");
> if (tasks)
>   tasks.addEventListener("popupshowing", gSyncUI.updateUI, false);

Good idea, implemented that locally. Had to use function () {gSyncUI.updateUI();} instead of just gSyncUI.updateUI though because otherwise "this" won't point to gSyncUI and already the first line of updateUI will fail (this._needsSetup won't exist).

Oh, and it's syncui.js for us. We're not limiting ourselves to the browser, are we? ;-)

Comment 57

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #55)
> Drive by comment:
> 
> > +++ b/suite/common/utilityOverlay.js
> > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> > +XPCOMUtils.defineLazyGetter(this, "Weave", function() {
> > +  let tmp = {};
> > +  Components.utils.import("resource://services-sync/main.js", tmp);
> > +  return tmp.Weave;
> > +});
> > +#include sync/syncui.js
> 
> Can we use mozIJSSubScriptLoader here to avoid pre processing?

Seems so. Had to add syncui.js to suite/common/jar.mn and change the #-prefixed comment block to C++-comment syntax, but otherwise it is working fine. Thanks! :-)

Also, while I was at it, I decided to rename syncui.js to syncUI.js. If we rename the file anyway, let's go for consistency.

Comment 58

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #54)
>>(bindings)
> Centralize these in suite/common/communicator.css so that you don't have to
> duplicate the bindings.

Changed locally, thanks!

>>(toolbarbutton)
> Centralize these in common/tasksOverlay.xul so that you don't have to
> duplicate. Also put the entities in tasksOverlay.dtd.

Erm, tasksOverlay is for the Tools menu. Did you mean utilityOverlay?

>>(broadcasters)
> In tasksOverlay.xul do something like:

This time really tasksOverlay I guess?

> In mailWindowOverlay.xul:
> add a stub <broadcasterset id="mainBroadcasterSet"/>

Why would I need that in the mailWindowOverlay but not in the navigatorOverlay?

Thanks so far, it's really appreciated!

Comment 59

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #58)
> > In mailWindowOverlay.xul:
> > add a stub <broadcasterset id="mainBroadcasterSet"/>
> 
> Why would I need that in the mailWindowOverlay but not in the navigatorOverlay?

KaiRo pointed out that this is because of the existing stub in navigator.xul, and I have little doubt the two broadcasters need to go to tasksOverlay, so the only question left open is about where to put the toolbarbuttons (i.e. in which overlay).

Comment 60

[Jens Hatlak (:InvisibleSmiley)]

Regarding the toolbar button icon:

I just saw that bug 594657 changed the Sync toolbar button images for Firefox. I won't incorporate those changes here, and consequently won't add that bug to the Depends on list. If anyone wants to investigate adopting those icons (e.g. they added a 24x24 image for gnomestripe, and we're currently lacking a 29x29 one), please file a follow-up bug and add it to the Blocks list of this one.

Comment 61

[Jens Hatlak (:InvisibleSmiley)]

Attachment: about:sync-tabs v2a

The broadcasters are now in the (upcoming new version of the) "integration" patch.

Comment 62

[Jens Hatlak (:InvisibleSmiley)]

Attachment: common v3

Now including Philip's excellent suggestions (no more preprocessing!) and the renaming of syncui.js to syncUI.js. I think we're mostly done with about:sync-tabs, so let's start review on this one.

Comment 63

[Jens Hatlak (:InvisibleSmiley)]

Attachment: integration v2

Applied Philip's suggestions, including moving the main toolbarbutton definition to utilityOverlay.xul as discussed on IRC. Of course the entity has been moved accordingly in the upcoming new version of the "locales" patch.

Comment 64

[Jens Hatlak (:InvisibleSmiley)]

Attachment: locales v2b

Just moved syncToolbarButton.label.

Comment 65

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dialogs v4

Implemented the changes from bug 600219.

BTW: Bug 600219 fix contained in latest "common" patch.

Comment 66

[Philip Chee]

Comment on attachment 482338 [details] [diff] [review]
integration v2

+++ b/suite/mailnews/msgMail3PaneWindow.js
+  // initialize the sync UI
+  gSyncUI.init();

Not a nit, but why is this in the integration patch but the navigator equivalent in the about:sync-tabs patch?

Comment 67

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #66)
> Comment on attachment 482338 [details] [diff] [review]
> integration v2
> 
> +++ b/suite/mailnews/msgMail3PaneWindow.js
> +  // initialize the sync UI
> +  gSyncUI.init();
> 
> Not a nit, but why is this in the integration patch but the navigator
> equivalent in the about:sync-tabs patch?

Only because that way I have (or had?) all suite/mailnews/ parts in one patch, see comment 23. Could be moved to the "about:sync-tabs" patch, yes. Will do if I need to come up with another version or reviewers require it.

Comment 68

[Philip Chee]

> Will do if
> I need to come up with another version or reviewers require it.
Don't bother. I assume once everything is approved you will use mq to roll all the patches into one for pushing.

Comment 69

[[e-mail]]

Comment on attachment 482337 [details] [diff] [review]
common v3

>+  init: function SUI_init() {
>+    // this will be the first notification fired during init
>+    // we can set up everything else later
>+    Services.obs.addObserver(this, "weave:service:ready", true);
>+
>+    // Remove the observer if the window is closed before the observer
>+    // was triggered.
>+    window.addEventListener("unload", function() {
>+      window.removeEventListener("unload", arguments.callee, false);
I read somewhere that the JS engine prefers you to name the function.

>+    let popup = null;
>+    if (popup = document.getElementById("alltabs-popup")) {
Weird. Just init the var and test it in the normal way please?

>+    taskPopup.addEventListener("popupshowing", function () {gSyncUI.updateUI();}, false);
Nit: the other addEventListener calls split this into three lines.

>+    // Force a style flush to ensure that our binding is attached.
>+    notificationbox.clientTop;
>+
>+    // notificationbox will listen to observers from now on.
>+    Services.obs.removeObserver(this, "weave:notification:added");
Not sure what the point of this method is. Why not just add the notificationbox in the XUL?

>+    let syncButton = null;
>+    if (syncButton = document.getElementById("sync-button")) {
Ditto.

>+    sep.tab = { "linkedBrowser": { "currentURI": { "spec": " " } } };
Why a space?

>+    window.openUILinkIn(statusURL, "tab");
Nit: don't need window. prefix

>+      Services.ww.activeWindow.openDialog(
>+        "chrome://communicator/content/sync/syncQuota.xul", "",
>+        "centerscreen,chrome,dialog,modal");
Why not window.openDialog?

>+  get bundle() {
>+    delete this.bundle;
>+    return this.bundle = Services.strings.createBundle("chrome://communicator/locale/sync/syncSetup.properties");
>+  },
Nit: should use XPCOMUtils

>+  // opens in a new window if we're in a modal prefwindow world, in a new tab otherwise
I didn't think prefwindow was modal...

>+var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]  
>+                       .getService(Components.interfaces.mozIJSSubScriptLoader);
>+loader.loadSubScript("chrome://communicator/content/sync/syncUI.js");
Why not add syncUI.js to utilityOverlay.xul?

Comment 70

[[e-mail]]

Comment on attachment 482338 [details] [diff] [review]
integration v2

>+#sync-notifications notification {
>+  -moz-binding: url("chrome://communicator/content/sync/syncNotification.xml#notification");
>+}
>

Comment 71

[[e-mail]]

Meaning, don't use descendent selectors when child selectors will do ;-)

Comment 72

[Philipp von Weitershausen [:philikon]]

(In reply to comment #69)
> >+    window.addEventListener("unload", function() {
> >+      window.removeEventListener("unload", arguments.callee, false);
> I read somewhere that the JS engine prefers you to name the function.

Yeah, arguments.callee is going away in ES5 strict mode.

> >+    // notificationbox will listen to observers from now on.
> >+    Services.obs.removeObserver(this, "weave:notification:added");
> Not sure what the point of this method is. Why not just add the notificationbox
> in the XUL?

We're basically following the same pattern as with the find bar in Firefox which is also created lazily. Reduces the amount of widgets for people who don't use Sync at all.

> >+      Services.ww.activeWindow.openDialog(
> >+        "chrome://communicator/content/sync/syncQuota.xul", "",
> >+        "centerscreen,chrome,dialog,modal");
> Why not window.openDialog?

I don't remember the exact details of it, but IIRC there were some cirumcstances where 'window' wasn't the right object. Perhaps because of the hidden window on Mac?

Comment 73

[Jens Hatlak (:InvisibleSmiley)]

Attachment: build services v1

This is now needed additionally after bug 586867 landed (see changeset below). Our build.mk is a bit different; AFAICS we don't need to add the comment through this bug since we only add the two services lines to the mozilla section. Please correct me if I'm wrong: I don't feel at home there, I just verified that this gives me a working build again.
http://hg.mozilla.org/mozilla-central/rev/36aa9c19420e

And yes, I know I'm still missing the four Sync UI changesets that landed on 2010-10-14. They only affect the "dialogs" patch, though, which I won't put up for review until I have ported the above. Probably not before Vienna; I still need to create a certain presentation. ;-)

Comment 74

[Justin Wood (:Callek)]

Comment on attachment 484026 [details] [diff] [review]
build services v1

Without reading the past patches here, are we using the ifdef: MOZ_SERVICES_SYNC if so, wrap this with that. if we are doing "always" then r+.

Comment 75

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #74)
> are we using the ifdef: MOZ_SERVICES_SYNC

No, we're not. Spares us preprocessing several files, too.

Comment 76

[Ian Neal]

Comment on attachment 482338 [details] [diff] [review]
integration v2

>+++ b/suite/common/tasksOverlay.xul

>+          <!-- only one of sync-setup or sync-menu will be showing at once -->
Is the comment correct or is it sync-setup and sync-syncnowitem?
>+          <menuitem id="sync-setup"
>+                    label="&syncSetup.label;"
>+                    accesskey="&syncSetup.accesskey;"
>+                    observes="sync-setup-state"
>+                    oncommand="gSyncUI.openSetup();"/>
>+          <menuitem id="sync-syncnowitem"
>+                    label="&syncSyncNowItem.label;"
>+                    accesskey="&syncSyncNowItem.accesskey;"
>+                    observes="sync-syncnow-state"
>+                    oncommand="gSyncUI.doSync(event);"/>

Comment 77

[Ian Neal]

Comment on attachment 482340 [details] [diff] [review]
locales v2b

>+++ b/suite/locales/en-US/chrome/common/aboutSyncTabs.dtd

>+<!-- LOCALIZATION NOTE (tabs.context.openTab.accesskey, tabs.context.openMultipleTabs.accesskey):
>+     Only one of these will show at a time (based on selection), so reusing accesskey is ok. -->
Shouldn't this note also mention tabs.context.bookmarkSingleTab.accesskey and tabs.context.bookmarkMultipleTabs.accesskey?
>+<!ENTITY tabs.context.openTab.label                   "Open This Tab">
>+<!ENTITY tabs.context.openTab.accesskey               "O">
>+<!ENTITY tabs.context.openMultipleTabs.label          "Open Selected Tabs">
>+<!ENTITY tabs.context.openMultipleTabs.accesskey      "O">
>+<!ENTITY tabs.context.bookmarkSingleTab.label         "Bookmark This Tabâ¦">
>+<!ENTITY tabs.context.bookmarkSingleTab.accesskey     "B">
>+<!ENTITY tabs.context.bookmarkMultipleTabs.label      "Bookmark Selected Tabsâ¦">
>+<!ENTITY tabs.context.bookmarkMultipleTabs.accesskey  "B">
>+<!ENTITY tabs.context.refreshList.label               "Refresh List">
>+<!ENTITY tabs.context.refreshList.accesskey           "R">

>+++ b/suite/locales/en-US/chrome/common/pref/pref-sync.dtd

>+<!-- Sync Settings -->
>+<!ENTITY syncPrefsCaption.label       "Browser Sync">
>+<!ENTITY syncComputerName.label       "Computer Name:">
>+<!ENTITY syncComputerName.accesskey   "c">
Nit: Upper case for accesskey to match label.
>+
>+<!ENTITY syncMy.label               "Sync My">
>+<!ENTITY engine.bookmarks.label     "Bookmarks">
>+<!ENTITY engine.bookmarks.accesskey "m">
What is wrong with using B?

>+<!ENTITY engine.prefs.label         "Preferences">
>+<!ENTITY engine.prefs.accesskey     "S">
Can we not use "e" here, if not then should be lower case "s".

>+++ b/suite/locales/en-US/chrome/common/sync/syncGenericChange.properties

>+# LOCALIZATION NOTE (change.synckey.warningText) "Sync" should match &syncBrand.shortName.label; from syncBrand.dtd
Shouldn't this note be more generalised for all occurrences of "Sync" in the file and thus also earlier in the file?

>+++ b/suite/locales/en-US/chrome/common/sync/syncSetup.dtd

>+<!-- New Account AND Existing Account -->
>+<!ENTITY server.label               "Server">
>+<!ENTITY serverType.main.label      "&syncBrand.fullName.label; Server">
>+<!ENTITY serverType.custom.label    "Use a custom server">
>+<!ENTITY signIn.account.label       "Email Address / User Name">
>+<!ENTITY signIn.account.accesskey   "E">
>+<!ENTITY signIn.password.label      "Password">
>+<!ENTITY signIn.password.accesskey  "P">
>+<!ENTITY signIn.serverURL.label     "Server URL">
>+<!ENTITY signIn.serverURL.accesskey "L">
I presume S and U are not available here?

>+<!-- New Account Page 1: Basic Account Info -->
>+<!ENTITY setup.newAccountDetailsPage.title.label "Account Details">
>+<!ENTITY setup.confirmPassword.label  "Confirm Password">
>+<!ENTITY setup.confirmPassword.accesskey  "m">
No earlier letters in "Confirm" available?

>+<!-- Sync Options -->
Same comments as for pref-sync.dtd file

>+++ b/suite/locales/en-US/chrome/common/tasksOverlay.dtd

>+<!ENTITY syncBrand.shortName.label "Sync">
Why is this entity needed, other than for the entity below, if only for below why not just use "Sync" instead below?
>+<!ENTITY syncSetup.label           "Set Up &syncBrand.shortName.label;â¦">
>+<!ENTITY syncSetup.accesskey       "Y">
If only one of syncSetup and syncSyncNowItem (urgh horrible entity name) are shown, can they not share the same accesskey? Otherwise should be lowercase "y".
>+<!ENTITY syncSyncNowItem.label     "Sync Now">
>+<!ENTITY syncSyncNowItem.accesskey "S">

r- due to the number of corrections.
I presume help changes would be a separate patch / spin off bug?

Comment 78

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #70)
> Comment on attachment 482338 [details] [diff] [review]
> integration v2
> 
> >+#sync-notifications notification {
> >+  -moz-binding: url("chrome://communicator/content/sync/syncNotification.xml#notification");
> >+}

(In reply to comment #71)
> Meaning, don't use descendent selectors when child selectors will do ;-)

So you just want me to add a ">" between "#sync-notifications" and "notification", right?

(In reply to comment #76)
> Comment on attachment 482338 [details] [diff] [review]
> integration v2
> 
> >+++ b/suite/common/tasksOverlay.xul
> 
> >+          <!-- only one of sync-setup or sync-menu will be showing at once -->
> Is the comment correct or is it sync-setup and sync-syncnowitem?

The comment is wrong. Fixed locally, will be part of the new patch (for which I first need an answer to the above).

@philiKON: I copied that from FF's browser-menubar.inc where it's still wrong, too. Also in browser.xul both IDs have a "-appmenu" suffix so strictly the comment there is wrong, too.

(In reply to comment #77)
> I presume help changes would be a separate patch / spin off bug?

Absolutely. Just look at the number of comments here. We're far from done with what's already in scope, and given the number of open Help bugs (even important ones, for new features and major changes) I'd be surprised if we could do the Help part of this in time for 2.1. But I'll at least file that follow-up now. Maybe we can achieve minimal coverage, like a short intro + pref pane desc.

Comment 79

[Jens Hatlak (:InvisibleSmiley)]

Attachment: integration v2a

Addressed comment 67 and comment 78.

Comment 80

[Jens Hatlak (:InvisibleSmiley)]

Attachment: about:sync-tabs v2b

Addressed comment 67.

Comment 81

[Jens Hatlak (:InvisibleSmiley)]

Attachment: prefpanel v2a

Fixed View Quota

Comment 82

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dialogs v4a

Fixed View Quota

Comment 83

[Jens Hatlak (:InvisibleSmiley)]

Attachment: locales v2c

(In reply to comment #77)

I checked and changed what I didn't include in this reply.

> >+++ b/suite/locales/en-US/chrome/common/pref/pref-sync.dtd
> >+<!ENTITY engine.prefs.label         "Preferences">
> >+<!ENTITY engine.prefs.accesskey     "S">
> Can we not use "e" here, if not then should be lower case "s".

'e' is available. So is 'r'. Or 'H' for 'History'. Generally I not only just copied from FF (bad excuse, so just ignore that), I also don't get what the logic behind choosing accesskeys is. Are some better than others? I mean other than choosing initials and capitals, and making sure none are used multiple times. Warning: Until I understand it, I'll just implement any suggestions that do not look completely wrong....

> >+++ b/suite/locales/en-US/chrome/common/tasksOverlay.dtd
> 
> >+<!ENTITY syncBrand.shortName.label "Sync">
> Why is this entity needed, other than for the entity below, if only for below
> why not just use "Sync" instead below?
> >+<!ENTITY syncSetup.label           "Set Up &syncBrand.shortName.label;â¦">

Good question. Basically I put it in there because it's also in FF's browser.dtd. No idea whether it's actually (still) used. philiKON? [If we get no answer I vote for keeping it for compatibility--the entity definition, not the use, which I stopped.]

Comment 84

[Robert Kaiser]

(In reply to comment #83)
> I also don't get what the
> logic behind choosing accesskeys is. Are some better than others? I mean other
> than choosing initials and capitals, and making sure none are used multiple
> times.

I think there's a doc somewhere that explains it in more detail, but the meat of it is that every character that either is badly available on international keyboads or where the underline is less visible is bad, all that are easily available and have the underline easily visible are good. Examples for bad ones therefore are yIjiltgr[) while good ones are e.g. YLmneRGTko and many others - starts of significant words are preferred, earlier letters in the string are generally weakly preferred to later ones, and the case should be the same as in the actual string ("S" for "Preferences" is buggy as there is no capital "S" in "Preferences").

Comment 85

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #84)
> Examples for bad ones therefore are yIjiltgr[) while good ones are e.g.
> YLmneRGTko and many others

Thanks for the explanation! I think we should create a Wiki page for these things.

@Ian: From the above I would infer that we should use H for History. Do you concur? (Actually I'll just wait for your review, just wanted to point this one out in particular.)

Comment 86

[Ian Neal]

(In reply to comment #85)
> (In reply to comment #84)
> > Examples for bad ones therefore are yIjiltgr[) while good ones are e.g.
> > YLmneRGTko and many others
> 
> Thanks for the explanation! I think we should create a Wiki page for these
> things.
> 
Already something on MDC - https://developer.mozilla.org/en/XUL_Accesskey_FAQ_and_Policies

> @Ian: From the above I would infer that we should use H for History. Do you
> concur? (Actually I'll just wait for your review, just wanted to point this one
> out in particular.)

H is used for the Help button usually.

Comment 87

[Ian Neal]

Comment on attachment 484838 [details] [diff] [review]
locales v2c

>+++ b/suite/locales/en-US/chrome/common/aboutSyncTabs.dtd
>+<!-- LOCALIZATION NOTE (tabs.context.openTab.accesskey, tabs.context.openMultipleTabs.accesskey;
>+     tabs.context.bookmarkSingleTab.accesskey, tabs.context.bookmarkMultipleTabs.accesskey):
>+     Only one of these will show at a time (based on selection), so reusing accesskey is ok. -->
Perhaps to make it clear: Only one of each of these pairs will show...
r=me with that addressed (either fixed or why it is better to leave as it is).

Comment 88

[Jens Hatlak (:InvisibleSmiley)]

Attachment: locales v2d

(In reply to comment #87)
> r=me with that addressed (either fixed or why it is better to leave as it is).

Actually I was just a little lazy. I guess using a semicolon is not explicit enough around here. ;-)

Comment 89

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dialogs v4b

Including changes from bug 600589, bug 600561, bug 600560 (syncUtils.js part will be in upcoming "common" patch v3a) and bug 600557.

Comment 90

[Jens Hatlak (:InvisibleSmiley)]

Attachment: common v3a

This addresses some/most of the issues brought up in review. I'll upload a new integration patch in a minute which will contain just the move of the syncUI.js in addition to what has already been reviewed.

In the below I left out what's either clear or has been commented on by philiKON in comment 72.

(In reply to comment #69)
> >+    window.addEventListener("unload", function() {
> >+      window.removeEventListener("unload", arguments.callee, false);
> I read somewhere that the JS engine prefers you to name the function.

Better now? I guess that's what you meant, but I can also use a variable for the event listener.

> >+    let popup = null;
> >+    if (popup = document.getElementById("alltabs-popup")) {
> Weird. Just init the var and test it in the normal way please?

Oh, sorry. PHP coding habit (where initializing a block-level variable is just a waste of time, so I wouldn't have the init line there).

> >+    sep.tab = { "linkedBrowser": { "currentURI": { "spec": " " } } };
> Why a space?

No idea at all. Hasn't been discussed in the original bug either. But then it's just a separator. If this is important to you, please suggest an alternative.

> >+  get bundle() {
> >+    delete this.bundle;
> >+    return this.bundle = Services.strings.createBundle("chrome://communicator/locale/sync/syncSetup.properties");
> >+  },
> Nit: should use XPCOMUtils

In case you wonder why I'm no longer using Services.strings: Because I kept it in sync with syncUI.js. I think we should use the same approach in both files, so if you want me to change it here, I'd change it there, too.

> >+  // opens in a new window if we're in a modal prefwindow world, in a new tab otherwise
> I didn't think prefwindow was modal...

Good catch: Ours isn't. Firefox makes it depend on instantApply:
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/utilityOverlay.js#433

> >+var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]  
> >+                       .getService(Components.interfaces.mozIJSSubScriptLoader);
> >+loader.loadSubScript("chrome://communicator/content/sync/syncUI.js");
> Why not add syncUI.js to utilityOverlay.xul?

Well, I was wondering why Firefox uses an inline preprocessor include in the first place, but then they're not making browser-syncui.js available through chrome:// at all. Now that we do, I agree it should be moved and that's what I did. New integration patch upcoming; re-requesting review just for that moved part.

Comment 91

[Jens Hatlak (:InvisibleSmiley)]

Attachment: integration v2b

Moved syncUI.js include from "common" patch.

Comment 92

[[e-mail]]

Comment on attachment 486555 [details] [diff] [review]
common v3a

>+    let self = this;
>+    obs.forEach(function(topic) {
>+      Services.obs.addObserver(self, topic, true);
>+    });
obs.forEach(function(topic) {
  Services.obs.addObserver(this, topic, true);
}, this);

Comment 93

[[e-mail]]

Comment on attachment 484807 [details] [diff] [review]
about:sync-tabs v2b

>+      // Create the client node, but don't add it in-case we don't show any tabs
>+      let appendClient = true;
Nit: comment is inaccurate, as you don't create the client without tabs.

I haven't actually run any builds with these patches; presumably I'd need to install such a build on at least two machines to see the full benefit?

Comment 94

[Jens Hatlak (:InvisibleSmiley)]

Attachment: common v3b

Comment 95

[Jens Hatlak (:InvisibleSmiley)]

Attachment: about:sync-tabs v2c

(In reply to comment #93)
> >+      // Create the client node, but don't add it in-case we don't show any tabs
> >+      let appendClient = true;
> Nit: comment is inaccurate, as you don't create the client without tabs.

OK; I removed the comment altogether since I didn't see much sense in having it around in a fixed form (which would just explain the code: create clients for clients with tabs).

> I haven't actually run any builds with these patches; presumably I'd need to
> install such a build on at least two machines to see the full benefit?

Not necessarily two machines, but at least two profiles with the same Sync account but different Computer Names (to be defined in Sync preferences). Or SM + Minefield, Fennec, ...

Comment 96

[Ian Neal]

With all patches applied when I select the sync pref pane I get in the error console the following two messages:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.import]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://communicator/content/pref/pref-sync.js :: <TOP_LEVEL> :: line 40"  data: no]
Error: gSyncPane is undefined
Source File: chrome://global/content/bindings/preferences.xml
Line: 730

Comment 97

[Jens Hatlak (:InvisibleSmiley)]

Attachment: locales v2e

fixed trailing whitespace errors (removed CRs from syncKey.dtd)

Comment 98

[Jens Hatlak (:InvisibleSmiley)]

Attachment: build v1b

Just unbitrot, carrying over r+.

Comment 99

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #96)
> With all patches applied when I select the sync pref pane I get in the error
> console the following two messages:

I cannot see either of those (and the second is probably a consequence of the first) with what I have locally. I found that the "build" patch didn't apply to tip anymore, though, so I wonder whether you somehow managed to build without it. :-/ Please retry with the updated version v1b.

Comment 100

[Ian Neal]

(In reply to comment #99)
> (In reply to comment #96)
> > With all patches applied when I select the sync pref pane I get in the error
> > console the following two messages:
> 
> I cannot see either of those (and the second is probably a consequence of the
> first) with what I have locally. I found that the "build" patch didn't apply to
> tip anymore, though, so I wonder whether you somehow managed to build without
> it. :-/ Please retry with the updated version v1b.

I had that sorted, I was missing build services patch :(
Couple of things I have noticed (before starting any real testing/reviewing):
1/ When starting up (before going into preferences) both "Set Up Sync" and "Sync Now" appear in the tools menu - this is without sync being setup. If I got into Preferences and select Sync pane then close without doing the setup the "Sync Now" item in the Tools menu has gone.
2/ The Sync pane looks strange with the text in a small box underneath the setup button, I'd expect it to be in a paragraph above the button.

Comment 101

[Jens Hatlak (:InvisibleSmiley)]

Attachment: menu update fix

(In reply to comment #100)
> 1/ When starting up (before going into preferences) both "Set Up Sync" and
> "Sync Now" appear in the tools menu - this is without sync being setup. If I
> got into Preferences and select Sync pane then close without doing the setup
> the "Sync Now" item in the Tools menu has gone.

Thanks for reminding me. That's actually because we're not using the onpopupshowing attribute, and the method that contains the Tools menu updating hook seems not to be called without Sync being set up. In the attached patch (which needs to be applied on top of the rest) I moved the hook to the main init method which is called in any case. An additional fix was needed for MailNews (adding the broadcasterset to the overlay has no effect, so I moved it to the two base files).

> 2/ The Sync pane looks strange with the text in a small box underneath the
> setup button, I'd expect it to be in a paragraph above the button.

Well, exchanging the text and button is simple. If you want further changes, I'll wait for your official review. Suggestions welcome, but the fewer the better: major changes should probably be done in follow-ups so that we don't fall too far behind the FF implementation which I won't port anything from in this bug anymore.

Comment 102

[Jens Hatlak (:InvisibleSmiley)]

Attachment: theming v2a

now with 32x32 toolbar icon (created the throbber by upscaling the 16x16 one, all in GIMP).

Comment 103

[[e-mail]]

Comment on attachment 489615 [details] [diff] [review]
menu update fix

>-      taskPopup.addEventListener("popupshowing", function() {
>-        gSyncUI.updateUI();
>-      }, false);
[We should start using Function.prototype.bind more e.g.
taskPopup.addEventListener("popupshowing", this.updateUI.bind(this), false); ]

Comment 104

[Jens Hatlak (:InvisibleSmiley)]

Attachment: hidden menuitems v1

(In reply to comment #103)
> Comment on attachment 489615 [details] [diff] [review]
> menu update fix
> 
> >-      taskPopup.addEventListener("popupshowing", function() {
> >-        gSyncUI.updateUI();
> >-      }, false);
> [We should start using Function.prototype.bind more e.g.
> taskPopup.addEventListener("popupshowing", this.updateUI.bind(this), false); ]

Well, why not.

I also found that we've been showing both Sync menu items in all places where gSyncUI was not loaded, i.e. basically in all windows with a Tools menu except the main browser and MailNews windows. While we could in theory load gSyncUI in more places, I'm actually favoring only loading it where we also have the Sync toolbar button available, which is the two windows mentioned above. So I chose to hide both menu items by default (including their broadcasters) and let gSyncUI unhide the active one through the broadcaster.

Sorry for the patches on top of each other, I'm still keeping only one Sync mq around here for now.

Comment 105

[[e-mail]]

Comment on attachment 490480 [details] [diff] [review]
hidden menuitems v1

>                     observes="sync-syncnow-state"
>-                    oncommand="gSyncUI.doSync(event);"/>
>+                    oncommand="gSyncUI.doSync(event);"
>+                    hidden="true"/>
And I guess this doesn't get picked up from the broadcaster because not all the windows will overlay the broadcaster either?

Maybe we should have a separate overlay for sync that just applies to the two windows where it's used?

Comment 106

[Philip Chee]

> attachment 489950 [details] [diff] [review]

> now with 32x32 toolbar icon (created the throbber by upscaling the 16x16 one,
> all in GIMP).
Can we get the original vector files from the Firefox/SyncUI team?

Comment 107

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #105)
> >+                    hidden="true"/>
> And I guess this doesn't get picked up from the broadcaster because not all the
> windows will overlay the broadcaster either?

Something like that, yes. Basically we need the attribute on both the broadcasters and the menuitems to make it work everywhere. Unless we don't take that road at all of course (see below).

> Maybe we should have a separate overlay for sync that just applies to the two
> windows where it's used?

Yes, maybe. But I feel that deserved its own bug then because it's pretty self-contained but might require some patch iterations, discussion etc. and I'm tired on having that on this bug. I just want to have something that works and isn't a complete hack. I think that applies to what we'd have with the patch I put up for review. Anything beyond that can go to a follow-up. Feel free to file one for investigating the use of an extra overlay, I'm not opposed to the idea at all! :-)

Comment 108

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #106)
> Can we get the original vector files from the Firefox/SyncUI team?

Well, Philipp might be able to answer that, I can't. I chose to do it myself because I'm a bit impatient. We should of course not be afraid to ask, though.

Comment 109

[Philipp von Weitershausen [:philikon]]

(In reply to comment #108)
> (In reply to comment #106)
> > Can we get the original vector files from the Firefox/SyncUI team?
> 
> Well, Philipp might be able to answer that, I can't. I chose to do it myself
> because I'm a bit impatient. We should of course not be afraid to ask, though.

Stephen Horlander did the Sync logo designs. CCing him.

Comment 110

[Philip Chee]

>>>> now with 32x32 toolbar icon (created the throbber by upscaling the 16x16 one,
>>>> all in GIMP).
>>> Can we get the original vector files from the Firefox/SyncUI team?
>> Well, Philipp might be able to answer that, I can't. I chose to do it myself
>> because I'm a bit impatient. We should of course not be afraid to ask, though.
> 
> Stephen Horlander did the Sync logo designs. CCing him.

We don't necessarily need the originals as long as someone can provide us with the proper 32x32 toolbar icon/throbber generated from the originals.

Comment 111

[Robert Kaiser]

(In reply to comment #110)
> We don't necessarily need the originals as long as someone can provide us with
> the proper 32x32 toolbar icon/throbber generated from the originals.

Shouldn't there be one as the add-on icon?

Comment 112

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #111)
> (In reply to comment #110)
> > We don't necessarily need the originals as long as someone can provide us with
> > the proper 32x32 toolbar icon/throbber generated from the originals.
> 
> Shouldn't there be one as the add-on icon?

This is not about the 32x32 icon. We have that. This is about the throbber version of it. There we (and FF) only have the 16x16 one.

Comment 113

[[e-mail]]

Comment on attachment 490480 [details] [diff] [review]
hidden menuitems v1

OK, r=me if we followup and only add all these elements in appropriate windows.

Comment 114

[Jens Hatlak (:InvisibleSmiley)]

For future reference/Mac, these were the original changesets affecting themes:
http://hg.mozilla.org/mozilla-central/rev/72fbbb3310e7
http://hg.mozilla.org/mozilla-central/rev/9ec78f93036c
http://hg.mozilla.org/mozilla-central/rev/728a4c3f0a53
http://hg.mozilla.org/mozilla-central/rev/6ebe141f304a
http://hg.mozilla.org/mozilla-central/rev/de003082f0e5
http://hg.mozilla.org/mozilla-central/rev/363c8bae0fb2
http://hg.mozilla.org/mozilla-central/rev/f2ee394164cf

Comment 115

[Jens Hatlak (:InvisibleSmiley)]

Lately I'm getting some errors like
  JavaScript component does not have a method named: "batching"'
  when calling method: [nsINavHistoryResultObserver::batching]"
  resource://services-sync/util.js :: batchedSync :: line 172
This is probably because we're missing the browser/ changes from bug 472343 (http://hg.mozilla.org/mozilla-central/rev/4503884294a7). Filed bug 613034 for that.

Comment 116

[Ian Neal]

I know this isn't part of the pref screen but Sync Setup first dialog looks strange with the positioning of the cancel button.
I know modern theme issues will be addressed in bug 612172
The rest of these comments refer to classic theme on Fedora 12 Gnome.
For new account setup in the Account Details dialog, it has a horizontal scroll bar.
When you click on the checkbox for agreeing to terms a small dot appears next to it, is this intentional?

Comment 117

[Ian Neal]

Comment on attachment 484824 [details] [diff] [review]
prefpanel v2a

>+++ b/suite/common/pref/pref-sync.js

>+  prefArray: ["engine.bookmarks", "engine.passwords", "engine.prefs",
>+              "engine.tabs", "engine.history"],
Nit: match this to the order in View Quota dialog.

>+++ b/suite/common/pref/pref-sync.xul

>+    <preferences>
>+      <preference id="engine.bookmarks" name="services.sync.engine.bookmarks" type="bool"/>
>+      <preference id="engine.history"   name="services.sync.engine.history"   type="bool"/>
>+      <preference id="engine.tabs"      name="services.sync.engine.tabs"      type="bool"/>
>+      <preference id="engine.prefs"     name="services.sync.engine.prefs"     type="bool"/>
>+      <preference id="engine.passwords" name="services.sync.engine.passwords" type="bool"/>
>+    </preferences>
Nit: These preferences should appear in the order they appear within this file.

>+        <vbox id="hasAccount">
>+          <groupbox>
>+            <caption label="&accountGroupboxCaption.label;"/>
>+            <grid>
>+              <rows>
>+                <row align="center">
>+                  <label value="&currentAccount.label;" control="currentAccount"/>
>+                  <textbox id="currentAccount" readonly="true">
>+                    <image/>
>+                  </textbox>
>+                  <hbox align="center">
>+                    <button id="connectButton" oncommand="gSyncPane.handleConnectCommand();"/>
This button needs an accesskey, possibly "o" so that it can be used on both Connect and Disconnect.

>+                    <image id="connectThrobber"
>+                           hidden="true"/>
>+                  </hbox>
>+                </row>
>+                <row id="loginFeedbackRow" hidden="true">
>+                  <spacer/>
>+                  <label id="loginError" value=""/>
>+                  <hbox>
>+                    <label class="text-link"
>+                           onclick="gSyncPane.updatePass(); return false;"
>+                           value="&updatePass.label;"/>
>+                    <label class="text-link"
>+                           onclick="gSyncPane.resetPass(); return false;"
>+                           value="&resetPass.label;"/>
>+                  </hbox>
>+                </row>
I've not seen the error message yet, what is the easiest way of generating one or do I have to trick it?
I'm not sure I like "links" instead of buttons, plus buttons are more accessible.

>+                <!-- XXXzpao We should make this behave like the "details" view in CRH.
>+                             do in followup (bug 583441) -->
>+                <row id="manageAccountControls" hidden="true">
>+                  <spacer/>
>+                    <vbox class="indent">
>+                      <label class="text-link"
>+                             onclick="gSyncPane.openQuotaDialog(); return false;"
>+                             value="&viewQuota.label;"/>
>+                      <label class="text-link"
>+                             onclick="gSyncUtils.changePassword(); return false;"
>+                             value="&changePassword.label;"/>
>+                      <label class="text-link"
>+                             onclick="gSyncUtils.resetPassphrase(); return false;"
>+                             value="&mySyncKey.label;"/>
>+                      <label class="text-link"
>+                             onclick="gSyncPane.resetSync(); return false;"
>+                             value="&resetSync.label;"/>
>+                      <label class="text-link"
>+                             onclick="gSyncPane.startOver(true); return false;"
>+                             value="&stopUsingAccount.label;"/>
>+                    </vbox>
>+                  <spacer/>
>+                </row>
>+                <row>
>+                  <spacer/>
>+                  <button id="manageAccountExpander"
>+                          class="expander-down"
>+                          label="&manageAccount.label;"
>+                          accesskey="&manageAccount.accesskey;"
>+                          align="left"
>+                          oncommand="gSyncPane.handleExpanderClick();"/>
>+                  <spacer/>
>+                </row>
I don't like the hide/unhide manage account button, can't we just leave the links/buttons showing?
Shouldn't some of these links/buttons be disabled when not connected?
Again I would prefer buttons with accesskeys to links. They don't have to be in a single column either.

>+                    <checkbox label="&engine.bookmarks.label;"
>+                              accesskey="&engine.bookmarks.accesskey;"
>+                              preference="engine.bookmarks"/>
>+                    <checkbox label="&engine.passwords.label;"
>+                              accesskey="&engine.passwords.accesskey;"
>+                              preference="engine.passwords"/>
>+                    <checkbox label="&engine.prefs.label;"
>+                              accesskey="&engine.prefs.accesskey;"
>+                              preference="engine.prefs"/>
>+                    <checkbox label="&engine.history.label;"
>+                              accesskey="&engine.history.accesskey;"
>+                              preference="engine.history"/>
>+                    <checkbox label="&engine.tabs.label;"
>+                              accesskey="&engine.tabs.accesskey;"
>+                              preference="engine.tabs"/>
Nit: for consistency this list should be the same as order in the View Quota page.

>+          <hbox id="tosPP" pack="center">
>+            <label class="text-link"
>+                   onclick="event.stopPropagation();gSyncUtils.openToS();"
>+                   value="&prefs.tosLink.label;"/>
>+            <label class="text-link"
>+                   onclick="event.stopPropagation();gSyncUtils.openPrivacyPolicy();"
>+                   value="&prefs.ppLink.label;"/>
>+          </hbox>
I can live with these two staying as links.

>+++ b/suite/common/pref/preferences.xul
>@@ -53,16 +53,19 @@
>             style="&prefWindow.size;"
>             windowtype="mozilla:preferences"
>             buttons="accept,cancel,help"
>             autopanes="true"> 
> 
>   <script type="application/javascript" src="chrome://communicator/content/pref/preferences.js"/>
>   <!-- Used by pref-smartupdate, pref-cookies, pref-images and pref-popups -->
>   <script type="application/javascript" src="chrome://communicator/content/permissions/permissionsOverlay.js"/>
>+  <!-- Used by pref-sync -->
>+  <script type="application/javascript"
>+          src="chrome://communicator/content/utilityOverlay.js"/>
Can you add into the comment which part of pref-sync uses this script?

r- for the moment.

Comment 118

[Robert Kaiser]

(In reply to comment #117)
> This button needs an accesskey, possibly "o" so that it can be used on both
> Connect and Disconnect.

I disagree in the choice of accesskey, as that poses a problem for localizations where the same letter might not be available for both variants. If you switch the label, please also switch the accesskey, so localizers can use decent choices for both variants.

Comment 119

[Justin Wood (:Callek)]

(In reply to comment #118)
> (In reply to comment #117)
> > This button needs an accesskey, possibly "o" so that it can be used on both
> > Connect and Disconnect.
> 
> I disagree in the choice of accesskey, as that poses a problem for
> localizations where the same letter might not be available for both variants.
> If you switch the label, please also switch the accesskey, so localizers can
> use decent choices for both variants.

As long as only one option is shown at a time, I don't disagree with the choice being "o". HOWEVER I would _want_ this to have two separate entities to choose the accesskey, for the same l10n reasons KaiRo cites.

Comment 120

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #116)
> I know this isn't part of the pref screen but Sync Setup first dialog looks
> strange with the positioning of the cancel button.
> I know modern theme issues will be addressed in bug 612172
> The rest of these comments refer to classic theme on Fedora 12 Gnome.
> For new account setup in the Account Details dialog, it has a horizontal scroll
> bar.

None of those issues apply to my Win7 build. I'm currently lacking the time to create a Linux build and as you said, that concerns the dialogs which are not up for review yet anyway, as is the themes patch (oh my, will this ever end?).

> When you click on the checkbox for agreeing to terms a small dot appears next
> to it, is this intentional?

No idea. Seems to be appear when the checkbox has focus. Here on Win7 it looks like a visual glitch. Same with Minefield. Maybe the label attached to it (DOMI suggests there is one, even though not in the XUL file).

Comment 121

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #117)
> >+  prefArray: ["engine.bookmarks", "engine.passwords", "engine.prefs",
> >+              "engine.tabs", "engine.history"],
> Nit: match this to the order in View Quota dialog.

Better still: Unused, removed.

@philiKON: You might want to do this for FF, too.

> >+    <preferences>
> >+      <preference id="engine.bookmarks" name="services.sync.engine.bookmarks" type="bool"/>
> >(...)
> Nit: These preferences should appear in the order they appear within this file.

I now applied alphabetic ordering (Bookmarks, History, Passwords, Preferences, Tabs) to the two lists in this file plus syncSetup.xul. See below for more on this.

> >+                    <button id="connectButton" oncommand="gSyncPane.handleConnectCommand();"/>
> This button needs an accesskey, possibly "o" so that it can be used on both
> Connect and Disconnect.

Added locally (two entries in the properties file, both "o" for en-US).

> >+                <row id="loginFeedbackRow" hidden="true">
> >(...)
> I've not seen the error message yet, what is the easiest way of generating one
> or do I have to trick it?

Switch to Offline mode.

> I'm not sure I like "links" instead of buttons, plus buttons are more
> accessible.

See below for more on this.


> >+                  <button id="manageAccountExpander"
> >(...)
> I don't like the hide/unhide manage account button, can't we just leave the
> links/buttons showing?

I think the idea here is to keep this part of the panel short so the list of engines below can grow. Keep in mind that extensions like MailNews Sync can add engines, which should also appear in that list.

> Shouldn't some of these links/buttons be disabled when not connected?

Don't know. Not really an issue for me given my ToDo list, frankly.

> Again I would prefer buttons with accesskeys to links. They don't have to be in
> a single column either.

Well, buttons will probably take more space.

> >+                    <checkbox label="&engine.bookmarks.label;"
> >(...)
> Nit: for consistency this list should be the same as order in the View Quota
> page.

That other list is generated by the Weave service (Weave.Engines.getEnabled()). It seems to be sorted alphabetically, though. At least for en-US we can achieve some consistency, but with other languages we are probably out of luck.

> >+  <!-- Used by pref-sync -->
> >+  <script type="application/javascript"
> >+          src="chrome://communicator/content/utilityOverlay.js"/>
> Can you add into the comment which part of pref-sync uses this script?

Added "(gSyncUtils.*open* -> openUILinkIn)" locally.

> r- for the moment.

Waiting for your consideration of the vertical size thought I brought up before attaching a new patch.

Comment 122

[Ian Neal]

(In reply to comment #121)
> (In reply to comment #117)
> > >+                    <button id="connectButton" oncommand="gSyncPane.handleConnectCommand();"/>
> > This button needs an accesskey, possibly "o" so that it can be used on both
> > Connect and Disconnect.
> 
> Added locally (two entries in the properties file, both "o" for en-US).
Sorry, I did mean two entries both using "o", I should have been clearer.

> > >+                  <button id="manageAccountExpander"
> > >(...)
> > I don't like the hide/unhide manage account button, can't we just leave the
> > links/buttons showing?
> 
> I think the idea here is to keep this part of the panel short so the list of
> engines below can grow. Keep in mind that extensions like MailNews Sync can add
> engines, which should also appear in that list.
That could end up being a fairly long list, so shouldn't we be looking to have it being a button that leads to a dialog containing the list?

> 
> > Shouldn't some of these links/buttons be disabled when not connected?
> 
> Don't know. Not really an issue for me given my ToDo list, frankly.
If you don't want to cover it in this bug then please spin it off into a separate bug. Thanks.
> 
> > Again I would prefer buttons with accesskeys to links. They don't have to be in
> > a single column either.
> 
> Well, buttons will probably take more space.
True, if they were in one column, but if you have 2 or 3 per row then probably about the same amount of space.

Another option would be to have tree children for some of the less used preferences, but you would have to work out to hide those children when sync is not setup.

Is it worth having the Terms of Service and Privacy Policy links showing whether sync is setup or not?

Comment 123

[Robert Kaiser]

(In reply to comment #122)
> That could end up being a fairly long list, so shouldn't we be looking to have
> it being a button that leads to a dialog containing the list?

I'd rather have it the way it is and only go for a different solution once we have cases that actually need that.

Comment 124

[Robert Kaiser]

This is blocking beta2 as discussed in the last status meeting, is this progressing?

Comment 125

[Jens Hatlak (:InvisibleSmiley)]

Attachment: prefpanel v2a add

This is again on top of the existing patches (prefpanel v2a, locales v2e, theming v2a). I'll only update those once I have a r+.

Changes over v2a:
- changed order of prefs
- changed order of desc and button for the noAccount case
- added access key for (dis)connect button
- expanded utilityOverlay.js usage comment
- replaced Manage Account drop-down by label (accesskey and expander gone)
- replaced links by buttons (except for the two at the bottom)
- moved error label and buttons into vbox inside middle column
- shuffled accesskeys around for full coverage
- filed a follow-up bug for disabling links/buttons in case of not being connected

Please be gentle. I was close to throwing the towel several times this week while thinking about this bug and what's still ahead (rather large dialogs patch, Mac changes for theming). I chose to follow the constructive path, but might revise my decision if it gets too tough. One way to put some weight off my shoulders would be to let someone else do the dialogs and/or theming parts. I'm especially concerned about the latter, given how much discussion there was regarding Lightweight Themes Mac issues (which I cannot test but would have to add/prepare).

Still I see this as a core 2.1 feature, and that this must make b2. We already have a list of bugs depending on this one, so pushing the core part further down the road is not an option.

Comment 126

[Jens Hatlak (:InvisibleSmiley)]

Moved the dialogs patch to bug 618709. Note that not all of the patches here apply cleanly anymore due to recent landings.

Comment 127

[Ian Neal]

Attachment: The two prefpanel v2a patches combined

Just so it is easier for me to review

Comment 128

[Ian Neal]

Comment on attachment 495370 [details] [diff] [review]
prefpanel v2a add

I am a lot more comfortable with this patch and any issues, other than the access keys below, can be in followup bugs.

<!-- Login error feedback -->
 <!ENTITY updatePass.label             "Update">
+<!ENTITY updatePass.accesskey         "U">
 <!ENTITY resetPass.label              "Reset">
+<!ENTITY resetPass.accesskey          "s">
 
 <!-- Manage Account -->
 <!ENTITY manageAccount.label          "Manage Account">
-<!ENTITY manageAccount.accesskey      "A">
 <!ENTITY viewQuota.label              "View Quota">
+<!ENTITY viewQuota.accesskey          "V">
 <!ENTITY changePassword.label         "Change Password">
+<!ENTITY changePassword.accesskey     "C">
 <!ENTITY mySyncKey.label              "My Sync Key">
+<!ENTITY mySyncKey.accesskey          "M">
 <!ENTITY resetSync.label              "Reset Sync">
+<!ENTITY resetSync.accesskey          "R">
Change we change this to "e"

 <!ENTITY stopUsingAccount.label       "Stop Using This Account">
+<!ENTITY stopUsingAccount.accesskey   "A">
 
 <!-- Sync Settings -->
 <!ENTITY syncPrefsCaption.label       "Browser Sync">
 <!ENTITY syncComputerName.label       "Computer Name:">
-<!ENTITY syncComputerName.accesskey   "C">
+<!ENTITY syncComputerName.accesskey   "N">
 
 <!ENTITY syncMy.label               "Sync My">
 <!ENTITY engine.bookmarks.label     "Bookmarks">
 <!ENTITY engine.bookmarks.accesskey "B">
 <!ENTITY engine.tabs.label          "Tabs">
 <!ENTITY engine.tabs.accesskey      "T">
 <!ENTITY engine.history.label       "History">
-<!ENTITY engine.history.accesskey   "r">
+<!ENTITY engine.history.accesskey   "y">
Leave this as "r" ("y" is not a very good accesskey)
 <!ENTITY engine.passwords.label     "Passwords">
 <!ENTITY engine.passwords.accesskey "P">
Have this as "w"
 <!ENTITY engine.prefs.label         "Preferences">
 <!ENTITY engine.prefs.accesskey     "e">
Have this as "P"

r=me with those changes.

Thanks for all your hard work, it is much appreciated.

Comment 129

[Jens Hatlak (:InvisibleSmiley)]

Attachment: locales v2f

including the prefpanel accesskey changes, plus one new accesskey for the generic dialog ("Generate" on the Sync Key dialog is now a button) for which I assume r=IanN

Comment 130

[Jens Hatlak (:InvisibleSmiley)]

Attachment: about:sync-tabs v2d

unbitrot

Comment 131

[Jens Hatlak (:InvisibleSmiley)]

Attachment: prefpanel v3

incorporated changes of the -add version into this patch, "locales v2f" and the upcoming and "theming v2b"

Comment 132

[Jens Hatlak (:InvisibleSmiley)]

Attachment: theming v2b

Comment 133

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dialogs v4c

including the first set of changes from bug 618709, in sync with locales v2f

Comment 134

[Jens Hatlak (:InvisibleSmiley)]

Attachment: common v3b + hidden menuitems v1

merged "menu update fix" and "hidden menuitems" into "common" and "integration"

Comment 135

[Jens Hatlak (:InvisibleSmiley)]

Attachment: integration v2b + hidden menuitems v1

merged "menu update fix" and "hidden menuitems" into "common" and "integration"

Comment 136

[Jens Hatlak (:InvisibleSmiley)]

Attachment: prefpanel v3a

Unbitrot after bug 588419 landing. Merged comment above utilityOverlay.js include, now reads:
<!-- Used by pref-smartupdate, pref-security, pref-cookies, pref-images, pref-popups and pref-passwords, as well as pref-sync (gSyncUtils.*open* -> openUILinkIn) -->

Comment 137

[Jens Hatlak (:InvisibleSmiley)]

Attachment: build v1c

This combines "build" v1b and "build services" v1, both of which Callek already reviewed. This is for two reasons:
1. un-bitrot after Standard8 added services-crypto-component.xpt to package-manifest.in for both TB and SM in changeset e65a8ebcdfc2
2. including the changes from m-c changesets 7916ff577de1 (Bug 601645) and a7dea879b4b4 (Bug 618195), which first moved "tier_app_dirs += services/crypto" to toolkit/toolkit-tiers.mk (which we already include in our build.mk), and then replaced "tier_app_dirs += services/sync" by "tier_app_dirs += services".

Since the second change is really minor and I already checked that it compiles and works, I'm carrying over the reviews, i.e. r=Callek.

Comment 138

[Jens Hatlak (:InvisibleSmiley)]

Attachment: common v3c

Changed Sync Key file extension to xhtml as per bug 618709 comment 30. Spares us porting bug 612584.

Comment 139

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 497663 [details] [diff] [review]
theming v2b

Ian, if it helps you I can split this into two parts (images, CSS).

Comment 140

[Stefan [:stefanh]]

Comment on attachment 497668 [details] [diff] [review]
integration v2b + hidden menuitems v1

This patch seems to have bitrottened a bit:
error: patch failed: suite/browser/navigator.xul:479
error: suite/browser/navigator.xul: patch does not apply
error: patch failed: suite/common/utilityOverlay.xul:393
error: suite/common/utilityOverlay.xul: patch does not apply

That is, unless I've misunderstood: I applied the patches starting from the top (this one was the first to fail).

Comment 141

[Jens Hatlak (:InvisibleSmiley)]

Attachment: integration v2c

This is just an un-bitrot of "integration v2b + hidden menuitems v1".

Comment 142

[Stefan [:stefanh]]

Comment on attachment 498769 [details] [diff] [review]
build v1c

This one have also bitrottened:
error: patch failed: configure.in:6744
error: configure.in: patch does not apply
error: patch failed: suite/build.mk:51
error: suite/build.mk: patch does not apply
error: patch failed: suite/confvars.sh:53
error: suite/confvars.sh: patch does not apply
error: patch failed: suite/installer/package-manifest.in:234
error: suite/installer/package-manifest.in: patch does not apply

Comment 143

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #142)
> Comment on attachment 498769 [details] [diff] [review]
> build v1c
> 
> This one have also bitrottened:

Works for me, mcsmurf and Mnyromyr... Do you have an otherwise clean tree / other things applied?

Comment 144

[Stefan [:stefanh]]

Is the order mentioned in comment #140 correct?

Comment 145

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #144)
> Is the order mentioned in comment #140 correct?

It doesn't matter, the patches are fully disjoint. "hg qimport bz://576970" should do (if you set up qimportbz correctly). :-)

Comment 146

[Stefan [:stefanh]]

(In reply to comment #145)
> (In reply to comment #144)
> > Is the order mentioned in comment #140 correct?
> 
> It doesn't matter, the patches are fully disjoint.

Aha, if they don't touch the same files my "git apply" should work. Probably my fault then.

Comment 147

[Stefan [:stefanh]]

Fwiw, it worked - I just used "hg import --no-commit --force" on all attachments. So it must have been my fault.

Comment 148

[Stefan [:stefanh]]

When I start seamonkey with all the patches applied, and open the error console (after the browser window have loaded), I see the following (related) in the error console:
"Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.import]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://communicator/content/utilityOverlay.js :: <TOP_LEVEL> :: line 50"  data: no]"

"Error: Weave is not defined
Source File: chrome://communicator/content/sync/syncUI.js
Line: 116"

(The above "Weave is not defined" error appears every time I open the tools menu)


When I go to the preference window and select "Sync", i see:

"Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.import]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://communicator/content/pref/pref-sync.js :: <TOP_LEVEL> :: line 40"  data: no]"

"Error: gSyncPane is undefined
Source File: chrome://global/content/bindings/preferences.xml
Line: 730"

The panel loads, but nothing happens when I click the "Set Up SeaMonkey Sync" button, except for the "gSyncPane is undefined" error message appearing again.

Comment 149

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #148)
> When I start seamonkey with all the patches applied, and open the error console
> (after the browser window have loaded), I see the following (related) in the
> error console

Neil was able to get it running so I think you either have not all patches applies properly, didn't do a full rebuild (from scratch, clobber, you name it), or it's a Mac issue. Please keep in mind that especially the "build" patch makes changes to configure.in, confvars.sh and package-manifest.in so a simple "make chrome" or similar won't do; you'll probably have to delete the objdir and do a full rebuild ("make -f client.mk build").

Comment 150

[Jens Hatlak (:InvisibleSmiley)]

Hmm, seems I overlooked bug 610307. If we fix it right now, pre-checkin, we don't have to add anything to removed-files.in.

Callek, can I assume r=you for removing the following two lines from suite/installer/package-manifest.in ("build" patch)? No other changes needed. I can provide an updated patch first if you want an explicit review request.

@BINPATH@/components/WeaveCrypto.manifest
@BINPATH@/components/WeaveCrypto.js

Comment 151

[Jens Hatlak (:InvisibleSmiley)]

Attachment: build v1d

Unbitrot after bug 588067 landing, including the changes described in comment 150.

Comment 152

[Justin Wood (:Callek)]

Comment on attachment 504809 [details] [diff] [review]
build v1d

rs=me per c#150

Comment 153

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 497660 [details] [diff] [review]
about:sync-tabs v2d

Just got this in the Error Console when entering some text in the search field on an empty about:sync-tabs page:

Error: currentClient is null
Source File: chrome://communicator/content/aboutSyncTabs.js
Line: 110

Reason:

>+++ b/suite/common/sync/aboutSyncTabs.js
(...)
>+
>+  filterTabs: function(event) {
>+    let val = event.target.value.toLowerCase();
>+    let numTabs = this._tabsList.getRowCount();
>+    let clientTabs = 0;
>+    let currentClient = null;
>+    for (let i = 0; i < numTabs; i++) {
(...)
>+    }
>+    if (clientTabs == 0)
>+      currentClient.hidden = true;
>+  },

If numTabs is 0, the for loop is not entered, in which case clientTabs is still 0, so the last "if" is entered, but at the same time currentClient is still null.

Neil, may I assume r=you for changing the last "if" to "if (currentClient && clientTabs == 0)"?

@philiKON: Same bug in FF (line 111).

Comment 154

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #114)
> For future reference/Mac, these were the original changesets affecting themes:
> http://hg.mozilla.org/mozilla-central/rev/72fbbb3310e7
> http://hg.mozilla.org/mozilla-central/rev/9ec78f93036c
> http://hg.mozilla.org/mozilla-central/rev/728a4c3f0a53
> http://hg.mozilla.org/mozilla-central/rev/6ebe141f304a
> http://hg.mozilla.org/mozilla-central/rev/de003082f0e5
> http://hg.mozilla.org/mozilla-central/rev/363c8bae0fb2
> http://hg.mozilla.org/mozilla-central/rev/f2ee394164cf

I went through those again now, doing a winstripe/pinstripe comparison of the current (!) FF trunk files contained in those changesets. Result:

  0. SM ONLY, NEW:
  sync-16-throbber.png
  sync-32-throbber.png

  1. EQUAL:
  aboutSyncTabs.css
  sync-16.png
  sync-32.png
  sync-bg.png (about:sync-tabs)
  sync-desktopIcon.png (about:sync-tabs)
  sync-mobileIcon.png (about:sync-tabs)
  syncCommon.css
  syncQuota.css
  syncSetup.css

  2. DIFFERENT:
  sync-throbber.png (files used in SM have different name and content, though)

  3. REMOVED THROUGH BUG 590805 -> NEED TO BE REMOVED FROM THEMING PATCH:
  sync-merge.png
  sync-wipeClient.png
  sync-wipeServer.png

  4. SM ONLY, CUSTOMIZABLE TOOLBAR BUTTON -> NEEDS MAC VERSION:
  messenger/primaryToolbar.css
  navigator/navigator.css

  5. SM ONLY, NO MAC VERSION -> NO ACTION NEEDED:
  communicator/prefpanels.css

I already addressed points 3 and 4 locally (i.e. removed the three obsolete images from the patch, including classic/jar.mn, and copied the customizable toolbars changes to the mac/ versions of the respective files). I'm currently waiting for the review of the last dialogs patch over at bug 618709, after which I'd upload a new theming patch version.

Point 1 implies that we could either spare us the creation of special Mac versions now and only create them as needed (in follow-up bugs), or we can create the files as exact copies (question then: only CSS or also the images?). I suggest we go for option 1 to make it quick; adding Mac versions later should be easy (copy + add jar.mn entries).

Comment 155

[Ian Neal]

Comment on attachment 497663 [details] [diff] [review]
theming v2b

I know there is a separate bug for modern, but it would be good to at least get some dialog box sizes sorted for modern within this bug (most visible to me are Server Quota and Change your Sync Key ones).

I think it will be easier to find any other issues once all the patches have landed and more people can test it.

r=me with css for Mac added and something in for modern dialog box sizes

Comment 156

[Ian Neal]

(In reply to comment #153)
> Comment on attachment 497660 [details] [diff] [review]
> about:sync-tabs v2d
> 
> Just got this in the Error Console when entering some text in the search field
> on an empty about:sync-tabs page:
> 
> Error: currentClient is null
> Source File: chrome://communicator/content/aboutSyncTabs.js
> Line: 110
> 
> Reason:
> 
> >+++ b/suite/common/sync/aboutSyncTabs.js
> (...)
> >+
> >+  filterTabs: function(event) {
> >+    let val = event.target.value.toLowerCase();
> >+    let numTabs = this._tabsList.getRowCount();
> >+    let clientTabs = 0;
> >+    let currentClient = null;
> >+    for (let i = 0; i < numTabs; i++) {
> (...)
> >+    }
> >+    if (clientTabs == 0)
> >+      currentClient.hidden = true;
> >+  },
> 
> If numTabs is 0, the for loop is not entered, in which case clientTabs is still
> 0, so the last "if" is entered, but at the same time currentClient is still
> null.
> 
> Neil, may I assume r=you for changing the last "if" to "if (currentClient &&
> clientTabs == 0)"?

Is it worth getting the similar code a few lines early matching? So either both "if (currentClient && clientTabs == 0)" or both with "if (currentClient)" followed by "if (clientTabs ==0)"?

Comment 157

[Jens Hatlak (:InvisibleSmiley)]

Attachment: about:sync-tabs currentClient fix

See comment 153 and comment 156 ("Absolutely!").

Comment 158

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dialogs v5

Bug 618709 is done! :-) This is dialogs v5, which contains all those changes and needs locales v2g (coming up next) and theming v3 for the complete experience.

Comment 159

[Jens Hatlak (:InvisibleSmiley)]

Attachment: locales v2g

Change from bug 618709: choice2.merge.recommended.label is now "(recommended)".

Comment 160

[[e-mail]]

There may be another way of writing it:

filterTabs: function(event) {
  let val = event.target.value.toLowerCase();
  let numTabs = this._tabsList.getRowCount();
  let previousClient = null;
  for (let i = 0; i < numTabs; i++) {
    let item = this._tabsList.getItemAtIndex(i);
    let hide = false;
    if (item.getAttribute("type") == "tab") {
      if (item.getAttribute("url").toLowerCase().indexOf(val) == -1 &&
          item.getAttribute("title").toLowerCase().indexOf(val) == -1)
        item.hidden = true;
      else
        previousClient = null; // don't need to hide the client item
    }
    else if (item.getAttribute("type") == "client") {
      if (previousClient)
        previousClient.hidden = true;
      previousClient = item;
    }
  }
  if (previousClient)
    previousClient.hidden = true;
},

Comment 161

[Jens Hatlak (:InvisibleSmiley)]

Attachment: theming v3

So, the (hopefully) final round of theming. Re-requesting review since I made more changes than anticipated. In detail:

- 3 images removed (see comment 154)
- .text-link border removed (see bug 618709 comment 54)
- .recommended rule added (see bug 618709 comment 59)
- #pickSetupDesc rule added (see bug 618709 comment 52)
- Mac and Modern customizable toolbar rules added
- currentUser rule removed (see bug 595066, which I ported between prefpanels v1 and v2, and later we removed the drop-down altogether)
- Modern additions: all files, but with only the sizes in the dialogs CSS files. Images and aboutSyncTabs.css copied 1:1 (the latter is basically a content page, not chrome, and if further tweaks are needed we can do it in bug 612172 or later)

I think adding all the files to Modern now has the advantage that it's less broken from the start, and things like the artwork are less likely to be fixed any time soon anyway. The other styling can wait until we get to it in bug 612172. However, since that is only my view, I also requested explicit UI review from yet another person to get all this double-checked.

Finally it would be nice if one of our Mac guys (Stefan, Karsten) could check whether this (hopefully) final set of patches works acceptably.

Thanks everyone! :-)

Comment 162

[Stefan [:stefanh]]

(In reply to comment #154)

> Point 1 implies that we could either spare us the creation of special Mac
> versions now and only create them as needed (in follow-up bugs), or we can
> create the files as exact copies (question then: only CSS or also the images?).
> I suggest we go for option 1 to make it quick; adding Mac versions later should
> be easy (copy + add jar.mn entries).

Point 1 sounds reasonable. I tested the earlier versions and didn't see any real issues. I guess I expected pinstripe to differ more. Anyway, I'll try to test the latest theme patch this week (lots of IRL stuff atm), but I don't expect any major issues.

Comment 163

[Stefan [:stefanh]]

Comment on attachment 506280 [details] [diff] [review]
theming v3

Impressive work :-)

I can only spot one issue when testing the mac default theme changes. We definitely need to fix it, but feel free to move it to another bug if you want:

The #sync-button doesn't line up very well with the other toolbar buttons in the toolbar. First of all, the other button icons are 30x30, but the sync one is 32x32. Then, the icon margins are a bit wrong. I made the large icon fit by scaling it down to 30x30 and then removing all margin but a -1px bottom margin.

The small button is ok with respect to size, but again, the icon margins are wrong. I haven't tried, but it looks like it should have 1px more bottom margin.

I'm curious to see whether it's the same on win/nix.

Now for a css nitpick:

+#sync-button > .toolbarbutton-icon {
+  margin: 1px;
+  width: 32px;
+  height: 32px;
+}
+
.
.
.
+toolbar[iconsize="small"] > #sync-button > .toolbarbutton-icon {
+  margin: 1px;

You shouldn't need the margin here - you already have it from the first rule. That is, the margin is wrong anyway, but if you decide to move that to a new bug... ;-)

Another thing: shouldn't there be a choice of a sync button in Addressbook/msgCompose customize as well?

Finally, sorry for being vague here - but at some point when testing I manage to get this error in the console:
Error: document.getElementById("sync-setup-state") is null
Source File: chrome://communicator/content/sync/syncUI.js
Line: 128

Oh, the sync panel in the prefs looks odd to me. I'm mostly thinking of the "Manage Account" section's buttons. Is there any plan on changing that (in another bug, of course)?

Comment 164

[Stefan [:stefanh]]

Oh, and another thing: On mac, you can close all window - but the application is still running with its menubar. Now, when closing all windows the "Sync Now" menuitem disappears from the tools menu. Could also be moved to a separate bug.

Comment 165

[Stefan [:stefanh]]

(In reply to comment #164)
> Oh, and another thing: On mac, you can close all window - but the application
> is still running with its menubar. Now, when closing all windows the "Sync Now"
> menuitem disappears from the tools menu. Could also be moved to a separate bug.

The console error mentioned in comment #163 is probably related to this.

Comment 166

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #160)
> There may be another way of writing it:

I like the removal of the "clientTabs" and "hide" variables (though the latter needs to go completely of course). I wonder whether we should also introduce a local variable for item.getAttribute("type") which is accessed twice.

I'm not so sure about the renaming from "currentClient" to "previousClient", though. "current" fits if you look at the client from the tabs POV which is supported by the method name. Of course, when the ".hidden" lines are reached it is not really the current client anymore. The other way around, "previous" fits better in those cases. I wonder whether we should just use "client" since we only have that one client-related variable anyway.

So my suggestion would look like this:

  filterTabs: function(event) {
    let val = event.target.value.toLowerCase();
    let numTabs = this._tabsList.getRowCount();
    let client = null;
    for (let i = 0; i < numTabs; i++) {
      let item = this._tabsList.getItemAtIndex(i);
      let type = item.getAttribute("type");
      if (type == "tab") {
        if (item.getAttribute("url").toLowerCase().indexOf(val) == -1 &&
            item.getAttribute("title").toLowerCase().indexOf(val) == -1)
          item.hidden = true;
        else
          client = null; // don't need to hide the client item
      }
      else if (type == "client") {
        if (client)
          client.hidden = true;
        client = item;
      }
    }
    if (client)
      client.hidden = true;
  },

Comment 167

[Stefan [:stefanh]]

Btw, there's no "Add a Device" functionality? Note that I'm not a sync expert and it's Monday, so I could have missed something here..

Comment 168

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #163)
> Comment on attachment 506280 [details] [diff] [review]
> theming v3
> 
> Impressive work :-)

Thx. :-)

> I can only spot one issue when testing the mac default theme changes. We
> definitely need to fix it, but feel free to move it to another bug if you want:

Filed bug 628404 for the Mac-specific follow-ups you mentioned.

> I'm curious to see whether it's the same on win/nix.

It's not perfect in either situation (Win/Linux, Classic/Modern, big/small). Basically I thought that since it's an optional icon and this detail should not block the initial version, I'd just go with the 32x32 and 16x16 versions for starters.

> You shouldn't need the margin here - you already have it from the first rule.
> That is, the margin is wrong anyway, but if you decide to move that to a new
> bug... ;-)

Yes please. I wouldn't mind the removal here, but the discussion. Filed bug 628415.

> Another thing: shouldn't there be a choice of a sync button in
> Addressbook/msgCompose customize as well?

Certainly not initially. I was glad when I had the Sync integration working in the browser and the main MailNews window. Fixing bug 612727 might help making the button available for more windows.

> Finally, sorry for being vague here - but at some point when testing I manage
> to get this error in the console:
> Error: document.getElementById("sync-setup-state") is null
> Source File: chrome://communicator/content/sync/syncUI.js
> Line: 128

As you already said, probably Mac-specific (hidden window); cf. bug 628404.

> Oh, the sync panel in the prefs looks odd to me. I'm mostly thinking of the
> "Manage Account" section's buttons. Is there any plan on changing that (in
> another bug, of course)?

Currently not. We first had what the Sync add-on (and FF4b7) provided, then Ian and I agreed on what we have now (and that wasn't so easy already). For now we only have bug 616821 about toggling the disabled state of some links/buttons according to the connection state.

(In reply to comment #167)
> Btw, there's no "Add a Device" functionality? Note that I'm not a sync expert
> and it's Monday, so I could have missed something here..

The initial Sync UI port is mostly what FF4b7 included. The whole "device" and J-PAKE stuff came after that and will be covered in bug 615950 and follow-ups (look at the depends list and you might understand why I tend to call our Sync UI my personal "Neverending Story"). Rest assured that the two words I mentioned above describe my personal minimal goal for 2.1 final additions over the initial version.

Comment 169

[Ian Neal]

Comment on attachment 506280 [details] [diff] [review]
theming v3

r=me any other theming fixes should be able to go into follow-up bugs.

Comment 170

[[e-mail]]

(In reply to comment #166)
>       let type = item.getAttribute("type");
>       if (type == "tab") {
Or you could use a switch.

>         if (client)
>           client.hidden = true;
Maybe add a comment // Hide the client item if it had no visible tabs.

Comment 171

[Jens Hatlak (:InvisibleSmiley)]

Attachment: filterTabs fix

This is on top of about:sync-tabs v2d.

(In reply to comment #170)
> (In reply to comment #166)
> >       let type = item.getAttribute("type");
> >       if (type == "tab") {
> Or you could use a switch.

Thought about that, but got confused: Some languages support strings as case values, some don't. Seems I mistook JavaScript for Java. Switched to switch now. :-)

I found a flaw in your suggestion: Items that have been hidden once never get revealed again (test case: filter, clear filter). Fixed it by re-introducing the "hide" variable and added some comments for future sanity.

Comment 172

[[e-mail]]

Comment on attachment 507247 [details] [diff] [review]
filterTabs fix

>+      let type = item.getAttribute("type");
>+      switch (type) {
Could switch (item.getAttribute("type")) {

>+            client.hidden = true; // Hide the last client.
I'd mention the no visible tabs here too.

Does this patch supersede attachment 506270 [details] [diff] [review]?

Comment 173

[Jens Hatlak (:InvisibleSmiley)]

Attachment: about:sync-tabs v2e

(In reply to comment #172)
> >+      let type = item.getAttribute("type");
> >+      switch (type) {
> Could switch (item.getAttribute("type")) {

Absolutely.

> >+            client.hidden = true; // Hide the last client.
> I'd mention the no visible tabs here too.

Added and harmonized.

> Does this patch supersede attachment 506270 [details] [diff] [review]?

Yes, and the attached "about:sync-tabs v2e" supersedes this one and "about:sync-tabs v2d". :-)

Comment 174

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dialogs v5a

From bug 618709 comment 66:
"Changed the name of a variable called "class" to "classname" in
syncSetup.js locally. Will update "dialogs v5" on the base bug including this
case (and carry forward r+ for it) once we've sorted out the Sync Key changes
here."

Comment 175

[Jens Hatlak (:InvisibleSmiley)]

Attachment: common v3d

Now including the changes from bug 618709 patch "remove license block, use content frame" (license block removed from saved Sync Key XHTML file, links underlined in the print output and potential crash prevented).

Comment 176

[Jens Hatlak (:InvisibleSmiley)]

Attachment: integration v2d

The changes to suite/mailnews/messageWindow.xul and suite/mailnews/messenger.xul from the "menu update fix" patch were missing ever since I did an incomplete merge into the "integration" patch (see comment 135). I didn't notice this since I've always been using my single mega patch here (I've been creating the individual patches using aliases, which missed the two new files). Sorry. Double check FTW!

Comment 177

[Jens Hatlak (:InvisibleSmiley)]

http://hg.mozilla.org/comm-central/rev/6a066cbe2cd0
http://hg.mozilla.org/comm-central/rev/a3c015715a4d
http://hg.mozilla.org/comm-central/rev/dd5d27ec85c7
http://hg.mozilla.org/comm-central/rev/5547e7013edb
http://hg.mozilla.org/comm-central/rev/ca1ca2ca2b68
http://hg.mozilla.org/comm-central/rev/e0b98cea736c
http://hg.mozilla.org/comm-central/rev/450f4ecfd112
http://hg.mozilla.org/comm-central/rev/ddb25b1eeab1
http://hg.mozilla.org/comm-central/rev/c11d58cfe431

/me can't stop smiling. But you won't see it!

Comment 178

[Serge Gautherie (:sgautherie)]

Comment on attachment 507500 [details] [diff] [review]
about:sync-tabs v2e

>diff --git a/suite/common/src/nsSuiteGlue.js b/suite/common/src/nsSuiteGlue.js
>--- a/suite/common/src/nsSuiteGlue.js
>+++ b/suite/common/src/nsSuiteGlue.js
>@@ -73,16 +73,39 @@ SuiteGlue.prototype = {
>+    let enum = Services.wm.getEnumerator("navigator:browser");
>+    while (enum.hasMoreElements()) {
>+      delay += enum.getNext().gBrowser.tabs.length;
>+    }

This busted about 10 xpcshell tests, all platforms.
{
JS Component Loader: ERROR (null):0
                     uncaught exception: 2147500037
JS Component Loader: ERROR resource://gre/components/nsSuiteGlue.js:93
                     SyntaxError: enum is a reserved identifier
}

I'll fix it in bug 628893.

Comment 179

[Axel Hecht [:Pike]]

Comment on attachment 506278 [details] [diff] [review]
locales v2g

The l10n.ini changes
+services_sync = mozilla/services/sync/locales/l10n.ini
lack the details section, ala toolkit.

[include_services_sync]
...

This breaks seamonkey on the l10n dashboard.

Comment 180

[[e-mail]]

Comment on attachment 506280 [details] [diff] [review]
theming v3

Had a look at the Modern theme but some of this applies to Classic too.

>+#tabs-display,
>+#tabsList {
>+  background-color: transparent;
>+  -moz-appearance: none;
>+  margin: 0;
>+}
None of this makes sense for #tabs-display. For #tabsList, only the background colour makes sense; the rest should be implemented via class="plain".

>+#tabsList {
>+  width: 100%;
Already the "default" in XUL. (% usually makes no sense in XUL anyway.)

>+#tabs-display {
>+  background: #fff url(chrome://communicator/skin/sync/sync-bg.png) repeat-x center -80px;
I was going to nit you on #FFFFFF but I found we are already using #FFF (although not #fff!) a few times.

>+  margin-top: 4px;
>+  -moz-margin-start: 2em;
>+  -moz-margin-end: 2em;
Same as margin: 4px 2px 0px;

>+#tabsListHeading {
>+  font-size: 140%;
>+  font-weight: bold;
[Interestingly HTML headings jump from 1.17em to 1.5em, a little too big.]

>+richlistitem[selected="true"],
>+richlistitem:focus {
>+  outline-style: none;
Modern's richlistitems don't use outlines, they change various colours.

>+  background-color: menu;
Not in Modern you don't! (Happens more than once)

>+.title,
>+.clientName {
>+  color: #000000;
>+  font-size: 1.1em;
>+}
>+
>+.title[selected="true"],
>+.url[selected="true"] {
>+  color: inherit;
Can't these just inherit all the time?

>+#connectThrobber {
>+  list-style-image: url("chrome://global/skin/icons/loading_16.png");
chrome://communicator/skin/icons/loading.gif

>@@ -0,0 +1,3 @@
>@@ -0,0 +1,4 @@
>@@ -0,0 +1,4 @@
Work in progress, I take it? ;-)

>+  skin/modern/communicator/aboutSyncTabs.css                       (communicator/aboutSyncTabs.css)
[Technically this belongs on line 15.]

>+#sync-button > .toolbarbutton-icon {
>+  margin: 1px;
>+  width: 32px;
>+  height: 32px;
Margin is wrong and width and height aren't essential.
(Same goes for small icons and both windows.]

Comment 181

[Robert Kaiser]

(In reply to comment #180)
> >+  margin-top: 4px;
> >+  -moz-margin-start: 2em;
> >+  -moz-margin-end: 2em;
> Same as margin: 4px 2px 0px;

You mean |margin: 4px 2em 0px;| ;-)

Comment 182

[Philip Chee]

Add this:
toolbar[iconsize="small"] > toolbarpaletteitem > #sync-button > .toolbarbutton-icon, 
Before this:
> toolbar[iconsize="small"] > #sync-button > .toolbarbutton-icon {
>   margin: 1px;
>   width: 16px;
>   height: 16px;
> }

In classic/mac/modern etc.

Comment 183

[Jens Hatlak (:InvisibleSmiley)]

Attachment: possible l10n.ini fix [Checkin: comment 186]

(In reply to comment #179) 
> The l10n.ini changes
> +services_sync = mozilla/services/sync/locales/l10n.ini
> lack the details section, ala toolkit.

I don't know how to test this, just wrote down what I understood. KaiRo, if you're not an appropriate reviewer for this, please switch the request to someone who is (Callek?).

Comment 184

[Robert Kaiser]

Comment on attachment 507891 [details] [diff] [review]
possible l10n.ini fix [Checkin: comment 186]

I assume that's as it should be, yes.

Comment 185

[Jens Hatlak (:InvisibleSmiley)]

Attachment: theming fixes [Checkin: comment 190]

(In reply to comment #180)

First of all, I'm skipping/ignoring all comments related to about:sync-tabs. See comment 161. Will add your comments to bug 612172. The goal is to close this bug here ASAP, which can only be achieved by concentrating on things that are crystal clear and need no discussion.

> >+#connectThrobber {
> >+  list-style-image: url("chrome://global/skin/icons/loading_16.png");
> chrome://communicator/skin/icons/loading.gif

Oops.

> >@@ -0,0 +1,3 @@
> >@@ -0,0 +1,4 @@
> >@@ -0,0 +1,4 @@
> Work in progress, I take it? ;-)

Yes, see comment 155 and comment 161.

> >+  skin/modern/communicator/aboutSyncTabs.css                       (communicator/aboutSyncTabs.css)
> [Technically this belongs on line 15.]

Maybe, but that list there is not fully sorted either so unless you insist I'll just leave it as it is (this way it at least has a logical locality).

> >+#sync-button > .toolbarbutton-icon {
> >+  margin: 1px;
> >+  width: 32px;
> >+  height: 32px;
> Margin is wrong and width and height aren't essential.
> (Same goes for small icons and both windows.]

(In reply to comment #182)
> Add this:
> toolbar[iconsize="small"] > toolbarpaletteitem > #sync-button >
> .toolbarbutton-icon, 
> Before this:
> > toolbar[iconsize="small"] > #sync-button > .toolbarbutton-icon {
> >   margin: 1px;
> >   width: 16px;
> >   height: 16px;
> > }
> 
> In classic/mac/modern etc.

Combined both requests.

Comment 186

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 507891 [details] [diff] [review]
possible l10n.ini fix [Checkin: comment 186]

http://hg.mozilla.org/comm-central/rev/274087ec9a8f

Comment 187

[Axel Hecht [:Pike]]

(In reply to comment #186)
> Comment on attachment 507891 [details] [diff] [review]
> possible l10n.ini fix [Checkin: comment 186]
> 
> http://hg.mozilla.org/comm-central/rev/274087ec9a8f

That seems to have worked out allright. Thanks. KaiRo, probably a good idea to verify the details, didn't go into that. But the builds succeed now.

Comment 188

[[e-mail]]

(In reply to comment #182)
> Add this:
> toolbar[iconsize="small"] > toolbarpaletteitem > #sync-button > .toolbarbutton-icon,
Ah, so it's not just me then!

Comment 189

[[e-mail]]

Comment on attachment 507970 [details] [diff] [review]
theming fixes [Checkin: comment 190]

Although I haven't looked at the rest of the Classic theme, so you may want to wait just in case I happen to spot something else.

Comment 190

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 507970 [details] [diff] [review]
theming fixes [Checkin: comment 190]

http://hg.mozilla.org/comm-central/rev/57d481be5f59

Any new findings will have to go to new bugs (which I'll create in case review comments pop up here).

Comment 191

[Philip Chee]

> Serge Gautherie (:sgautherie)  2011-12-30 03:24:44 PST
> Blocks: 714251
What does navigator.mozApps have to do with SyncUI?

Comment 192

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Philip Chee from comment #191)
> > Serge Gautherie (:sgautherie)  2011-12-30 03:24:44 PST
> > Blocks: 714251
> What does navigator.mozApps have to do with SyncUI?

See the description of the patch over there. ;-)

Comment 193

[Philip Chee]

> See the description of the patch over there. ;-)
I don't see any direct dependencies there. Removing.