969511 - Don't update snippets cache unless the update timer fired

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Attachment: patch

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.

Comment 1

[Wesley Johnston (:wesj)]

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.

Comment 2

[:Margaret Leibovic]

(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

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 4

[:Margaret Leibovic]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/77ba19df83b2