964511 - Delay snippets initialization until delayed startup.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Bug 964510 is adding a delayed startup notification. We should use it for snippets.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 2

[:Margaret Leibovic]

Comment on attachment 8366481 [details] [diff] [review]
Patch

Review of attachment 8366481 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/components/Snippets.js
@@ +323,5 @@
>      switch(topic) {
>        case "profile-after-change":
> +        Services.obs.addObserver(this, "browser-delayed-startup-finished", false);
> +        break;
> +      case "browser-delayed-startup-finished":

Can't you just change the category here to listen for browser-delayed-startup instead?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/MobileComponents.manifest?force=1#107

Comment 3

[:Margaret Leibovic]

Comment on attachment 8366481 [details] [diff] [review]
Patch

Review of attachment 8366481 [details] [diff] [review]:
-----------------------------------------------------------------

The answer appears to be no, so I'm fine with this approach.

Comment 4

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/9c564b61b7f8

Comment 5

[Carsten Book [:Tomcat]]

Hi, sorry backedout as part of https://tbpl.mozilla.org/?tree=Fx-Team&rev=c15e29ead092 for the suspicion of causing robocop testfailures on android like https://tbpl.mozilla.org/php/getParsedLog.php?id=33734468&tree=Fx-Team

Comment 6

[Wesley Johnston (:wesj)]

Try looked good. back in:
https://hg.mozilla.org/integration/fx-team/rev/ed28aeea074a

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ed28aeea074a

Comment 8

[Wesley Johnston (:wesj)]

backed this out again for suspicion of a startup regression:
https://hg.mozilla.org/integration/fx-team/rev/1b5dfc70d6cd

Comment 9

[:Margaret Leibovic]

We should try re-landing something like this before re-enabling snippets, since it seems like that was guilty of eideticker regressions, even with bug 969511.

I can take this over. I think we should write a patch that re-adds the delayed start-up notification, but only use it for snippets at first, not a bunch of different things that might cause regressions.

Comment 10

[:Margaret Leibovic]

Actually, scratch that, there's not actually any new patch to write here. We just need to land bug 964510 by itself (no consumers), make sure that doesn't cause a regression, then try landing this patch by itself. Although that shouldn't do anything until bug 962349 re-lands as well.

Comment 11

[:Margaret Leibovic]

No regression from bug 964510 \o/ (if there were, I would be really confused).

So, shall we try re-landing this? However, this won't actually do anything until snippets are enabled, so maybe we should re-land bug 962349 at the same time and hope for the best?

Comment 12

[:Margaret Leibovic]

Re-landing:
https://hg.mozilla.org/integration/fx-team/rev/9e58486f1658

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9e58486f1658

Comment 14

[:Margaret Leibovic]

Eideticker is looking pretty stable still, so I think we should uplift this bug and bug 964510 to aurora, since snippets are still enabled there. Wes, do you want to nom them?

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 8366481 [details] [diff] [review]
Patch

Sure. The rel-drivers, this is a bit confusing. This stuff landed in 29, but was then backed out. Now that we know it wasn't the cause of the regression, we'd like to put it back in.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 962349
User impact if declined: Startup regression
Testing completed (on m-c, etc.): Has been on mc for a week. Things seem better
Risk to taking this patch (and alternatives if risky): really low risk. we can't really ship with snippets disabled anymore, so we don't have any good alternatives.
String or IDL/UUID changes made by this patch: none

Comment 16

[Ryan VanderMeulen [:RyanVM]]

There's nothing to re-land on Aurora. This never got backed out from 29.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

This component still gets initialized at startup just so it can connect to the browser-delayed-startup-finished event.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Delay Snippets.js initialization until after startup

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8450236 [details] [diff] [review]
Delay Snippets.js initialization until after startup

I like this. Seems safe.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1db80a4c08

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a1db80a4c08

Comment 22

[:Margaret Leibovic]

This follow-up really should have gone in a new bug, since the tracking flags for this bug are now confusing.

Running a local build, I'm seeing this error:

[JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]" {file: "jar:jar:file:///data/app/org.mozilla.fennec_leibovic-1.apk!/assets/omni.ja!/components/Snippets.js" line: 389}]

Looks like you forgot to remove this line:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/Snippets.js#389