46 bytes, text/x-phabricator-request
|Details | Review|
Currently if nsDocShell::SetTitle is called twice in a row with the same title, it will attempt to re-set the title on the parent, in history and in the session history. This causes unnecessary disk i/o, and maybe some other perf impact. I can see this by adding a little bit of debug, and then browsing around sites, e.g. - If I navigate somewhere, and then reload a page, the title will be set a second time. - If I load https://www.reddit.com/, then the site seems to set the title a couple of times. - Also navigating around reddit will hit this (e.g. load a sub-group, load the "all" page).
I've a try push in progress for this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=806a5351278408092a69c3222d2269f6eb7a566d My only minor concern is that the patch as-is currently will also ignore an initial setTitle where the title is the empty string. I think that is probably OK, but hopefully the test run will pull out any issues.
Status: NEW → ASSIGNED
So my basic change didn't quite work. It appears we need to check that both the URI and the title haven't changed (rather than just the title). This is to protect against e.org -> e.com transitions where the pages have the same title, this was caught by browser_e10s_switchbrowser.js. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6a7f62baa6ef5f0ba5d8fecd4a32648f2be776a
This avoids setting the title a second time when the title and uri remain the same across setTitle calls. This means we can avoid unnecessary history updates which currently result in extra disk i/o.
Boris: did you see the patch update here fixing your review comments?
I did. I was on unexpected PTO for a while and am now working through my backlog... ETA sometime today.
No problem. Thank you.
Comment on attachment 9007785 [details] Bug 1489503 - In docShell, don't set the same title a second time. Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.
Attachment #9007785 - Flags: review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/3266c319db3b In docShell, don't set the same title a second time. r=bzbarsky
Would it be possible, in a separate bug, to add a test for this? I think it should be enough to make a mochitest-browser that, after the first visit and title were notified, sets location.hash and checks we don't get a history notification for a title change.
(In reply to Marco Bonardo [::mak] from comment #10) > Would it be possible, in a separate bug, to add a test for this? > > I think it should be enough to make a mochitest-browser that, after the > first visit and title were notified, sets location.hash and checks we don't > get a history notification for a title change. I debated that with myself previously, and I wasn't quite sure as it would mean that we were attempting to check for the existence of something that happens asynchronously. Replacing the history service seemed pretty hairy, and possibly only doable from c++ tests. The other issue here is that the history code didn't used to send out two updates - it checks for the current title on the page before doing the write: https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/toolkit/components/places/History.cpp#1409-1417 So we're saving ourselves some internal updates and a database read, but I don't see a way to test for that.
You need to log in before you can comment on or make changes to this bug.