972098 - Cursors returned from the SQLiteBridge don't update when data updates

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect, P1)

Description

[:Margaret Leibovic]

This poses a problem if the add-on updates its data in the background while its panel fragment is still alive. We'll also need this if we want bug 970707 to work.

Comment 1

[:Margaret Leibovic]

Wes, do you have any ideas how we should go about doing this? This is something we'll want for the RSS feeds we want to ship in Fx30, especially if we want pull-to-refresh to work.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Factor out code to restart a dataset loader (r=margaret)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Refresh DynamicPanels when the dataset changes (r=margaret)

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8391175 [details] [diff] [review]
Refresh DynamicPanels when the dataset changes (r=margaret)

I initially considered implementing this in SQLiteBridgeContentProvider somehow but the DB changes done directly from Gecko are completely unaware of content providers and their URIs. One way to approach it could be to create generic gecko messages for content provider URIs and then have SQLiteBridgeContentProvider handle those. But making HomeProvider aware of those URIs felt a bit smelly. I preferred to go with a more dataset-specific message that is handled in DynamicPanel.

I don't feel strongly about either approach. Let me know what you think.

Comment 5

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 8391175 [details] [diff] [review]
> Refresh DynamicPanels when the dataset changes (r=margaret)
> 
> I initially considered implementing this in SQLiteBridgeContentProvider
> somehow but the DB changes done directly from Gecko are completely unaware
> of content providers and their URIs. One way to approach it could be to
> create generic gecko messages for content provider URIs and then have
> SQLiteBridgeContentProvider handle those. But making HomeProvider aware of
> those URIs felt a bit smelly. I preferred to go with a more dataset-specific
> message that is handled in DynamicPanel.
> 
> I don't feel strongly about either approach. Let me know what you think.

Yeah, SQLiteBridgeContentProvider is special, so I was afraid that fixing this bug would be tricky. However, I like the approach you took here, since it solves our immediate problem without getting bogged down in trying to refactor SQLiteBridge.

This approach takes advantage of the fact that we only alter the contents of HomeProvider in the save/delete methods in HomeProvider.jsm, so we'll need to remember this if we ever decide to change that, but I don't see that changing in the near future.

Comment 6

[Wesley Johnston (:wesj)]

Clearing request. I agree though, this is probably the best way to do this. We could try to install some update hooks in the db, but it actually seems best to just require consumers to do the notification themselves. That way there are ways to update without notifying as well....

Comment 7

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/792df7675500
https://hg.mozilla.org/integration/fx-team/rev/2243f5184140

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/792df7675500
https://hg.mozilla.org/mozilla-central/rev/2243f5184140

Comment 9

[:Margaret Leibovic]

Can we uplift this to aurora? For my world cup add-on, I want to let the user choose a country when the install the add-on, and I want the panel to update as soon as we fetch the feed data for that country.

Comment 10

[:Margaret Leibovic]

Actually, I just realized that we'll want to uplift this if we're going to uplift bug 994734 as part of uplifting bug 976064 and bug 965622 (sigh, lots of bugs).

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #10)
> Actually, I just realized that we'll want to uplift this if we're going to
> uplift bug 994734 as part of uplifting bug 976064 and bug 965622 (sigh, lots
> of bugs).

Yep, we'll need to uplift the whole "dataset refresh" package (bug 972098, bug 976064, bug 994734) in order to uplift bug 965622.

Comment 12

[:Margaret Leibovic]

Attachment: patch for aurora

Note to release managers: I'm requesting uplift for a series of Firefox Hub bugs. The main fixes we need are in bugs at the end of the series, but trying to rebase those patches proved difficult and risky, so I think we should just uplift the dependecies.

Note to sheriffs: I have a local patch series that I can land on aurora when these bugs all get approval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dependency for initial Firefox Hub release (promoted feed add-ons in fx30)
User impact if declined: panels won't update when data changes
Testing completed (on m-c, etc.): baked on m-c
Risk to taking this patch (and alternatives if risky): only affects dynamic panels, need an add-on to trigger this
String or IDL/UUID changes made by this patch: none

Comment 13

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e3c2a55f23e3

Comment 14

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.