Closed Bug 1277092 Opened 8 years ago Closed 8 years ago

Use EARLY_BETA_OR_EARLIER to auto-disable webkit prefix support and "background-clip:text" support after 48 "early beta" period

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed
firefox49 --- wontfix

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 1 obsolete file)

Quoting myself from bug 1259345 comment #16:
> in the interests of getting broader early testing for
> this feature (webkit prefix support/emulation), we'd like to do a short
> trial (maybe 2 weeks) on beta, and then disable it well before it hits
> release.
> 
> (We don't want it to quite let it ride all the way to release in [...] 48
> because there are some known bugs at this point, which are small &
> not-known-to-be-hit-by-any-real-world-content but worth fixing before
> release, and we anticipate that more bugs will be filed once this gets to a
> broader audience.

I'm filing this bug to cover landing the patch on beta48 to turn off these prefs during the beta period, as described above.  I expect this should happen around June 20th -- that'll be 2 weeks into the beta48 period, at least.
(In reply to Daniel Holbert [:dholbert] from comment #0)
> Quoting myself from bug 1259345 comment #16:
> > in the interests of getting broader early testing for
> > this feature (webkit prefix support/emulation), we'd like to do a short
> > trial (maybe 2 weeks) on beta, and then disable it well before it hits
> > release.

#ifdef EARLY_BETA_OR_EARLIER will do this automatically.
Ah, thanks! That's absolutely what we should be using here then.

(Surprisingly, "hg log -p" shows no record of that flag ever having been used in modules/libpref/init/all.js before. But, local testing indicates that it does work correctly there -- at least, an "ifdef" for that token succeeds in my mozilla-central build.)

I'll morph this bug to use EARLY_BETA_OR_EARLIER.
Summary: Disable webkit prefix support and "background-clip:text" support for 48 beta, after they've gotten some early-beta testing → Use EARLY_BETA_OR_EARLIER to auto-disable webkit prefix support and "background-clip:text" support after 48 "early beta" period
Attached patch fix v1Splinter Review
emk, could you review this from a correctness perspective?

This is what we should have landed in the first place, instead of these two commits on the dependent bugs:
https://hg.mozilla.org/releases/mozilla-aurora/rev/b8e3e5ba3736
https://hg.mozilla.org/releases/mozilla-aurora/rev/d907d4d71e68
Attachment #8758554 - Flags: review?(VYV03354)
Comment on attachment 8758554 [details] [diff] [review]
fix v1

LGTM
Attachment #8758554 - Flags: review?(VYV03354) → review+
Comment on attachment 8758554 [details] [diff] [review]
fix v1

sylvestre, sorry to tag you with another approval request; this one is just changing the way in which the pref is enabled, so that it auto-disables nicely.
==============

Approval Request Comment
[Feature/regressing bug #]: bug 1259345 (and associated feature bug 1264905), which we're already approved to test during "early beta" per uplifts on those bugs.

[User impact if declined]:  Extra work for me; I'd have to go in and land a patch partway through the beta period to turn off these features for later-beta/release.  (This patch lets those features automagically turn off after the "early beta" period.)

[Describe test coverage new/current, TreeHerder]: I tested locally to be sure that this still lets the pref get set to "true" in my mozilla-central build.  (I'll take it for granted that it gets set appropriately in Aurora/early-Beta/late-Beta, since we have (and have had) other features that depend on it in thos releases.)

[Risks and why]: None

[String/UUID change made/needed]: None
Attachment #8758554 - Flags: approval-mozilla-aurora?
Comment on attachment 8758554 [details] [diff] [review]
fix v1

Dan told me that this is something that was already planned to be tested in early Beta48 builds. Sylvestre fyi.
Flags: needinfo?(sledru)
Attachment #8758554 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks, Ritu! Landed patch: https://hg.mozilla.org/releases/mozilla-aurora/rev/277ad8a2a286

(Marking "wontfix" for version 49, because we don't want to take this patch there -- we just want to let these features ride the trains in 49, with no #ifdef guards / autodisabling necessary.)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: needinfo?(sledru)
Resolution: --- → FIXED
I'm noticing this feature is still on in Firefox 48 beta 5 (which normally[1][2] should be the first "late beta", i.e. the first beta with EARLY_BETA_OR_EARLIER turned off).

Ritu, do you know when we're turning off that build flag for 48 beta?  (Hopefully that's happening in beta 6?  If we think it already happened for beta 5, then something's wrong here...)

[1] https://wiki.mozilla.org/Release_Management/Beta_Release_Checklist#Set_EARLY_BETA_OR_EARLIER
[2] https://wiki.mozilla.org/Platform/Channel-specific_build_defines
Flags: needinfo?(rkothari)
(It looks like https://hg.mozilla.org/releases/mozilla-beta/file/tip/build/defines.sh [the file where this is defined] has:
> EARLY_BETA_OR_EARLIER=
(i.e. defining the value, but setting it to be empty.  So my "#ifdef" here might've really wanted to be "#if"...)

Also, that file (build/defines.sh in the mozilla-beta repo) doesn't seem to have been touched since 2014, so it's not immediately clear to me if/when we ever do actually switch from setting it to "1" to setting it to be empty.
Note that beta has a mix of "#ifdef" & "#if" checks for this flag. :-/
 https://dxr.mozilla.org/mozilla-beta/search?q=EARLY_BETA_OR_EARLIER

If the code quoted in comment 9 is really putting this symbol in the "off" state, then I suspect of the "#ifdef" checks are broken. They'll see that the symbol is defined, and will incorrectly pass.

The "#ifdef" check that I cribbed from is in HTMLInputElement.cpp, which happens to use #ifdef. (just like we do for RELEASE_BUILD and NIGHTLY_BUILD in all.js, which made it seem reasonable to crib from)
Oh, EARLY_BETA_OR_EARLIER was not as automatic as I thought at first? :(
See Also: → 1285401
Philor pointed me to this commit, where the flag was turned off a few days ago:
  https://hg.mozilla.org/releases/mozilla-beta/rev/9512f571459bbc816dda70d2a554cad8e848e67e
(not sure why hg isn't showing me that in the blame, but whatever)

That confirms that #ifdef is the wrong way to check this flag.  I believe "#if" would have been the correct thing here.

I could land a patch that would switch #ifdef to #if (not 100% sure that would work in all.js) -- but at this point, as long as we're already needing to land something, I think the simplest thing is just to do what I was originally planning in comment 0 here and just drop the #if guards altogether, and explicitly set the prefs here to false.
Flags: needinfo?(rkothari)
I'm going ahead and requesting approval for this, to accelerate the process since review will be trivial.

Approval Request Comment
[Feature/regressing bug #]: This bug here, basically.  We were only planning on enabling these features for "early" betas (to get some extra testing), but my patch for auto-disabling them was broken (used #ifdef, but probably should've used #if), as discussed above.

[User impact if declined]: webkit prefix support (and related background-clip:text) won't get auto-disabled as originally intended for the end of the beta period. This would mean the configuration we're shipping to *release* 48 users (with these features disabled) would get no beta testing, and that would be bad.

[Describe test coverage new/current, TreeHerder]: Not really applicable. This patch is about disabling some features.  We've tested that these prefs do properly control these features.

[Risks and why]: No new risk; this is just carrying out the process that we were already planning on (but which was bungled by my broken check for this build flag).

[String/UUID change made/needed]: None.
See Also: → 1285458
Comment on attachment 8769040 [details] [diff] [review]
followup patch (actually disable the features by removing broken EARLY_BETA_OR_EARLIER checks)

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

Take it in 48 beta 7 to disable webkit & background-clip:tex.
Attachment #8769040 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8769040 - Flags: review?(VYV03354) → review+
Per bug 1285458 comment 1 & bug 1285458 comment 2, it's possible things here are actually fine, and I was just misled by a typo in the commit message in the EARLY_BETA_OR_EARLIER tweak linked in comment 12. (See bug 1285458 for more details.)    Also, glandium seems pretty sure that things should work as-expected here.

So, I'd like to walk back the followup and hold off on landing it for now, and I'll check beta 6 when it ships (since that's where sledru's EARLY_BETA_OR_EARLIER tweak will *actually* be included), to verify that this feature is correctly disabled there.
Comment on attachment 8769040 [details] [diff] [review]
followup patch (actually disable the features by removing broken EARLY_BETA_OR_EARLIER checks)

--> Marking the followup as obsolete, in the hopes that this prevents it from getting prematurely checked in by a tree sheriff who's using queries to find newly-approved patches.
Attachment #8769040 - Attachment is obsolete: true
(In reply to Daniel Holbert [:dholbert] from comment #16)
> So, I'd like to walk back the followup and hold off on landing it for now,
> and I'll check beta 6 when it ships (since that's where sledru's
> EARLY_BETA_OR_EARLIER tweak will *actually* be included), to verify that
> this feature is correctly disabled there.

Following up here: I've just tested Firefox 48 beta7 (the latest beta release) and I've confirmed that both of these prefs (for webkit prefix support & background-clip:text) are indeed disabled in that release (as expected).

So, EARLY_BETA_OR_EARLIER does work as gracefully as I was originally hoping, and comment 8 through comment 17 were an unnecessary diversion. Hooray!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.