Bug 73272

Created attachment 116867 [details] Proposed change 1: implement the new syntax.

Attachment 116867 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/CSSStyleSelector.cpp:2522: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 116867 [details] Proposed change 1: implement the new syntax. View in context: https://bugs.webkit.org/attachment.cgi?id=116867&action=review > Source/WebCore/css/CSSStyleSelector.cpp:2520 > + if (type == CSSPrimitiveValue::CSS_PERCENTAGE) > + length = Length(primitiveValue->getDoubleValue(), Percent); > + return true; > + } Missing { here. I think that confused the style checker. > Source/WebCore/css/CSSStyleSelector.cpp:2544 > + // Handle the special property 'none'. Nit: You might want the comment to say how you 'none' is different.

Comment on attachment 116867 [details] Proposed change 1: implement the new syntax. View in context: https://bugs.webkit.org/attachment.cgi?id=116867&action=review >> Source/WebCore/css/CSSStyleSelector.cpp:2520 >> + } > > Missing { here. I think that confused the style checker. Good catch, I missed it and thought style checker was confused. >> Source/WebCore/css/CSSStyleSelector.cpp:2544 >> + // Handle the special property 'none'. > > Nit: You might want the comment to say how you 'none' is different. I am not sure I follow your nit here. Do you mind clarifying?

Comment on attachment 116867 [details] Proposed change 1: implement the new syntax. View in context: https://bugs.webkit.org/attachment.cgi?id=116867&action=review >>> Source/WebCore/css/CSSStyleSelector.cpp:2544 >>> + // Handle the special property 'none'. >> >> Nit: You might want the comment to say how you 'none' is different. > > I am not sure I follow your nit here. Do you mind clarifying? Something like, "We use a single undefined length to specify a track-list of 'none'." Or maybe just remove the comment.

Comment on attachment 116867 [details] Proposed change 1: implement the new syntax. View in context: https://bugs.webkit.org/attachment.cgi?id=116867&action=review >>>> Source/WebCore/css/CSSStyleSelector.cpp:2544 >>>> + // Handle the special property 'none'. >>> >>> Nit: You might want the comment to say how you 'none' is different. >> >> I am not sure I follow your nit here. Do you mind clarifying? > > Something like, "We use a single undefined length to specify a track-list of 'none'." Or maybe just remove the comment. Fine enough, I will remove it in the patch to land. I have seen a couple of missing of return in the implementation. I don't understand why it was not caught by the cases but I need to investigate before landing this patch as it may mean that our current test coverage is not good enough.

> I have seen a couple of missing of return in the implementation. I don't understand why it was not caught by the cases but I need to investigate before landing this patch as it may mean that our current test coverage is not good enough. I found out the issue: some code was unneeded (basically I was trying to handle several values on a CSSPrimitiveValue which was never reached). I have extended the testing a bit and removed this dead code in the to-be-landed-soon patch.

Created attachment 117982 [details] Patch for landing

Comment on attachment 117982 [details] Patch for landing Something is fishy on this patch.

(In reply to comment #9) > (From update of attachment 117982 [details]) > Something is fishy on this patch. Actually it was fine, I just needed to double-check the logic. My comment #7 was half true: some code was unneeded and it is not because it would try to parse several values for a CSSPrimitiveValue but because the parser consider most of our input as a CSSValueList (even for 1 item).

Created attachment 117987 [details] Patch for landing for tomorrow.

Comment on attachment 117987 [details] Patch for landing for tomorrow. Rejecting attachment 117987 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: 112974 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 109. Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' No such file or directory at /mnt/git/webkit-commit-queue/Tools/Scripts/webkitdirs.pm line 2078. Full output: http://queues.webkit.org/results/10736846

Committed r102183: <http://trac.webkit.org/changeset/102183>