| 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: | P2 | ||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||
| Bug Depends on: | 62114 | ||||||||||||||||||||||||||||||||
| Bug Blocks: | 57311, 62991 | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Alexandru Chiculita
2011-05-30 04:32:25 PDT
Created attachment 95482 [details]
Patch
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 on attachment 95482 [details] Patch Attachment 95482 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8755393 Created attachment 95543 [details]
Patch
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. Created attachment 96228 [details]
Patch
Updated to ENABLE(CSS_EXCLUSIONS)
Comment on attachment 96228 [details] Patch Attachment 96228 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8788161 Created attachment 96229 [details]
Patch
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. 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? 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 Created attachment 98130 [details]
Patch
Reposting to trigger the bots again. The dependency bug was fixed and the patch should apply correctly now.
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 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
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.
Created attachment 98721 [details]
Patch - updated license
Changed the license in CSSWrapShape.h/cpp to BSD.
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. 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.
Can you please review the latest patch? Comment on attachment 99531 [details]
Patch - fixed variables naming
r=me
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 Created attachment 100567 [details]
Patch
Rebased the previous patch, so that commit queue can apply it.
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 on attachment 100567 [details] Patch Clearing flags on attachment: 100567 Committed r90863: <http://trac.webkit.org/changeset/90863> Created attachment 100639 [details] Patch - fixes for Comment #23 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.
Created attachment 100641 [details] Patch - fixes for Comment #23 Fixed styling. It looks like CSSWrapShapes needs to be before CounterDirectives.h. 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? (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 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. (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. |