976064 - Create a loader per panel view instead of per dataset id

Status: RESOLVED FIXED

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

Description

[Lucas Rocha (:lucasr)]

With the introduction of filter stacks (per panel view) in bug 942295, different views might be associated with the same datasets but with different filters. We should have a 1-1 match between loaders and views instead of loaders and dataset IDs.

Comment 1

[Lucas Rocha (:lucasr)]

Downgrading this to P2 as we'll only have FRAME panel layouts in Fx30.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Add 'index' property to ViewConfig (r=margaret)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Create a loader per panel view instead of per dataset id (r=margaret)

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8399491 [details] [diff] [review]
Add 'index' property to ViewConfig (r=margaret)

We need ViewConfigs to be aware of its index so that we can use the index to identify dataset requests from the panel layout.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 8399491 [details] [diff] [review]
> Add 'index' property to ViewConfig (r=margaret)
> 
> We need ViewConfigs to be aware of its index so that we can use the index to
> identify dataset requests from the panel layout.

It's probably worth noting that the ViewConfig index is not written to HomeConfig. It's just a runtime state of the ViewConfig instance.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8399492 [details] [diff] [review]
Create a loader per panel view instead of per dataset id (r=margaret)

This changes PanelLayout and DynamicPanel to create a loader per view, not per dataset id. This is necessary to support, for example, having multiple views bound the same dataset but using with different initial filters and filter stacks. This also changes how we keep track of the state of each 'live' panel view. With this patch, the ViewState entry is dependant on the View instance being around i.e. you should be able to implement bug 965622 safely now.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8399492 [details] [diff] [review]
Create a loader per panel view instead of per dataset id (r=margaret)

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

This is looking good, but I have one question about the change notification.

::: mobile/android/base/home/DynamicPanel.java
@@ +304,4 @@
>          Log.d(LOGTAG, "Refresh request for dataset: " + datasetId);
>  
> +        final ContentResolver cr = activity.getContentResolver();
> +        cr.notifyChange(getDatasetNotificationUri(datasetId), null);

This is an issue that already exists, but we're registering a "HomePanels:RefreshDataset" listener for each DynamicPanel instance, so if there are multiple DynamicPanels, this notification will fire mutliple times.

Is there any way we can move this "HomePanels:RefreshDataset" handler over to HomeProvider (and perhaps rename it "HomeProvider:RefreshDataset")? That way we can just let the content provider take care of this change notification.

@@ +334,5 @@
>          }
>  
>          @Override
> +        public void resetDataset(int viewIndex) {
> +            Log.d(LOGTAG, "Resetting dataset: " + viewIndex);

Nit: Update this log message to make it clear that we're logging the view index.

::: mobile/android/base/home/PanelLayout.java
@@ +232,5 @@
> +        super.onDetachedFromWindow();
> +
> +        final int count = mViewStates.size();
> +        for (int i = 0; i < count; i++) {
> +            final ViewState viewState = mViewStates.valueAt(i);

I think we need a null check here.

@@ +361,5 @@
>      /**
>       * Dispose any dataset references associated with the
>       * given view.
>       */
>      protected final void disposePanelView(View view) {

I don't see anywhere this method is used, can we get rid of it?

Comment 8

[:Margaret Leibovic]

Comment on attachment 8399492 [details] [diff] [review]
Create a loader per panel view instead of per dataset id (r=margaret)

I also want jdover to look at this, since he's more familiar with the filter logic than me.

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #7)
> Comment on attachment 8399492 [details] [diff] [review]
> Create a loader per panel view instead of per dataset id (r=margaret)
> 
> Review of attachment 8399492 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking good, but I have one question about the change notification.
> 
> ::: mobile/android/base/home/DynamicPanel.java
> @@ +304,4 @@
> >          Log.d(LOGTAG, "Refresh request for dataset: " + datasetId);
> >  
> > +        final ContentResolver cr = activity.getContentResolver();
> > +        cr.notifyChange(getDatasetNotificationUri(datasetId), null);
> 
> This is an issue that already exists, but we're registering a
> "HomePanels:RefreshDataset" listener for each DynamicPanel instance, so if
> there are multiple DynamicPanels, this notification will fire mutliple times.
> 
> Is there any way we can move this "HomePanels:RefreshDataset" handler over
> to HomeProvider (and perhaps rename it "HomeProvider:RefreshDataset")? That
> way we can just let the content provider take care of this change
> notification.

Yeah, I considered that. However, we have no control over the life-cycle of content providers. For instance, there's no hook for when the ContentProvider is destroyed. This means we'd have to register an event listener in GeckoAppShell and hope that we'll have no dangling listeners once the process is killed and the ContentProvider is destroyed. Feels a bit too hand-wavy.

You do have a good point about the multiple HomePanels:RefreshDataset handlers though. With this patch, the HomePanels:RefreshDataset handlers don't need to be coupled with the DynamicPanel's loaders anymore, which means we can move them anywhere we want (as long as we ensure the handler is alive across different Fennec activities). I'll post a new patch with covering this.

> @@ +334,5 @@
> >          }
> >  
> >          @Override
> > +        public void resetDataset(int viewIndex) {
> > +            Log.d(LOGTAG, "Resetting dataset: " + viewIndex);
> 
> Nit: Update this log message to make it clear that we're logging the view
> index.

Done.

> ::: mobile/android/base/home/PanelLayout.java
> @@ +232,5 @@
> > +        super.onDetachedFromWindow();
> > +
> > +        final int count = mViewStates.size();
> > +        for (int i = 0; i < count; i++) {
> > +            final ViewState viewState = mViewStates.valueAt(i);
> 
> I think we need a null check here.

Null check on viewState? Why?

> @@ +361,5 @@
> >      /**
> >       * Dispose any dataset references associated with the
> >       * given view.
> >       */
> >      protected final void disposePanelView(View view) {
> 
> I don't see anywhere this method is used, can we get rid of it?

This is not used now but it's definitely going to be used when we implement more complex layouts with more complex view management. It's the createPanelView() counterpart. I'd like to keep it for the sake of API completeness, and for future reference.

Comment 10

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> (In reply to :Margaret Leibovic from comment #7)
> > Comment on attachment 8399492 [details] [diff] [review]
> > Create a loader per panel view instead of per dataset id (r=margaret)
> > 
> > Review of attachment 8399492 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is looking good, but I have one question about the change notification.
> > 
> > ::: mobile/android/base/home/DynamicPanel.java
> > @@ +304,4 @@
> > >          Log.d(LOGTAG, "Refresh request for dataset: " + datasetId);
> > >  
> > > +        final ContentResolver cr = activity.getContentResolver();
> > > +        cr.notifyChange(getDatasetNotificationUri(datasetId), null);
> > 
> > This is an issue that already exists, but we're registering a
> > "HomePanels:RefreshDataset" listener for each DynamicPanel instance, so if
> > there are multiple DynamicPanels, this notification will fire mutliple times.
> > 
> > Is there any way we can move this "HomePanels:RefreshDataset" handler over
> > to HomeProvider (and perhaps rename it "HomeProvider:RefreshDataset")? That
> > way we can just let the content provider take care of this change
> > notification.
> 
> Yeah, I considered that. However, we have no control over the life-cycle of
> content providers. For instance, there's no hook for when the
> ContentProvider is destroyed. This means we'd have to register an event
> listener in GeckoAppShell and hope that we'll have no dangling listeners
> once the process is killed and the ContentProvider is destroyed. Feels a bit
> too hand-wavy.
> 
> You do have a good point about the multiple HomePanels:RefreshDataset
> handlers though. With this patch, the HomePanels:RefreshDataset handlers
> don't need to be coupled with the DynamicPanel's loaders anymore, which
> means we can move them anywhere we want (as long as we ensure the handler is
> alive across different Fennec activities). I'll post a new patch with
> covering this.

Sounds good, thanks.
 
> > ::: mobile/android/base/home/PanelLayout.java
> > @@ +232,5 @@
> > > +        super.onDetachedFromWindow();
> > > +
> > > +        final int count = mViewStates.size();
> > > +        for (int i = 0; i < count; i++) {
> > > +            final ViewState viewState = mViewStates.valueAt(i);
> > 
> > I think we need a null check here.
> 
> Null check on viewState? Why?

mViewStates is a SparseArray, and if I'm following your logic correctly in other places, we aren't necessarily guaranteed to have an entry at each index.

> > @@ +361,5 @@
> > >      /**
> > >       * Dispose any dataset references associated with the
> > >       * given view.
> > >       */
> > >      protected final void disposePanelView(View view) {
> > 
> > I don't see anywhere this method is used, can we get rid of it?
> 
> This is not used now but it's definitely going to be used when we implement
> more complex layouts with more complex view management. It's the
> createPanelView() counterpart. I'd like to keep it for the sake of API
> completeness, and for future reference.

Okay, thanks for the clarification. Usually I'm more of a fan of introducing code when it's needed, but this is a pretty small method to maintain.

Comment 11

[Josh Dover [:jdover]]

(In reply to Lucas Rocha (:lucasr) from comment #5)
> (In reply to Lucas Rocha (:lucasr) from comment #4)
> > Comment on attachment 8399491 [details] [diff] [review]
> > Add 'index' property to ViewConfig (r=margaret)
> > 
> > We need ViewConfigs to be aware of its index so that we can use the index to
> > identify dataset requests from the panel layout.
> 
> It's probably worth noting that the ViewConfig index is not written to
> HomeConfig. It's just a runtime state of the ViewConfig instance.

As far as I can tell, this index allows us to uniquely identify each view internally, let me know if I'm wrong.

I think it may be simpler have view IDs just like panels do. I realized I would probably need some unique identifier for the pull-to-refresh work in bug 970707 to identify which viewconfig's JS "onrefresh" callback to call when the refresh is triggered. I find IDs much easier to read (more semantic) than internally defining view indexes at runtime. And later if we allow addons to modify views in anyway, they don't have to hold on to references or indexes to the view and can simply use a string ID to make an API call.

Comment 12

[Josh Dover [:jdover]]

In theory I could use the view index to do this, I just see the ID approach is being safer.

Comment 13

[Josh Dover [:jdover]]

Comment on attachment 8399492 [details] [diff] [review]
Create a loader per panel view instead of per dataset id (r=margaret)

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

Clearing NI as I don't see any glaring issues with filters here.

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to Josh Dover [:jdover] from comment #11)
> (In reply to Lucas Rocha (:lucasr) from comment #5)
> > (In reply to Lucas Rocha (:lucasr) from comment #4)
> > > Comment on attachment 8399491 [details] [diff] [review]
> > > Add 'index' property to ViewConfig (r=margaret)
> > > 
> > > We need ViewConfigs to be aware of its index so that we can use the index to
> > > identify dataset requests from the panel layout.
> > 
> > It's probably worth noting that the ViewConfig index is not written to
> > HomeConfig. It's just a runtime state of the ViewConfig instance.
> 
> As far as I can tell, this index allows us to uniquely identify each view
> internally, let me know if I'm wrong.
> 
> I think it may be simpler have view IDs just like panels do. I realized I
> would probably need some unique identifier for the pull-to-refresh work in
> bug 970707 to identify which viewconfig's JS "onrefresh" callback to call
> when the refresh is triggered. I find IDs much easier to read (more
> semantic) than internally defining view indexes at runtime. And later if we
> allow addons to modify views in anyway, they don't have to hold on to
> references or indexes to the view and can simply use a string ID to make an
> API call.

You don't need to identify specific views in order to implement a refresh request per view in bug 970707. What you need is just a way to connect the requester to the matching response somehow. You can identify the request with any kind of id, as long as:
1. The ID is unique across multiple parallel requests.
2. The response contains the request ID so that the caller can match the response with its request.

In practice, this means you'd add a request ID parameter to both HomePanels:Refresh and HomePanels:RefreshDone. It would probably be analogous to how requestPanelsById is implemented. See:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelManager.java#74
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Home.jsm#168

Comment 15

[Lucas Rocha (:lucasr)]

(In reply to Josh Dover [:jdover] from comment #12)
> In theory I could use the view index to do this, I just see the ID approach
> is being safer.

Just to be clear, there's a reason why I chose to use a runtime index instead of an 'ID' here: I'd really like to avoid forcing add-on developers to define an id for each view. It's error-prone and we don't have a strong enough reason to require it.

I'm actually fine to rename the term 'index' to 'id' if that makes the PanelLayout code easier to follow somehow (as long as this is not exposed in the add-on API).

Comment 16

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #15)
> (In reply to Josh Dover [:jdover] from comment #12)
> > In theory I could use the view index to do this, I just see the ID approach
> > is being safer.
> 
> Just to be clear, there's a reason why I chose to use a runtime index
> instead of an 'ID' here: I'd really like to avoid forcing add-on developers
> to define an id for each view. It's error-prone and we don't have a strong
> enough reason to require it.

I agree I don't see a need for views to have unique ids, so I don't think we should require them to have ids. Also, there isn't a need for an id in the most recent patches in bug 970707.

> I'm actually fine to rename the term 'index' to 'id' if that makes the
> PanelLayout code easier to follow somehow (as long as this is not exposed in
> the add-on API).

I prefer the term index, since that's what it is.

Comment 17

[:Margaret Leibovic]

Comment on attachment 8399492 [details] [diff] [review]
Create a loader per panel view instead of per dataset id (r=margaret)

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

Given the answers to my questions and the conversation in the bug, I'm upgrading my f+ to an r+ now :)

Comment 18

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/060116aac57f
https://hg.mozilla.org/integration/fx-team/rev/c821c23327da

Comment 19

[Lucas Rocha (:lucasr)]

FYI: I filed bug 994734 to work on the cursor change notification improvements.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/060116aac57f
https://hg.mozilla.org/mozilla-central/rev/c821c23327da

Comment 21

[:Margaret Leibovic]

Can we uplift this to aurora? I want to uplift bug 965622, but it depends on the patches here.

Comment 22

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #21)
> Can we uplift this to aurora? I want to uplift bug 965622, but it depends on
> the patches here.

Yes, I think should uplift if this blocks important stuff from uplifting. But I really think we should fix bug 994734. My plan for bug 994734 was actually pretty simple: rename HomeConfigInvalidator to HomePanelsInvalidator (or something along these lines) and handle HomePanels:RefreshDataset there. I probably won't have time to work on it before leaving on holidays until Tuesday. Feel free to take it.

Comment 23

[: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 24

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6eb15f722598

Comment 25

[:Margaret Leibovic]

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

Filter on epic-hub-bugs.