Closed
Bug 1407351
Opened 7 years ago
Closed 7 years ago
Remove E10S_TESTING_ONLY features
Categories
(Firefox :: General, enhancement)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: Felipe, Assigned: Felipe)
References
Details
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
mikedeboer
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jaws
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jaws
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
gregtatum
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8917151 [details] Bug 1407351 - Remove E10S_TESTING_ONLY defines. https://reviewboard.mozilla.org/r/188162/#review193454
Attachment #8917151 -
Flags: review?(mh+mozilla) → review+
Comment 7•7 years ago
|
||
mozreview-review |
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)'
Attachment #8917147 -
Flags: review?(mdeboer) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
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!
Attachment #8917147 -
Flags: review?(mdeboer) → review+
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8917148 [details] Bug 1407351 - Remove Nightly-only e10s checkbox in about:preferences. https://reviewboard.mozilla.org/r/188156/#review194180
Attachment #8917148 -
Flags: review?(jaws) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8917149 [details] Bug 1407351 - Simplify check for e10s in about:preferences. https://reviewboard.mozilla.org/r/188158/#review194182
Attachment #8917149 -
Flags: review?(jaws) → review+
Comment 16•7 years ago
|
||
mozreview-review |
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.
Attachment #8917150 -
Flags: review?(gtatum) → review+
Comment 17•7 years ago
|
||
Pushed by felipc@gmail.com: 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•7 years ago
|
||
Pushed by felipc@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a58f8ab311d Back out to avoid a merge conflict with bug 1406212. I'll reland tomorrow
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
![]() |
||
Comment 19•7 years ago
|
||
bugherder |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
Pushed by felipc@gmail.com: 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
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
![]() |
||
Comment 26•7 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 27•7 years ago
|
||
Backed out for causing bug 1409395 and breaking today's nightlies. https://hg.mozilla.org/mozilla-central/rev/f27105b62753c71ecadad2f8d632ec7e5ac96bbd
Status: RESOLVED → REOPENED
Flags: needinfo?(felipc)
Resolution: FIXED → ---
Target Milestone: Firefox 58 → ---
Comment 28•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc78c6804b62663393a1fa79a226f961da5d5cdf https://tools.taskcluster.net/index/artifacts/gecko.v2.try.revision.dc78c6804b62663393a1fa79a226f961da5d5cdf
Flags: needinfo?(felipc)
Assignee | ||
Comment 35•7 years ago
|
||
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•7 years ago
|
||
Pushed by felipc@gmail.com: 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•7 years ago
|
||
bugherder |
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
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
![]() |
||
Comment 38•7 years ago
|
||
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?
Flags: needinfo?(felipc)
Assignee | ||
Comment 39•7 years ago
|
||
(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
Flags: needinfo?(felipc)
You need to log in
before you can comment on or make changes to this bug.
Description
•