Bug 116638

Created attachment 202750 [details] Patch Add support for parsing inset-rectangle

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.

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.

Created attachment 202835 [details] Patch Fix calculation error

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.

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.

(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.

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.

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.

(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.

> > > > > > > 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.

(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.

Created attachment 202863 [details] Patch Update tests.

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.

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?

(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.

(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?

Created attachment 203205 [details] Patch Update for review comments.

Comment on attachment 203205 [details] Patch Clearing flags on attachment: 203205 Committed r150904: <http://trac.webkit.org/changeset/150904>

All reviewed patches have been landed. Closing bug.