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
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".
Created attachment 61340 [details] Remove -khtml support, continue supporting two -apple properties Typo fix
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.
Created attachment 61342 [details] Idem, but with a style fix Style fix.
Comment on attachment 61342 [details] Idem, but with a style fix WebCore/css/CSSParser.cpp:5583 + 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.
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 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.
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.
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?
This bug is now tracking multiple changes to vendor prefixes that may need to be discussed separately.
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 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 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
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 on attachment 62164 [details] same patch, but with test fixes LayoutTests/fast/block/positioning/height-change.html: + <div style="position: relative; display: -apple-box;"> Does this even need "-webkit-" anymore? LayoutTests/fast/borders/border-image-omit-right-slice.html:17 + -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- LayoutTests/fast/css/limited-vendor-prefix-behavior.html:22 + 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!
Created attachment 62170 [details] Patch 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 on attachment 62170 [details] Patch > @@ -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.
Missed that, excuse me. Should I remove the value-translation for the -apple/-khtml prefixes in total or would another approach be preferred?
(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.
Created attachment 62213 [details] Patch 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 on attachment 62213 [details] Patch > + (WebCore::cssValueKeywordID): Change log now lists this function, but the patch does not modify it.
Comment on attachment 62213 [details] Patch 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: trunk/LayoutTests/fast/block/positioning/resources/window-height-change-frame-flex.html 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
Created attachment 62220 [details] Patch Gah. Updated the changelog per Darin's comment and replaced the tabs which two positioning tests were based on with four spaces.
Comment on attachment 62220 [details] Patch Clearing flags on attachment: 62220 Committed r63854: <http://trac.webkit.org/changeset/63854>
All reviewed patches have been landed. Closing bug.
Some tests in fast/backgrounds were missed. The pixel tests on chromium found them. I'll make a patch to fix.
This completely broke the layout of Safari RSS pages. See bug 42990 for details.
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.
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.
I did that roll-out in <http://trac.webkit.org/changeset/64071>. Reopening.
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 on attachment 62213 [details] Patch Cleared Darin Adler's review+ from obsolete attachment 62213 [details] so that this bug does not appear in http://webkit.org/pending-commit.
abarth took over and landed http://trac.webkit.org/changeset/113795.
*** This bug has been marked as a duplicate of bug 83256 ***