941844 - Don't call BrowserToolbar.updateTitle six times when about:home loads

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Richard Newman [:rnewman]]

We are doing more work than we need to. Found while adding icon pre-fetching to Bug 938153.

about:home

11-21 12:45:21.065 I/GeckoToolbar(25034): XXX updateTitle.
11-21 12:45:21.065 I/GeckoToolbar(25034): zerdatime 630245043 - Throbber stop
11-21 12:45:21.205 I/GeckoToolbar(25034): XXX updateTitle.
11-21 12:45:21.215 I/GeckoToolbar(25034): XXX updateTitle.
11-21 12:45:21.215 I/GeckoToolbar(25034): zerdatime 630245192 - Throbber stop
11-21 12:45:21.235 I/GeckoToolbar(25034): XXX updateTitle.
11-21 12:45:21.245 I/GeckoToolbar(25034): XXX updateTitle.
11-21 12:45:21.285 I/GeckoToolbar(25034): zerdatime 630245264 - Throbber stop
11-21 12:45:21.285 I/GeckoToolbar(25034): XXX updateTitle.


Another page:

11-21 12:45:54.650 I/GeckoToolbar(25034): XXX updateTitle.
11-21 12:45:54.721 I/GeckoToolbar(25034): XXX updateTitle.
11-21 12:45:54.721 I/GeckoToolbar(25034): zerdatime 630278704 - Throbber start
11-21 12:45:54.871 I/GeckoToolbar(25034): XXX updateTitle.
11-21 12:45:57.303 I/GeckoToolbar(25034): XXX updateTitle.
11-21 12:45:57.924 I/InputMethodManager(25034): org.mozilla.fennec_rnewman called startInputInner(Do not restart input for non input field, ic=null) from android.view.inputmethod.InputMethodManager.restartInput(InputMethodManager.java:1125) <- org.mozilla.gecko.GeckoInputConnection.restartInput(GeckoInputConnection.java:413)
11-21 12:45:58.765 I/GeckoToolbar(25034): XXX updateTitle.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Perhaps bug 924712 is related?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

We could look at a few things here:
* about:home is a real HTML page. It exists in the browser back/forward session. It is (or was) bookmark-able. That said, it has some HTML tags in it that we might want to remove if they are not surfaced anywhere:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutHome.xhtml

I see <title> and <link rel="icon"> and I wonder if any part of the Java UI depend on them.

* We send <title> messages (DOMTitleChanged) to Java.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3555

notice that we trim to 255 chars since we were OOM'ing on pathological tests. We might get some perf improvements by not even sending the title change message if the 255 chars are not different than the current title. I think, at the time, I skipped the check in browser.js since it would require that I hold a variable for the lastTitle. Hopefully we do the check on the Java side and don't just always pass the same string to the UI. I don't think the Android widget short circuits if the text is the same.

Crap, we don't do the check and we fire off a notification that the title has changed:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#316

Notifying onTabChanged causes a lot of code to be fired, even if the majority ignore the TITLE part:
http://mxr.mozilla.org/mozilla-central/ident?i=onTabChanged

I found these that respond to TITLE:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.java#159
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java#674

Comment 3

[Richard Newman [:rnewman]]

This is kinda funnysad.

We load about:home. We send a LOCATION_CHANGE event.

Tabs goes "oh, the location changed". It tells the toolbar, which refreshes itself, which updates its title by asking the current tab.

The Tab also notices the location change, and sends a TITLE event. This ends up at the toolbar, which updates its title.

Then the page finishes loading, and we get a DOMTitleChanged event.

Comment 4

[Richard Newman [:rnewman]]

Attachment: Part 1: fix the bug. v1

This patch does the following.

* General cleanup along the way.

* If a tab's title is told to change, and it was already set to the same string, don't set it and don't broadcast a title change event!

* Make BrowserToolbar.refresh private, because it's only called from one place.

* Don't call BrowserToolbar.updateTitle on a LOADED event. There are a thousand other events that do the same thing.

* Don't call BrowserToolbar.updateTitle on a LOCATION_CHANGE event. `Tab` observes the same event, and will broadcast a TITLE event if its title changed as a result of the location change. (Which it usually will, because we null it out first.)

Comment 5

[Richard Newman [:rnewman]]

Attachment: Part 2: refactor BrowserToolbar.onTabChanged

Oh so much less awful to read and change.

Comment 6

[Brian Nicholson (:bnicholson)]

Comment on attachment 8337023 [details] [diff] [review]
Part 1: fix the bug. v1

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

Looks fine to me, although it's difficult to follow some of this flow because of the events spewed everywhere. I doubt we have a lot of test coverage for title changes, so we should be on the lookout for any regressions. While here, feel free to add more such tests if you'd like :)

::: mobile/android/base/Tab.java
@@ +319,5 @@
>              return;
> +        }
> +
> +        // If there was a title, but it hasn't changed, do nothing.
> +        if (mTitle != null &&

Any reason for this null check?

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8337028 [details] [diff] [review]
Part 2: refactor BrowserToolbar.onTabChanged

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

Much better!

Comment 8

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #6)

> Looks fine to me, although it's difficult to follow some of this flow
> because of the events spewed everywhere.

I have an idea rattling around in my head about an event grapher... which I'll probably never have time to build. Need moar interns.

> I doubt we have a lot of test
> coverage for title changes, so we should be on the lookout for any
> regressions. While here, feel free to add more such tests if you'd like :)

I'll see what I can do!

> Any reason for this null check?

Yeah -- it avoids us performing the new short-circuiting when mTitle has never been set before. I figured that was an edge case worth avoiding.

Comment 9

[Richard Newman [:rnewman]]

Gonna follow up on this with a little extra Robocop love.

  https://hg.mozilla.org/integration/fx-team/rev/d1a2f394c684
  https://hg.mozilla.org/integration/fx-team/rev/34e6d95c9eb5

Comment 10

[Matt Brubeck (:mbrubeck)]

part 1: https://hg.mozilla.org/mozilla-central/rev/d1a2f394c684
part 2: https://hg.mozilla.org/mozilla-central/rev/34e6d95c9eb5

Comment 11

[Richard Newman [:rnewman]]

Attachment: Messages sent during page loads.

There are at least 89 messages processed by BrowserToolbar, Tab, and BrowserApp during loading about:home, navigating to a page, and then hitting Back.

Here they are.

(This is after I've already made some improvements!)

Comment 12

[Richard Newman [:rnewman]]

Finally landed the follow-up test.

https://hg.mozilla.org/integration/fx-team/rev/f3f8f2713180

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3f8f2713180