598221 - Page Title not shown in Title Bar on Session Restore

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Mycroft Project (CC)]

(May be Shell Integration... not sure)

Think this may only be the case for Multi window sessions.

1. File Exit
2. Open browser
3. Note title is 'Minefield' until selecting a diff tab
(Seems to not happen in the active window but this may be a auth. required prompt doing the tab changing and hiding the symptom)

pretty sure this has regressed at some point but not exactly sure when
(fairly sure it was the case before Cascaded Session Restore arrived though)

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Thanks Charles. If you'd be willing to help find a regression range, that would be helpful!

Comment 2

[[not reading bugmail]]

hmm, do you have about:home in your list of tabs?

Comment 3

[Mycroft Project (CC)]

?me... nope
Paul, I'll try and hunt down a range at some point over the w/e

Comment 4

[[not reading bugmail]]

Attachment: 2 windows open: both logged into bugzilla, bad page title in nonactive restored window

Ignore that last comment, this is it here, this shows the problem.  I have an example 2 windows after session restore using save&quit.

Example shown in the screenshot: 1 window has 2 tabs in the background on session restore from "save & quit", the other foreground window has 1 tab.  The background window didn't update the titlebar with the page title, but after switching to next tab and back, it shows up.  The foreground window updates the page title.

Comment 5

[Will Roberts [:bws42]]

Here's the regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=46310d87f848&tochange=a73c063e52cb

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Could be because I explicitly set the tab title right after the load starts (haven't tested though)
http://hg.mozilla.org/mozilla-central/file/1c33f957836a/browser/components/sessionstore/src/nsSessionStore.js#l2532

Looks like tabbrowser only calls setTabTitle(tab) after a load completes if the tab title was previously the loading string.

Perhaps we shouldn't force a tab title when loading like I did (especially since the <title> might have actually changed).

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I think I'm seeing this too. I just updated to the latest Mac nightly (71e8b5aee972), and all of the tabs one of my two windows all have the correct favicon, but their titles are set to "New Tab", and they aren't actually loaded. When I switch to them, they load and the title gets corrected. I don't have the any non-default "*session*" prefs, apart from privacy_level, so I think this bug is more than just failure to set the title.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Hmm, I just updated again and my session restore properly this time... given the lack of changes between this builds, maybe that means the bug is intermittent? :(

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=71e8b5aee972&tochange=6eb42326798b

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Please disregard my comments here - I misunderstood what this bug was about!

Comment 10

[tabmix.onemen]

just call aWindow.gBrowser.updateTitlebar() once after updating all labels or when set selected tab label

Comment 11

[:Gavin Sharp [email: [e-mail]]]

I think we should instead add an aTitle parameter to setTabTitle that is preferred over browser.contentTitle, and then have sessionstore just call that.

Comment 12

[Shawn Wilsher :sdwilsh]

This also breaks titles in aero peek on windows 7.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> This also breaks titles in aero peek on windows 7.
...and I'm informed that that is not this issue

Comment 14

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

added aTitle parameter to setTabTitle and then used it. There's another place in sessionstore where we might want to use this (http://hg.mozilla.org/mozilla-central/file/ff37d20b4fc7/browser/components/sessionstore/src/nsSessionStore.js#l2413) but I think we should actually be ok. If we get into that if block, then we'll get into this block in restoreTab so it's not super important.

However I think this could lead to an issue if the page's <title> has been updated between saving the session and restoring it. This should actually already be a problem, it's just wouldn't be super common. We'll force the old title via this change (because we only call setTabTitle if the previous title was tabs.connecting) - http://hg.mozilla.org/mozilla-central/file/ff37d20b4fc7/browser/base/content/tabbrowser.xml#l467 - I guess I'll make sure that's actually the case and file.

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Even though Shawn was wrong about this breaking aero peek (my bad) we should still block final on it.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 482723 [details] [diff] [review]
Patch v0.1

This looks like it will set crop="end" when it should be "center" for URIs.

Comment 17

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #16)
> Comment on attachment 482723 [details] [diff] [review]
> Patch v0.1
> 
> This looks like it will set crop="end" when it should be "center" for URIs.

Ah true. If we explicitly set the label to a URI already then yea, that will happen. We could pass aCrop which would set the be used by default instead of "end"? Then we could just pull off what we already set crop to and use that. Or just explicitly reset it again (amounts to pretty much the same)

Comment 18

[Dão Gottwald [:dao]]

Does your patch even fix this bug, i.e. does the document have a title at that point? updateTitlebar needs that.

Comment 19

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #18)
> Does your patch even fix this bug, i.e. does the document have a title at that
> point? updateTitlebar needs that.

It indeed does not fix this bug...

Why doesn't updateTitlebar (or I guess getWindowTitleForBrowser) use the tab title? That's set to use the page title or URI anyway...

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> (In reply to comment #18)
> > Does your patch even fix this bug, i.e. does the document have a title at that
> > point? updateTitlebar needs that.
> 
> It indeed does not fix this bug...
> 
> Why doesn't updateTitlebar (or I guess getWindowTitleForBrowser) use the tab
> title? That's set to use the page title or URI anyway...

As opposed to the tab label, the window title doesn't display the URI, nor does it display "Loading...".

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 482723 [details] [diff] [review]
Patch v0.1

Can these lines just be dropped?

        // gotoIndex will force the "loading" string, so set the title
        aTab.label = label;

Comment 22

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #21)
> Comment on attachment 482723 [details] [diff] [review]
> Patch v0.1
> 
> Can these lines just be dropped?
> 
>         // gotoIndex will force the "loading" string, so set the title
>         aTab.label = label;

I'm fine with that. I offered that up in comment #6. UI will get a little busier than it has been since CSR landed, but not really much different than 3.6.

Comment 23

[Dão Gottwald [:dao]]

It would be consistent with the throbber as well. Seems like we should do this and file a follow-up bug on properly suppressing the loading indicators if that's really wanted.

Comment 25

[Dave Garrett]

From bug 604550: This can be reproduced by restoring only one tab with undo close. With 2 tabs open, ctrl+w, ctrl+shift+t -> no page title in title bar.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Just removing the title setting sounds good to me.

Comment 27

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Get rid of the manual label setting completely here.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 483599 [details] [diff] [review]
Patch v0.2

>       try {
>         didStartLoad = true;
>-        // Get the tab title (set in restoreHistoryPrecursor) for later
>-        let label = aTab.label;
>         browser.webNavigation.gotoIndex(activeIndex);
>-        // gotoIndex will force the "loading" string, so set the title
>-        aTab.label = label;
>       }
>       catch (ex) {
>         // ignore page load errors

It looks like didStartLoad = true; should be outside of the try block. And maybe there should be a didStartLoad = false; in the catch block? Not relevant for this bug, though.

Comment 29

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #28)
> Comment on attachment 483599 [details] [diff] [review]
> Patch v0.2
> 
> >       try {
> >         didStartLoad = true;
> >-        // Get the tab title (set in restoreHistoryPrecursor) for later
> >-        let label = aTab.label;
> >         browser.webNavigation.gotoIndex(activeIndex);
> >-        // gotoIndex will force the "loading" string, so set the title
> >-        aTab.label = label;
> >       }
> >       catch (ex) {
> >         // ignore page load errors
> 
> It looks like didStartLoad = true; should be outside of the try block. And
> maybe there should be a didStartLoad = false; in the catch block? Not relevant
> for this bug, though.

Ah yea good call. I'll file & fix.

Comment 30

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/7b3cecd25e60

Comment 31

[[not reading bugmail]]

Attachment: Verified Fixed - 2 windows

Verified fixed with:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101019 Firefox/4.0b8pre ID:20101019165901

Comment 32

[Dave Garrett]

Verified fixed for comment 25 (bug 604550) STR as well.