1407351 - Remove E10S_TESTING_ONLY features

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[:Felipe Gomes (needinfo for replies!)]

This #ifdef has Nightly-only features that include the checkbox for e10s in about:preferences, and the "Open Non-e10s Window" toolbar button.  Now that e10s is completing its rollout and becomes the de-facto configuration, we can remove these things.

This should land before or together with bug 1406212 because otherwise we'd need to waste time adjusting the code for the checkbox in about:preferences.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1407351 - Remove nightly-only e10s testing features from the main browser window.

The only thing that I didn't remove was the process ID on the tab tooltip, which I find to be super helpful. For that, I changed the check from E10S_TESTING_ONLY to NIGHTLY_BUILD.

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

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1407351 - Remove Nightly-only e10s checkbox in about:preferences.

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

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1407351 - Simplify check for e10s in about:preferences.

This code that checks the status code is not necessary, the boolean answer is already stored in Services.appinfo.browserTabsRemoteAutostart.

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

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1407351 - Remove E10S_TESTING_ONLY from devtools.

E10s is always supported now, so there's no need for the unsupported notice anymore

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

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1407351 - Remove E10S_TESTING_ONLY defines.

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

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8917151 [details]
Bug 1407351 - Remove E10S_TESTING_ONLY defines.

https://reviewboard.mozilla.org/r/188162/#review193454

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8917147 [details]
Bug 1407351 - Remove nightly-only e10s testing features from the main browser window.

https://reviewboard.mozilla.org/r/188154/#review193538

This looks good to me (I'm always very grateful for patches that clean things up :-) ), but the r- is because you need to add a migration step for profiles that may have customized that widget.
You can do this in `CustomizableUI._updateForNewVersion()`, which is very similar to `_migrateUI()` in nsBrowserGlue.js. You can find a good example at the bottom of that function where we remove the 'loop' and 'pocket' buttons.

::: browser/base/content/tabbrowser.xml:5535
(Diff revision 1)
>            } else {
>              label = tab._fullLabel || tab.getAttribute("label");
> -            if (AppConstants.E10S_TESTING_ONLY &&
> +            if (AppConstants.NIGHTLY_BUILD &&
>                  tab.linkedBrowser &&
>                  tab.linkedBrowser.isRemoteBrowser) {
>                label += " - e10s";

I'd like you to remove this label addition then, because the tab being e10s is kinda implicit.

::: browser/base/content/tabbrowser.xml:5538
(Diff revision 1)
>                  tab.linkedBrowser &&
>                  tab.linkedBrowser.isRemoteBrowser) {
>                label += " - e10s";
>                if (tab.linkedBrowser.frameLoader &&
>                    Services.appinfo.maxWebProcessCount > 1) {
>                  label += " (" + tab.linkedBrowser.frameLoader.tabParent.osPid + ")";

And somehow denote this string so that it becomes clear to Nightly users what the number means, like 'Nightly Start Page (pid 318)'

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917147 [details]
Bug 1407351 - Remove nightly-only e10s testing features from the main browser window.

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

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917148 [details]
Bug 1407351 - Remove Nightly-only e10s checkbox in about:preferences.

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

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917149 [details]
Bug 1407351 - Simplify check for e10s in about:preferences.

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

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917150 [details]
Bug 1407351 - Remove E10S_TESTING_ONLY from devtools.

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

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917151 [details]
Bug 1407351 - Remove E10S_TESTING_ONLY defines.

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

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8917147 [details]
Bug 1407351 - Remove nightly-only e10s testing features from the main browser window.

https://reviewboard.mozilla.org/r/188154/#review194126

Prima!

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8917148 [details]
Bug 1407351 - Remove Nightly-only e10s checkbox in about:preferences.

https://reviewboard.mozilla.org/r/188156/#review194180

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8917149 [details]
Bug 1407351 - Simplify check for e10s in about:preferences.

https://reviewboard.mozilla.org/r/188158/#review194182

Comment 16

[Greg Tatum [:gregtatum]]

Comment on attachment 8917150 [details]
Bug 1407351 - Remove E10S_TESTING_ONLY from devtools.

https://reviewboard.mozilla.org/r/188160/#review194230

I think these changes look fine, thanks.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aef2a6367c8
Remove nightly-only e10s testing features from the main browser window. r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/537e32fd3d48
Remove Nightly-only e10s checkbox in about:preferences. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8938454c1f0
Simplify check for e10s in about:preferences. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/f60f54d7af0c
Remove E10S_TESTING_ONLY from devtools. r=gregtatum
https://hg.mozilla.org/integration/mozilla-inbound/rev/5725fbd3af70
Remove E10S_TESTING_ONLY defines. r=glandium

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a58f8ab311d
Back out to avoid a merge conflict with bug 1406212. I'll reland tomorrow

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/3aef2a6367c8
https://hg.mozilla.org/mozilla-central/rev/537e32fd3d48
https://hg.mozilla.org/mozilla-central/rev/b8938454c1f0
https://hg.mozilla.org/mozilla-central/rev/f60f54d7af0c
https://hg.mozilla.org/mozilla-central/rev/5725fbd3af70

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917147 [details]
Bug 1407351 - Remove nightly-only e10s testing features from the main browser window.

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

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917148 [details]
Bug 1407351 - Remove Nightly-only e10s checkbox in about:preferences.

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

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917149 [details]
Bug 1407351 - Simplify check for e10s in about:preferences.

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

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917150 [details]
Bug 1407351 - Remove E10S_TESTING_ONLY from devtools.

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

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917151 [details]
Bug 1407351 - Remove E10S_TESTING_ONLY defines.

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86a360e12b8a
Remove nightly-only e10s testing features from the main browser window. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/a53f34a45f9b
Remove Nightly-only e10s checkbox in about:preferences. r=jaws
https://hg.mozilla.org/integration/autoland/rev/effeee94316e
Simplify check for e10s in about:preferences. r=jaws
https://hg.mozilla.org/integration/autoland/rev/8e04412824b5
Remove E10S_TESTING_ONLY from devtools. r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/e6ee25ad6a9f
Remove E10S_TESTING_ONLY defines. r=glandium

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/86a360e12b8a
https://hg.mozilla.org/mozilla-central/rev/a53f34a45f9b
https://hg.mozilla.org/mozilla-central/rev/effeee94316e
https://hg.mozilla.org/mozilla-central/rev/8e04412824b5
https://hg.mozilla.org/mozilla-central/rev/e6ee25ad6a9f

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1409395 and breaking today's nightlies.

https://hg.mozilla.org/mozilla-central/rev/f27105b62753c71ecadad2f8d632ec7e5ac96bbd

Comment 28

[Mike de Boer [:mikedeboer]]

Comment on attachment 8917147 [details]
Bug 1407351 - Remove nightly-only e10s testing features from the main browser window.

https://reviewboard.mozilla.org/r/188154/#review195458

::: browser/components/customizableui/CustomizableUI.jsm:461
(Diff revision 3)
> +    // "New non-e10s window" button.
> +    if (currentVersion < 13 && gSavedState.placements) {
> +      for (let placements of Object.values(gSavedState.placements)) {
> +        let buttonIndex = placements.indexOf("e10s-button");
> +        if (buttonIndex != -1) {
> +          placements.spice(buttonIndex, 1);

s/spice/splice/ is your culprit, Felipe... Sorry I didn't catch this as well - only human, after all.

Comment 29

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917147 [details]
Bug 1407351 - Remove nightly-only e10s testing features from the main browser window.

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

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917148 [details]
Bug 1407351 - Remove Nightly-only e10s checkbox in about:preferences.

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

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917149 [details]
Bug 1407351 - Simplify check for e10s in about:preferences.

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

Comment 32

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917150 [details]
Bug 1407351 - Remove E10S_TESTING_ONLY from devtools.

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

Comment 33

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917151 [details]
Bug 1407351 - Remove E10S_TESTING_ONLY defines.

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

Comment 34

[:Felipe Gomes (needinfo for replies!)]

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

https://tools.taskcluster.net/index/artifacts/gecko.v2.try.revision.dc78c6804b62663393a1fa79a226f961da5d5cdf

Comment 35

[:Felipe Gomes (needinfo for replies!)]

Alright, Ryan confirmed using these try builds that the new patch works, so I'm gonna reland this.

Sorry about the trouble that this may have caused to anyone!

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e4f906fcfb4
Remove nightly-only e10s testing features from the main browser window. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/54dd64f2be2e
Remove Nightly-only e10s checkbox in about:preferences. r=jaws
https://hg.mozilla.org/integration/autoland/rev/8bbabfafd7e2
Simplify check for e10s in about:preferences. r=jaws
https://hg.mozilla.org/integration/autoland/rev/0691578815e5
Remove E10S_TESTING_ONLY from devtools. r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/a509b9a421a1
Remove E10S_TESTING_ONLY defines. r=glandium

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/9e4f906fcfb4
https://hg.mozilla.org/mozilla-central/rev/54dd64f2be2e
https://hg.mozilla.org/mozilla-central/rev/8bbabfafd7e2
https://hg.mozilla.org/mozilla-central/rev/0691578815e5
https://hg.mozilla.org/mozilla-central/rev/a509b9a421a1

Comment 38

[Ekanan Ketunuti]

It seems like some of the backed out patch in comment 27 got merged on top of patches in comment 37.

see http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/browser/base/content/tabbrowser.xml#3957-3969
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/browser/base/content/tabbrowser.xml#5547,5549-5553
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/browser/base/content/browser.xul#99-104

any thought?

Comment 39

[:Felipe Gomes (needinfo for replies!)]

(In reply to Ekanan Ketunuti from comment #38)
> It seems like some of the backed out patch in comment 27 got merged on top
> of patches in comment 37.

Thanks for the notice! I fixed these via bug 1410279