Closed Bug 1364127 Opened 6 years ago Closed 6 years ago

When restoring a tab, don't change its title to "New Tab" and then back to the previous title

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: marco, Assigned: dao)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1364127 - Set the initial tab label to the URL for new tabs and to the saved title for restored tabs, and make sure that label doesn't subsequently get clobbered with a placeholder
59 bytes, text/x-review-board-request
past
: review+
Details 
It should be possible to prevent this specifically for the "tab is restoring" case.
Blocks: 1359352
Component: Tabbed Browser → Session Restore
See Also: 1359352 →
Attached patch prototype patch (obsolete) — — Splinter Review 
Like this.

Should probably write a test for this.
Attachment #8866860 - Flags: feedback?(past)
Comment on attachment 8866860 [details] [diff] [review]
prototype patch

Review of attachment 8866860 [details] [diff] [review]:
-----------------------------------------------------------------

Oooh, I see! Yeah, that would work.
Attachment #8866860 - Flags: feedback?(past) → feedback+
Attachment #8866860 - Attachment is obsolete: true
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Comment on attachment 8866982 [details]
Bug 1364127 - Set the initial tab label to the URL for new tabs and to the saved title for restored tabs, and make sure that label doesn't subsequently get clobbered with a placeholder

https://reviewboard.mozilla.org/r/138582/#review141832

Perfect.
Attachment #8866982 - Flags: review?(past) → review+
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43c3f5bc92ee
Suppress unnecessary tab label changes when restoring a tab. r=past
backed out for developer's request, the test fails in non-e10s mode or so
Flags: needinfo?(dao+bmo)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b3de3cdbf99
Backed out changeset 43c3f5bc92ee for developer's request
Flags: needinfo?(dao+bmo)
Comment on attachment 8866982 [details]
Bug 1364127 - Set the initial tab label to the URL for new tabs and to the saved title for restored tabs, and make sure that label doesn't subsequently get clobbered with a placeholder

https://reviewboard.mozilla.org/r/138582/#review141894

::: browser/base/content/tabbrowser.xml
(Diff revision 2)
>          <parameter name="aReferrerURI"/>
>          <parameter name="aCharset"/>
>          <parameter name="aPostData"/>
>          <parameter name="aLoadInBackground"/>
>          <parameter name="aAllowThirdPartyFixup"/>
> -        <parameter name="aIsPrerendered"/>

This is just drive-by cleanup. No new <parameter/>s should be added to loadOneTab and addTab as the amount of optional parameters was getting unreasonably long years ago already. Instead, we use { option: value, ... } style parameters.

::: browser/components/sessionstore/SessionStore.jsm:928
(Diff revision 2)
>          // Update tab label and icon again after the tab history was updated.
> -        this.updateTabLabelAndIcon(tab, tabData);
> +        let label = this.getTabLabel(tabData);
> +        if (label) {
> +          tab.label = label;
> +        }
> +        this.updateTabIcon(tab, tabData);

FWIW, it's not clear to me why we're setting the label and icon here again... why would the tab data change between restoreTab() and SessionStore:restoreHistoryComplete?
Comment on attachment 8866982 [details]
Bug 1364127 - Set the initial tab label to the URL for new tabs and to the saved title for restored tabs, and make sure that label doesn't subsequently get clobbered with a placeholder

https://reviewboard.mozilla.org/r/138582/#review141988

::: browser/components/sessionstore/SessionStore.jsm:3312
(Diff revision 3)
>        let tab = tabbrowser.addTab(url,
>                                    { createLazyBrowser,
>                                      skipAnimation: true,
> +                                    initialLabel: this.getTabLabel(tabData),
>                                      userContextId,
>                                      skipBackgroundNotify: true });

I think I'll revert this again. Setting the label this early probably makes resizing the tabs more expensive while we're adding them...
Comment on attachment 8866982 [details]
Bug 1364127 - Set the initial tab label to the URL for new tabs and to the saved title for restored tabs, and make sure that label doesn't subsequently get clobbered with a placeholder

https://reviewboard.mozilla.org/r/138582/#review142010

::: browser/base/content/tabbrowser.xml:1437
(Diff revision 4)
>  
> +            if (aTab._suppressTransientPlaceholderLabel) {
> +              if (!title) {
> +                return false;
> +              }
> +              delete aTab._suppressTransientPlaceholderLabel;

In my first patch, I was deleting this flag regardless of the title. Keeping the flag until we get a non-empty title fixes the non-e10s failure in my test.
Comment on attachment 8866982 [details]
Bug 1364127 - Set the initial tab label to the URL for new tabs and to the saved title for restored tabs, and make sure that label doesn't subsequently get clobbered with a placeholder

I can't figure out how to re-request review in reviewboard.
Attachment #8866982 - Flags: review?(past)
Comment on attachment 8866982 [details]
Bug 1364127 - Set the initial tab label to the URL for new tabs and to the saved title for restored tabs, and make sure that label doesn't subsequently get clobbered with a placeholder

https://reviewboard.mozilla.org/r/138582/#review142072
Attachment #8866982 - Flags: review?(past) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9f3f63f8acb7
Set the initial tab label to the URL for new tabs and to the saved title for restored tabs, and make sure that label doesn't subsequently get clobbered with a placeholder r=past
https://hg.mozilla.org/mozilla-central/rev/9f3f63f8acb7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1365780
about:blank tabs restored with 'New Tab' label correctly, but once the tab selected or reload its label changes to 'about:blank'
(In reply to tabmix.onemen from comment #20)
> about:blank tabs restored with 'New Tab' label correctly, but once the tab
> selected or reload its label changes to 'about:blank'

Add to that if before setting the tabs reload all tabs, preference is not honoured .
Depends on: 1367630
I have reproduced this bug with Nightly 55.0a1 (2017-05-11) on Windows 8.1 (64 bit).

This bug's fix is verified on Latest Nightly 55.0a1.

Build ID : 20170531030204
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170531]
Thanks.
Status: RESOLVED → VERIFIED
Depends on: 1371896
Let's make sure this works as expected on Mac and Linux as well. Flagging for additional regression testing.
Flags: qe-verify+
Depends on: 1375007
No longer depends on: 1375007
I reproduced this issue using Fx55.0a1, build ID: 20170511063838, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx55.0b3, on Windows 10 x64, macOS X 10.12.5 and Ubuntu 14.04 LTS.

Cheers!
status-firefox55: fixed → verified
Flags: qe-verify+
Depends on: 1401091
No longer depends on: 1367630
Regressions: 1367630
You need to log in before you can comment on or make changes to this bug.