999756 - Empty view flashes while view is refreshing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P2)

Description

[:Margaret Leibovic]

In my add-on's onrefresh handler, I make a deleteAll call then save a new set of items, and I can see the empty view flash in between.

The solution to this bug may overlap with the solution to bug 999483.

Comment 1

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #0)
> In my add-on's onrefresh handler, I make a deleteAll call then save a new
> set of items, and I can see the empty view flash in between.
> 
> The solution to this bug may overlap with the solution to bug 999483.

The flashing is most likely caused by the quick sequence of deleteAll() and save() calls on the add-on side (which will trigger two refreshes on the panel). An easy way to workaround that could be to delay the HomePanels:RefreshDataset message for a few milliseconds (on the HomeProvider.jsm side) to avoid unnecessary successive refreshes.

Comment 2

[:Margaret Leibovic]

Attachment: Delay sending "HomePanels:RefreshDataset" message to Java

Went with the suggestion to delay sending the messages to Java. Testing with my Instagram panel, I tried 50ms, and found I was still seeing the flashing, so I went with 100ms, but this doesn't feel like a very robust solution, since it depends on how quickly the save transaction takes.

Maybe a better solution would be to allow add-ons to batch actions together, so that they can specify "this delete and save call are part of the same transaction".

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8419045 [details] [diff] [review]
Delay sending "HomePanels:RefreshDataset" message to Java

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

Looks good with the suggested change.

(In reply to :Margaret Leibovic from comment #2)
> Created attachment 8419045 [details] [diff] [review]
> Delay sending "HomePanels:RefreshDataset" message to Java
> 
> Went with the suggestion to delay sending the messages to Java. Testing with
> my Instagram panel, I tried 50ms, and found I was still seeing the flashing,
> so I went with 100ms, but this doesn't feel like a very robust solution,
> since it depends on how quickly the save transaction takes.

Yeah, when I suggested the timer idea, I didn't account for the fact that these calls are asynchronous. I still think it makes sense to have this timer though as it will avoid redundant refresh requests in certain cases.

> Maybe a better solution would be to allow add-ons to batch actions together,
> so that they can specify "this delete and save call are part of the same
> transaction".

Agree. Maybe it would be simpler to just add a new method for the deleteall-and-savenew case? Your idea for an API to group operations together sounds good too.

::: mobile/android/modules/HomeProvider.jsm
@@ +289,5 @@
> + */
> +function refreshDataset(datasetId) {
> +  let timer = gRefreshTimers[datasetId];
> +  if (timer) {
> +    timer.cancel();

Given that the goal here is to simply avoid redundant successive refreshes, I'd expect the refreshDataset() call to simply bail if there's already a scheduled timer for the given datasetId instead of cancelling the existing timer and postponing the refresh a bit more.

Comment 4

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #3)

> > Maybe a better solution would be to allow add-ons to batch actions together,
> > so that they can specify "this delete and save call are part of the same
> > transaction".
> 
> Agree. Maybe it would be simpler to just add a new method for the
> deleteall-and-savenew case? Your idea for an API to group operations
> together sounds good too.

I like the idea to add a new API for deleteall-and-savenew, since this pattern is something we're promoting in our demo add-ons (and a very common use case).

> ::: mobile/android/modules/HomeProvider.jsm
> @@ +289,5 @@
> > + */
> > +function refreshDataset(datasetId) {
> > +  let timer = gRefreshTimers[datasetId];
> > +  if (timer) {
> > +    timer.cancel();
> 
> Given that the goal here is to simply avoid redundant successive refreshes,
> I'd expect the refreshDataset() call to simply bail if there's already a
> scheduled timer for the given datasetId instead of cancelling the existing
> timer and postponing the refresh a bit more.

When implementing this, I was thinking of the solution as "only send a refresh message once a series of updates is done", so postponing the refresh a bit more would catch a series of more than 2 updates. However, since we're really just trying to optimize for this delete-then-save case, tweaking the solution to be more like "only send a refresh message once every N ms" would be fine as well (and make the refresh happen more quickly).

Comment 5

[:Margaret Leibovic]

Attachment: Add parameter to HomeStorage.save to allow replacing existing items

Instead of creating a whole new API, how about adding a parameter to the save method? This won't break exiting add-ons, but we can update our docs and boilerplate add-on to take advantage of this. This also has the benefit of being part of the same transaction as the save call, so if we fail to save items, the user won't be left with nothing.

I was also contemplating making this the default behavior, but that probably wouldn't be expected.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8419537 [details] [diff] [review]
Add parameter to HomeStorage.save to allow replacing existing items

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

Sounds like a good idea. Just wonder if a boolean argument is the best way to expose it. Maybe an options object with a 'replace' boolean key is more future-proof? No strong opinions here. Up to you.

Comment 7

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 8419537 [details] [diff] [review]
> Add parameter to HomeStorage.save to allow replacing existing items
> 
> Review of attachment 8419537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sounds like a good idea. Just wonder if a boolean argument is the best way
> to expose it. Maybe an options object with a 'replace' boolean key is more
> future-proof? No strong opinions here. Up to you.

Sure, I'll update it to an options object, that's a good idea.

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/6112f207dbc6
https://hg.mozilla.org/integration/fx-team/rev/463a0f7fb61b

Comment 9

[:Margaret Leibovic]

It would be nice to uplift this, since the flashing can look pretty bad.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6112f207dbc6
https://hg.mozilla.org/mozilla-central/rev/463a0f7fb61b

Comment 11

[:Margaret Leibovic]

Comment on attachment 8419045 [details] [diff] [review]
Delay sending "HomePanels:RefreshDataset" message to Java

I realized this only need to go on aurora, not beta, since we don't have the onrefresh API on beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): home panels refresh API
User impact if declined: users can see empty views flash when data in lists refreshes
Testing completed (on m-c, etc.): landed on m-c 5/13
Risk to taking this patch (and alternatives if risky): low-risk, small change to how we refresh home panels, only affects add-on panels 
String or IDL/UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d7fde0258fb2
https://hg.mozilla.org/releases/mozilla-aurora/rev/b753b027758e

Comment 13

[:Margaret Leibovic]

Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release).

Filter on epic-hub-bugs.