1364571 - Delay Weave import

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Edouard Oger [:eoger]]

See bug 1358798, we don't need to initialize Sync that early, we can do it in nsBrowserGlue#_finalUIStartup instead.

Comment 1

[Edouard Oger [:eoger]]

Attachment: Bug 1364571 - Delay Weave startup.

Review commit: https://reviewboard.mozilla.org/r/138880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138880/

Comment 2

[Lina Butler [:lina]]

Comment on attachment 8867354 [details]
Bug 1364571 - Delay Weave startup.

https://reviewboard.mozilla.org/r/138880/#review142218

LGTM! We don't use this on Android, SeaMonkey is already broken because it doesn't support FxA, and Graphene and B2G are no longer a thing.

::: browser/components/nsBrowserGlue.js:21
(Diff revision 1)
>  
>  XPCOMUtils.defineLazyServiceGetter(this, "WindowsUIUtils", "@mozilla.org/windows-ui-utils;1", "nsIWindowsUIUtils");
>  XPCOMUtils.defineLazyServiceGetter(this, "AlertsService", "@mozilla.org/alerts-service;1", "nsIAlertsService");
> +XPCOMUtils.defineLazyGetter(this, "WeaveService", () => {
> +  return Components.classes["@mozilla.org/weave/service;1"]
> +                   .getService(Components.interfaces.nsISupports)

Nit: `Cc` instead of `Components.classes`, `Ci` instead of `Components.interfaces`?

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8867354 [details]
Bug 1364571 - Delay Weave startup.

https://reviewboard.mozilla.org/r/138880/#review142234

Thanks for looking at this! final-ui-startup is right before the first window is shown, that's still too early. Given that all this does is add a 10s timer to then do stuff later, it should really wait until the first window is done initializing. "browser-delayed-startup-finished" /  _onFirstWindowLoaded in nsBrowserGlue.js would be a much better place to start this.

::: browser/components/nsBrowserGlue.js:21
(Diff revision 1)
>  
>  XPCOMUtils.defineLazyServiceGetter(this, "WindowsUIUtils", "@mozilla.org/windows-ui-utils;1", "nsIWindowsUIUtils");
>  XPCOMUtils.defineLazyServiceGetter(this, "AlertsService", "@mozilla.org/alerts-service;1", "nsIAlertsService");
> +XPCOMUtils.defineLazyGetter(this, "WeaveService", () => {
> +  return Components.classes["@mozilla.org/weave/service;1"]
> +                   .getService(Components.interfaces.nsISupports)

nit: I think getService() (ie. without parameter) is equivalent to getting the nsISupports interface.

::: browser/components/nsBrowserGlue.js:670
(Diff revision 1)
>      TabCrashHandler.init();
>      if (AppConstants.MOZ_CRASHREPORTER) {
>        PluginCrashReporter.init();
>        UnsubmittedCrashHandler.init();
>      }
> +    WeaveService.init();

Can we check if sync is enabled before accessing 'WeaveService', and if not completely avoid loading this component?
From a quick look at the code, it seems Weave.js does a check on this.enabled before doing any real initialization... and that's just a check for Services.prefs.prefHasUserValue("services.sync.username").

::: services/sync/Weave.js:120
(Diff revision 1)
>          if (isConfigured) {
>            getHistogramById("WEAVE_CONFIGURED_MASTER_PASSWORD").add(Utils.mpEnabled());
>            this.ensureLoaded();
>          }
>        }
>      }, 10000, Ci.nsITimer.TYPE_ONE_SHOT);

We really need to get rid of this timer. But if this is in an xpcom component, I'm afraid we'll need to wait until bug 1353206 is fixed to do it.

Comment 4

[Edouard Oger [:eoger]]

Comment on attachment 8867354 [details]
Bug 1364571 - Delay Weave startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138880/diff/1-2/

Comment 5

[Edouard Oger [:eoger]]

Thanks Florian!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/135031bdb7d0
Delay Weave startup. r=kitcambridge

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/135031bdb7d0