Closed
Bug 1085622
Opened 10 years ago
Closed 10 years ago
"Enable E10S (multi-process)" preference checkbox should not be displayed in Aurora's about:preferences (E10S_TESTING_ONLY somehow defined for Aurora builds)
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
RESOLVED
FIXED
Firefox 36
People
(Reporter: cpeterson, Assigned: mconley)
References
Details
Attachments
(3 files, 1 obsolete file)
8.81 KB,
patch
|
ally
:
review+
glandium
:
review+
|
Details | Diff | Splinter Review |
8.74 KB,
patch
|
Gavin
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
6.72 KB,
application/zip
|
Details |
STR:
1. In Aurora 35, open about:preferences
RESULT:
Witness the "Enable E10S (multi-process)" checkbox which should only be shown on the Nightly channel.
Possibly related issues:
* The configure.in check for NIGHTLY_BUILD looks backwards because it is sets E10S_TESTING_ONLY=1 if NIGHTLY_BUILD is not defined:
https://hg.mozilla.org/mozilla-central/rev/7d04d18f5eeb#l8.16
* browser/base/moz.build may be defining inadvertently "defining" DEFINES['E10S_TESTING_ONLY'] with a zero value from CONFIG['E10S_TESTING_ONLY']. #ifdef will be true, even if the macro is defined even if the macro *value* is zero.
https://hg.mozilla.org/mozilla-central/rev/7d04d18f5eeb#l5.12
Comment 1•10 years ago
|
||
From just-built objdirs from Aurora/Nightly:
[gavin@gavin-mbp:~/mozilla/aurora]$ ag NIGHTLY obj-ff-opt/config.status
354: (''' NIGHTLY_BUILD ''', r''' '''),
(from "defines")
[gavin@gavin-mbp:~/mozilla/mozilla-central]$ ag NIGHTLY obj-ff-opt/config.status
74: (''' NIGHTLY_BUILD ''', ' 1 '),
358: (''' NIGHTLY_BUILD ''', r''' 1 '''),
from ("defines" and "substs", respectively)
Reporter | ||
Comment 2•10 years ago
|
||
This is not a high priority issue because about:preferences because non-Nightly channels still default to the Preferences dialog (which does not have this problem).
Comment 3•10 years ago
|
||
Isn't E10S_TESTING_ONLY used in other places in similar ways? Those might be more problematic.
Updated•10 years ago
|
Assignee: nobody → mconley
Assignee | ||
Comment 4•10 years ago
|
||
I'm seeing the "Open e10s window" toolbar buttons in Aurora, which seems to suggest that E10S_TESTING_ONLY is busted.
Assignee | ||
Comment 5•10 years ago
|
||
I'm not, thankfully, seeing it in Beta.
Comment 6•10 years ago
|
||
The bigger mystery then is, since it shows up on Aurora, why doesn't it show up in Beta/Release?
Updated•10 years ago
|
Summary: "Enable E10S (multi-process)" preference checkbox should not be displayed in Aurora's about:preferences → "Enable E10S (multi-process)" preference checkbox should not be displayed in Aurora's about:preferences (E10S_TESTING_ONLY somehow defined for Aurora builds)
Assignee | ||
Comment 7•10 years ago
|
||
We were defining the E10S_TESTING_ONLY build-time define with the value of the E10S_TESTING_ONLY config variable,
regardless of the value of E10S_TESTING_ONLY. Even if E10S_TESTING_ONLY in configure was blank, we'd still define
E10S_TESTING_ONLY, which resulted in E10S_TESTING_ONLY ifdef'd code to be included when we didn't want it to.
This also fixes a busted E10S_TESTING_ONLY ifdef that didn't have an endif.
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8516216 [details] [diff] [review]
Fix E10S_TESTING_ONLY define so that it only ever applies when E10S_TESTING_ONLY config is true. r=?
So the E10S_TESTING_ONLY build-time config and define are both pretty busted.
Here's what went wrong:
We were using test -z on $NIGHTLY_BUILD to set the E10S_TESTING_ONLY to 1. With test -z, this roughly translates to:
"If NIGHTLY_BUILD is not set, then set E10S_TESTING_ONLY=1."
That's wrong - we wanted the relationship to be that if NIGHTLY_BUILD *is* set, then E10S_TESTING_ONLY=1.
Secondly, it looks like regardless of what value E10S_TESTING_ONLY had, we were always defining it. In our moz.build files, we had code like this:
DEFINES['E10S_TESTING_ONLY'] = CONFIG['E10S_TESTING_ONLY']
Even if E10S_TESTING_ONLY was set to ' ', which seems to be the case if NIGHTLY_BUILD is not set, then we'd still define E10S_TESTING_ONLY.
Meaning that E10S_TESTING_ONLY was *always* defined.
I've switched it so that the E10S_TESTING_ONLY define is only ever set in a moz.build if CONFIG['E10S_TESTING_ONLY'] is truthy - otherwise, it remains undefined.
I also fixed a case in tabbrowser.xml where we had no #endif for an #ifdef E10S_TESTING_ONLY, which resulted in a busted browser when I first fixed this.
Kinda bad news all around. Luckily, the tabbrowser.xml stuff hasn't reached beta yet - it's only on Aurora and Central.
It's pretty mysterious why the e10s stuff isn't visible in beta or release. I honestly have no idea. Perhaps the build rules are drastically different - I'm really not sure.
Anyhow, this patch applied on central still shows the e10s testing UI. Applied to Aurora (after manually applying) gets rid of the e10s testing UI.
r?ing glandium for the configure.in change, ally for everything else.
Attachment #8516216 -
Flags: review?(mh+mozilla)
Attachment #8516216 -
Flags: review?(ally)
Comment 9•10 years ago
|
||
We should probably AC_DEFINE this variable so that we don't need to explicitly add it to DEFINES in a whole bunch of separate moz.build files.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #9)
> We should probably AC_DEFINE this variable so that we don't need to
> explicitly add it to DEFINES in a whole bunch of separate moz.build files.
Oh, yeah - thanks, I wanted to ask about that.
I agree with gavin that this probably makes the most sense - and I'm curious about why we didn't do it that way in the first place.
Incidentally, bug 1004533 (which introduced E10S_TESTING_ONLY) actually discussed this DEFINE[foo] = CONFIG[foo], with nalexander suggesting the conditional: https://bugzilla.mozilla.org/show_bug.cgi?id=1004533#c6
glandium - can you think of any objections to just using AC_DEFINE in configure.in? It'd be temporary - likely until e10s ships.
Flags: needinfo?(mh+mozilla)
Comment 11•10 years ago
|
||
Comment on attachment 8516216 [details] [diff] [review]
Fix E10S_TESTING_ONLY define so that it only ever applies when E10S_TESTING_ONLY config is true. r=?
Review of attachment 8516216 [details] [diff] [review]:
-----------------------------------------------------------------
::: configure.in
@@ +3546,5 @@
> dnl ========================================================
> dnl Multiprocess Firefox Nightly Testing UI
> dnl To be removed in Bug 1003313
> dnl ========================================================
> +if test -n "$NIGHTLY_BUILD"; then
Seems to me at least this should be backported.
Attachment #8516216 -
Flags: review?(mh+mozilla) → review+
Comment 12•10 years ago
|
||
Comment on attachment 8516216 [details] [diff] [review]
Fix E10S_TESTING_ONLY define so that it only ever applies when E10S_TESTING_ONLY config is true. r=?
Review of attachment 8516216 [details] [diff] [review]:
-----------------------------------------------------------------
Sounds like AC_DEFINES is the way to go. So let's see that patch. :)
As for why it wasn't used in the original place, because no one from #build bothered to suggest it to the author.
Attachment #8516216 -
Flags: review?(ally)
Assignee | ||
Comment 13•10 years ago
|
||
Alright, let's go the AC_DEFINE route.
Once this gets r+'d, the backporting to mozilla-aurora will be straight-forward.
Attachment #8516216 -
Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)
Attachment #8516759 -
Flags: review?(mh+mozilla)
Attachment #8516759 -
Flags: review?(ally)
Comment 14•10 years ago
|
||
Comment on attachment 8516759 [details] [diff] [review]
Patch v2
Review of attachment 8516759 [details] [diff] [review]:
-----------------------------------------------------------------
r+ w/nit
::: browser/base/moz.build
@@ +23,5 @@
> 'content/test/social/browser.ini',
> ]
>
> DEFINES['MOZ_APP_VERSION'] = CONFIG['MOZ_APP_VERSION']
> +
nit: whitespace?
Attachment #8516759 -
Flags: review?(ally) → review+
Reporter | ||
Comment 15•10 years ago
|
||
This bug also affects Firefox Developer Edition, so we should try to land a fix this week. :)
Updated•10 years ago
|
Attachment #8516759 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 16•10 years ago
|
||
Thanks!
remote: https://hg.mozilla.org/integration/fx-team/rev/032d2db91f36
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 17•10 years ago
|
||
We were defining the E10S_TESTING_ONLY build-time define with the value of the E10S_TESTING_ONLY config variable,
regardless of the value of E10S_TESTING_ONLY. Even if E10S_TESTING_ONLY in configure was blank, we'd still define
E10S_TESTING_ONLY, which resulted in E10S_TESTING_ONLY ifdef'd code to be included when we didn't want it to.
We now only ever define E10S_TESTING_ONLY iff NIGHTLY_BUILD is defined, and we do it globally.
This also fixes a busted E10S_TESTING_ONLY ifdef that didn't have an endif.
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8517187 [details] [diff] [review]
mozilla-aurora backport
I'll wait for the other patch to merge to central before requesting approval.
Attachment #8517187 -
Attachment description: Fix E10S_TESTING_ONLY define so that it only ever applies when E10S_TESTING_ONLY config is true. a=? → mozilla-aurora backport
Flags: needinfo?(mconley)
Comment 19•10 years ago
|
||
Comment on attachment 8516759 [details] [diff] [review]
Patch v2
>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>+#ifdef E10S_TESTING_ONLY
> gRemoteTabsUI.init();
>+#endif
There are two of these calls in browser.js - you should probably ifdef both? (I assume this is not a crucial change.)
Assignee | ||
Comment 20•10 years ago
|
||
Yikes - sorry I missed that. I'll ifdef the other one in a follow-up.
Flags: needinfo?(mconley)
Assignee | ||
Updated•10 years ago
|
Attachment #8517187 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Landed follow-up:
remote: https://hg.mozilla.org/integration/fx-team/rev/c41cd725fd1e
I'll fold that into my Aurora backport patch and post a new one.
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8517187 [details] [diff] [review]
mozilla-aurora backport
Huh, there's only a single call to gRemoteTabsUI.init in mozilla-aurora.
Again, needinfo'ing myself to request approval once the other patch merges to central.
Attachment #8517187 -
Attachment is obsolete: false
Flags: needinfo?(mconley)
Updated•10 years ago
|
Attachment #8517187 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 23•10 years ago
|
||
Alright, eff it, why wait - I've landed on mozilla-aurora:
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/e4a43029e627
Flags: needinfo?(mconley)
Comment 24•10 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> Alright, eff it, why wait - I've landed on mozilla-aurora:
Because standard practice says to?
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> > Alright, eff it, why wait - I've landed on mozilla-aurora:
>
> Because standard practice says to?
Hrm - I figured gavin was signaling me to jump the gun because of that approval. Sorry if that was a faulty assumption.
I'll keep my eye on Aurora, and if you feel strongly about it, I can back out if we want to wait for baking on central.
Comment 26•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> > Alright, eff it, why wait - I've landed on mozilla-aurora:
>
> Because standard practice says to?
"Standard practice" is "use your judgement", and that's what we did here. No harm.
Comment 27•10 years ago
|
||
https://wiki.mozilla.org/Tree_Rules#mozilla-aurora says otherwise.
Comment 28•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/032d2db91f36
https://hg.mozilla.org/mozilla-central/rev/c41cd725fd1e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Comment 29•10 years ago
|
||
It's good to have rules to clarify expected behavior in the common case, or to establish a baseline when relevant judgement is lacking, but it's also fine to break rules when there's no downside to doing so.
Assignee | ||
Comment 30•10 years ago
|
||
bugnotes |
Updated•10 years ago
|
QA Whiteboard: [good first verify]
You need to log in
before you can comment on or make changes to this bug.
Description
•