Closed Bug 1353571 Opened 7 years ago Closed 7 years ago

init methods from browser-fxaccounts.js and browser-syncui.js participate in the window opening jank

Categories

(Firefox :: Sync, enhancement, P1)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: florian, Assigned: eoger)

References

Details

(Keywords: qawanted)

Attachments

(3 files, 2 obsolete files)

Bug 1353571 part 1 - Remove pref identity.fxaccounts.profile_image.enabled.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1353571 part 3 - Refactor browser-syncui and browser-fxaccounts.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1353571 part 2 - Remove unused customization mode code for the FxA status bar.
59 bytes, text/x-review-board-request
markh
: review+
Details 
On this profile: https://perf-html.io/public/104f627fad48b743da7a6b8fafa5a71dff2ca653/calltree/?jsOnly&range=4.9186_5.5073&thread=0 browser-fxaccounts.js' init() contributes 4.1% of the jank that happens right after a new window is opened, and browser-syncui.js contributes 2.2%.

From looking at the code of these methods, it seems a lot of what is done there will only be needed if the user is going to open the menu panel; could we delay initialization of these UI components until the user hovers the hamburger button?
Yeah, there probably is alot we can do here, and is probably right up eoger's alley :) One clarification, by "jank" do you mean actual jank in terms of events not being processed, or is the concern simply the time it takes? For example, I notice that much of browser-syncui's time if after a couple of promises are resolved, so it's not clear that actually is janky or that the UI is blocked during this time.
(In reply to Mark Hammond [:markh] from comment #1)
> One clarification, by "jank" do you mean actual jank in
> terms of events not being processed, or is the concern simply the time it
> takes?

The former, as shown in the profile I linked where there's a 588ms event processing delay (this profile was taken on a slow laptop with an i3 CPU).

> For example, I notice that much of browser-syncui's time if after a
> couple of promises are resolved, so it's not clear that actually is janky or
> that the UI is blocked during this time.

My guess is that these promises are resolved immediately when it's not the first browser window.

This is a profile of jank when opening a new window, not of opening the first window on startup.
Attachment #8855945 - Flags: feedback?(markh)
This is VERY wip.
We move the observers and the "cached" state in a module shared between all windows and fetch this state on mouseover on the PanelUI button.
I know that Mark probably would want this patch to be more ambitious, which will probably happen in the next iterations, but we have to start somewhere :)
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1
Gijs, you might be able to offer some advice here. We are trying to update the hamburger panel's sync state as late as possible (ie, just before the panel is shown). This patch adds:

> document.getElementById("PanelUI-button").addEventListener("mouseover", () => {
>       this.updateUI();

but I'm not sure that's ideal, especially with a11y in mind. What do you suggest?
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8855945 [details]
Bug 1353571 part 3 - Refactor browser-syncui and browser-fxaccounts.

https://reviewboard.mozilla.org/r/127824/#review130826

In general this looks OK, but as you mention, I think we should take this opportunity to be as aggressive as we can, and we must use the profiler to inform us of how effective we are being. I'm sure Florian will be happy to help you interpret your own profiler tests.

::: browser/base/content/browser-fxaccounts.js:5
(Diff revision 1)
>  /* 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/. */
>  
> +Cu.import("resource://services-sync/State.jsm");

I think we need a better name for this module. UIState might be OK (but there might be something better still)

::: browser/base/content/browser-fxaccounts.js:119
(Diff revision 1)
>      // Bail out if we're already initialized and for pop-up windows.
>      if (this._initialized || !window.toolbar.visible) {
>        return;
>      }
>  
>      for (let topic of this.topics) {

ISTM there might be an opportunity to split the initialization into 2 parts:

* stuff that must be done as the window is created. This is hopefully limited to what we need to show the "needs reauth" badge.

* Almost everything else, done as the panel is shown the first time.

Eg, most observers and these customize events would also fall into that second category. IOW, I think we should try and defer far more than just the updateUI if we can.

We also don't want to fall into a trap of making the second window faster at the cost of making the first window case *slower* - eg, a new module here will almost certainly will do that. We need to be guided by the profiler.

::: browser/base/content/browser-fxaccounts.js:129
(Diff revision 1)
>      gNavToolbox.addEventListener("customizationending", this);
> +    document.getElementById("PanelUI-button").addEventListener("mouseover", () => {
> +      this.updateUI();
> +    });
>  
>      EnsureFxAccountsWebChannel();

I wonder if we should consider getting another bug on file for on-demand web channel initialization?

::: browser/base/content/browser-fxaccounts.js:188
(Diff revision 1)
>      let errorLabel = this.panelUIStatus.getAttribute("errorlabel");
>      let unverifiedLabel = this.panelUIStatus.getAttribute("unverifiedlabel");
>      // The localization string is for the signed in text, but it's the default text as well
>      let defaultTooltiptext = this.panelUIStatus.getAttribute("signedinTooltiptext");
>  
> -    let updateWithUserData = (userData) => {
> +    const status = state.status;

I think this is missing |await|?
Comment on attachment 8855944 [details]
Bug 1353571 part 1 - Remove pref identity.fxaccounts.profile_image.enabled.

https://reviewboard.mozilla.org/r/127822/#review130830

Yeah, I don't see a good reason for this pref, thanks.
Attachment #8855944 - Flags: review?(markh) → review+
Attachment #8855945 - Flags: feedback?(markh) → feedback+
(In reply to Mark Hammond [:markh] from comment #6)
> Gijs, you might be able to offer some advice here. We are trying to update
> the hamburger panel's sync state as late as possible (ie, just before the
> panel is shown). This patch adds:
> 
> > document.getElementById("PanelUI-button").addEventListener("mouseover", () => {
> >       this.updateUI();
> 
> but I'm not sure that's ideal, especially with a11y in mind. What do you
> suggest?

Can we use the popupshowing event instead?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(eoger)
(In reply to Mark Hammond [:markh] from comment #7)
> We also don't want to fall into a trap of making the second window faster at
> the cost of making the first window case *slower* - eg, a new module here
> will almost certainly will do that. We need to be guided by the profiler.

Obviously that's not true though if the module is lazy :) To be clear, I'm not against the module. I'm also not opposed to unifying browser-fxaccounts and browser-syncui (which may or may not make a module less attractive.
(In reply to :Gijs from comment #9)
> Can we use the popupshowing event instead?

I suspect that whatever route we take, there's a possibility that due to async things we might end up updating the UI after it has actually shown. If that's true, it might make sense to find/invent something like gBrowserInit.delayedStartupFinished, but for a window, and have that force initialization - ie, accept a slightly-less-than-ideal experience in the edge-case of hamburger shown immediately on opening in exchange for faster window creation time.
(In reply to Mark Hammond [:markh] from comment #11)
> (In reply to :Gijs from comment #9)
> > Can we use the popupshowing event instead?
> 
> I suspect that whatever route we take, there's a possibility that due to
> async things we might end up updating the UI after it has actually shown. If
> that's true, it might make sense to find/invent something like
> gBrowserInit.delayedStartupFinished, but for a window, and have that force
> initialization - ie, accept a slightly-less-than-ideal experience in the
> edge-case of hamburger shown immediately on opening in exchange for faster
> window creation time.

I haven't looked into this bug (or the patches) in detail. But there's a few things here that seem potentially relevant. In no particular order:

- use a push rather than pull model for the state. This means state should be synchronously available, pushed from whatever handlers we need to have for the sync stuff, rather than asynchronously fetching sync state and thus risking races and other stuff. Then we can easily ensure we update the state of the panel only onpopupshowing, avoiding extra work on window startup.
- avoid triggering reflows from the sync code that changes state in the panel. Ideally by avoiding markup and/or hidden/visible changes for nodes (or ensuring their parents sizes remain the same, in a way that can be determined by the layout code such that it doesn't need to reflow everything else.
- extra JSMs are more problematic in the content than the parent process, though if we can avoid fetching sync state immediately when the initial window opens that would be ideal
- I'm confused by the delayedStartupFinished thing. We already have this, I think? What's wrong with delayed startup finished?
> I'm confused by the delayedStartupFinished thing. We already have this, I
> think? What's wrong with delayed startup finished?

delayedStartupFinished is per-process, right? I'm thinking something for per-window state - eg, I could imagine that listening for popupshowing could be the *only* thing we do on window creation, with other init happening soon after (or by the event handler if necessary)
(In reply to Mark Hammond [:markh] from comment #13)
> > I'm confused by the delayedStartupFinished thing. We already have this, I
> > think? What's wrong with delayed startup finished?
> 
> delayedStartupFinished is per-process, right?

It feels like we're talking about different things. https://dxr.mozilla.org/mozilla-central/rev/f914d40a48009c5acd1093e9939cc0ec035696dd/browser/base/content/browser.js#1249 - gBrowserInit._delayedStartup, and its resulting finished bool, is per-*parent-window*. Not per-process. Do we have a different delayedStartupFinished?
Flags: needinfo?(markh)
Yes, I misunderstood the semantics of that.
Flags: needinfo?(markh)
No longer blocks: photon-performance-triage
Whiteboard: [photon]
> Can we use the popupshowing event instead?
Yes

The work I just posted I still WIP. No need to review for now, consider this a work-backup.
I still intend to merge browser-fxa and browser-syncui.

Notable changes include:
- State is fetched synchronously, which removes a bunch of edge cases and makes everything easier to understand. This is a trade-off of course, because the first time we fetch the state we basically block on I/O. Maybe not a great idea, since the first time we fetch the state is in browser-fxa init().
- Remove the un-used customization mode code.
Flags: needinfo?(eoger)
(In reply to Edouard Oger [:eoger] from comment #20)
> > Can we use the popupshowing event instead?
> Yes
> 
> The work I just posted I still WIP. No need to review for now, consider this
> a work-backup.
> I still intend to merge browser-fxa and browser-syncui.
> 
> Notable changes include:
> - State is fetched synchronously, which removes a bunch of edge cases and
> makes everything easier to understand. This is a trade-off of course,
> because the first time we fetch the state we basically block on I/O. Maybe
> not a great idea, since the first time we fetch the state is in browser-fxa
> init().

Block synchronously (main thread) or asynchronously (panel won't show until we have the data) ?

We shouldn't add main-thread IO in an effort to fix this perf bug.
Flags: needinfo?(eoger)
Yes, I discussed this with Mark yesterday, this will go away in favor of browser-fxa calling a UIState method that roughly translates to "please tell me if I need to show the badge". UIState eventually sends back an observer notification at some point in the future.
We assume by default that we don't need to show a badge since most users will not encounter the problem.
Flags: needinfo?(eoger)
Here's a new shot at this.
Notable changes:

*
The most important logic part is in setTimeout() in gSync#init. Basically we wait 1 sec before requesting the UI state.
  - If we are the first window, ensureReady will be false and we will get an ON_UPDATE message shortly after UIState initializes (side effect of ensureReady).
  - For the subsequent windows, we only update the UI if we are in a "sync configured" state, otherwise we do not need to update the UI at all, because it's already in the right state (hence the part 3 of this patch).
* browser-fxa and browser-syncui merged into browser-sync. Yay!
* We listen to WAY WAY less observer notifications now (see UIState).
  If we have regressions, we can always add them back later, this is OK.
  We DEFINITELY want extensive QA testing if this patch sticks, because we could break many things.
* The UIState now tracks the last sync date and the current syncing status.
* I kept the broadcasters,  we can revisit that later if we want to. This is not critical.
(In reply to Edouard Oger [:eoger] from comment #32)
> Here's a new shot at this.
> Notable changes:
> 
> *
> The most important logic part is in setTimeout() in gSync#init. Basically we
> wait 1 sec before requesting the UI state.

Could this use https://developer.mozilla.org/en-US/docs/Web/API/Window/requestIdleCallback instead of the timeout?

I've recently seen a profile (on a slow netbook) where a 10s timeout wasn't enough for the download manager's delayed initialization to run after we were fully done with startup.
That sounds like a great idea, we will also need to confirm using the profiler that we are getting the behavior that we want using this.
https://perf-html.io/public/45d4b30af3ae2216f905291d7a978d65a8a738d8/calltree/?jsOnly&range=1.6624_1.8778&thread=0 (search for browser-sync)

Notes:
- FxAccountsWebChannel is the main problem now (19ms).
- Pulling FxAccountsCommon is expensive, I replaced FxAccountsCommon.ON* observers by their string value directly.
- Importing UIState is pretty cheap (~1ms).
Comment on attachment 8859668 [details]
Bug 1353571 part 5 - Merge browser-fxaccounts and browser-syncui in browser-sync.

https://reviewboard.mozilla.org/r/131684/#review135152

this looks great!

::: browser/base/content/browser-sync.js:70
(Diff revision 1)
> +  get sendTabToDeviceEnabled() {
> +    return Services.prefs.getBoolPref("services.sync.sendTabToDevice.enabled");
> +  },
> +
> +  get remoteClients() {
> +    return Weave.Service.clientsEngine.remoteClients

Where does "Weave" come from? I can't see an import that will make it available.

::: browser/base/content/browser-sync.js:95
(Diff revision 1)
> +    EnsureFxAccountsWebChannel();
> +
> +    setTimeout(() => {
> +      if (UIState.ensureReady()) {
> +        const state = UIState.get();
> +        // The UI is already in the right state, optimize this.

this comment is a little confusing

::: browser/base/content/browser-sync.js:186
(Diff revision 1)
> +      this.panelUIStatus.setAttribute("tooltiptext", tooltipDescription);
> +    } else {
> +      this.panelUIFooter.setAttribute("fxastatus", "signedin");
> +      this.panelUILabel.setAttribute("label", state.email);
> +    }
> +    this.panelUIFooter.setAttribute("fxaprofileimage", "enabled");

does this attribute need to be removed when not configured (ie, after a disconnect)?

::: browser/base/content/browser-sync.js:268
(Diff revision 1)
> +
> +    PanelUI.hide();
> +  },
> +
> +  openPreferences() {
> +    openPreferences("paneSync", { urlParams: { entrypoint: "menupanel" } });

this looks odd - maybe we should either add a comment or an explicit "window." to make it clear we aren't recursing? :)

::: browser/base/content/browser-sync.js:455
(Diff revision 1)
> +      setTimeout(() => Weave.Service.errorHandler.syncAndReportErrors(), 0);
> +    }
> +    Services.obs.notifyObservers(null, "cloudsync:user-sync");
> +  },
> +
> +  // Handle clicking the toolbar button - which either opens the Sync setup

I guess you copied this comment, but I think we should change "the toolbar button" to something like "the sync button in the menu" or similar.

::: browser/base/content/browser-sync.js:604
(Diff revision 1)
> +  },
> +
> +  // Note that we don't show login errors in a notification bar here, but do
> +  // still need to track a login-failed state so the "Tools" menu updates
> +  // with the correct state.
> +  loginFailed() {

this doesn't seem to have a caller?

::: browser/base/content/global-scripts.inc:41
(Diff revision 1)
>  
>  #ifdef MOZ_DATA_REPORTING
>  <script type="application/javascript" src="chrome://browser/content/browser-data-submission-info-bar.js"/>
>  #endif
>  
> -<script type="application/javascript" src="chrome://browser/content/browser-fxaccounts.js"/>
> +<script type="application/javascript" src="chrome://browser/content/browser-sync.js"/>

I think this may as well be moved to where browser-syncui was
Comment on attachment 8858564 [details]
Bug 1353571 part 2 - Remove unused customization mode code for the FxA status bar.

https://reviewboard.mozilla.org/r/130532/#review135168
Attachment #8858564 - Flags: review?(markh) → review+
Comment on attachment 8858565 [details]
Bug 1353571 part 3 - Display a not-logged-in state by default in the fxa status bar.

https://reviewboard.mozilla.org/r/130534/#review135170

This looks fine but I don't think it needs to be in its own patch (ie, I think adding it to the "merge -fxaccounts and -syncui" patch makes more sense.
Comment on attachment 8855945 [details]
Bug 1353571 part 3 - Refactor browser-syncui and browser-fxaccounts.

https://reviewboard.mozilla.org/r/127824/#review135172

::: services/sync/modules/UIState.jsm:26
(Diff revision 4)
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts",
> +                                  "resource://gre/modules/FxAccounts.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Async",

this looks unused

::: services/sync/modules/UIState.jsm:31
(Diff revision 4)
> +XPCOMUtils.defineLazyModuleGetter(this, "Async",
> +                                  "resource://services-common/async.js");
> +XPCOMUtils.defineLazyModuleGetter(this, "Weave",
> +                                  "resource://services-sync/main.js");
> +XPCOMUtils.defineLazyGetter(this, "FxAccountsCommon", () =>
> +  Components.utils.import("resource://gre/modules/FxAccountsCommon.js", {})

Use Cu.import here - although IIUC you said you were going to avoid using this module entirely here?

::: services/sync/modules/UIState.jsm:33
(Diff revision 4)
> +XPCOMUtils.defineLazyModuleGetter(this, "Weave",
> +                                  "resource://services-sync/main.js");
> +XPCOMUtils.defineLazyGetter(this, "FxAccountsCommon", () =>
> +  Components.utils.import("resource://gre/modules/FxAccountsCommon.js", {})
> +);
> +XPCOMUtils.defineLazyGetter(this, "log", () => {

I'm a little torn here, but I don't think this will end up in sync logs which removes alot of the benefits in using log.jsm. Scanning this module, it seems that Cu.reportError() might be fine for now and we could tweak it in a later bug if we felt it necessary.

(alternatively, just use the log named "browserwindow.syncui" - it's not *quite* correct, but policies.js already arranges for that log to go to the sync log files)

::: services/sync/modules/UIState.jsm:82
(Diff revision 4)
> +    return Object.assign({}, this._state, { syncing: this._syncing });
> +  },
> +
> +  ensureReady() {
> +    if (!this._initialized) {
> +      this.init(); // => Will send ON_UPDATE

this is a promise so will return before things are truly initialized.

::: services/sync/modules/UIState.jsm:188
(Diff revision 4)
> +    } catch (e) {
> +      // This is most likely in tests, were we quickly log users in and out.
> +      // The most likely scenario is a user logged out, so reflect that.
> +      // Bug 995134 calls for better errors so we could retry if we were
> +      // sure this was the failure reason.
> +      FxAccountsCommon.log.error("Error updating FxA account info", e);

You should probably just use log here (or see comments above - maybe we can kill the logs completely?)

::: services/sync/modules/UIState.jsm:214
(Diff revision 4)
> +      state.lastSync = Services.prefs.getCharPref("services.sync.lastSync");
> +    }
> +  },
> +
> +  _calculateStateChecksum(state) {
> +    let desc = `${state.status}|${state.email}|${state.displayName}|

I'm still slightly skeptical of this - I wonder if it might be better to instrument this code to see how often we get notified when the state hasn't actually changed and see if we can optimize that further, particularly for the common startup cases (eg, the fact we might send a couple of redundant notifications in edge-cases such as "account needs reauth" isn't really a concern IMO)
Comment on attachment 8859668 [details]
Bug 1353571 part 5 - Merge browser-fxaccounts and browser-syncui in browser-sync.

https://reviewboard.mozilla.org/r/131684/#review135174

::: browser/base/content/browser-sync.js:92
(Diff revision 1)
> +
> +    this.maybeMoveSyncedTabsButton();
> +
> +    EnsureFxAccountsWebChannel();
> +
> +    setTimeout(() => {

I assume this is where you'd use :florian's suggestion? If so, it seems like alot more could go into this block and we'd still want onpopupshowing to "pre-empt" the idle timer with an explicit call - eg, something like:

init() {
  ... stuff we really must do now - eg, webchannel?
  setupIdleTimer(blah, this._reallyInit);
  button.addEventListener("popupshowing", this._reallyInit);
},
_reallyInit() {
  if (haveBeenHere) return;
  etc
}
This is looking great, thanks! I'd be inclined to fold the last 3 patches together, but don't really mind.
Blocks: 1358648
Attachment #8858565 - Attachment is obsolete: true
Attachment #8859668 - Attachment is obsolete: true
I think this is ready for a formal review now.
The previous comments have been addressed and I added tests for both browser-sync and UIState.
Notes from talking with Markh today:
- Remove the panelUI event listener
- Make FxAccountsWebChannelHelper lazy in FxAccountsWebChannel, and stop pulling FxAccountCommon.
Comment on attachment 8855945 [details]
Bug 1353571 part 3 - Refactor browser-syncui and browser-fxaccounts.

https://reviewboard.mozilla.org/r/127824/#review137970

Nice work - this looks great, thanks!

There are quite a few comments - feel free to ask me to look again if you aren't 100% confident in the changes I requested, but also feel free to make the changes and push it if you are.

::: browser/base/content/browser-sync.js:74
(Diff revision 6)
> +    return Weave.Service.clientsEngine.remoteClients
> +           .sort((a, b) => a.name.localeCompare(b.name));
> +  },
> +
> +  init() {
> +    // Bail out if we're already initialized and for pop-up windows.

nit - I think s/and/or/ reads better in this comment.

::: browser/base/content/browser-sync.js:83
(Diff revision 6)
> +
> +    for (let topic of this._obs) {
> +      Services.obs.addObserver(this, topic, true);
> +    }
> +
> +    // initial label for the sync buttons.

I assume that we can't set the sync-status attributes in initUI due to the "tools" menu? If so, I think it might be worth both renaming initUI to, say, initHamburgerUI or similar.

(hrm, but initUI calls updateAllUI, so I guess it isn't true to say it only updates the hamburger menu - so maybe just a comment to indicate why you are updating *some* UI in init()? Your call)

::: browser/base/content/browser-sync.js:363
(Diff revision 6)
> +      return true;
> +    }
> +  },
> +
> +  updateTabContextMenu(aPopupMenu, aTargetTab) {
> +    if (!this.sendTabToDeviceEnabled ||

this looks like it would fit on one line

::: browser/base/content/browser-sync.js:376
(Diff revision 6)
> +    ["context_sendTabToDevice", "context_sendTabToDevice_separator"]
> +    .forEach(id => { document.getElementById(id).hidden = !showSendTab });
> +  },
> +
> +  initPageContextMenu(contextMenu) {
> +    if (!this.sendTabToDeviceEnabled ||

ditto - one line

::: browser/base/content/browser-sync.js:441
(Diff revision 6)
> +  },
> +
> +  // doSync forces a sync - it *does not* return a promise as it is called
> +  // via the various UI components.
> +  doSync() {
> +    const state = UIState.get();

It seems possible there's a race here if this ends up getting hit before the state module has finished initializing. That might be OK because the commands which call this should be disabled in that case - but if that's true it seems like we don't need to check |state| at all?  (ie, it's either true that we must be signed in for this to ever be called (in which case we don't need to check the state), or it isn't (in which case we need to handle UIState.get() returning null))

::: browser/base/content/browser-sync.js:469
(Diff revision 6)
> +      // It is placed somewhere else - just try and show it.
> +      PanelUI.showSubView("PanelUI-remotetabs", anchor, area);
> +    }
> +  },
> +
> +  /* After Sync is initialized we perform a once-only check for the sync

I'm sure this comment was copied, but it's not quite correct - "After sync is initialized" should probably read something like "After we are initialized"...)

::: browser/base/content/browser-sync.js:503
(Diff revision 6)
> +    // 1/2 FxA related - so for some strings we use Sync strings, but for
> +    // others we reach into gSync for strings.
> +    let tooltiptext;
> +    if (status == UIState.STATUS_NOT_VERIFIED) {
> +      // "needs verification"
> +      tooltiptext = gSync.strings.formatStringFromName("verifyDescription", [state.email], 1);

I think we should rename "strings" to "fxastrings" or similar, and also that this function should be consistent re use of |this| vs |gSync|.

::: browser/base/content/browser-sync.js:578
(Diff revision 6)
> +    Ci.nsIObserver,
> +    Ci.nsISupportsWeakReference
> +  ])
> +};
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "EnsureFxAccountsWebChannel",

I think it would be clearer (and have no side-effects) if these were moved to the top of the file?

::: browser/base/content/browser.js
(Diff revision 6)
>  
>      // initialize the private browsing UI
>      gPrivateBrowsingUI.init();
>  
> -    // initialize the sync UI
> -    gSyncUI.init();

I meant to mention this last time, but is there a good reason to not call init() here too?

::: browser/base/content/test/general/browser.ini:379
(Diff revision 6)
>  skip-if = os == "linux" # Linux: Intermittent failures - bug 941575.
>  # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
> -[browser_fxaccounts.js]
> -support-files = fxa_profile_handler.sjs
> -# DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
>  [browser_fxa_web_channel.js]

seeing as you have created a new file for tests, can you please either move these other sync/fxa related tests into that new directory, or open another (maybe good-first-) bug to do so?

::: services/sync/modules/UIState.jsm:65
(Diff revision 6)
> +      return DEFAULT_STATE;
> +    }
> +    return Object.assign({}, this._state, { syncing: this._syncing });
> +  },
> +
> +  ensureReady() {

I'm a little torn on the name here - a casual reading of browser-sync.js implies that ensureReady returning false implies some kind of error. "isReady" might be a better name (but even that's not ideal, as it has the side-effect of starting an initialization if it isn't ready.

WDYT?

(I was also going to suggest we tweak things so this function is called, say, "requestNotification()", and it always causes the observer to be sent, either immediately or after init - but I guess that would defeat your optimization to avoid changing data in the common "not configured" case in browser-sync.js - that might still be worthwhile if you can figure out how to have our cake and eat it too :)

::: services/sync/modules/UIState.jsm:171
(Diff revision 6)
> +
> +  async _getUserData() {
> +    try {
> +      return await this.fxAccounts.getSignedInUser();
> +    } catch (e) {
> +      // This is most likely in tests, were we quickly log users in and out.

nit - s/were/where/

::: services/sync/modules/UIState.jsm:186
(Diff revision 6)
> +    if (this._cachedProfile) {
> +      return this._cachedProfile;
> +    }
> +    try {
> +      let profile = await this.fxAccounts.getSignedInUserProfile();
> +      this._cachedProfile = profile;

We added this concept in bug 1337026, but TBH I don't understand why we need it seeing as the FxA modules also cache it. Do you think we can remove it?

::: services/sync/modules/UIState.jsm:196
(Diff revision 6)
> +    }
> +  },
> +
> +  _setLastSyncTime(state) {
> +    if (state.status == UIState.STATUS_SIGNED_IN) {
> +      state.lastSync = Services.prefs.getCharPref("services.sync.lastSync", null);

We could consider moving the |new Date(...)| from browser-sync to here (probably still with the exception handler, and setting it to null on failure). We should probably also reset it to null if we aren't signed in
Attachment #8855945 - Flags: review?(markh) → review+
In manual testing I noticed a bug - sign in to sync, then immediately open the hamburger menu - it remains in the "not signed in" state until the sync *completes*. I don't understand why as you do listed for onlogin, but also didn't try and debug :)
I'm still unable to reproduce the bug you cited mark, I even tried with a full build on Windows. Could you provide some debug info maybe?
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63b3b41d531b
part 1 - Remove pref identity.fxaccounts.profile_image.enabled. r=markh
https://hg.mozilla.org/integration/autoland/rev/250c84d30feb
part 2 - Remove unused customization mode code for the FxA status bar. r=markh
https://hg.mozilla.org/integration/autoland/rev/0a0138825fb5
part 3 - Refactor browser-syncui and browser-fxaccounts. r=markh
Backed out for eslint failures in test_uistate.js:

https://hg.mozilla.org/integration/autoland/rev/0a0138825fb5888760842319de27e18a81efca11
https://hg.mozilla.org/integration/autoland/rev/250c84d30febff3f6f7d79635f867262a0217716
https://hg.mozilla.org/integration/autoland/rev/63b3b41d531b4ae7bc06f4e3a9e56b5373f74d4a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0a0138825fb5888760842319de27e18a81efca11&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=96367018&repo=autoland

 TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/services/sync/tests/unit/test_uistate.js:127:5 | Expected property shorthand. (object-shorthand)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/services/sync/tests/unit/test_uistate.js:150:5 | Expected property shorthand. (object-shorthand)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/services/sync/tests/unit/test_uistate.js:176:5 | Expected property shorthand. (object-shorthand)
Flags: needinfo?(eoger)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/cd1d4a1a1eea
Backed out changeset 0a0138825fb5 
https://hg.mozilla.org/integration/autoland/rev/7ba08fc96336
Backed out changeset 250c84d30feb 
https://hg.mozilla.org/integration/autoland/rev/1a5fd94651fb
Backed out changeset 63b3b41d531b for eslint failures in test_uistate.js. r=backout
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c97dc15a4297
part 1 - Remove pref identity.fxaccounts.profile_image.enabled. r=markh
https://hg.mozilla.org/integration/autoland/rev/6f30e9012fc9
part 2 - Remove unused customization mode code for the FxA status bar. r=markh
https://hg.mozilla.org/integration/autoland/rev/619b0f41a72a
part 3 - Refactor browser-syncui and browser-fxaccounts. r=markh
https://hg.mozilla.org/mozilla-central/rev/c97dc15a4297
https://hg.mozilla.org/mozilla-central/rev/6f30e9012fc9
https://hg.mozilla.org/mozilla-central/rev/619b0f41a72a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
I see a performance improvement with this patch:
== Change summary for alert #6368 (as of May 03 2017 23:16 UTC) ==

Improvements:

  3%  tpaint windows7-32 opt      296.09 -> 288.23

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6368
Yay!

Kanchan, this patch changed a lot how/when we re-draw the Sync UI.
This includes the Sync/FxA status bar in the hamburger menu, the synced tabs panel in the hamburger menu and the Tools->Sync Now/"Sign-in to Sync" menu item in the menu bar.
We want to make sure this patch didn't break anything, especially when the user account state changes (unconnected/unverified/password changed somewhere else-needs re-connection/connected).

Could you give it a round of QA once it's in Nightly? Thanks!
Flags: needinfo?(eoger) → needinfo?(kkumari)
Keywords: qawanted
and more improvements have come in now that we have mostly a full set of data:
== Change summary for alert #6368 (as of May 03 2017 23:16 UTC) ==

Improvements:

 13%  sessionrestore osx-10-10 opt e10s     1034.33 -> 900
 12%  sessionrestore_no_auto_restore osx-10-10 opt e10s1063.75 -> 931.42
  3%  tpaint linux64 opt                    271.26 -> 262.09
  3%  tpaint windows8-64 opt                285.53 -> 276.57
  3%  ts_paint osx-10-10 opt e10s           1189.92 -> 1156.75
  3%  tpaint windows7-32 opt                296.09 -> 288.23

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6368
(In reply to Edouard Oger [:eoger] from comment #68)
> Kanchan, this patch changed a lot how/when we re-draw the Sync UI.
> This includes the Sync/FxA status bar in the hamburger menu, the synced tabs
> panel in the hamburger menu and the Tools->Sync Now/"Sign-in to Sync" menu
> item in the menu bar.
> We want to make sure this patch didn't break anything, especially when the
> user account state changes (unconnected/unverified/password changed
> somewhere else-needs re-connection/connected).
> 
> Could you give it a round of QA once it's in Nightly? Thanks!

I encounter an intermittent bug 1362190 during smog testing of this bug fix. 

Sync/FxA status bar, Tools->Sync Now/"Sign-in to Sync" menu item in the menu bar looks/works good. 

Other than bug 1362190, I didn't encounter any issue with various user account state changes (unconnected/unverified/re-connection/connected/password changed) or with any other FxA setting as of yet. 

I will do thorough testing on this next week. Thanks!
Flags: needinfo?(kkumari)
Depends on: 1365375
No longer depends on: 1365375
Depends on: 1365713
Depends on: 1369092
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: