Bug 61726 - [CSS Exclusions] Parse wrap-shape property
: [CSS Exclusions] Parse wrap-shape property
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Enhancement
Assigned To: Alexandru Chiculita
:
Depends on: 62114
Blocks: 57311 62991
  Show dependency treegraph
 
Reported: 2011-05-30 04:32 PDT by Alexandru Chiculita
Modified: 2011-07-24 23:50 PDT (History)
8 users (show)

See Also:


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-
gns: 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

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 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/
Comment 1 Alexandru Chiculita 2011-05-31 14:25:10 PDT
Created attachment 95482 [details]
Patch
Comment 2 Alexandru Chiculita 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.
Comment 3 WebKit Review Bot 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
Comment 4 Alexandru Chiculita 2011-05-31 21:14:54 PDT
Created attachment 95543 [details]
Patch
Comment 5 Alexandru Chiculita 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.
Comment 6 Alexandru Chiculita 2011-06-07 04:27:57 PDT
Created attachment 96228 [details]
Patch

Updated to ENABLE(CSS_EXCLUSIONS)
Comment 7 Gustavo Noronha (kov) 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
Comment 8 Alexandru Chiculita 2011-06-07 04:41:23 PDT
Created attachment 96229 [details]
Patch
Comment 9 Simon Fraser (smfr) 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.
Comment 10 Alexandru Chiculita 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?
Comment 11 Alexandru Chiculita 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
Comment 12 Alexandru Chiculita 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.
Comment 13 WebKit Review Bot 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
Comment 14 WebKit Review Bot 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
Comment 15 Alexandru Chiculita 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.
Comment 16 Alexandru Chiculita 2011-06-27 07:25:32 PDT
Created attachment 98721 [details]
Patch - updated license

Changed the license in CSSWrapShape.h/cpp to BSD.
Comment 17 Alexandru Chiculita 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.
Comment 18 Alexandru Chiculita 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.
Comment 19 Alexandru Chiculita 2011-07-12 12:06:15 PDT
Can you please review the latest patch?
Comment 20 Dave Hyatt 2011-07-12 13:49:37 PDT
Comment on attachment 99531 [details]
Patch - fixed variables naming

r=me
Comment 21 WebKit Review Bot 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
Comment 22 Alexandru Chiculita 2011-07-12 14:50:58 PDT
Created attachment 100567 [details]
Patch

Rebased the previous patch, so that commit queue can apply it.
Comment 23 Tony Chang 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.
Comment 24 WebKit Review Bot 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>
Comment 25 Alexandru Chiculita 2011-07-13 01:05:19 PDT
Created attachment 100639 [details]
Patch - fixes for Comment #23
Comment 26 WebKit Review Bot 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.
Comment 27 Alexandru Chiculita 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.
Comment 28 Tony Chang 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?
Comment 29 Alexandru Chiculita 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 ?
Comment 30 Luke Macpherson 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.
Comment 31 Alexandru Chiculita 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.