Open
Bug 1003434
Opened 10 years ago
Updated 6 years ago
Add support for about:sync-progress
Categories
(SeaMonkey :: Sync UI, defect)
SeaMonkey
Sync UI
Tracking
(Not tracked)
NEW
People
(Reporter: InvisibleSmiley, Unassigned)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
72.37 KB,
patch
|
Details | Diff | Splinter Review |
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.
Attachment #8414747 -
Flags: review?(neil)
Reporter | ||
Comment 1•10 years ago
|
||
[The actual base URL is http://www.mozilla.com/firefox/sync/ of course - I copied from the location bar after the redirects happened.]
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. ;-)
Reporter | ||
Comment 3•10 years ago
|
||
(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•10 years ago
|
||
(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•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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?
(In reply to neil@parkwaycc.co.uk 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•10 years ago
|
||
(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.]
Reporter | ||
Comment 11•10 years ago
|
||
(In reply to neil@parkwaycc.co.uk 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•10 years ago
|
||
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.
Attachment #8414747 -
Flags: review?(neil) → review-
Reporter | ||
Comment 13•10 years ago
|
||
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.
Attachment #8414747 -
Attachment is obsolete: true
Reporter | ||
Comment 14•10 years ago
|
||
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.
Assignee: jh → nobody
Status: ASSIGNED → NEW
You need to log in
before you can comment on or make changes to this bug.
Description
•