Closed Bug 1364571 Opened 3 years ago Closed 3 years ago

Delay Weave import

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

Attachments

(1 file)

See bug 1358798, we don't need to initialize Sync that early, we can do it in nsBrowserGlue#_finalUIStartup instead.
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`?
Attachment #8867354 - Flags: review?(kit) → review+
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.
Attachment #8867354 - Flags: review-
Thanks Florian!
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/135031bdb7d0
Delay Weave startup. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/135031bdb7d0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.