509055 - Changing document.title does not change SHEntry's title if document was loaded from history

Status: RESOLVED FIXED

(Core Graveyard :: History: Global, defect)

Description

[Justin Lebar (not reading bugmail)]

Attachment: Demo page

STR:
 * Open attached HTML document
 * Click down arrow next to forward button and observe that the current session history entry's title is "Initial title".
 * Click the Title 1 button.  Observe that the document's title has changed to "Title 1", and the session history entry's title has changed accordingly.
 * Click the link to mozilla.org, then click back.  Observe that the titles of the current document and current session history entry are both "Title 1".
 * Click the "Title 2" button.  Observe that the document's title is now "Title 2", but the session history entry's title is still "Title 1".

Expected behavior:
The session history entry's title should always change when the document's title changes.  After the last step, we expect the session history entry's title to be "Title 2", not "Title 1".

Reproduced on FF 3.5.2 and tip of trunk (changeset 4efc65d6447d).
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090807 Minefield/3.6a1pre

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Proposed patch

I have two issues with this patch:

 * I haven't been able to figure out a purpose for the checks I removed in nsDocShell::SetTitle, but that doesn't mean that they didn't serve a purpose.

  This patch will cause extra set title calls upon page loads.  I don't think they'll affect anything, because they'll just be setting the title back to what it was.  If we care about maintaining the same behavior we had before, we could add an extra parameter to SetTitle indicating whether we should ignore things if the load type is HISTORY or BYPASS_HISTORY.  But I'm not convinced that's necessary.

  * My test uses two one-second pauses.  I can't figure out how to get rid of them, but I know they're bad.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.1

Turns out, I could replace my setTimeout(1000) calls with setTimeout(0)s.  These are much better.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Do you really need to remove the LOAD_BYPASS_HISTORY check?
I'll test that.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 393270 [details] [diff] [review]
Patch v1.1


>-    // Update SessionHistory with the document's title. If the
>-    // page was loaded from history or the page bypassed history,
>-    // there is no need to update the title. There is no need to
>-    // go to mSessionHistory to update the title. Setting it in mOSHE 
>-    // would suffice. 
>-    if (mOSHE && (mLoadType != LOAD_BYPASS_HISTORY) &&
>-        (mLoadType != LOAD_HISTORY) && (mLoadType != LOAD_ERROR_PAGE)) {
>+    // Update SessionHistory with the document's title.
>+    if (mOSHE && mLoadType != LOAD_ERROR_PAGE) {
>         mOSHE->SetTitle(mTitle);    
>     }

Your tests test only LOAD_HISTORY, so please change only that behavior and leave 
LOAD_BYPASS_HISTORY there. Or add some tests for LOAD_BYPASS_HISTORY and explain
why we want to change that behavior.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.2

In IRC, bz and I determined that it's difficult (maybe impossible) for a user to trigger a LOAD_BYPASS_HISTORY.  So I think the right thing to do for now is to leave that check in the code, rather than to try to trigger a load of that type from the mochitest.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> In IRC, bz and I determined that it's difficult (maybe impossible) for a user
> to trigger a LOAD_BYPASS_HISTORY.
I think so too.

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Patch for checkin

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in:

http://hg.mozilla.org/mozilla-central/rev/24fbcd3c1dc2

Comment 9

[Justin Lebar (not reading bugmail)]

Marking as fixed.