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

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Marco Castelluccio [:marco]]

See bug 1359352 comment 24.

Comment 1

[Dão Gottwald [:dao]]

It should be possible to prevent this specifically for the "tab is restoring" case.

Comment 2

[Dão Gottwald [:dao]]

Attachment: prototype patch

Like this.

Should probably write a test for this.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8866860 [details] [diff] [review]
prototype patch

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

Oooh, I see! Yeah, that would work.

Comment 4

[Dão Gottwald [:dao]]

Attachment: 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

Review commit: https://reviewboard.mozilla.org/r/138582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138582/

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43c3f5bc92ee
Suppress unnecessary tab label changes when restoring a tab. r=past

Comment 7

[Iris Hsiao [:ihsiao]]

backed out for developer's request, the test fails in non-e10s mode or so

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b3de3cdbf99
Backed out changeset 43c3f5bc92ee for developer's request

Comment 9

[Dão Gottwald [:dao]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138582/diff/1-2/

Comment 10

[Dão Gottwald [:dao]]

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 11

[Dão Gottwald [:dao]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138582/diff/2-3/

Comment 12

[Dão Gottwald [:dao]]

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 13

[Dão Gottwald [:dao]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138582/diff/3-4/

Comment 14

[Dão Gottwald [:dao]]

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 15

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5b7ea5b8414957487cc79302f7262e53205bcac

Comment 16

[Dão Gottwald [:dao]]

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.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9f3f63f8acb7

Comment 20

[tabmix.onemen]

about:blank tabs restored with 'New Tab' label correctly, but once the tab selected or reload its label changes to 'about:blank'

Comment 21

[u595641]

(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 .

Comment 22

[Azmina]

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]

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Thanks.

Comment 24

[Andrei Vaida [:avaida]]

Let's make sure this works as expected on Mac and Linux as well. Flagging for additional regression testing.

Comment 25

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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!