Bug 111890 – Legacy CSS vendor prefixes should only work for Dashboard

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

ENABLE(LEGACY_CSS_VENDOR_PREFIXES) should be conditional on ENABLE(DASHBOARD_SUPPORT)

[Adam Barth]

Created attachment 192287 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 192287 [details]
Patch

LGTM, but someone from the Mac port should really review.

[Build Bot]

Comment on attachment 192287 [details]
Patch

Attachment 192287 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17025669

New failing tests:
editing/selection/selection-modify-crash.html

[Mike West]

Comment on attachment 192287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192287&action=review

> Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:-128
> -#if ENABLE(LEGACY_CSS_VENDOR_PREFIXES)

Drive-by nit: It's probably worth mentioning in the ChangeLog that this patch also removes support for the legacy prefixes from V8 entirely.

[Adam Barth]

Comment on attachment 192287 [details]
Patch

Yeah, there aren't any configurations that build with ENABLE(DASHBOARD_SUPPORT) and USE(V8) at the same time.

We discussed this a bit on webkit-dev and we're going to take a slightly different approach.

[Adam Barth]

Created attachment 192611 [details]
Patch

[Adam Barth]

Created attachment 192632 [details]
Patch

[Build Bot]

Comment on attachment 192632 [details]
Patch

Attachment 192632 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17157071

[Adam Barth]

@Maciej: Are you interested in reviewing this patch, or should I look for another reviewer?

[Ryosuke Niwa]

Comment on attachment 192632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192632&action=review

> Source/WebKit/mac/ChangeLog:23
> [phone]  Adam Barth  <[e-mail]>
> +
> +        Legacy CSS vendor prefixes should only work for Dashboard
> +        https://bugs.webkit.org/show_bug.cgi?id=111890
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Enable legacy CSS vendor prefixes when we've been asked to turn on
> +        Dashboard compatibility mode.
> +
> +        * WebView/WebView.mm:
> +        (-[WebView _setDashboardBehavior:to:]):
> +

Double change log.

> LayoutTests/platform/mac/TestExpectations:539
> +# -apple- vendor prefixes are no longer supported.
> +fast/css/apple-prefix.html
> +

Why don't we simply delete the test then? Nobody is going to run this test, right?
We should probably add a testRunner or internal method to enable legacy dashboard mode though (probably as a separate patch).

[Adam Barth]

> Double change log.

http://www.youtube.com/watch?v=OQSNhk5ICTI

> > LayoutTests/platform/mac/TestExpectations:539
> > +# -apple- vendor prefixes are no longer supported.
> > +fast/css/apple-prefix.html
> > +
> 
> Why don't we simply delete the test then? Nobody is going to run this test, right?

Sure, I'm happy to do that.

[Maciej Stachowiak]

(In reply to comment #9)
> @Maciej: Are you interested in reviewing this patch, or should I look for another reviewer?

I don't feel a need to personally review it. I can give it a pass if you'd prefer I do so.

[Ryosuke Niwa]

Comment on attachment 192632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192632&action=review

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:198
> +    static void setLegacyCSSVendorPrefixesEnabled(bool isEnabled) { isLegacyCSSVendorPrefixesEnabled = isEnabled; }
> +    static bool legacyCSSVendorPrefixesEnabled() { return isLegacyCSSVendorPrefixesEnabled; }

Since we're getting rid of -apple- prefix, should we simply call this khtmlPrefixesEnabled and setKhtmlPrefixesEnabled instead?

[Eric Seidel (no email)]

Comment on attachment 192632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192632&action=review

The patch looks fantastic.  Looks like you have some good review comments from rniwa, but with those addressed, I'm ready to r+ this (or I imagine he might be ready to either).

>> Source/WebKit/mac/ChangeLog:23
>> +
> 
> Double change log.

For emPHAsis.

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:196
> +#if ENABLE(DASHBOARD_SUPPORT)

This is not the same #if as you used in the .cpp.

>> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:198
>> +    static bool legacyCSSVendorPrefixesEnabled() { return isLegacyCSSVendorPrefixesEnabled; }
> 
> Since we're getting rid of -apple- prefix, should we simply call this khtmlPrefixesEnabled and setKhtmlPrefixesEnabled instead?

It appears this patch is not removing -apple-, but that seems like a good suggestion were that to come to pass.

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:123
> +        if (RuntimeEnabledFeatures::legacyCSSVendorPrefixesEnabled() && matchesCSSPropertyNamePrefix(propertyName, "apple"))

It's unclear to me what the scope of the -apple- prefix is.  But it seems reasonable that you would only be disabling it it from the web in this patch, and leave someone on the Mac WK1 port to worry about removing -apple- entirely.

> LayoutTests/inspector/styles/vendor-prefixes-expected.txt:8
> +opacity: 1;
> +    #inspected - 1 vendor-prefixes.html:4

Why the change?  This was attempting to see a later -apple-opacity overriding a previous "opacity"?

>> LayoutTests/platform/mac/TestExpectations:539
>> +
> 
> Why don't we simply delete the test then? Nobody is going to run this test, right?
> We should probably add a testRunner or internal method to enable legacy dashboard mode though (probably as a separate patch).

Agreed.  I guess since -apple-prefix support isn't actually gone, we could leave this to the follow-up bug to delete.  But if you do that, please file the follow-up bug about removing -apple- prefixes entirely for the Mac port (and rerence that bug in this test expectations).  Thanks. :)

[Adam Barth]

Comment on attachment 192632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192632&action=review

>>> Source/WebKit/mac/ChangeLog:23
>>> +
>> 
>> Double change log.
> 
> For emPHAsis.

Fixed.

>> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:196
>> +#if ENABLE(DASHBOARD_SUPPORT)
> 
> This is not the same #if as you used in the .cpp.

Fixed.

>> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:123
>> +        if (RuntimeEnabledFeatures::legacyCSSVendorPrefixesEnabled() && matchesCSSPropertyNamePrefix(propertyName, "apple"))
> 
> It's unclear to me what the scope of the -apple- prefix is.  But it seems reasonable that you would only be disabling it it from the web in this patch, and leave someone on the Mac WK1 port to worry about removing -apple- entirely.

Yes, this patch is just one step in the plan Maciej outlined on webkit-dev.  The next step is to reduce the usage of these prefixes in Dashboard widgets, and then remove more code from WebCore.

>> LayoutTests/inspector/styles/vendor-prefixes-expected.txt:8
>> +    #inspected - 1 vendor-prefixes.html:4
> 
> Why the change?  This was attempting to see a later -apple-opacity overriding a previous "opacity"?

Yeah.

>>> LayoutTests/platform/mac/TestExpectations:539
>>> +# -apple- vendor prefixes are no longer supported.
>>> +fast/css/apple-prefix.html
>>> +
>> 
>> Why don't we simply delete the test then? Nobody is going to run this test, right?
>> We should probably add a testRunner or internal method to enable legacy dashboard mode though (probably as a separate patch).
> 
> Agreed.  I guess since -apple-prefix support isn't actually gone, we could leave this to the follow-up bug to delete.  But if you do that, please file the follow-up bug about removing -apple- prefixes entirely for the Mac port (and rerence that bug in this test expectations).  Thanks. :)

I think it's reasonable to remove fast/css/apple-prefix.html give that after this patch no port is going to be running that test.

[Adam Barth]

Created attachment 193471 [details]
Patch

[Eric Seidel (no email)]

Windows was sad at you, btw.

[Eric Seidel (no email)]

Comment on attachment 193471 [details]
Patch

LGTM.  Please make sure you've fixed Win compile before landing.  (Aka, let the bot roll green.)

[Adam Barth]

> LGTM.  Please make sure you've fixed Win compile before landing.  (Aka, let the bot roll green.)

Sure thing.  I'm pretty sure that was caused by the ifdef mismatch you noted in the review.  Hopefully the bubble will turn green with this patch.

[Adam Barth]

Comment on attachment 193471 [details]
Patch

The win-ews bubble is green.

[WebKit Review Bot]

Comment on attachment 193471 [details]
Patch

Clearing flags on attachment: 193471

Committed r146025: <http://trac.webkit.org/changeset/146025>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

The web thanks you. :)