Bug 116638 – [CSS Shapes] Support parsing inset-rectangle shapes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bem Jones-Bey]

Add support to parse inset-rectangle shapes for shape-inside and shape-outside

[Bem Jones-Bey]

Created attachment 202750 [details]
Patch

Add support for parsing inset-rectangle

[WebKit Commit Bot]

Attachment 202750 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/exclusions/parsing/parsing-shape-lengths-expected.txt', u'LayoutTests/fast/exclusions/parsing/script-tests/parsing-shape-lengths.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/BasicShapeFunctions.cpp', u'Source/WebCore/css/CSSBasicShapes.cpp', u'Source/WebCore/css/CSSBasicShapes.h', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/rendering/ExclusionShapeInsideInfo.cpp', u'Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp', u'Source/WebCore/rendering/style/BasicShapes.cpp', u'Source/WebCore/rendering/style/BasicShapes.h']" exit_code: 1
Source/WebCore/rendering/style/BasicShapes.cpp:177:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/BasicShapes.cpp:178:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/BasicShapes.cpp:179:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/BasicShapes.cpp:181:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSBasicShapes.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSBasicShapes.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/BasicShapes.h:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/BasicShapes.h:53:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 8 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bem Jones-Bey]

All of the following style errors reflect issues that area already in the file and for consistency's sake, I don't want to change it just for the lines I've added. If this needs to be fixed, I would be happy to do so in a different patch, since it will require changes elsewhere as well.

(In reply to comment #2)
> Attachment 202750 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/exclusions/parsing/parsing-shape-lengths-expected.txt', u'LayoutTests/fast/exclusions/parsing/script-tests/parsing-shape-lengths.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/BasicShapeFunctions.cpp', u'Source/WebCore/css/CSSBasicShapes.cpp', u'Source/WebCore/css/CSSBasicShapes.h', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/rendering/ExclusionShapeInsideInfo.cpp', u'Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp', u'Source/WebCore/rendering/style/BasicShapes.cpp', u'Source/WebCore/rendering/style/BasicShapes.h']" exit_code: 1
> Source/WebCore/rendering/style/BasicShapes.cpp:177:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/rendering/style/BasicShapes.cpp:178:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/rendering/style/BasicShapes.cpp:179:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/rendering/style/BasicShapes.cpp:181:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/css/CSSBasicShapes.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Source/WebCore/css/CSSBasicShapes.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Source/WebCore/rendering/style/BasicShapes.h:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Source/WebCore/rendering/style/BasicShapes.h:53:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Total errors found: 8 in 13 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Bem Jones-Bey]

Created attachment 202835 [details]
Patch

Fix calculation error

[WebKit Commit Bot]

Attachment 202835 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/exclusions/parsing/parsing-shape-lengths-expected.txt', u'LayoutTests/fast/exclusions/parsing/script-tests/parsing-shape-lengths.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/BasicShapeFunctions.cpp', u'Source/WebCore/css/CSSBasicShapes.cpp', u'Source/WebCore/css/CSSBasicShapes.h', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/rendering/ExclusionShapeInsideInfo.cpp', u'Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp', u'Source/WebCore/rendering/style/BasicShapes.cpp', u'Source/WebCore/rendering/style/BasicShapes.h']" exit_code: 1
Source/WebCore/rendering/style/BasicShapes.cpp:179:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/BasicShapes.cpp:180:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/BasicShapes.cpp:182:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSBasicShapes.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSBasicShapes.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/BasicShapes.h:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/BasicShapes.h:53:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 7 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hans Muller]

The style bot is unhappy..

> Source/WebCore/rendering/ExclusionShapeInsideInfo.cpp
> return (shapeValue && shapeValue->type() == ExclusionShapeValue::Shape && shapeValue->shape()->type() != BasicShape::BASIC_SHAPE_INSET_RECTANGLE) ? shapeValue->shape() : 0;

This change, and the similar one for ExclusionShapeOutsideInfo, assumes that shapeValue->shape() is non-null, which it can't.

> Source/WebCore/rendering/style/BasicShapes.cpp
>   path.addRoundedRect(FloatRect(left + boundingBox.x(), top + boundingBox.y(),
>                                  std::max<float>(boundingBox.width() - left - floatValueForLength(m_right, boundingBox.width()), 0),
>                                  std::max<float>(boundingBox.height() - top - floatValueForLength(m_bottom, boundingBox.height()), 0)),
>                       FloatSize(m_cornerRadiusX.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusX, boundingBox.width()),
>                                  m_cornerRadiusY.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusY, boundingBox.height())));

I don't think the corner radius logic is correct here. If just radiusX is defined, it's supposed to be the value of radiusY too.

Likewise for BasicShapeInsetRectangle::blend().

> LayoutTests...

You need to update all of the parsing tests, not just parsing-shape-lengths.

[Bem Jones-Bey]

(In reply to comment #6)
> The style bot is unhappy..

Yeah, to make it happy, it will require another patch. I could change the formatting in this method to be different than all the others, which will make it happy about the indents, but to make it happy about the enum naming would require a new patch entirely that renames all of the enum values.

> > Source/WebCore/rendering/ExclusionShapeInsideInfo.cpp
> > return (shapeValue && shapeValue->type() == ExclusionShapeValue::Shape && shapeValue->shape()->type() != BasicShape::BASIC_SHAPE_INSET_RECTANGLE) ? shapeValue->shape() : 0;
> 
> This change, and the similar one for ExclusionShapeOutsideInfo, assumes that shapeValue->shape() is non-null, which it can't.

Ok, I'll fix that.

> 
> > Source/WebCore/rendering/style/BasicShapes.cpp
> >   path.addRoundedRect(FloatRect(left + boundingBox.x(), top + boundingBox.y(),
> >                                  std::max<float>(boundingBox.width() - left - floatValueForLength(m_right, boundingBox.width()), 0),
> >                                  std::max<float>(boundingBox.height() - top - floatValueForLength(m_bottom, boundingBox.height()), 0)),
> >                       FloatSize(m_cornerRadiusX.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusX, boundingBox.width()),
> >                                  m_cornerRadiusY.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusY, boundingBox.height())));
> 
> I don't think the corner radius logic is correct here. If just radiusX is defined, it's supposed to be the value of radiusY too.
> 
> Likewise for BasicShapeInsetRectangle::blend().

Hrm.. that's the same logic that BasicShapeRectangle uses. Is that incorrect there as well?

> 
> > LayoutTests...
> 
> You need to update all of the parsing tests, not just parsing-shape-lengths.

Ok, I was thinking I needed to update those when I added support for it in shape-inside and shape-outside, but I'll add it now.

[Bem Jones-Bey]

Created attachment 202844 [details]
Patch

Check for null shape. The style bot will be unhappy with this. I have filed Bug 116734 to fix the style problems, since they are bigger than just this patch.

[WebKit Commit Bot]

Attachment 202844 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/exclusions/parsing/parsing-shape-lengths-expected.txt', u'LayoutTests/fast/exclusions/parsing/script-tests/parsing-shape-lengths.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/BasicShapeFunctions.cpp', u'Source/WebCore/css/CSSBasicShapes.cpp', u'Source/WebCore/css/CSSBasicShapes.h', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/rendering/ExclusionShapeInsideInfo.cpp', u'Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp', u'Source/WebCore/rendering/style/BasicShapes.cpp', u'Source/WebCore/rendering/style/BasicShapes.h']" exit_code: 1
Source/WebCore/rendering/style/BasicShapes.cpp:179:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/BasicShapes.cpp:180:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/BasicShapes.cpp:182:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSBasicShapes.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSBasicShapes.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/BasicShapes.h:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/BasicShapes.h:53:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 7 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bem Jones-Bey]

(In reply to comment #7)
> (In reply to comment #6)
> > The style bot is unhappy..
> 
> Yeah, to make it happy, it will require another patch. I could change the formatting in this method to be different than all the others, which will make it happy about the indents, but to make it happy about the enum naming would require a new patch entirely that renames all of the enum values.

I have filed Bug 116734 to fix these problems.
 
> > 
> > > Source/WebCore/rendering/style/BasicShapes.cpp
> > >   path.addRoundedRect(FloatRect(left + boundingBox.x(), top + boundingBox.y(),
> > >                                  std::max<float>(boundingBox.width() - left - floatValueForLength(m_right, boundingBox.width()), 0),
> > >                                  std::max<float>(boundingBox.height() - top - floatValueForLength(m_bottom, boundingBox.height()), 0)),
> > >                       FloatSize(m_cornerRadiusX.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusX, boundingBox.width()),
> > >                                  m_cornerRadiusY.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusY, boundingBox.height())));
> > 
> > I don't think the corner radius logic is correct here. If just radiusX is defined, it's supposed to be the value of radiusY too.
> > 
> > Likewise for BasicShapeInsetRectangle::blend().
> 
> Hrm.. that's the same logic that BasicShapeRectangle uses. Is that incorrect there as well?

If you can verify that this is incorrect, I'll file a bug and fix it in a subsequent patch, since it will need to be fixed for rectangle as well.

> 
> > 
> > > LayoutTests...
> > 
> > You need to update all of the parsing tests, not just parsing-shape-lengths.
> 
> Ok, I was thinking I needed to update those when I added support for it in shape-inside and shape-outside, but I'll add it now.

I took a look, and the others don't test anything having to do with shapes, they just test parsing of the particular properties. So it seems to me that I don't need to add it to the other ones, or we would have needed to do so for circle, ellipse, and polygon. It also seems like adding all the shapes to the other ones wouldn't actually test anything that lengths isn't testing.

[Hans Muller]

> > > 
> > > > Source/WebCore/rendering/style/BasicShapes.cpp
> > > >   path.addRoundedRect(FloatRect(left + boundingBox.x(), top + boundingBox.y(),
> > > >                                  std::max<float>(boundingBox.width() - left - floatValueForLength(m_right, boundingBox.width()), 0),
> > > >                                  std::max<float>(boundingBox.height() - top - floatValueForLength(m_bottom, boundingBox.height()), 0)),
> > > >                       FloatSize(m_cornerRadiusX.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusX, boundingBox.width()),
> > > >                                  m_cornerRadiusY.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusY, boundingBox.height())));
> > > 
> > > I don't think the corner radius logic is correct here. If just radiusX is defined, it's supposed to be the value of radiusY too.
> > > 
> > > Likewise for BasicShapeInsetRectangle::blend().
> > 
> > Hrm.. that's the same logic that BasicShapeRectangle uses. Is that incorrect there as well?

I'm not sure what is correct.  

What confused me is your comment at the top of CSSParser::parseBasicShapeInsetRectangle():

    // inset-rectangle(top, right, bottom, left, [[rx], ry])

Does that mean that if only one radius parameter is specified rx=0 and ry=parameterValue? The code doesn't appear to work that way. Since one or two radius parameters are acceptable, then there should be tests to verify as much (see below).

> > > > LayoutTests...
> > > 
> > > You need to update all of the parsing tests, not just parsing-shape-lengths.
> > 
> > Ok, I was thinking I needed to update those when I added support for it in shape-inside and shape-outside, but I'll add it now.
> 
> I took a look, and the others don't test anything having to do with shapes, they just test parsing of the particular properties. So it seems to me that I don't need to add it to the other ones, or we would have needed to do so for circle, ellipse, and polygon. It also seems like adding all the shapes to the other ones wouldn't actually test anything that lengths isn't testing.

fast/exclusions/parsing/ashape-inside.html and fast/exclusions/parsing/shape-outside.html check that valid/invalid shape values are handled correctly for ALL shapes. You should update the validShapeValues and invalidShapeValues in fast/exclusions/parsing/script-tests/parsing-test-utils.js.

[Bem Jones-Bey]

(In reply to comment #11)
> > > > 
> > > > > Source/WebCore/rendering/style/BasicShapes.cpp
> > > > >   path.addRoundedRect(FloatRect(left + boundingBox.x(), top + boundingBox.y(),
> > > > >                                  std::max<float>(boundingBox.width() - left - floatValueForLength(m_right, boundingBox.width()), 0),
> > > > >                                  std::max<float>(boundingBox.height() - top - floatValueForLength(m_bottom, boundingBox.height()), 0)),
> > > > >                       FloatSize(m_cornerRadiusX.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusX, boundingBox.width()),
> > > > >                                  m_cornerRadiusY.isUndefined() ? 0 : floatValueForLength(m_cornerRadiusY, boundingBox.height())));
> > > > 
> > > > I don't think the corner radius logic is correct here. If just radiusX is defined, it's supposed to be the value of radiusY too.
> > > > 
> > > > Likewise for BasicShapeInsetRectangle::blend().
> > > 
> > > Hrm.. that's the same logic that BasicShapeRectangle uses. Is that incorrect there as well?
> 
> I'm not sure what is correct.  
> 
> What confused me is your comment at the top of CSSParser::parseBasicShapeInsetRectangle():
> 
>     // inset-rectangle(top, right, bottom, left, [[rx], ry])
> 
> Does that mean that if only one radius parameter is specified rx=0 and ry=parameterValue? The code doesn't appear to work that way. Since one or two radius parameters are acceptable, then there should be tests to verify as much (see below).

Ok, I looked at the SVG spec (Referenced by the shape spec), and you are correct that rx is supposed to default to ry if unspecified and vice-versa. However, the normal rectangle code doesn't do that, and thus the inset-rectangle code doesn't either, since it's based off the rectangle code. I've filed bug 116745 to fix this for both the rectangle and inset rectangle cases.

> 
> > > > > LayoutTests...
> > > > 
> > > > You need to update all of the parsing tests, not just parsing-shape-lengths.
> > > 
> > > Ok, I was thinking I needed to update those when I added support for it in shape-inside and shape-outside, but I'll add it now.
> > 
> > I took a look, and the others don't test anything having to do with shapes, they just test parsing of the particular properties. So it seems to me that I don't need to add it to the other ones, or we would have needed to do so for circle, ellipse, and polygon. It also seems like adding all the shapes to the other ones wouldn't actually test anything that lengths isn't testing.
> 
> fast/exclusions/parsing/ashape-inside.html and fast/exclusions/parsing/shape-outside.html check that valid/invalid shape values are handled correctly for ALL shapes. You should update the validShapeValues and invalidShapeValues in fast/exclusions/parsing/script-tests/parsing-test-utils.js.

You're right, I read the code all wrong. I'll push up a patch with that fixed.

[Bem Jones-Bey]

Created attachment 202863 [details]
Patch

Update tests.

[WebKit Commit Bot]

Attachment 202863 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/exclusions/parsing/parsing-shape-inside-expected.txt', u'LayoutTests/fast/exclusions/parsing/parsing-shape-lengths-expected.txt', u'LayoutTests/fast/exclusions/parsing/parsing-shape-outside-expected.txt', u'LayoutTests/fast/exclusions/parsing/script-tests/parsing-shape-lengths.js', u'LayoutTests/fast/exclusions/parsing/script-tests/parsing-test-utils.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/BasicShapeFunctions.cpp', u'Source/WebCore/css/CSSBasicShapes.cpp', u'Source/WebCore/css/CSSBasicShapes.h', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/rendering/ExclusionShapeInsideInfo.cpp', u'Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp', u'Source/WebCore/rendering/style/BasicShapes.cpp', u'Source/WebCore/rendering/style/BasicShapes.h']" exit_code: 1
Source/WebCore/rendering/style/BasicShapes.cpp:179:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/BasicShapes.cpp:180:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/BasicShapes.cpp:182:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSBasicShapes.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSBasicShapes.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/BasicShapes.h:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/BasicShapes.h:53:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 7 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

The review tool is broken for me, so I'll do this old skool!!!

ChangeLog:

- Could you include per-function comments here?
- Also, please remove the (WebCore:) lines until the tool is updated.

CSSBasicShapes.cpp:

- I'm certainly not an expert here, but is it worth calculating how much to allocate up front in buildInsetRectangleString rather than just picking a sufficiently large value? I have no opinion.

CSSParser.cpp

- typo in the BNF comment (missing ])
- Do you think we should put an explicit return 0 if argumentNumber gets to 6?

ExclusionShapeInsideInfo.cpp
ExclusionShapeOutsideInfo.cpp
- we typically write the conditional the other way around so that we return false early

BasicShapes.cpp
- For BasicShapeInsetRectangle::blend, would it be better to blend from/to a 0 radius if only one of states specifies a radius?

[Bem Jones-Bey]

(In reply to comment #15)
> The review tool is broken for me, so I'll do this old skool!!!
> 
> ChangeLog:
> 
> - Could you include per-function comments here?
> - Also, please remove the (WebCore:) lines until the tool is updated.

Sure, I'll do that.

> 
> CSSBasicShapes.cpp:
> 
> - I'm certainly not an expert here, but is it worth calculating how much to allocate up front in buildInsetRectangleString rather than just picking a sufficiently large value? I have no opinion.

I don't know either, in this case, I'm doing the same thing that all of the other basic shapes are doing. Given that you don't have an opinion, I wouldn't be inclined to change things unless there was some data saying that it isn't good to do it the way it is done. (And then, I'd like to do it in a follow up patch to change all of the CSS Basic Shapes to do things the same way)

> 
> CSSParser.cpp
> 
> - typo in the BNF comment (missing ])

Oops. I'll fix that.

> - Do you think we should put an explicit return 0 if argumentNumber gets to 6?

By my understanding of the code, it should already have returned because of the if statement at the top. This could be an argument to remove the assert, but then I'd like to remove it from all of the shape parsing methods, again for consistency. I'm personally kind of like having the assert there since it helps document the code and also is a failsafe if one happens to edit the if statement and gets it wrong (since it's a bit strange in that it has to count the commas as well as the arguments.

> 
> ExclusionShapeInsideInfo.cpp
> ExclusionShapeOutsideInfo.cpp
> - we typically write the conditional the other way around so that we return false early

You're right. I'll fix that.

> 
> BasicShapes.cpp
> - For BasicShapeInsetRectangle::blend, would it be better to blend from/to a 0 radius if only one of states specifies a radius?

It might be. I'll talk it over with Hans (who understands this better than I do), and handle changes here as part of fixing bug 116745, since right now, rectangle also does it the same way and will need to be changed as well.

[Bem Jones-Bey]

(In reply to comment #16)
> (In reply to comment #15)
> > CSSParser.cpp
> > 
> > - typo in the BNF comment (missing ])
> 
> Oops. I'll fix that.

It looks to me (And Vim's brace matching) that all of my []'s are matched in the comment. Are you sure one is missing?

[Bem Jones-Bey]

Created attachment 203205 [details]
Patch

Update for review comments.

[WebKit Commit Bot]

Comment on attachment 203205 [details]
Patch

Clearing flags on attachment: 203205

Committed r150904: <http://trac.webkit.org/changeset/150904>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.