Bug 61726

Summary:

[CSS Exclusions] Parse wrap-shape property

Product:

WebKit

Reporter:

Alexandru Chiculita <achicu>

Component:

CSS

Assignee:

Alexandru Chiculita <achicu>

Status:

RESOLVED FIXED

Severity:

Enhancement

CC:

dglazkov, gustavo, hyatt, mihnea, shanestephens, stearns, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

62114

Bug Blocks:

57311, 62991

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	achicu: review-, gustavo: commit-queue-
Patch	none
Patch	none
Patch	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03	none
Patch	none
Patch - updated license	none
Patch - updated test folder	none
Patch - fixed variables naming	hyatt: review+, webkit.review.bot: commit-queue-
Patch	none
Patch - fixes for Comment #23	none
Patch - fixes for Comment #23	tony: review-, tony: commit-queue-

Alexandru Chiculita

Reported 2011-05-30 04:32:25 PDT

Add support to parse the wrap-shape property. http://www.interoperabilitybridges.com/css3-floats/ http://dev.w3.org/csswg/css3-exclusions/

Attachments
Patch (50.43 KB, patch) 2011-05-31 14:25 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch (50.44 KB, patch) 2011-05-31 21:14 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch (29.36 KB, patch) 2011-06-07 04:27 PDT, Alexandru Chiculita	achicu: review- gustavo: commit-queue-	Details Formatted Diff Diff
Patch (50.29 KB, patch) 2011-06-07 04:41 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch (57.02 KB, patch) 2011-06-21 07:52 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch (57.02 KB, patch) 2011-06-22 00:57 PDT, Alexandru Chiculita	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.36 MB, application/zip) 2011-06-22 01:38 PDT, WebKit Review Bot	no flags	Details
Patch (57.47 KB, patch) 2011-06-22 02:15 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch - updated license (58.07 KB, patch) 2011-06-27 07:25 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch - updated test folder (55.03 KB, patch) 2011-07-01 02:23 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch - fixed variables naming (55.83 KB, patch) 2011-07-01 15:46 PDT, Alexandru Chiculita	hyatt: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Patch (55.80 KB, patch) 2011-07-12 14:50 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch - fixes for Comment #23 (8.82 KB, patch) 2011-07-13 01:05 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch - fixes for Comment #23 (8.83 KB, patch) 2011-07-13 01:14 PDT, Alexandru Chiculita	tony: review- tony: commit-queue-	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Alexandru Chiculita

Comment 1 2011-05-31 14:25:10 PDT

Created attachment 95482 [details] Patch

Alexandru Chiculita

Comment 2 2011-05-31 14:28:50 PDT

Didn't touch the -webkit-wrap-shape: path() because that will make the patch too big. For path() I can modify the tokenizer to catch the path(....) similar to how uri() works and let the existing SVG path data parser produce the shape. I will keep that in a different patch.

WebKit Review Bot

Comment 3 2011-05-31 14:44:04 PDT

Comment on attachment 95482 [details] Patch Attachment 95482 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8755393

Alexandru Chiculita

Comment 4 2011-05-31 21:14:54 PDT

Created attachment 95543 [details] Patch

Alexandru Chiculita

Comment 5 2011-06-06 05:49:40 PDT

The patch is obsolete because it used ENABLE(EXCLUSIONS) but the flag was renamed to ENABLE(CSS_EXCLUSIONS). I've also added https://bugs.webkit.org/show_bug.cgi?id=62114 because I need to add new optional properties in CSSPropertyNames.in that need to be defined only when ENABLE_CSS_EXCLUSIONS flag is used. I might need to update the patch depending on the resolution on that bug. I will update the patch after bug #62114 is closed.

Alexandru Chiculita

Comment 6 2011-06-07 04:27:57 PDT

Created attachment 96228 [details] Patch Updated to ENABLE(CSS_EXCLUSIONS)

Gustavo Noronha (kov)

Comment 7 2011-06-07 04:35:23 PDT

Comment on attachment 96228 [details] Patch Attachment 96228 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8788161

Alexandru Chiculita

Comment 8 2011-06-07 04:41:23 PDT

Created attachment 96229 [details] Patch

Simon Fraser (smfr)

Comment 9 2011-06-17 07:50:47 PDT

Comment on attachment 96229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96229&action=review > LayoutTests/fast/exclusions/wrap_shape_parsing.html:9 > +<!DOCTYPE html> > +<html> > + <head> > + <title>CSS Exclusion Test: Parse wrap-shape</title> > + </head> > +<body> > + > +<p>Testing the parsing of the -webkit-wrap-shape property. There's should be only PASS lines below.</p> > +<pre id="console"></pre> This would be better as a pure JS test. See LayoutTests/fast/css/parsing* for examples. Note that there's a script to generate the HTML file from the script-tests/foo.js file: ./Tools/Scripts/make-script-test-wrappers > Source/WebCore/css/Shape.h:1 > +/* Shape feel slightly too generic for the name of this file. CSShape? ExclusionShape? Also it has more than one class, so the name would be better as a plural. > Source/WebCore/rendering/style/RenderStyle.cpp:574 > + // FIXME: use the correct diff result > + if (rareNonInheritedData->m_wrapShape != other->rareNonInheritedData->m_wrapShape) > + return StyleDifferenceRepaint; It's going to be easy to loose track of this. You should either fix it now, or file a bug and reference the bug in the comment. Shouldn't a shape change be a layout change? > Source/WebCore/rendering/style/RenderStyle.h:1155 > + void setWrapShape(PassRefPtr<Shape> shape) > + { > + if (rareNonInheritedData->m_wrapShape != shape) > + rareNonInheritedData.access()->m_wrapShape = shape; > + } > + Shape* wrapShape() const { return rareNonInheritedData->m_wrapShape.get(); } > + static Shape* initialWrapShape() { return 0; } We don't usually have RenderStyle things reference CSSValue things directly. For one thing, this will make transitions/animations difficult to implement. So you should probably have a CSSShapeValue distinct from your RenderStyle shape representation.

Alexandru Chiculita

Comment 10 2011-06-17 08:31:28 PDT

Thank you for the review! > This would be better as a pure JS test. See LayoutTests/fast/css/parsing* for examples. Note that there's a script to generate the HTML file from the script-tests/foo.js file: ./Tools/Scripts/make-script-test-wrappers I will use that instead and add an updated patch. > Shape feel slightly too generic for the name of this file. CSShape? ExclusionShape? ExclusionShapes: The shapes are not used only for exclusions. It will also be used to define the interior of a box. CSSShapes: It contains 3 "S" letters inside, little hard to type/read. I think I will use "CSSWrapShapes" because the currently the wrap-shape is the only property using it. I will also update the name of the classes. > It's going to be easy to loose track of this. You should either fix it now, or file a bug and reference the bug in the comment. I will add a bug for that. > Shouldn't a shape change be a layout change? The current spec is being reworked to remove dependencies between exclusions and affected content. There's a proposal to use floats instead. In that case, wrap-shape should actually relayout the parent container. For sure, I will have to revisit this code, but for now I've added that in order to avoid having diff(x, y) == None where wrap-shapes actually differ. > We don't usually have RenderStyle things reference CSSValue things directly. For one thing, this will make transitions/animations difficult to implement. > So you should probably have a CSSShapeValue distinct from your RenderStyle shape representation. I followed the implementation for Rect and Counter in CSSPrimitiveValue, so Shape is not inheriting from CSSValue. It is wrapped in the CSSPrimitiveValue class. Isn't that similar to having CSSShapeValue?

Alexandru Chiculita

Comment 11 2011-06-21 07:52:45 PDT

Created attachment 97986 [details] Patch - Created pure JS test - Renamed Shape.h/cpp to CSSWrapShapes.h/cpp - Renamed class Shape to CSSWrapShape. Also its friends are renamed to CSSWrapShapeRect, CSSWrapShapeCircle, CSSWrapShapeEllipse and CSSWrapShapePolygon - Uses the pre-processing of the CSSPropertyNames.in and CSSValueKeywords.in. That is added in https://bugs.webkit.org/show_bug.cgi?id=62114 which was reviewed, but is not yet in trunk, so this is why the build-bot should have failed. Because the CSSWrapShapePolygon needs evenodd and nonzero, I moved that enum from Path.h to its own WindRule.h file. Evenodd and nonzero are keywords used by both SVG and Wrap Shapes. Because of that I had to add that #if in SVGCSSValueKeywords.in

Alexandru Chiculita

Comment 12 2011-06-22 00:57:12 PDT

Created attachment 98130 [details] Patch Reposting to trigger the bots again. The dependency bug was fixed and the patch should apply correctly now.

WebKit Review Bot

Comment 13 2011-06-22 01:38:04 PDT

Comment on attachment 98130 [details] Patch Attachment 98130 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8919322 New failing tests: fast/css/exclusions/parsing-wrap-shape.html

WebKit Review Bot

Comment 14 2011-06-22 01:38:09 PDT

Created attachment 98133 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Alexandru Chiculita

Comment 15 2011-06-22 02:15:57 PDT

Created attachment 98137 [details] Patch Added skipped for chromium. The test should be skipped on all the platforms until the flag is enabled by default.

Alexandru Chiculita

Comment 16 2011-06-27 07:25:32 PDT

Created attachment 98721 [details] Patch - updated license Changed the license in CSSWrapShape.h/cpp to BSD.

Alexandru Chiculita

Comment 17 2011-07-01 02:23:57 PDT

Created attachment 99447 [details] Patch - updated test folder Bug https://bugs.webkit.org/show_bug.cgi?id=63751 moved the tests to fast/exclusions/ and also added the folder to the skipped lists on all the platforms. Updated the patch to add the test in the same folder. I'm not a commiter, so if you review+ please add the commit-queue flag.

Alexandru Chiculita

Comment 18 2011-07-01 15:46:55 PDT

Created attachment 99531 [details] Patch - fixed variables naming Renamed "a" to "argument" and "i" to "argumentNumber". Also changed if's to switches. It removes the need to write if (!argumentNumber) instead of if (argumentNumber == 0). Also there's is no way to get more arguments then we account for in the switch. That's because it checks for the size of the arguments list. Also added an assert to check that.

Alexandru Chiculita

Comment 19 2011-07-12 12:06:15 PDT

Can you please review the latest patch?

Dave Hyatt

Comment 20 2011-07-12 13:49:37 PDT

Comment on attachment 99531 [details] Patch - fixed variables naming r=me

WebKit Review Bot

Comment 21 2011-07-12 13:54:05 PDT

Comment on attachment 99531 [details] Patch - fixed variables naming Rejecting attachment 99531 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2 Last 500 characters of output: patching file Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp Hunk #1 FAILED at 55. Hunk #2 FAILED at 94. Hunk #3 FAILED at 140. 3 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp.rej patching file Source/WebCore/rendering/style/StyleRareNonInheritedData.h Hunk #2 succeeded at 134 with fuzz 2. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'David Hyatt', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/9019336

Alexandru Chiculita

Comment 22 2011-07-12 14:50:58 PDT

Created attachment 100567 [details] Patch Rebased the previous patch, so that commit queue can apply it.

Tony Chang

Comment 23 2011-07-12 15:05:40 PDT

Comment on attachment 100567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100567&action=review > Source/WebCore/css/CSSParser.cpp:86 > +#include "CSSWrapShapes.h" Nit: It's better to put the #if in the .h file and always include the file. This reduces the number of #ifs and makes it easier for people to move files even if a feature is disabled. > Source/WebCore/css/CSSParser.cpp:3742 > + if (!valid) > + break; Nit: Wouldn't it be simpler to just return 0 here? > Source/WebCore/css/CSSParser.cpp:3765 > + valid = false; > + break; Nit: ... and here. > Source/WebCore/css/CSSParser.cpp:3771 > + if (!valid || argumentNumber < 3) Nit: Then you wouldn't need the variable valid anymore. > Source/WebCore/css/CSSWrapShapes.h:152 > + PassRefPtr<CSSPrimitiveValue> getXAt(unsigned i) { return m_values.at(i << 1); } Nit: Please use * 2 rather than bit shifting. > Source/WebCore/css/CSSWrapShapes.h:153 > + PassRefPtr<CSSPrimitiveValue> getYAt(unsigned i) { return m_values.at((i << 1) & 1); } This looks wrong, isn't it always 0? I would just write (i * 2) + 1.

WebKit Review Bot

Comment 24 2011-07-12 15:34:17 PDT

Comment on attachment 100567 [details] Patch Clearing flags on attachment: 100567 Committed r90863: <http://trac.webkit.org/changeset/90863>

Alexandru Chiculita

Comment 25 2011-07-13 01:05:19 PDT

Created attachment 100639 [details] Patch - fixes for Comment #23

WebKit Review Bot

Comment 26 2011-07-13 01:07:51 PDT

Attachment 100639 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/style/StyleRareNonInheritedData.h:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alexandru Chiculita

Comment 27 2011-07-13 01:14:34 PDT

Created attachment 100641 [details] Patch - fixes for Comment #23 Fixed styling. It looks like CSSWrapShapes needs to be before CounterDirectives.h.

Tony Chang

Comment 28 2011-07-13 10:22:45 PDT

Comment on attachment 100641 [details] Patch - fixes for Comment #23 Thanks for the follow up! I would also add #if ENABLE(CSS_EXCLUSIONS) to CSSWrapShapes.h. Also, we normally try to do a patch per bug so can you upload the new patch to a new bug?

Alexandru Chiculita

Comment 29 2011-07-13 10:58:19 PDT

(In reply to comment #28) > (From update of attachment 100641 [details]) > I would also add #if ENABLE(CSS_EXCLUSIONS) to CSSWrapShapes.h. I've already added #if ENABLE(CSS_EXCLUSIONS) just after the #define CSSWrapShapes_h > Also, we normally try to do a patch per bug so can you upload the new patch to a new bug? Created new bug and added new patch and updated just the changelog file. Can you please review https://bugs.webkit.org/show_bug.cgi?id=64464 ?

Luke Macpherson

Comment 30 2011-07-21 21:09:43 PDT

Comment on attachment 99531 [details] Patch - fixed variables naming View in context: https://bugs.webkit.org/attachment.cgi?id=99531&action=review > Source/WebCore/css/CSSStyleSelector.cpp:5314 > + if (isInitial) { remove this whole if and use the HANDLE_INHERIT_AND_INITIAL(wrapShape, WrapShape) instead. > Source/WebCore/css/CSSStyleSelector.cpp:5318 > + if (!primitiveValue) return; here, and remove the later checks.

Alexandru Chiculita

Comment 31 2011-07-24 23:50:51 PDT

(In reply to comment #30) Added bug https://bugs.webkit.org/show_bug.cgi?id=65096 . I will add a patch for those two issues there.

Note You need to log in before you can comment on or make changes to this bug.