Bug 42093

Summary: Change handling of -khtml- and -apple- vendor-prefixes
Product: WebKit Reporter: Peter Beverloo <peter>
Component: CSSAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: ap, arv, commit-queue, darin, hyatt, me, mrowe, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
Removing the translation altogether
Remove -khtml support, continue supporting two -apple properties
Remove -khtml support, continue supporting two -apple properties
Idem, but with a style fix
eric: review-
Added 3 test-cases
Same code, but with two script-tests
Patch reduced to removing -khtml and limiting -apple
same patch, but with test fixes
darin: review-, darin: commit-queue-
Patch none

Description Peter Beverloo 2010-07-12 10:39:25 PDT
Created attachment 61244 [details]
Removing the translation altogether

Currently WebKit supports "-apple" and "-khtml" as an alternative for its own "-webkit" vendor prefix. I believe this behavior should be removed, or at least, phased out. A thread[1] to discuss this has been created on webkit-dev.

The added patch will remove the translation altogether, however, since the thread is still active I'm not marking it for review.

[1] https://lists.webkit.org/pipermail/webkit-dev/2010-July/013519.html
Comment 1 Peter Beverloo 2010-07-13 00:38:46 PDT
Created attachment 61337 [details]
Remove -khtml support, continue supporting two -apple properties

Patch which removes support for all -khtml-prefixed properties, and removes support for all -apple-prefixed properties except for "-apple-dashboard-region" and "-apple-line-clamp".
Comment 2 Peter Beverloo 2010-07-13 00:43:14 PDT
Created attachment 61340 [details]
Remove -khtml support, continue supporting two -apple properties

Typo fix
Comment 3 WebKit Review Bot 2010-07-13 00:45:16 PDT
Attachment 61340 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSParser.cpp:5593:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Peter Beverloo 2010-07-13 00:51:32 PDT
Created attachment 61342 [details]
Idem, but with a style fix

Style fix.
Comment 5 Eric Seidel (no email) 2010-07-14 01:57:50 PDT
Comment on attachment 61342 [details]
Idem, but with a style fix

 +          if (!strcmp(buffer, "-apple-dashboard-region") || !strcmp(buffer, "-apple-line-clamp")) {
This looks wrong.  Previously the variable "length" was updated by "hasPrefix".  That's no longer the case, so the memmove will be wrong.  Do we not have tests for these?  We should add them.
Comment 6 Peter Beverloo 2010-07-14 04:35:08 PDT
Created attachment 61501 [details]
Added 3 test-cases

The hasPrefix function does not touch the length property, therefore I believe
the code to be correct.

I have added three testcases:
 - Testing (limited) support for the -apple vendor prefix.
 - Testing whether the -khtml prefix actually got removed.
 - Testing whether the -webkit-opacity legacy-property got removed.

One test-cases got updated, "apple-prefix.html" which tests -apple-line-clamp
and relied on -khtml.
One test-case got removed, "legacy-opacity-styles.html", now tested by the
case which tests whether the property got removed.
Comment 7 Eric Seidel (no email) 2010-07-15 12:09:43 PDT
Comment on attachment 61501 [details]
Added 3 test-cases

I think the change looks fine.  The tests could be better.  I'd rather see them as scipt-tests, and see them test more conditions (you only test two properties your new tests).

It's entertaining that you're even removing a script test.  Notice how because it doesn't have any boiler plate its easier to more exhaustivly test what it was testing?

You don't test for example that "-webkit-line-clamp" is or is not defined.
Comment 8 Peter Beverloo 2010-07-15 13:40:34 PDT
Created attachment 61710 [details]
Same code, but with two script-tests

Added two script-tests for testing the new behavior.

limited-vendor-prefix-behavior tests for removal of -khtml and limitation of -apple.

no-legacy-webkit-opacity-expected tests whether "-webkit-opacity" is now unsupported.
Comment 9 Darin Adler 2010-07-15 14:44:32 PDT
Removing -webkit-opacity goes against what we have repeatedly told developers for years. We promised them that even when we supported the standard version of the CSS property we’d keep the prefixed version working for a while.

Generally speaking we tell people that we’ll keep the prefixed versions indefinitely, so it’s safe to use them. Once the standard version is available, it’s preferred. There’s no guarantee that we’ll ever remove the prefixed version entirely. We can do it if that opportunity arises organically.

Hyatt, do you really want to remove -webkit-opacity? Has something changed since we set this strategy in place?
Comment 10 Darin Adler 2010-07-15 14:44:55 PDT
This bug is now tracking multiple changes to vendor prefixes that may need to be discussed separately.
Comment 11 Peter Beverloo 2010-07-16 01:38:44 PDT
Created attachment 61779 [details]
Patch reduced to removing -khtml and limiting -apple

Hyatt solely responded to -khtml and -apple, not -webkit-opacity. Therefore I limited the patch to only address that change.
Comment 12 Darin Adler 2010-07-19 13:06:37 PDT
Comment on attachment 61779 [details]
Patch reduced to removing -khtml and limiting -apple

> +            // -webkit-border-*-*-radius worked in Safari 4 and earlier. -webkit-border-radius syntax
> +            // differs from border-radius, so it is remains as a distinct property.

As long as you are moving this code, we should consider fixing "it is remains" here.

> +        // If the prefix is -apple, support two properties to maintain
> +        // backward compatibility with older versions of Safari.
> +        if (!strcmp(buffer, "-apple-dashboard-region") || !strcmp(buffer, "-apple-line-clamp")) {

This comment isn’t quite right. The most important context for this is not Safari itself, but rather Dashboard. But I suppose we are OK with the comment as-is.
Comment 13 WebKit Commit Bot 2010-07-19 13:33:41 PDT
Comment on attachment 61779 [details]
Patch reduced to removing -khtml and limiting -apple

Rejecting patch 61779 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20713 test cases.
fast/block/positioning/height-change.html -> failed

Exiting early after 1 failures. 5500 tests run.
94.23s total testing time

5499 test cases (99%) succeeded
1 test case (<1%) had incorrect layout

Full output: http://queues.webkit.org/results/3598159
Comment 14 Peter Beverloo 2010-07-21 04:46:48 PDT
Created attachment 62164 [details]
same patch, but with test fixes

Fixes tests, code remains unchanged. Most notably are the flexbox tests, all of which used -khtml- properties.
Comment 15 Eric Seidel (no email) 2010-07-21 04:55:52 PDT
Comment on attachment 62164 [details]
same patch, but with test fixes

 +  <div style="position: relative; display: -apple-box;">
Does this even need "-webkit-" anymore?

 +  		-webkit-border-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAoAAAAKCAIAAAACUFjqAAAAAXNSR0IArs4c6QAAAUFJREFUGNMBNgHJ/gPcUkznH/n8lGR5YkMIIMDXFhgoVNnfqeM5dbre55MDGkF/U6tMmi12K6QBpfkB4H3NyPZAiyjm6kp//uPtAA4pZhHPJ8xbkSop4chy5ecPsVSqkC03ozAmk77SOwOHR+fDXmoF5UUJU9YsnVfehg/iLC7p3etC3w3LEA8D0eGf/DZozPPzoSSX6gTVlQ/L4Ylt4SrFiOgqNDQPAPvl/JQuLv33xK66eAgGCn+OAIUu1Fugs2hsZNIB6gPhrZ80q7sXNUEuIE7EGNC5Dc2ExcIe9tseb4r7usAAralzLgdas9URX21wGUqSgDnf/jMydhxMIEx4w9TtAfCQPsbi30EEVABXFJYnOZgfbQK3CLkwg6PDGG27KwOYF7jD+sIHhdwoUyILAF5sv/6bI6S56fbULB5OToUjFpS7UHGrVgAAAABJRU5ErkJggg==) 1 2 3;
I'm certain this no longer needs -webkit-

 +  shouldBe("document.getElementById('test-no-khtml').style.khtmlUserDrag", "undefined");
You never test that "lineClamp" doesn't get se when we use -khtml-line-clamp.  I would think it's important to test both for khtmlLineClamp and that it doesn't get mapped to lineClamp.

In general this looks good though!
Comment 16 Peter Beverloo 2010-07-21 05:46:34 PDT
Created attachment 62170 [details]

Eric: The "box" value for the display-property still needs to be prefixed. Same goes for the "border-image" property. Considering "border-image" already is a CR the prefix could be dropped, but that's not in the scope of this bug.

I have fixed the typo Darin pointed out, updated the backward-compatibility comment to say Dashboard rather than Safari and updated the test-case to include the "userDrag" and "lineClamp" properties, which both pass as expected.
Comment 17 Darin Adler 2010-07-21 08:15:18 PDT
Comment on attachment 62170 [details]

> @@ -5672,9 +5668,9 @@ int cssValueKeywordID(const CSSParserStr
>      buffer[length] = '\0';
>      if (buffer[0] == '-') {
> -        // If the prefix is -apple- or -khtml-, change it to -webkit-.
> -        // This makes the string one character longer.
> -        if (hasPrefix(buffer, length, "-apple-") || hasPrefix(buffer, length, "-khtml-")) {
> +        // If the prefix is -apple, support two properties to maintain
> +        // backward compatibility with the Apple Dashboard.
> +        if (!strcmp(buffer, "-apple-dashboard-region") || !strcmp(buffer, "-apple-line-clamp")) {

This change is incorrect. -apple-dashboard-region and -apple-line-clamp are CSS property names. But this function is for CSS value keywords. So it's pointless to rename these strings, since they are not legal value keywords. I'm not sure which CSS value keywords need to work with the "-apple" prefix.

The rest of the patch looks good.
Comment 18 Peter Beverloo 2010-07-21 08:24:53 PDT
Missed that, excuse me. Should I remove the value-translation for the -apple/-khtml prefixes in total or would another approach be preferred?
Comment 19 Darin Adler 2010-07-21 08:28:36 PDT
(In reply to comment #18)
> Missed that, excuse me. Should I remove the value-translation for the -apple/-khtml prefixes in total or would another approach be preferred?

Someone has to research which keyword values might still need to be handled. Once we're confident there are none, then yes, removing the code altogether is fine.

I tried and could not quickly find anything.
Comment 20 Peter Beverloo 2010-07-21 11:15:40 PDT
Created attachment 62213 [details]

I removed the changes to the css value-parsing. Updating the cssValueKeywordID method is rather easy and could be done in a follow-up patch. 

Not touching the tests, as using -webkit-box (or any other value) is preferred over -apple-box either way. I will be doing more research to the usage of these properties later on, although they seems to be rarely used, if at all.
Comment 21 Darin Adler 2010-07-21 11:23:22 PDT
Comment on attachment 62213 [details]

> +        (WebCore::cssValueKeywordID):

Change log now lists this function, but the patch does not modify it.
Comment 22 WebKit Commit Bot 2010-07-21 12:20:52 PDT
Comment on attachment 62213 [details]

Rejecting patch 62213 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
M	WebCore/css/CSSParser.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:


    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/git/libexec/git-core/git-svn line 570

Full output: http://queues.webkit.org/results/3561290
Comment 23 Peter Beverloo 2010-07-21 12:42:23 PDT
Created attachment 62220 [details]

Gah. Updated the changelog per Darin's comment and replaced the tabs which two positioning tests were based on with four spaces.
Comment 24 WebKit Commit Bot 2010-07-21 13:39:31 PDT
Comment on attachment 62220 [details]

Clearing flags on attachment: 62220

Committed r63854: <http://trac.webkit.org/changeset/63854>
Comment 25 WebKit Commit Bot 2010-07-21 13:39:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Tony Chang 2010-07-21 14:11:42 PDT
Some tests in fast/backgrounds were missed.  The pixel tests on chromium found them.  I'll make a patch to fix.
Comment 27 Mark Rowe (bdash) 2010-07-26 12:46:14 PDT
This completely broke the layout of Safari RSS pages.  See bug 42990 for details.
Comment 28 Peter Beverloo 2010-07-26 12:52:33 PDT
I got cc'ed on these bugs. If you could make a small list of css properties used by the RSS reader I could make a patch to re-introduce these properties.
Comment 29 Mark Rowe (bdash) 2010-07-26 13:36:57 PDT
Sadly it’s not just Safari RSS that uses these properties.  I did a quick grep over the various loose .css files I could see on my system.  Many of them referenced -khtml properties.  Dashboard, system Dashboard widgets, Dashcode templates, QuickLook, and even third-party applications like Yahoo! Messenger all appear to make use of -khtml or -apple properties.

In order to restore the functionality of the affected applications I am going to roll out the WebCore portion of <http://trac.webkit.org/changeset/63854>.  I would suggest that we revisit this change in the following way:
1) Set things up such that all existing properties that support the -khtml or -apple prefix continue to work, but so that newly-added properties do not support the legacy prefixes.
2) Establish a set of tests that make it explicit which of the properties we intend to support with the legacy prefixes.
3) Work to incrementally reduce the set of properties that are supported with the legacy prefix after we determine that individual properties are not used in the wild with the legacy prefixes.
Comment 30 Mark Rowe (bdash) 2010-07-26 13:54:16 PDT
I did that roll-out in <http://trac.webkit.org/changeset/64071>.  Reopening.
Comment 31 Eric Seidel (no email) 2010-07-27 04:59:37 PDT
Comment on attachment 61779 [details]
Patch reduced to removing -khtml and limiting -apple

Cleared Darin Adler's review+ from obsolete attachment 61779 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 32 Eric Seidel (no email) 2010-07-27 04:59:46 PDT
Comment on attachment 62213 [details]

Cleared Darin Adler's review+ from obsolete attachment 62213 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 33 Peter Beverloo 2012-04-25 04:25:01 PDT
abarth took over and landed http://trac.webkit.org/changeset/113795.
Comment 34 Alexey Proskuryakov 2012-04-25 09:53:19 PDT

*** This bug has been marked as a duplicate of bug 83256 ***