ENABLE(LEGACY_CSS_VENDOR_PREFIXES) should be conditional on ENABLE(DASHBOARD_SUPPORT)
Created attachment 192287 [details] Patch
Comment on attachment 192287 [details] Patch LGTM, but someone from the Mac port should really review.
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
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.
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.
Created attachment 192611 [details] Patch
Created attachment 192632 [details] Patch
Comment on attachment 192632 [details] Patch Attachment 192632 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17157071
@Maciej: Are you interested in reviewing this patch, or should I look for another reviewer?
Comment on attachment 192632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192632&action=review > Source/WebKit/mac/ChangeLog:23 > +2013-03-11 Adam Barth <abarth@webkit.org> > + > + 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).
> 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.
(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.
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?
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. :)
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.
Created attachment 193471 [details] Patch
Windows was sad at you, btw.
Comment on attachment 193471 [details] Patch LGTM. Please make sure you've fixed Win compile before landing. (Aka, let the bot roll green.)
> 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.
Comment on attachment 193471 [details] Patch The win-ews bubble is green.
Comment on attachment 193471 [details] Patch Clearing flags on attachment: 193471 Committed r146025: <http://trac.webkit.org/changeset/146025>
All reviewed patches have been landed. Closing bug.
The web thanks you. :)