Closed Bug 509055 Opened 11 years ago Closed 11 years ago

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

Categories

(Core Graveyard :: History: Global, defect, trivial)

x86
Linux
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

(Whiteboard: [T.M.: mozilla1.9.2a2])

Attachments

(2 files, 3 obsolete files)

Attached file 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
Attached patch Proposed patch (obsolete) — Splinter Review
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.
Attachment #393235 - Flags: review?(Olli.Pettay)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Turns out, I could replace my setTimeout(1000) calls with setTimeout(0)s.  These are much better.
Attachment #393235 - Attachment is obsolete: true
Attachment #393270 - Flags: review?(Olli.Pettay)
Attachment #393235 - Flags: review?(Olli.Pettay)
Do you really need to remove the LOAD_BYPASS_HISTORY check?
I'll test that.
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.
Attachment #393270 - Flags: review?(Olli.Pettay) → review-
Attached patch Patch v1.2 (obsolete) — Splinter Review
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.
Attachment #393270 - Attachment is obsolete: true
Attachment #393532 - Flags: review?(Olli.Pettay)
(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.
Attachment #393532 - Flags: review?(Olli.Pettay) → review+
Attachment #393532 - Attachment is obsolete: true
Attachment #393590 - Flags: review+
Flags: wanted1.9.2?
Keywords: checkin-needed
Marking as fixed.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: wanted1.9.2?
Depends on: 562080
No longer depends on: 562080
Depends on: 628890
Flags: in-testsuite+
Whiteboard: [T.M.: mozilla1.9.2a2]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.