Closed Bug 675822 Opened 14 years ago Closed 14 years ago

First sync desktop status page with upload progress bar

Categories

(Firefox :: Sync, defect)

Product:
Firefox
Component:
Sync
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla10

People

(Reporter: rnewman, Assigned: ally)

References

Details

(Whiteboard: [verified in services])

Attachments

(6 files, 18 obsolete files)

tab screen shot
1.18 MB, image/png
Details
part 1.5 (v1) about:sync-progress
3.39 KB, patch
philikon
: review+
Details | Diff | Splinter Review
Part 2 (v3): Wizard Changes
13.21 KB, patch
philikon
: review+
Details | Diff | Splinter Review
Part 3 (v3) tests
4.23 KB, patch
ally
: review+
Details | Diff | Splinter Review
Part 1 (4.3) sync Progress page
100.66 KB, patch
philikon
: review+
Details | Diff | Splinter Review
fix to broken desktop to desktop v2
2.49 KB, patch
philikon
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: 675826
Assignee: nobody → ally
What needs to happen here: We write a chrome page which per mockups will live in a tab. The setup wizard will open it as soon as it's completed. The page shows a progress meter which is advanced for each sync step. Right now we don't have very fine grained notifications for sync progress, and I would even go as far as making those a follow-up bug. What's in place right now? Each engine sends the following notifications: 1) weave:engine:sync:start before an engine begins syncing 2) weave:engine:sync:status before it begins downloading 3) weave:engine:sync:status before it begins uploading 4) weave:engine:sync:finish or :error when the engine is done Of course, (1) -> (2) will be really quick every single time and on a freshly created account (2) -> (3) will be quick too because there's nothing to download. So for now, as a very crude first implementation, I suggest we watch weave:engine:sync:finish/:error notifications and update the progress meter each time by 1/Weave.Engines.getEnabled().length.
Comment on attachment 564721 [details] [diff] [review] WIP, progress bar tied to upload, no styling, not hooked to end of setup wizard >--- /dev/null >+++ b/browser/base/content/syncProgress.css >+ * The Original Code is aboutHome.xhtml. Not really :) The MPL is a bit stupid for making you fill all these things in. Nevertheless, it's more useful to mention the component that the stuff belongs to rather than repeating the filename, e.g. "Firefox Sync". >+ * The Initial Developer of the Original Code is the Mozilla Foundation. >+ * Portions created by the Initial Developer are Copyright (C) 2010 2011 (please fix in all created files) >--- /dev/null >+++ b/browser/base/content/syncProgress.js ... >+const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; >+Cu.import("resource://gre/modules/Services.jsm"); >+Cu.import("resource://services-sync/main.js"); >+ >+function onLoad(event) >+{ Style nit: open brace at the end of the previous line (please fix everywhere). [1] >+ dump("\nally\nonLoad called.\n"); >+ >+ Services.obs.addObserver(handleFinishOrError, "weave:engine:sync:finish", false); >+ Services.obs.addObserver(handleFinishOrError, "weave:engine:sync:error", false); >+ >+ document.getElementById('uploadProgressBar').max = Weave.Engines.getEnabled().length; Style nit: please use double quotes everywhere. [1] >+function increaseProgressBar() >+{ >+ var incrementSize = 1/ Weave.Engines.getEnabled().length; Actually, if you set max = Weave.Engines.getEnabled().length above, then the increment size would just be 1, right. >+ document.getElementById('uploadProgressBar').value += incrementSize; You're going to refer to this element a bunch of times, so I suggest saving it in a global variable in onLoad: let gProgressBar; function onLoad(event) { ... gProgressBar = document.getElementById("uploadProgressBar"); gProgressBar.max = Weave.Engines.getEnabled().length; } and then in increaseProgressBar you can refer to it as well: function increaseProgressBar() { gProgressBar.value += 1; } >+//ally todo rename ? >+function handleFinishOrError() >+{ >+ dump("\n\nally: finish or erro handler called"); >+ increaseProgressBar() >+} You might as well use increaseProgressBar directly above and kill this. Unless something else will happen here? >+<!DOCTYPE html [ >+ <!ENTITY % htmlDTD >+ PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" >+ "DTD/xhtml1-strict.dtd"> >+ %htmlDTD; >+ <!ENTITY % netErrorDTD >+ SYSTEM "chrome://global/locale/netError.dtd"> >+ %netErrorDTD; I doubt you'll need those strings ;) >+<html xmlns="http://www.w3.org/1999/xhtml"> >+ <head> >+ <title>&syncProgress.pageTitle;</title> >+ >+ <link rel="stylesheet" type="text/css" media="all" >+ href="chrome://browser/content/syncProgress.css"/> In addition to or instead of this stylesheet we will probably need platform specific stylesheets in browser/themes/<platform>/browser/syncProgress.css. This would then be referred to via chrome://browser/skin/syncProgress.css. >+ <script type="text/javascript;version=1.8" >+ src="chrome://browser/content/syncProgress.js"/> Nit: line up 'src' with 'type'. >+ </head> >+ <body dir="&locale.dir;" onload="onLoad(event)" onunload="onUnload(event)"> >+ start >+ <progress id="uploadProgressBar" value="0"/> >+ end Needs content :). For now I would just go with this mockup: https://people.mozilla.com/~faaborg/files/projects/sync/setup-i3/n-m-d%2012%20-%20first%20sync.html which adds a title, a logo, a little of bit text and a button. I don't think we have a version of the Sync logo in the tree that's as large as mockup wants it. In fact, I remember that the Sync setup wizard was supposed to have that on its last page too, but we never managed to put one in. See my 1 year old comment in e.g. [2] LOL. [1] https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide [2] https://mxr.mozilla.org/services-central/source/services-central/browser/themes/pinstripe/browser/syncSetup.css#121
Attached image tab screen shot (obsolete) —
looking for preliminary ui-review on the new set up complete tab.
Attachment #565401 - Flags: feedback?(faaborg)
Comment on attachment 565401 [details] tab screen shot functionally this is fine. Stephen can provide additional details on the styling for in-content pages (we are trying to get them all to match, and also rolling out some new graphics for the background). Otherwise just had some line breaks before and after the confirmation text, and get it to wrap a bit closer so that it doesn't expand to the full width.
Attachment #565401 - Flags: feedback?(faaborg) → feedback+
(In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #5) > Comment on attachment 565401 [details] > tab screen shot > > functionally this is fine. Stephen can provide additional details on the > styling for in-content pages (we are trying to get them all to match, and > also rolling out some new graphics for the background). Otherwise just had > some line breaks before and after the confirmation text, and get it to wrap > a bit closer so that it doesn't expand to the full width. Alex, I pulled the style sheet from the error boxen & the about:robots page. If you are serious about getting them to match, then they should all pull from the same style file. Whose style would you like me to be based off of? IIRC, philikon believes the view in the mocks comes from the addon manager. Would you like me to base it off of that? What exactly do you mean by 'wrap a bit closer'? 'a bit' is an ambiguous quantity. :) How much would you like? Stephen == shorlander? Should I ask him for the next round of UI review?
(In reply to :Ally Naaktgeboren from comment #6) > Alex, I pulled the style sheet from the error boxen & the about:robots page. > If you are serious about getting them to match, then they should all pull > from the same style file. Let's not scope-creep for this bug :). I agree about using as much of the same styles across pages with matching layouts, though in reality stylesheets for XUL (e.g. about:addons, about:permissions), may not apply as well to HTML pages. > Whose style would you like me to be based off of? > IIRC, philikon believes the view in the mocks comes from the addon manager. Right. I think about:addons and the recently landed about:permissions pages are examples of what Faaborg refers to as "in-content pages". In comparision, the error and about:robots pages are much older and thus carry an older style. > What exactly do you mean by 'wrap a bit closer'? 'a bit' is an ambiguous > quantity. :) How much would you like? Ally, if you have a look at the mockups (e.g. https://people.mozilla.com/~faaborg/files/projects/sync/setup-i3/n-m-d%2012%20-%20first%20sync.html), the padding of the box in the center larger. Also, it is more square than in your screenshot.
Stephen: we're going to need feedback on the in-content UI style we want to use on this page. I'm not sure when we are switching over to the new style, so initially we might just want to match the add-ons manager? Either way your call.
Stephen: also, if you'd could make said call by say, tuesday, I'd really appreciate that. Bonus points if you tell me exactly which css rules you'd like. :). It all kind of looks the same to me. :/
Attachment #565984 - Flags: feedback?(philipp)
ally: in case stephen is too busy you can try copying css used in the add-ons manager.
Comment on attachment 565984 [details] [diff] [review] Part 2 (v-1): Wizard Changes (rough & ugly) As discussed IRL, this breaks the Sync Options and Reset Sync functionality. I think at this point it's easier to leave a a dummy <wizardpage/> element at the end of the <wizard> around so that the wizard widget doesn't assume whatever last page we have in our page stack is should send the 'wizardfinish' event. (Please be sure to include that in a comment.) With that we will also never cause the 'wizardfinish' event to occur naturally, so we should rename onWizardFinish to just wizardFinish or finishWizard or something like that and remove the reference from <wizard onwizardfinish>. >- case SETUP_SUCCESS_PAGE: >- this.wizard.canRewind = false; >- this.wizard.canAdvance = true; >- this.wizard.getButton("back").hidden = true; >- this.wizard.getButton("next").hidden = true; >- this.wizard.getButton("cancel").hidden = true; >- this.wizard.getButton("finish").hidden = false; >- this._handleSuccess(); Please also remove the _handleSuccess method as it's no longer needed and the strings it refers to. - case SETUP_SUCCESS_PAGE: - this.wizard.canRewind = false; - this.wizard.canAdvance = true; - this.wizard.getButton("back").hidden = true; - this.wizard.getButton("next").hidden = true; - this.wizard.getButton("cancel").hidden = true; - this.wizard.getButton("finish").hidden = false; - this._handleSuccess(); - if (this.wizardType == "pair") { - this.completePairing(); - } - break; Eeek! That last 'if' has important functionality for the "J-PAKE First" flow. Please don't remove it! It should probably move to onWizardFinish. Please test that it still works once you've done that. > Weave.Svc.Prefs.set(prefs[i], isChecked(prefs[i])); > } > this._handleNoScript(false); > if (Weave.Svc.Prefs.get("firstSync", "") == "notReady") > Weave.Svc.Prefs.reset("firstSync"); > > Weave.Service.persistLogin(); > Weave.Svc.Obs.notify("weave:service:setup-complete"); >- if (this._settingUpNew) >- gSyncUtils.openFirstClientFirstrun(); >- else >- gSyncUtils.openAddedClientFirstrun(); >+ //TODO text on chrome tab should change depending on value of this._settingUpNew ... and probably also this._resettingSync. But please don't leave TODO comments in the code unless you can reference a follow-up bug. A TODO or XXX comment is a bug and should therefore be filed :) > openAddedClientFirstrun: function () { > let url = this._baseURL + "secondrun.html"; > this._openLink(url); > }, You can remove this method from gSyncUtils now since you removed the last reference to it.
Attachment #565984 - Flags: feedback?(philipp)
Comment on attachment 565984 [details] [diff] [review] Part 2 (v-1): Wizard Changes (rough & ugly) I forgot to mention two other things concerning the UX: * The page should be in the inContentWhitelist (see XULBrowserWindow in browser.js) so that we don't show the toolbar or URL for it. Of course, when you have tabs on top, users still get to see the ugly chrome:// URL, so I suggest we register an about:sync-progress alias or similar for it. Only downside is that it will show up in about:about. * We need to figure out what happens when the user leaves that tab open and just forgets about it. It will continue to show 100% of course which is fine. But what should happen on browser restart? Unfortunately we can't exclude the tab from session restore due to technical limitations and we having it close itself as soon as it's loaded is also not viable since the user might have "Load Tabs On Demand" enabled. So I propose that if the page gets loaded outside the first sync context, it hides the progress bar and just says something like Sync is set up and operates periodically. You can close this tab and continue using Firefox.
How about: "Firefox will now automatically sync in the background. You can close this tab and continue using Firefox."
So I propose that if the page > gets loaded outside the first sync context, it hides the progress bar and > just says something like > > Sync is set up and operates periodically. You can close this tab > and continue using Firefox. sounds reasonable. how do we know we just started from a session restore?
(In reply to :Ally Naaktgeboren from comment #15) > So I propose that if the page > > gets loaded outside the first sync context, it hides the progress bar and > > just says something like > > > > Sync is set up and operates periodically. You can close this tab > > and continue using Firefox. > > sounds reasonable. how do we know we just started from a session restore? Any time we're not opening the page from the setup wizard. The 'services.sync.firstSync' preference is used already for various reset values (resetClient, wipeClient, wipeRemote), we could easily add two more values that get set by the wizard: * nextAccount * existingAccount (This way we also already have the stuff in place to support Faaborg's idea for varying text on the syncProgress page.) But for now the syncProgress page would just check whether the 'services.sync.firstSync' pref exists. If it does, show the progress bar. If it doesn't, show the text suggested by Faaborg.
css only updated for pinstripe until ui-review acquired
Attachment #564721 - Attachment is obsolete: true
Attached image tab screen shot (obsolete) —
taken on a mac
Attachment #565401 - Attachment is obsolete: true
Attachment #566124 - Flags: feedback?(faaborg)
Comment on attachment 566123 [details] [diff] [review] Part1 (v2) syncProgress page (paired to screenshot) >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >@@ -4154,17 +4154,17 @@ > status: "", > defaultStatus: "", > jsStatus: "", > jsDefaultStatus: "", > overLink: "", > startTime: 0, > statusText: "", > isBusy: false, >- inContentWhitelist: ["about:addons", "about:permissions"], >+ inContentWhitelist: ["about:addons", "about:permissions", "chrome://browser/content/syncProgress.xhtml"], See comment 13: we probably want to alias the page to "about:sync-progress" anyway, in case people have Tabs On Top enabled and see the URL. Which means that we also need open it via that URL in the part 2 patch. See browser/components/about/AboutRedirector.cpp for the browser about:* map. We want an entry like this: { "sync-progress", "chrome://browser/content/syncProgress.xhtml", nsIAboutModule::ALLOW_SCRIPT }, >diff --git a/browser/base/content/syncProgress.xhtml b/browser/base/content/syncProgress.xhtml >new file mode 100644 >--- /dev/null >+++ b/browser/base/content/syncProgress.xhtml >@@ -0,0 +1,87 @@ >+<?xml version="1.0" encoding="UTF-8"?> >+ >+# ***** BEGIN LICENSE BLOCK ***** >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+# >+# The contents of this file are subject to the Mozilla Public License >+# Version >+# 1.1 (the "License"); you may not use this file except in compliance with >+# the License. You may obtain a copy of the License at >+# http://www.mozilla.org/MPL/ >+# >+# Software distributed under the License is distributed on an "AS IS" >+# basis, >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+# for the specific language governing rights and limitations under the >+# License. >+# >+# The Original Code is firefox Sync. Nit: small case 'f' >+ <body onload="onLoad(event)" onunload="onUnload(event)"> >+ <title>&setup.successPage.title;</title> >+ <div id="floatingBox" class="main-content"> >+ <div id="titleDiv"> >+ <h1> &syncProgress.boxTitle;</h1> Nit: space before the string. Do you make use of the "titleDiv" element anywhere? I don't see it. Seems like you can get rid of it. >+ </div> >+ <div id="successLogo"> >+ <img id="brandSyncLogo" src="chrome://browser/skin/sync-128.png" alt="" /> Please provide 'alt', it's important for people on screen readers. >+ </div> >+ <div id="loadingText"> >+ <span id="blurb"> &syncProgress.textBlurb; </span> >+ </div> You can use <p> instead of a <span> in a <div>, it's better semantic markup. Also nit: spaces around the string. >+ <div id="bottomRow"> >+ <button onclick="closeTab()"> &syncProgress.closeButton; </button> Same nit: spaces around the string. >diff --git a/browser/locales/en-US/chrome/browser/syncProgress.dtd b/browser/locales/en-US/chrome/browser/syncProgress.dtd >new file mode 100644 >--- /dev/null >+++ b/browser/locales/en-US/chrome/browser/syncProgress.dtd >@@ -0,0 +1,10 @@ >+<!ENTITY % brandDTD >+ SYSTEM "chrome://branding/locale/brand.dtd"> >+ %brandDTD; >+ >+<!-- These strings are used in the sync progress upload page --> >+<!ENTITY syncProgress.pageTitle "Your First Sync"> >+<!ENTITY syncProgress.textBlurb "Your data is now being encrypted and uploaded in the background. You can close this tab and continue using &brandShortName;."> >+<!ENTITY syncProgress.closeButton "Close"> >+<!ENTITY syncProgress.boxTitle "Set Up Complete"> We have an existing string for that (also with IMHO the correct spelling for "setup" the noun, as opposed to "set up" the verb): <!ENTITY setup.successPage.title "Setup Complete"> >diff --git a/browser/themes/pinstripe/browser/syncProgress.css b/browser/themes/pinstripe/browser/syncProgress.css >new file mode 100644 >--- /dev/null >+++ b/browser/themes/pinstripe/browser/syncProgress.css >@@ -0,0 +1,43 @@ Nit: missing license header. >+#floatingBox { >+ margin: 4em auto; This means the box isn't actually vertically centered on the page like in the mockup. I'll let Faaborg make the call on this, just pointing it out. >+ max-width: 40em; According to my measurements, this makes the box slightly wider than in the mockup (and by that I mean the aspect ratio is off, I didn't compare absolute values). Once again, just pointing it out, letting Faaborg make the call. Also, with HTML we get to use glorious Times as the default font which is wrong for in-content UI. I'm hoping to handle this upstream in inContentUI.css, see bug 693461 comment 3. >+#progressBar progress { Nit: You can refer to this directly as #uploadProgressBar instead of '#progressBar progress'. Child selectors, especially without a direct child qualifier, are quite slow. >diff --git a/toolkit/themes/pinstripe/global/inContentUI.css b/toolkit/themes/pinstripe/global/inContentUI.css >--- a/toolkit/themes/pinstripe/global/inContentUI.css >+++ b/toolkit/themes/pinstripe/global/inContentUI.css We're fixing this in bug 675822 :)
(In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #8) > Stephen: we're going to need feedback on the in-content UI style we want to > use on this page. I'm not sure when we are switching over to the new style, > so initially we might just want to match the add-ons manager? Either way > your call. Yeah for now we should just match the current add-ons manager style. If it uses IncontentUI.css it should get a lot of the the new style for free when it lands. (In reply to :Ally Naaktgeboren from comment #9) > Stephen: also, if you'd could make said call by say, tuesday, I'd really > appreciate that. Bonus points if you tell me exactly which css rules you'd > like. :). It all kind of looks the same to me. :/ Attachment 566124 [details] looks good. A few thoughts: * The text should use the system font * It might be nice to break the message "Set Up Complete" up from the rest a little bit * HTML Mockups: - Windows: http://people.mozilla.com/~shorlander/incontent-wizard-old-windows/incontentWizard-windows-01.html - Mac: http://people.mozilla.com/~shorlander/incontent-wizard-old-mac/incontentWizard-mac-01.html * The layout feels very centered and a little tall/big, which may be intentional. Faaborg did you consider a more horizontal layout at all? It would be a little more compact and might flow a little better: - Windows: http://people.mozilla.com/~shorlander/incontent-wizard-old-windows/incontentWizard-windows-02.html - Mac: http://people.mozilla.com/~shorlander/incontent-wizard-old-mac/incontentWizard-mac-02.html
(In reply to Stephen Horlander from comment #20) > (In reply to Alex Faaborg [:faaborg] (Firefox UX) from comment #8) > > Stephen: we're going to need feedback on the in-content UI style we want to > > use on this page. I'm not sure when we are switching over to the new style, > > so initially we might just want to match the add-ons manager? Either way > > your call. > > Yeah for now we should just match the current add-ons manager style. If it > uses IncontentUI.css it should get a lot of the the new style for free when > it lands. Indeed that's the plan and what Unfocused said, too. > (In reply to :Ally Naaktgeboren from comment #9) > > Stephen: also, if you'd could make said call by say, tuesday, I'd really > > appreciate that. Bonus points if you tell me exactly which css rules you'd > > like. :). It all kind of looks the same to me. :/ > > Attachment 566124 [details] looks good. A few thoughts: > > * The text should use the system font I'm going to fix this upstream in inContentUI.css in bug 675822.
Attached patch part 3 (v0) about:sync-progress (obsolete) — — Splinter Review
Attachment #566263 - Flags: feedback?(philipp)
Attached image tab screen shot —
Attachment #566124 - Attachment is obsolete: true
Attachment #566124 - Flags: feedback?(faaborg)
Comment on attachment 566263 [details] [diff] [review] part 3 (v0) about:sync-progress >diff --git a/browser/components/about/AboutRedirector.cpp b/browser/components/about/AboutRedirector.cpp >--- a/browser/components/about/AboutRedirector.cpp >+++ b/browser/components/about/AboutRedirector.cpp >@@ -92,16 +92,19 @@ > nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT | > nsIAboutModule::ALLOW_SCRIPT }, > { "robots", "chrome://browser/content/aboutRobots.xhtml", > nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT | > nsIAboutModule::ALLOW_SCRIPT }, > { "sessionrestore", "chrome://browser/content/aboutSessionRestore.xhtml", > nsIAboutModule::ALLOW_SCRIPT }, > #ifdef MOZ_SERVICES_SYNC >+ { "sync-progress", "chrome://browser/content/syncProgress.xhtml", >+ nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT | >+ nsIAboutModule::ALLOW_SCRIPT }, Please remove nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT. Otherwise the page won't run with chrome privileges and can't use stuff like Component.utils. C.f. the comment at the top of that block: Entries which do not have URI_SAFE_FOR_UNTRUSTED_CONTENT will run with chrome privileges. ... Also you need to change the URI that gets opened in syncUtils.js (openFirstSyncProgressPage) to use about:sync-progress.
Attachment #566263 - Flags: feedback?(philipp)
Comment on attachment 566124 [details] tab screen shot looking good, just need to use the default system UI font
Attachment #566124 - Flags: feedback+
precedes the wizard patch.
Attachment #566263 - Attachment is obsolete: true
Attachment #566335 - Flags: feedback?(philipp)
Comment on attachment 566335 [details] [diff] [review] part 1.5 (v1) about:sync-progress This is the same patch that you uploaded before. Forgot to qrefresh perhaps?
Attachment #566335 - Flags: feedback?(philipp)
because clown shoes.
Attachment #566335 - Attachment is obsolete: true
Attachment #566377 - Flags: feedback?
Attachment #566377 - Flags: feedback? → feedback?(philipp)
Comment on attachment 566377 [details] [diff] [review] part 1.5 (v1) about:sync-progress r=me Over to Curtis for the security review for the chrome-privileged about page. Background for Curtis: This adds an about:sync-progress alias for an in-content page that shows a progress meter for the first sync. It needs chrome privileges to register observers for updating the progress meter and read preferences to find out what type of first sync it is.
Attachment #566377 - Flags: feedback?(philipp) → review?(curtisk)
Attached patch Part 1 (v3) syncProgress page (obsolete) — — Splinter Review
Notes on this WIP - blocked on 693461 - css subject to change? ---css in winstripe & gnomestripe are empty until finalized in pinstripe. - div around title left for system font rule to latch onto.
Attachment #566123 - Attachment is obsolete: true
>* The layout feels very centered and a little tall/big, which may be intentional. not super intentional, I was moving through the mockups quickly and mostly just focused on basic functionality. >Faaborg did you consider a more horizontal layout at all? It would be a little more compact >and might flow a little better: yeah that looks good
Attached patch Part 2 (v2): Wizard Changes (obsolete) — — Splinter Review
Attachment #565984 - Attachment is obsolete: true
Attachment #566607 - Flags: feedback?(philipp)
Blocks: 694132
full review to be sched
Whiteboard: [secr: curtisk]
Comment on attachment 566607 [details] [diff] [review] Part 2 (v2): Wizard Changes This came together very nicely! Just nits below. > case "reset": > this._resettingSync = true; > this.wizard.pageIndex = OPTIONS_PAGE; >+ Weave.Svc.Prefs.set("firstSync", "reset"); This is not necessary. We're not going to show the progress bar after Reset Sync. Remove please. > onPageShow: function() { >+ dump("/n/nally--onPageShow: "+this.wizard.pageIndex+"\n\n"); Remove please :) >- onWizardFinish: function () { >+ wizardFinish: function () { > this.setupInitialSync(); > >+ if (this.wizardType == "pair") { >+ this.completePairing(); >+ } Nit: indention of closing brace. >- <wizardpage label="&setup.successPage.title;" >- id="successfulSetup" >- onextra1="gSyncSetup.onSyncOptions()" >- onpageshow="gSyncSetup.onPageShow()"> >- <vbox align="center"> >- <image id="successPageIcon"/> >- </vbox> >- <separator/> >- <description class="normal"> >- <html:span id="firstSyncAction">replace me</html:span> >- <html:strong id="firstSyncActionWarning">replace me</html:strong> >- <html:span id="firstSyncActionChange">replace me</html:span> >- </description> >- <description> >- &continueUsing.label; >- </description> >- <separator flex="1"/> >- </wizardpage> >+ </wizardpage> Nit: Spurious indenting of </wizardpage>. >+# To prevent undesired automatic wizard behavior (Options confirm jumps >+# back into the set up flow) in the last page, there is a dummy page. The >+# WizardFinish() must be called manually. I'm suggesting the following tweaks for clarity: # In terms of the wizard flow shown to the user, the 'syncOptionsConfirm' # page above is not the last wizard page. To prevent the wizard binding from # assuming that it is, we're inserting this dummy page here. This also means # that the wizard needs to always be closed manually via wizardFinish(). >+ <wizardpage id="dummyEndPage"> >+ </wizardpage> No id needed. <wizardpage/> should be enough. >- openAddedClientFirstrun: function () { >- let url = this._baseURL + "secondrun.html"; >+ openFirstSyncProgressPage: function () { >+ let url = "about:sync-progress"; > this._openLink(url); this._openLink("about:sync-progress"); :)
Attachment #566607 - Flags: feedback?(philipp)
Attachment #566607 - Attachment is obsolete: true
Attachment #566659 - Flags: review?(philipp)
Attachment #566659 - Attachment is patch: true
Attachment #566659 - Flags: review?(philipp) → review+
:philikon please take a look at the secreview calendar and pick a date that works for the key players here so we can do a full review of this with secteam. Let me know if you have any quetions and what date/time you all would like. Any date that is just SecReview is an open slot.
Attached file Part 3 (v0.1) tests (WIP, broken) (obsolete) —
Attachment #566696 - Attachment is obsolete: true
Attached patch Part 3 (v1) tests (obsolete) — — Splinter Review
Attachment #566992 - Attachment is obsolete: true
Attachment #567206 - Flags: feedback?(philipp)
Attached patch Part 1 (v3.1) syncProgress page (obsolete) — — Splinter Review
Attachment #566659 - Attachment is obsolete: true
Comment on attachment 567206 [details] [diff] [review] Part 3 (v1) tests Looks good! Just some nits below. General nit: please follow the coding style of opening the brace on the same line as the function definition. (Yes, I realize the file you copied doesn't do that, but just because it violates the coding style doesn't give us permission to do the same.) >+let gTests = [ >+ >+{ >+ desc: "Makes sure the progress bar appears if firstSync pref is set", >+ setup: function () >+ { >+ Services.prefs.setCharPref("services.sync.firstSync", "newAccount"); >+ }, >+ run: function () >+ { >+ let doc = gBrowser.selectedTab.linkedBrowser.contentDocument; >+ let progressBar = doc.getElementById('uploadProgressBar'); >+ >+ ok((progressBar.style.display != "none"), "progress bar should be visible"); Better to use `isnot(a, b, descr)` instead of `ok(a != b, descr)` >+ executeSoon(runNextTest); >+ } >+}, >+ >+{ >+ desc: "Makes sure the progress bar is hidden if firstSync pref is not set", >+ setup: function () >+ { >+ Services.prefs.clearUserPref("services.sync.firstSync"); >+ ok(((Services.prefs.getPrefType("services.sync.firstSync") == >+ Ci.nsIPrefBranch.PREF_INVALID)), "pref DNE" ); Use `is(a, b, descr)` here instead of `ok(a == b, descr)` >+ }, >+ run: function () >+ { >+ let doc = gBrowser.selectedTab.linkedBrowser.contentDocument; >+ let progressBar = doc.getElementById('uploadProgressBar'); >+ >+ ok((progressBar.style.display == "none"), >+ "progress bar should not be visible"); Use is() here, too. >+ executeSoon(runNextTest); >+ } >+}, >+{ >+ desc: "Makes sure the observer updates are reflected in the progress bar", >+ setup: function () >+ { >+ }, >+ run: function () >+ { >+ let doc = gBrowser.selectedTab.linkedBrowser.contentDocument; >+ let progressBar = doc.getElementById('uploadProgressBar'); Nit: Use double quotes. >+ >+ Services.obs.notifyObservers(null, "weave:engine:sync:finish", null); >+ Services.obs.notifyObservers(null, "weave:engine:sync:error", null); >+ >+ let received = progressBar.getAttribute("value"); >+ >+ is(received, 2, "progress bar recieved correct notifications"); >+ executeSoon(runNextTest); >+ } >+}, >+{ >+ desc: "Close button should close tab", >+ setup: function () >+ { >+ }, >+ run: function () >+ { >+ function onTabClosed() { >+ ok(1, "received TabClose notification"); s/1/true/ -- this ain't C :) >+ executeSoon(runNextTest); >+ } >+ let doc = gBrowser.selectedTab.linkedBrowser.contentDocument; >+ let button = doc.getElementById('closeButton'); Nit: Use double quotes. >+ let window = doc.defaultView; >+ gBrowser.tabContainer.addEventListener("TabClose", onTabClosed, false); >+ EventUtils.sendMouseEvent({type: "click"}, button, window); >+ gBrowser.tabContainer.removeEventListener("TabClose", onTabClosed, false); You want to move the `removeEventListener` call into onTabClosed(). Otherwise you're assuming that EventUtils.sendMouseEvent() works synchronously which may how it works right now, but it's not an assumption I'd be comfortable making. >+function test() >+{ >+ waitForExplicitFinish(); >+ >+ // browser-chrome test harness inits browser specifying an hardcoded page >+ // and this causes nsIBrowserHandler.defaultArgs to not be evaluated since >+ // there is a predefined argument. >+ // About:home localStorage is populated with overridden homepage, that is >+ // setup in the defaultArgs getter. >+ // Thus to populate about:home we need to get defaultArgs manually. >+ Cc["@mozilla.org/browser/clh;1"].getService(Ci.nsIBrowserHandler).defaultArgs; >+ >+ // Ensure that by default we don't try to check for remote snippets since that >+ // could be tricky due to network bustages or slowness. >+ let storage = getStorage(); >+ storage.setItem("snippets-last-update", Date.now()); >+ storage.removeItem("snippets"); >+ >+ executeSoon(runNextTest); >+} We don't need any of the about:home specific stuff in there. The test() function can just be: function test () { waitForExplicitFinish(); executeSoon(runNextTest); } >+function getStorage() >+{ >+ let aboutSyncProgressURI = Services.io.newURI("moz-safe-about:sync-progress", null, null); >+ let principal = Components.classes["@mozilla.org/scriptsecuritymanager;1"]. >+ getService(Components.interfaces.nsIScriptSecurityManager). >+ getCodebasePrincipal(Services.io.newURI("about:sync-progress", null, null)); >+ let dsm = Components.classes["@mozilla.org/dom/storagemanager;1"]. >+ getService(Components.interfaces.nsIDOMStorageManager); >+ return dsm.getLocalStorageForPrincipal(principal, ""); >+}; More copypasta that can be yanked :)
Attachment #567206 - Flags: feedback?(philipp)
Attachment #566383 - Attachment is obsolete: true
Attachment #566659 - Attachment is obsolete: false
Attached patch Part 3 (v2) tests (obsolete) — — Splinter Review
Attachment #567206 - Attachment is obsolete: true
Attachment #567514 - Flags: feedback?(philipp)
Comment on attachment 567514 [details] [diff] [review] Part 3 (v2) tests >+ desc: "Makes sure the progress bar is hidden if firstSync pref is not set", >+ setup: function () { >+ Services.prefs.clearUserPref("services.sync.firstSync"); >+ dump("\n\nally-pref type--"+Services.prefs.getPrefType("services.sync.firstSync")+"--\n"); >+ dump("\nally-invalid val--"+Ci.nsIPrefBranch.PREF_INVALID+"--\n"); Remove pls :) >+ is(Services.prefs.getPrefType("services.sync.firstSync"), >+ Ci.nsIPrefBranch.PREF_INVALID, "pref DNE" ); Nit: align 'Ci' with 'Services. >+ desc: "Makes sure the observer updates are reflected in the progress bar", >+ setup: function () { >+ }, >+ run: function () { >+ let doc = gBrowser.selectedTab.linkedBrowser.contentDocument; >+ let progressBar = doc.getElementById("uploadProgressBar"); >+ >+ Services.obs.notifyObservers(null, "weave:engine:sync:finish", null); >+ Services.obs.notifyObservers(null, "weave:engine:sync:error", null); >+ >+ let received = progressBar.getAttribute("value"); >+ >+ is(received, 2, "progress bar recieved correct notifications"); i before e except after c :) r=me with those
Attachment #567514 - Flags: feedback?(philipp) → review+
Attachment #567514 - Attachment is obsolete: true
Attachment #567595 - Flags: review+
Attached patch Part 1 (v4) syncProgress page (obsolete) — — Splinter Review
adjusted for changes in 693461 for the content ui. css does not appear to conflict.
Attachment #567207 - Attachment is obsolete: true
Attachment #567625 - Flags: feedback?(philipp)
Attached patch Part 1 (v4.1) syncProgress page (obsolete) — — Splinter Review
Attachment #567625 - Attachment is obsolete: true
Attachment #567625 - Flags: feedback?(philipp)
Attachment #567636 - Flags: feedback?(philipp)
Attached patch Part 1 (v4.2) syncProgress page (obsolete) — — Splinter Review
Attachment #567636 - Attachment is obsolete: true
Attachment #567636 - Flags: feedback?(philipp)
Attachment #567645 - Flags: feedback?(philipp)
additional winstripe def
Attachment #567645 - Attachment is obsolete: true
Attachment #567645 - Flags: feedback?(philipp)
Attachment #567651 - Flags: feedback?(philipp)
Attachment #567651 - Flags: feedback?(philipp) → review+
Comment on attachment 567651 [details] [diff] [review] Part 1 (4.3) sync Progress page ># HG changeset patch ># Parent 7377922326b79754f2e78128bc4f5af09d3dc5c6 ># User Allison Naaktgeboren <ally@mozilla.com> >675822- First Sync desktop status page with upload progress bar Please make sure to follow the convention for checkin comments for landing: Bug NNN - <bug title>. r=foobar For multi-part bugs as this, it's also ok to do Bug NNN - Part M: <part title>. r=foobar
Attachment #567651 - Attachment is patch: true
Comment on attachment 566377 [details] [diff] [review] part 1.5 (v1) about:sync-progress sec review is done (thanks secteam!), r+ from me. ship it.
Attachment #566377 - Flags: review?(curtisk) → review+
Whiteboard: [secr: curtisk] → [fixed in services]
STR for QA: (Steps To Reproduce) -After account set up (applies to new & existing accounts) - Once the setup is complete, a chrome tab (about:sync-progress) should launch - progress bar updates with 1/# of engines - progress is uneven. some engines take longer than others - progress bar should always move forwards - page should not exit once the progress bar completes - close button should close tab - no setup combination should land users on http://www.mozilla.org/en-US/firefox/sync/firstrun.html or http://www.mozilla.org/en-US/firefox/sync/secondrun.html -If the tab is closed while the progress bar is still running - page should close normally. nothing unusual should happen -If the tab is opened from session restore - the progress bar should not appear - the close button should still work
Two issues of concern: - Page is launched on first client setup. But there is no indication for 1/# of engines, just a progress bar. Also. Once it's complete, shouldn't it indicate so with "Done" or something? - Sync isn't firing on setup completion for added clients. Tested with the three types of client add (Merge, wipe cloud and overwrite local) Besides those, the feature seems to be working. The first can probably be fixed as follow up. However, the second issue is not good and should be fixed.
(In reply to Tracy Walker [:tracy] from comment #54) > - Page is launched on first client setup. But there is no indication for 1/# > of engines, just a progress bar. Also. Once it's complete, shouldn't it > indicate so with "Done" or something? We're sticking to the mockups which don't have any of that. There are some plans to make the strings a bit fancier which is follow-up material. > - Sync isn't firing on setup completion for added clients. Tested with the > three types of client add (Merge, wipe cloud and overwrite local) Thanks for finding that. We found the problem, working on a fix.
Comment on attachment 566659 [details] [diff] [review] Part 2 (v3): Wizard Changes > case EXISTING_ACCOUNT_LOGIN_PAGE: > Weave.Service.account = Weave.Utils.normalizeAccount( > document.getElementById("existingAccountName").value); > Weave.Service.password = document.getElementById("existingPassword").value; > let pp = document.getElementById("existingPassphrase").value; > Weave.Service.passphrase = Weave.Utils.normalizePassphrase(pp); >- if (Weave.Service.login()) >- this.wizard.pageIndex = SETUP_SUCCESS_PAGE; >+ Weave.Svc.Prefs.set("firstSync", "existingAccount"); I should've caught this in review. This pref is set for the wrong page. It needs to be set in EXISTING_ACCOUNT_CONNECT_PAGE.
Comment on attachment 566659 [details] [diff] [review] Part 2 (v3): Wizard Changes > if (!this._resettingSync) { ... >- if (this._settingUpNew) >- gSyncUtils.openFirstClientFirstrun(); >- else >- gSyncUtils.openAddedClientFirstrun(); >+ >+ gSyncUtils.openFirstSyncProgressPage(); >+ window.close(); > } > Weave.Utils.nextTick(Weave.Service.sync, Weave.Service); >+ window.close(); > }, Another thing I missed :(. There are two window.close() calls here. The one in the if block should go.
Attached patch fix to broken desktop to desktop (obsolete) — — Splinter Review
missing commit msg. editor keeps crashing.
Attachment #569190 - Flags: review?(philipp)
Attachment #569190 - Attachment is obsolete: true
Attachment #569190 - Flags: review?(philipp)
Attachment #569198 - Flags: review?(philipp)
Attachment #569198 - Flags: review?(philipp) → review+
verified fix on respin from late 10/24
Whiteboard: [fixed in services] → [verified in services]
Depends on: 754800
Flags: sec-review+
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: