Right now, on each browser window we have code that updates the hamburger menu sync status bar. We could do better and share this code between browser window instances by using a JS Module. The patch I provide "works", but is not complete because I didn't update the old tests yet.

And also a first review for an older version of this patch was provided in bug 1201331 comment 11.

We should still consider something like this.

Here's another try at this, using some of the ideas of the patch I submitted last year and feedback from Mark's review. Notes: * All the view handling code has been moved to a separate file * I removed the UI migration code: pretty much everyone should have been auto-disconnected from legacy sync by now. We should consider opening a follow-up bug to remove the rest of the code. * The profile-image-not-loading detection code is not working (the src is never loaded) * No tests -yet- until we validate this approach * It looks very clean!

Woops, uploaded an old version of the patch

Comment on attachment 8747419 [details] [diff] [review] bug-1214849.patch I think the general approach is good, but at first glance it looks like it doesn't quite go far enough - there's a strange mix of things that are moved out and not moved out - I'm not saying that's a fault of the patch, it might just reflect reality - but I need to ponder on this a little more and I've alot of other things I'm trying to get done, so it might take me a week or longer to get back to this.

Are you thinking of the last sync date tooltip and the spinning icon state who are in browser-syncui.js?

Comment on attachment 8747419 [details] [diff] [review] bug-1214849.patch Review of attachment 8747419 [details] [diff] [review]: ----------------------------------------------------------------- One confusing this is that the new modules are called "Sync" but in reality it's only dealing with "FxA" - but more generally, I think that we are missing an opportunity to clean some of this up further and the risk is that we are just making things worse (ie, instead of 2 slightly confused files in which to find the implementation of things, we now still have the same 2 slightly confused files, plus 2 new less-confused files - with the total confusion probably increasing). browser-fxaccounts has some sync-specific stuff (eg, doorhanger etc) that isn't being factored out, and browser-syncui has lots of cruft that's no longer used. PanelUI.inc.xul has an element called "PanelUI-fxa-icon" which calls into gSyncUI, etc. IOW, I think we need to think bigger - probably by completely removing one of these browser- files, accepting the reality that from the browser's menu UI, Sync and FxA are indistinguishable. (Another nit on the naming - SyncStatusBar is neither related to Sync (it's FxA) and it's not what we typically call a "status bar". Maybe it makes sense to have all "panelui" stuff in a module, in which case it might even make sense to have some of the synced-tabs panel UI code from CustomizableWidgets there too?) I guess the tl;dr is that this looks like a "simple" move of some of the existing confusion into a module, but doesn't really help that existing confusion and may actually add to it. ::: browser/components/sync/SyncStatusBar.jsm @@ +40,5 @@ > + > +let SyncStatusBarInternal = { > + _initialized: false, > + _viewByWindow: new WeakMap(), > + _state: {}, // Do not mute directly, use the state setter instead s/mute/mutate/ @@ +111,5 @@ > + registerStatusBar(statusBarNode) { > + let win = statusBarNode.ownerDocument.defaultView; > + let view = new SyncStatusBarView(win, statusBarNode); > + this._viewByWindow.set(win, view); > + this.onStartOver(win, view, this.state); I think we should rename onStartOver() as Sync already has a concept of startOver (which is another name for "disconnect). @@ +118,5 @@ > + // Note that updateState() returns a Promise that's only used by tests. > + updateState() { > + let newState = {}; > + > + // Bail out if FxA is disabled. Rather than "FxA is disabled" you should say "Sync is configured with the legacy provider" - it's a subtle but real distinction. @@ +185,5 @@ > + } > + }.bind(this)); > + }, > + > + _forEachView(cb) { I think we want a '*' here as the function is a generator.

Unassigning myself for now.

From Edouard's work, It turns out this looks like being lots of pain with no gain - I think we should just kill it.