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

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

({perf})

Trunk
Firefox 28
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
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.
Perhaps bug 924712 is related?
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
(Assignee)

Updated

5 years ago
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 8337023 [details] [diff] [review]
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.)
Attachment #8337023 - Flags: review?(bnicholson)
(Assignee)

Comment 5

5 years ago
Created attachment 8337028 [details] [diff] [review]
Part 2: refactor BrowserToolbar.onTabChanged

Oh so much less awful to read and change.
Attachment #8337028 - Flags: review?(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?
Attachment #8337023 - Flags: review?(bnicholson) → review+
Comment on attachment 8337028 [details] [diff] [review]
Part 2: refactor BrowserToolbar.onTabChanged

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

Much better!
Attachment #8337028 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
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
Whiteboard: wat → [leave open]
Target Milestone: --- → Firefox 28
(Assignee)

Comment 11

5 years ago
Created attachment 8337967 [details]
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!)

Updated

5 years ago
tracking-fennec: --- → ?
Keywords: perf
(Assignee)

Comment 12

5 years ago
Finally landed the follow-up test.

https://hg.mozilla.org/integration/fx-team/rev/f3f8f2713180
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/f3f8f2713180
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.