Closed Bug 789887 Opened 7 years ago Closed 7 years ago

Multiple successive calls to getTopSites() in AboutHomeContent

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 2 obsolete files)

I've been investigating slowness in our top sites query for the awesome screen in bug 785945, and that led me to find that on startup on about:home we end up kicking off *3* top sites queries, which sounds like something we should avoid.
Looks like this is happening because onActivityContentChanged is getting called multiple times, which we call from BrowserApp.onContentChanged().

Lucas, I see you in the blame history here, so do you remember why we chose this place to call loadTopSites? I feel like we need to figure out a way to only get the cursor once per time the page is shown.
(In reply to Margaret Leibovic [:margaret] from comment #1)
> Looks like this is happening because onActivityContentChanged is getting
> called multiple times, which we call from BrowserApp.onContentChanged().
> 
> Lucas, I see you in the blame history here, so do you remember why we chose
> this place to call loadTopSites? I feel like we need to figure out a way to
> only get the cursor once per time the page is shown.

IIRC, I simply kept this call in onContentChanged() when I implemented the final design for about:home. Brad was the one who added this call in the original code. Feels like this call could be removed now as I made about:home get updated every time it's shown, see:

  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#618
Actually, upon further investigation, I found that the very first one of these loadTopSites calls came from finishProfileMigration, which seems like it might not be necessary, but that only happens on first run, so that's not our worst problem for now.

Then I'm seeing another 3 come from that AboutHomeRunnable call. I added some more logging and found that those come from initializeChrome, then the location change tab event and the selected tab event :/

I think the simplest thing to do here is to just cache the cursor and not run the query more than every 30 seconds or so. However, that means we'd also need to add some sort of force-query flag for things like the call to update top sites after history has been cleared.
Attached patch WIP (obsolete) — Splinter Review
I'm not sure that I like adding a force parameter like I did in here, but I'm not sure if there's a better way to do this without refactoring the way we handle the different UpdateFlags.
Assignee: nobody → margaret.leibovic
Attachment #659688 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 659688 [details] [diff] [review]
WIP

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

We still get the multiple successive queries even if you remove the onContentChanged() callback? The timeout code is ok-ish but I'd prefer a solution where we only re-run the query when necessary.

::: mobile/android/base/AboutHomeContent.java
@@ +294,5 @@
>              }
>          });
>      }
>  
> +    void update(final EnumSet<UpdateFlags> flags, final boolean force) {

I'd probably just add an UpdateFlags.FORCE instead of this extra argument here.

@@ +299,4 @@
>          GeckoAppShell.getHandler().post(new Runnable() {
>              public void run() {
>                  if (flags.contains(UpdateFlags.TOP_SITES))
> +                    loadTopSites(force);

Then this would become loadTopSites(flags.contains(UpdateFlags.FORCE))
Attachment #659688 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #5)

> We still get the multiple successive queries even if you remove the
> onContentChanged() callback? The timeout code is ok-ish but I'd prefer a
> solution where we only re-run the query when necessary.

Yeah, see comment 3. I agree that it would be good to try just to re-run the query when necessary, but that would involve refactoring the way we update about:home to decouple updating the cursor from updating the UI, since right now those concepts are bundled together in showAboutHome(). This problem traces its way back to calling showAboutHome() from different places, like when we get a location change event and a tab select event for the first tab on startup. I suppose the best solution would make it so that we only call showAboutHome() once in any given situation when we show it, which perhaps we can do by checking if it's already showing. I can investigate that, since that would prevent making a whole new AboutHomeRunnable when it's not necessary.

> ::: mobile/android/base/AboutHomeContent.java
> @@ +294,5 @@
> >              }
> >          });
> >      }
> >  
> > +    void update(final EnumSet<UpdateFlags> flags, final boolean force) {
> 
> I'd probably just add an UpdateFlags.FORCE instead of this extra argument
> here.

I was thinking that, but then we'd have to change UpdateFlags.ALL. I guess that's not a big deal, though. Definitely more elegant.
This is simpler. As a I mentioned in comment 6, the root problem is really that we create multiple AboutHomeRunnables that aren't necessary. This should save us from doing that extra work.
Attachment #659688 - Attachment is obsolete: true
Attachment #663555 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 663555 [details] [diff] [review]
avoid creating unnecessary AboutHomeRunnables

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

Nice.

::: mobile/android/base/BrowserApp.java
@@ +604,5 @@
>          Runnable r = new AboutHomeRunnable(false);
>          mMainHandler.postAtFrontOfQueue(r);
>      }
>  
> +    private class AboutHomeRunnable implements Runnable {

Seems unrelated.
Attachment #663555 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #8)

> ::: mobile/android/base/BrowserApp.java
> @@ +604,5 @@
> >          Runnable r = new AboutHomeRunnable(false);
> >          mMainHandler.postAtFrontOfQueue(r);
> >      }
> >  
> > +    private class AboutHomeRunnable implements Runnable {
> 
> Seems unrelated.

Yeah, I guess it's a little scope-creepy, but I took this opportunity to clean up the access level of these methods/class to restrict them to what's actually needed.
https://hg.mozilla.org/mozilla-central/rev/c1003f039c77
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 794731
Backed out in bug 794731.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Shouldn't mAboutHomeShowing be initialised to something in a constructor?
Attached patch patch v2Splinter Review
The problem that caused bug 794731 was that we depend on hiding the about:home content in the onTabChanged handler. We weren't hiding it because mAboutHomeShowing was never set to true, so it was hanging around and obscuring the normal content view.

This also makes me realize that right now we're creating/running a new AboutHomeRunnable on every location change event, which will be nice to avoid.

I experimented with making AboutHomeContent hidden by default in gecko_app, but that led to some ugly flickering when launching the app regularly, so I think this approach is better.
Attachment #663555 - Attachment is obsolete: true
Attachment #665951 - Flags: review?(lucasr.at.mozilla)
(In reply to Robert Longson from comment #13)
> Shouldn't mAboutHomeShowing be initialised to something in a constructor?

In Java, a primitive boolean defaults to false.
Comment on attachment 665951 [details] [diff] [review]
patch v2

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

Nice catch.
Attachment #665951 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/57c89d10f0ab
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.