503832 - Visiting a new fragmentid doesn't set title in Global History

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[John P Baker]

When the fragmentid of the current document is changed the docshell sets the title for the Session History entry, however it doesn't do the the equivalent for the Global History.

See bug 420605 comment #20.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Might be reasonable...  Patches accepted!

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: A fix based on John's suggestion, including test case

I agree with John's analysis that Bug 420605 is really two bugs because the favicon is set by the tabbrowser whereas the page title is stored in global history by the docshell.

The attached patch (made against mozilla-central) is for this bug, the docshell part. It's based John's suggestion to explicitly set the page title for anchor URIs in nsDocShell::InternalLoad (the only difference being the IMHO correct parameters passed to SetPageTitle). I have also amended the call to OnNewURI to include values for all *six* parameter. Previously the 'aAddToGlobalHistory' parameter was omitted.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 430858 [details] [diff] [review]
A fix based on John's suggestion, including test case

r=bzbarsky.

For future reference, patches with at least 8 lines of context and the -p switch used are a lot easier to review...

Philipp, do you need this pushed or can you do that yourself?

Comment 4

[Philipp von Weitershausen [:philikon]]

Thanks Boris! Will make sure to generate diffs with -pU8 next time. I don't have a committer account, so a push would be appreciated. :)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/dc835fb21c0c

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Landed:
http://hg.mozilla.org/mozilla-central/rev/2da8ac54d264
to fix a massive leak caused by the test.

Comment 7

[Philipp von Weitershausen [:philikon]]

Thanks Boris for the commit and David for catching the missing tear down!

Comment 8

[John P Baker]

(In reply to comment #5)
> Pushed http://hg.mozilla.org/mozilla-central/rev/dc835fb21c0c

(In reply to comment #6)
> http://hg.mozilla.org/mozilla-central/rev/2da8ac54d264

These were backed out in
http://hg.mozilla.org/mozilla-central/rev/eaf1574b10b6 and
http://hg.mozilla.org/mozilla-central/rev/44ef1e212970

When they go back in it is likely that
http://hg.mozilla.org/mozilla-central/diff/817634616c8b/docshell/test/browser/browser_bug503832.js
may also be required - depending on state of bug 544097

Comment 9

[Philipp von Weitershausen [:philikon]]

Boris writes in the commit log that the patch had to be backed out "on suspicion of causing a Tsspider regression." I've tried finding a corresponding failure log on tinderbox, but wasn't successful. Do we know what exactly broke? It would be good if we can reproduce it so that we know how to move this bug forward in a way that won't cause this regression.

Sorry for my ignorance, any pointers would be gladly appreciated :)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Philipp, the regression in question is a performance regression.  Here's an example mail (there were such mails for WinXP, Win7, Linux, and Linux64) in m.d.tree-management:

  Regression: SunSpider increase 5.00% on Win7 Firefox

  Previous results: 38.0 from build 20100308163523 of revision 1cb03235231e
  at 2010-03-08 17:39:00 on talos-r3-w7-015 run # 0

  New results: 39.9 from build 20100308171451 of revision 1008f5c2e057 at
  2010-03-08 18:18:00 on talos-r3-w7-010 run # 0

  Suspected checkin range: from revision 1cb03235231e to revision 1008f5c2e057

That last is a link to http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1cb03235231e&tochange=1008f5c2e057

The "increase 5.00%" is a link to http://graphs.mozilla.org/graph.html#tests=[{%22machine%22:351,%22test%22:25,%22branch%22:1},{%22machine%22:352,%22test%22:25,%22branch%22:1},{%22machine%22:353,%22test%22:25,%22branch%22:1},{%22machine%22:354,%22test%22:25,%22branch%22:1},{%22machine%22:355,%22test%22:25,%22branch%22:1},{%22machine%22:356,%22test%22:25,%22branch%22:1},{%22machine%22:357,%22test%22:25,%22branch%22:1},{%22machine%22:358,%22test%22:25,%22branch%22:1},{%22machine%22:359,%22test%22:25,%22branch%22:1},{%22machine%22:360,%22test%22:25,%22branch%22:1},{%22machine%22:361,%22test%22:25,%22branch%22:1},{%22machine%22:362,%22test%22:25,%22branch%22:1},{%22machine%22:363,%22test%22:25,%22branch%22:1},{%22machine%22:364,%22test%22:25,%22branch%22:1},{%22machine%22:365,%22test%22:25,%22branch%22:1},{%22machine%22:366,%22test%22:25,%22branch%22:1},{%22machine%22:367,%22test%22:25,%22branch%22:1},{%22machine%22:368,%22test%22:25,%22branch%22:1},{%22machine%22:369,%22test%22:25,%22branch%22:1},{%22machine%22:370,%22test%22:25,%22branch%22:1}]&sel=1268014680,1268187480

I backed out this bug and bug 550882 as the most likely causes of the regression, and that made the performance go back to where it used to be.  I'm now going to reland them separately from each other so we can figure out which of the two is responsible for the regression.

I figured I wouldn't reopen the bug unless the problem was pinned on this patch to keep the bugmail down, but it's probably safer to reopen when backing out...

Comment 11

[Boris Zbarsky [:bzbarsky]]

I just verified that the regression was due to bug 550882.  I will try to reland this bug (both patches rolled into one commit) tomorrow.

Comment 12

[Philipp von Weitershausen [:philikon]]

Excellent, thanks Boris! Any chance this could be merged to mozilla-1.9.2 as well?

Comment 13

[Boris Zbarsky [:bzbarsky]]

You'll want to request approval on the patch, and make your case to the branch drivers for that.

Tree's been broken all day today, hence not landed yet.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Relanded: http://hg.mozilla.org/mozilla-central/rev/eaa7cd37329f (contains both the original push and dbaron's test fix).

Comment 15

[Boris Zbarsky [:bzbarsky]]

And pushed http://hg.mozilla.org/mozilla-central/rev/9eaa1ebf0caa to fix orange because someone checked in bug 544097 between when I loaded this bug and when I finally got to push this patch...