Closed Bug 1000849 Opened 6 years ago Closed 6 years ago

Panel view sometimes does replace empty view after dataset updates

Categories

(Firefox for Android :: General, defect, P1)

31 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox30 --- verified
firefox31 --- verified
fennec 30+ ---

People

(Reporter: TeoVermesan, Assigned: Margaret)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

Steps to reproduce:
1. Install "Goal.com Feed"
2. From the pop-up "Welcome to your Goal.com panel!" choose which edition to follow
3. Go to Tools -> Add-ons
4. Disable it
5. Re-enable it

Actual result:
- The user is redirected to the "GOAL.COM" panel and the pop-up shows up again. After choosing the feed edition, the panel is blank. 
Only after closing all tabs and going to that panel, it will be populated with feeds.
Expected result:
- The add-on is re-enabled and all functionalities are restored.
Step 0 is referring to using: http://people.mozilla.org/~mleibovic/worldcupfeed.xpi (https://github.com/leibovic/world-cup-feed)

I wasn't able to reproduce this, but this would be solved via bug 999483. Is there anything in console after you pick a language and then see the empty pane?
I just tried to reinstall the add-on and selected English and got an empty pane too actually.

D/GeckoDynamicPanel(10993): Loading layout
D/GeckoPanelLayout(10993): Creating panel view: list
D/GeckoDynamicPanel(10993): Created layout of type: frame
D/GeckoFramePanelLayout(10993): Loading
D/GeckoFramePanelLayout(10993): Requesting child request: { index: 0, type: DATASET_LOAD, dataset: world.cup.feed.dataset@mozilla.org, filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoPanelLayout(10993): Requesting request: { index: 0, type: DATASET_LOAD, dataset: world.cup.feed.dataset@mozilla.org, filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoDynamicPanel(10993): Requesting request: { index: 0, type: DATASET_LOAD, dataset: world.cup.feed.dataset@mozilla.org, filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoDynamicPanel(10993): Creating loader for request: { index: 0, type: DATASET_LOAD, dataset: world.cup.feed.dataset@mozilla.org, filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoHomeProvider(10993): No profile provided, using 'default'
D/GeckoDynamicPanel(10993): Finished loader for request: { index: 0, type: DATASET_LOAD, dataset: world.cup.feed.dataset@mozilla.org, filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoPanelLayout(10993): Delivering request: { index: 0, type: DATASET_LOAD, dataset: world.cup.feed.dataset@mozilla.org, filter: org.mozilla.gecko.home.PanelLayout$FilterDetail@42d2bd40 }
D/GeckoPanelLayout(10993): Creating empty view: list
Summary: Disable/re-enable "Goal.com" exits add-on manager, being redirected to its panel from about:home → No feed shown in pane with the Wold Cup feed add-on after selecting a language
Summary: No feed shown in pane with the Wold Cup feed add-on after selecting a language → No feed shown in pane with the World Cup feed add-on after selecting a language
Severity: minor → normal
tracking-fennec: --- → ?
This sounds like a bug related to empty views.
Assignee: nobody → margaret.leibovic
Blocks: lists
tracking-fennec: ? → 30+
Summary: No feed shown in pane with the World Cup feed add-on after selecting a language → Panel view sometimes does replace empty view after dataset updates
Priority: -- → P1
This is caused by the null cursor issue that I "fixed" in bug 995420. If the cursor is null, we don't register to get notifications, and therefore the view never hears that there's new data to show.

I think the proper fix here is to make sure SQLiteBridgeContentProvider always returns a cursor, or make HomeProvider.query always return an empty cursor, even if the bridge returns null (that might be safer, since other SQLiteBridgeContentProvider consumers might be expecting null).
Comment on attachment 8412127 [details] [diff] [review]
Make a dummy MatrixCursor to listen for dataset changed notifications

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

::: mobile/android/base/db/HomeProvider.java
@@ +99,5 @@
> +
> +        // SQLiteBridgeContentProvider may return a null Cursor if the database hasn't been created yet.
> +        // However, we need a non-null cursor in order to listen for notifications.
> +        if (c == null) {
> +            c = new MatrixCursor(ITEMS_COLUMNS);

Strictly speaking, the columns in the resulting cursor should be defined by the given projection in the query() call. Even though this code will work for our current usage (i.e. projection is always null, select all columns), I think we can make this code a bit more future-proof.

Here's what I suggest:
1. Define a HomeItems.DEFAULT_PROJECTION in BrowserContract with the contents of your ITEMS_COLUMNS here.
2. Decide which projection to use:
2.1 If projection is null, do new Matrix(HomeItems.DEFAULT_PROJECTION)
2.2 If projection is not null, do new Matrix(projection)

If we want to be really correct here, we should update queryFakeItems() to only return the columns from the given projection. But I'm fine with just always using HomeItems.DEFAULT_PROJECTION in the fake query as it's only used by us for debugging.
Attachment #8412127 - Flags: review?(wjohnston) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 8412127 [details] [diff] [review]
> Make a dummy MatrixCursor to listen for dataset changed notifications
> 
> Review of attachment 8412127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/HomeProvider.java
> @@ +99,5 @@
> > +
> > +        // SQLiteBridgeContentProvider may return a null Cursor if the database hasn't been created yet.
> > +        // However, we need a non-null cursor in order to listen for notifications.
> > +        if (c == null) {
> > +            c = new MatrixCursor(ITEMS_COLUMNS);
> 
> Strictly speaking, the columns in the resulting cursor should be defined by
> the given projection in the query() call. Even though this code will work
> for our current usage (i.e. projection is always null, select all columns),
> I think we can make this code a bit more future-proof.
> 
> Here's what I suggest:
> 1. Define a HomeItems.DEFAULT_PROJECTION in BrowserContract with the
> contents of your ITEMS_COLUMNS here.
> 2. Decide which projection to use:
> 2.1 If projection is null, do new Matrix(HomeItems.DEFAULT_PROJECTION)
> 2.2 If projection is not null, do new Matrix(projection)
> 
> If we want to be really correct here, we should update queryFakeItems() to
> only return the columns from the given projection. But I'm fine with just
> always using HomeItems.DEFAULT_PROJECTION in the fake query as it's only
> used by us for debugging.

Good call, I'll update to do this. Right now we only ever query HomeProvider with a null projection, so this wasn't an issue, but your suggestion is more future-proof.
Updated, thanks for the suggestion.
Attachment #8412127 - Attachment is obsolete: true
Attachment #8412644 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8412644 [details] [diff] [review]
(v2) Make a dummy MatrixCursor to listen for dataset changed notifications

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

Look good, thanks.
Attachment #8412644 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8412644 [details] [diff] [review]
(v2) Make a dummy MatrixCursor to listen for dataset changed notifications

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new hub APIs to ship in Fx30
User impact if declined: panel view won't update to show data the first time data is stored
Testing completed (on m-c, etc.): just landed on fx-team, tested locally
Risk to taking this patch (and alternatives if risky): low-risk, only affects add-ons
String or IDL/UUID changes made by this patch: none
Attachment #8412644 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/528ed2029bc2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8412644 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
After step 4&5 from my first comment, the add-on is re-enabled and all functionalities are restored, so:
Verified fixed on:
Build: Firefox for Android 30 Beta 4
Device: Alcatel One Touch (Android 4.1.2)
Status: RESOLVED → VERIFIED
After step 4&5, the add-on is re-enabled and all functionalities are restored, so:
Verified fixed on:
Build: Firefox for Android 31.0a2 (2014-05-16)
Device: Alcatel One Touch (Android 4.1.2)
Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.
Blocks: 1014025
You need to log in before you can comment on or make changes to this bug.