Closed Bug 969511 Opened 6 years ago Closed 6 years ago

Don't update snippets cache unless the update timer fired

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
The delayed statup change from bug 964511 would still be nice to prevent us from doing reading the cache until later in startup, but this will prevent us from ever doing a snippets-related network request during startup.

Until the first update timer fires, we won't have any snippets to show the user, but I think that's fine.
Attachment #8372450 - Flags: review?(wjohnston)
Comment on attachment 8372450 [details] [diff] [review]
patch

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

This will still do file i/o at startup if the snippets file DOES exist, but moving the request seems like a win.

::: mobile/android/components/Snippets.js
@@ +135,5 @@
>   */
>  function loadSnippetsFromCache() {
>    let promise = OS.File.read(gSnippetsPath);
>    promise.then(array => updateBanner(gDecoder.decode(array)), e => {
>      // If snippets.json doesn't exist, update data from the server.

This comment is not wrong.
Attachment #8372450 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #1)
> Comment on attachment 8372450 [details] [diff] [review]
> patch
> 
> Review of attachment 8372450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This will still do file i/o at startup if the snippets file DOES exist, but
> moving the request seems like a win.

Yeah, I think it's definitely good to see if we can re-land bug 964511 (or something similar) to delay when we do that i/o.

https://hg.mozilla.org/integration/fx-team/rev/a9785c0a4e7d
https://hg.mozilla.org/mozilla-central/rev/a9785c0a4e7d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment on attachment 8372450 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 962349 (enabling snippets)
User impact if declined: performance slow-downs on first run
Testing completed (on m-c, etc.): landed on m-c 2/8
Risk to taking this patch (and alternatives if risky): low-risk, removes a call to fetch snippets from the snippets server
String or IDL/UUID changes made by this patch: none
Attachment #8372450 - Flags: approval-mozilla-aurora?
Attachment #8372450 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.