1003434 - Add support for about:sync-progress

Status: NEW

(SeaMonkey :: Sync UI, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: aboutSyncProgress.patch

Having a Sync issue with bookmarks I discovered today that FF has support for about:sync-progress while SM does not. Unfortunately it didn't turn out to be what I had hoped for (real-time Sync status/progress), but I found that we still need it since http://www.mozilla.org/de/firefox/sync/firstrun.html and http://www.mozilla.org/de/firefox/sync/secondrun.html are now 404.

As a neat side effect, this resolves an old TODO:
"xxxInvisibleSmiley - we should really have our own pages since these refer to Firefox in the page contents"

I copied most of the changes and files verbatim from m-c (browser/themes/windows).

We'd probably need this on all branches, but unfortunately this includes l10n additions.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

[The actual base URL is http://www.mozilla.com/firefox/sync/ of course - I copied from the location bar after the redirects happened.]

Comment 2

[rsx11m]

Comment on attachment 8414747 [details] [diff] [review]
aboutSyncProgress.patch

>+++ b/suite/common/sync/aboutSyncProgress.xhtml
>+ <button id="closeButton" onclick="closeTab();">&syncProgress.closeButton;</button>

You'll have to define the "onclick" as an event listener now. ;-)

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

(In reply to rsx11m from comment #2)
> You'll have to define the "onclick" as an event listener now. ;-)

Yeah, that thought crossed my mind (same for onload and onunload I guess). But first and foremost I wanted to have something working. Today I'll first have to prepare the 2.26 website changes, so even if Neil gets to the review meanwhile it'll take me some time before I can get back to this one.

Comment 4

[[e-mail]]

(In reply to Jens Hatlak)
> We'd probably need this on all branches, but unfortunately this includes
> l10n additions.

I guess l10n repackaging won't let you import files from the browser locale...

Comment 5

[[e-mail]]

(In reply to rsx11m from comment #2)
> (From update of attachment 8414747 [details] [diff] [review])
> >+++ b/suite/common/sync/aboutSyncProgress.xhtml
> >+ <button id="closeButton" onclick="closeTab();">&syncProgress.closeButton;</button>
> 
> You'll have to define the "onclick" as an event listener now. ;-)

And onload and onunload of course, but that will at least allow onServiceSync to remove the unload event listener, thus avoiding the need for the try/catch.

Comment 6

[[e-mail]]

Comment on attachment 8414747 [details] [diff] [review]
aboutSyncProgress.patch

>+const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
[You use Cu twice and Ci twice for the same value, and that's it...]

>+let gProgressBar;
>+let gCounter = 0;
[As if "let" makes sense with global variables...]

>+  if (Services.prefs.getPrefType("services.sync.firstSync") != Ci.nsIPrefBranch.PREF_INVALID) {
>+    gProgressBar.hidden = false;
>+  }
>+  else {
>+    gProgressBar.hidden = true;
>+  }
Wow, enjoy negations much?
Either
gProgressBar.hidden = Services.prefs.getPrefType("services.sync.firstSync") == Ci.nsIPrefBranch.PREF_INVALID;
Or maybe, given that the progressbar defaults to visible,
if (Services.prefs.getPrefType("services.sync.firstSync") == Ci.nsIPrefBranch.PREF_INVALID)
  gProgressBar.hidden = true;

>+function onUnload(event) {
>+  cleanUpObservers();
>+}
A function just to call another function?

>+  if (!gCounter &&
>+      Services.prefs.getPrefType("services.sync.firstSync") != Ci.nsIPrefBranch.PREF_INVALID) {
Could use !gProgressBar.hidden here. Wait, if the progress bar is hidden, why are we bothering to set its value, or even add the observers?

>+  gCounter += 1;
My + key is twitching ;-)

>+  gProgressBar.setAttribute("value", gCounter);
[Huh, I wonder why gCounter was created instead of just updating the value directly like bug 675822 comment #3 suggests.]

>+  <!ENTITY % htmlDTD
>+    PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
>+    "DTD/xhtml1-strict.dtd">
>+  %htmlDTD;
I don't think this belongs here.

>+        <button id="closeButton" onclick="closeTab();">&syncProgress.closeButton;</button>
This is going to look wrong on Modern.

>+++ b/suite/themes/modern/communicator/aboutSyncProgress.css
>@@ -0,0 +1,46 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+@import url(chrome://global/skin/inContentUI.css);
This isn't going to work.

>diff --git a/suite/themes/modern/communicator/sync/sync-128.png b/suite/themes/modern/communicator/sync/sync-128.png
[I assume this file has already been optimised.]

Comment 7

[[e-mail]]

OK, so when I tried this, my source browser said that I'd connected, but my target browser didn't respond, and the following errors in the Error Console may be relevant:

Error: [Exception... "basicPassword setter should be not used in BrowserIDManager'basicPassword setter should be not used in BrowserIDManager' when calling method: [nsIRunnable::run]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]

No chrome package registered for chrome://browser/locale/sync.properties

1398890889212	Sync.BrowserIDManager	ERROR	Could not authenticate: no user is logged in

Comment 8

[[e-mail]]

I had a checkout of an old version to which I was able to apply the patch and it opened the first run tab but I didn't see any progress, how do I achieve that?

Comment 9

[rsx11m]

(In reply to [e-mail] from comment #7)
> Error: [Exception... "basicPassword setter should be not used in
> BrowserIDManager'basicPassword setter should be not used in
> BrowserIDManager' when calling method: [nsIRunnable::run]"  nsresult:
> "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "native frame ::
> <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]

That's bug 998807 - Sync account creation or device pairing fails with exception in BrowserIDManager.

Comment 10

[[e-mail]]

(In reply to comment #6)
> (From update of attachment 8414747 [details] [diff] [review])
> > diff --git a/suite/themes/modern/communicator/sync/sync-128.png b/suite/themes/modern/communicator/sync/sync-128.png
> [I assume this file has already been optimised.]
[I assume wrongly.]

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #8)
> I had a checkout of an old version to which I was able to apply the patch
> and it opened the first run tab but I didn't see any progress, how do I
> achieve that?

No idea, sorry. I only tested the page with a profile that already had Sync active. And on top of that, I mostly just copied stuff from FF without checking whether it even works - all in order to fix the problem that the pages we currently direct people to are now 404.

Comment 12

[[e-mail]]

Comment on attachment 8414747 [details] [diff] [review]
aboutSyncProgress.patch

OK, so apart from the previous comments, it also looks as if there are other parts of bug 675822 that need to be ported.

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Attachment: aboutSyncProgress.patch

This addresses only the most simple review nits. Even though I didn't finish it, I thought I'd upload it so at least no effort gets lost.

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

I'm not going to get to this any time soon, and in any case bug 998807 would have to be fixed first.

Beyond that I thought about:sync-progress should really be a page that one should be able to open at any time, even after Sync has been set up - just to see how far the current Sync run progressed. Maybe we'd just need alternate strings for that; didn't check in detail.