Bug 61726 - [CSS Exclusions] Parse wrap-shape property
: [CSS Exclusions] Parse wrap-shape property
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Enhancement
Assigned To:
:
:
: 62114
: 57311 62991
  Show dependency treegraph
 
Reported: 2011-05-30 04:32 PST by
Modified: 2011-07-24 23:50 PST (History)


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


Note

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


------- Comment #1 From 2011-05-31 14:25:10 PST -------
Created an attachment (id=95482) [details]
Patch
------- Comment #2 From 2011-05-31 14:28:50 PST -------
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 From 2011-05-31 14:44:04 PST -------
(From update of attachment 95482 [details])
Attachment 95482 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8755393
------- Comment #4 From 2011-05-31 21:14:54 PST -------
Created an attachment (id=95543) [details]
Patch
------- Comment #5 From 2011-06-06 05:49:40 PST -------
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 From 2011-06-07 04:27:57 PST -------
Created an attachment (id=96228) [details]
Patch

Updated to ENABLE(CSS_EXCLUSIONS)
------- Comment #7 From 2011-06-07 04:35:23 PST -------
(From update of attachment 96228 [details])
Attachment 96228 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8788161
------- Comment #8 From 2011-06-07 04:41:23 PST -------
Created an attachment (id=96229) [details]
Patch
------- Comment #9 From 2011-06-17 07:50:47 PST -------
(From update of attachment 96229 [details])
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 From 2011-06-17 08:31:28 PST -------
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 From 2011-06-21 07:52:45 PST -------
Created an attachment (id=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 From 2011-06-22 00:57:12 PST -------
Created an attachment (id=98130) [details]
Patch

Reposting to trigger the bots again. The dependency bug was fixed and the patch should apply correctly now.
------- Comment #13 From 2011-06-22 01:38:04 PST -------
(From update of attachment 98130 [details])
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 From 2011-06-22 01:38:09 PST -------
Created an attachment (id=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 From 2011-06-22 02:15:57 PST -------
Created an attachment (id=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 From 2011-06-27 07:25:32 PST -------
Created an attachment (id=98721) [details]
Patch - updated license

Changed the license in CSSWrapShape.h/cpp to BSD.
------- Comment #17 From 2011-07-01 02:23:57 PST -------
Created an attachment (id=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 From 2011-07-01 15:46:55 PST -------
Created an attachment (id=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 From 2011-07-12 12:06:15 PST -------
Can you please review the latest patch?
------- Comment #20 From 2011-07-12 13:49:37 PST -------
(From update of attachment 99531 [details])
r=me
------- Comment #21 From 2011-07-12 13:54:05 PST -------
(From update of attachment 99531 [details])
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 From 2011-07-12 14:50:58 PST -------
Created an attachment (id=100567) [details]
Patch

Rebased the previous patch, so that commit queue can apply it.
------- Comment #23 From 2011-07-12 15:05:40 PST -------
(From update of attachment 100567 [details])
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 From 2011-07-12 15:34:17 PST -------
(From update of attachment 100567 [details])
Clearing flags on attachment: 100567

Committed r90863: <http://trac.webkit.org/changeset/90863>
------- Comment #25 From 2011-07-13 01:05:19 PST -------
Created an attachment (id=100639) [details]
Patch - fixes for Comment #23
------- Comment #26 From 2011-07-13 01:07:51 PST -------
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 From 2011-07-13 01:14:34 PST -------
Created an attachment (id=100641) [details]
Patch - fixes for Comment #23 

Fixed styling. It looks like CSSWrapShapes needs to be before CounterDirectives.h.
------- Comment #28 From 2011-07-13 10:22:45 PST -------
(From update of attachment 100641 [details])
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 From 2011-07-13 10:58:19 PST -------
(In reply to comment #28)
> (From update of attachment 100641 [details] [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 From 2011-07-21 21:09:43 PST -------
(From update of attachment 99531 [details])
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 From 2011-07-24 23:50:51 PST -------
(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.