Closed Bug 964511 Opened 7 years ago Closed 7 years ago

Delay snippets initialization until delayed startup.


(Firefox for Android Graveyard :: General, defect)

Not set


(firefox29 fixed, firefox30 fixed, fennec29+)

Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
fennec 29+ ---


(Reporter: wesj, Assigned: wesj)




(2 files)

Bug 964510 is adding a delayed startup notification. We should use it for snippets.
Attached patch PatchSplinter Review
Attachment #8366481 - Flags: review?(margaret.leibovic)
Comment on attachment 8366481 [details] [diff] [review]

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?
Comment on attachment 8366481 [details] [diff] [review]

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

The answer appears to be no, so I'm fine with this approach.
Attachment #8366481 - Flags: review?(margaret.leibovic) → review+
Hi, sorry backedout as part of for the suspicion of causing robocop testfailures on android like
Assignee: nobody → wjohnston
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
backed this out again for suspicion of a startup regression:
Resolution: FIXED → ---
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.
Assignee: wjohnston → nobody
Blocks: 962349
Assignee: nobody → 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.
Assignee: margaret.leibovic → wjohnston
tracking-fennec: --- → ?
Depends on: 964510
tracking-fennec: ? → 29+
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?
Flags: needinfo?(wjohnston)
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 29 → Firefox 30
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 on attachment 8366481 [details] [diff] [review]

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
Attachment #8366481 - Flags: approval-mozilla-aurora?
Flags: needinfo?(wjohnston)
Attachment #8366481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
There's nothing to re-land on Aurora. This never got backed out from 29.
This component still gets initialized at startup just so it can connect to the browser-delayed-startup-finished event.
Resolution: FIXED → ---
Attachment #8450236 - Flags: review?(wjohnston) → review+
Comment on attachment 8450236 [details] [diff] [review]
Delay Snippets.js initialization until after startup

I like this. Seems safe.
Attachment #8450236 - Flags: review?(margaret.leibovic) → review+
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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:
Depends on: 1042502
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.