Bug 100398

Created attachment 170726 [details] Initial implementation Initial implementation

Comment on attachment 170726 [details] Initial implementation View in context: https://bugs.webkit.org/attachment.cgi?id=170726&action=review Looks like a solid patch submission. Overall, I would try to minimize the compile-time guards and factor out member variables. As a precursor to landing this patch, you could implement ExclusionShapeOutsideInfo (perhaps inheriting from the same parent as ExclusionShapeInsideInfo). For the tests, if much of their content is repeating, I would use a script. The polygon and rounded rectangle tests are already doing so, as is bug 98189. > Source/WebCore/rendering/RenderBlock.cpp:91 > + OwnPtr<ExclusionShape> shapeOutside; If there is one of these checks, it generally means the bar for adding a member is pretty high. Shouldn't this be available via the renderer anyways? > Source/WebCore/rendering/RenderBlock.cpp:3799 > + if (basicShapeOutside) { Does a style change cause relayout? I would suggest following shape-inside's example for processing shape-outside. See RenderBlock::styleDidChange and RenderStyle::diff. This could potentially come in a subsequent patch. You might also consider putting in a FIXME with a bug number mentioning fixing up the content's location. > Source/WebCore/rendering/RenderBlock.cpp:3987 > +#if ENABLE(CSS_EXCLUSIONS) > + ExclusionShape* shapeOutside = floatingObject->shapeOutside(); > + if (shapeOutside) { > + setLogicalLeftForChild(childBox, floatLogicalLocation.x()); > + setLogicalTopForChild(childBox, floatLogicalLocation.y()); > + } else { > +#endif > + setLogicalLeftForChild(childBox, floatLogicalLocation.x() + childLogicalLeftMargin); > + setLogicalTopForChild(childBox, floatLogicalLocation.y() + marginBeforeForChild(childBox)); > +#if ENABLE(CSS_EXCLUSIONS) > + } > +#endif Here you could create two variables and factor out the #if ENABLES, eg float logicalXOffset = childLogicalLeftMargin; #if ENABLE(CSS_EXCLUSIONS) if (shapeOutside) logicalXOffset = 0; #endif setLogicalLeftForChild(childBox, floatLogicalLocation.x() + logicalXOffset); > Source/WebCore/rendering/RenderBlock.cpp:4025 > +#if ENABLE(CSS_EXCLUSIONS) > + if (shapeOutside) { > + setLogicalLeftForChild(childBox, floatLogicalLocation.x()); > + setLogicalTopForChild(childBox, floatLogicalLocation.y()); > + } else { > +#endif > + setLogicalLeftForChild(childBox, floatLogicalLocation.x() + childLogicalLeftMargin); > + setLogicalTopForChild(childBox, floatLogicalLocation.y() + marginBeforeForChild(childBox)); > +#if ENABLE(CSS_EXCLUSIONS) > + } > +#endif Ditto. > Source/WebCore/rendering/RenderBlock.cpp:4039 > +#if ENABLE(CSS_EXCLUSIONS) > + if (shapeOutside) > + setLogicalHeightForFloat(floatingObject, shapeOutside->shapeLogicalBoundingBox().height()); > + else > +#endif > + setLogicalHeightForFloat(floatingObject, logicalHeightForChild(childBox) + marginBeforeForChild(childBox) + marginAfterForChild(childBox)); Here I would suggest making a similar alteration (eg, a height variable), but it's more stylistic than necessary. > Source/WebCore/rendering/RenderBlock.h:664 > + OwnPtr<ExclusionShape> m_shapeOutside; Even more so than the floating object struct, the bar for adding member variables to render block is extremely high. Most new 'members' must be maintained in a static map. See RenderBlock.cpp::gColumnInfoMap or the map maintained by ExclusionShapeInsideInfo.

Comment on attachment 170726 [details] Initial implementation View in context: https://bugs.webkit.org/attachment.cgi?id=170726&action=review Thanks for the feedback! >> Source/WebCore/rendering/RenderBlock.cpp:91 >> + OwnPtr<ExclusionShape> shapeOutside; > > If there is one of these checks, it generally means the bar for adding a member is pretty high. Shouldn't this be available via the renderer anyways? I'll add it as a member to RenderBox and remove it from FloatingObject. >> Source/WebCore/rendering/RenderBlock.cpp:3799 >> + if (basicShapeOutside) { > > Does a style change cause relayout? I would suggest following shape-inside's example for processing shape-outside. See RenderBlock::styleDidChange and RenderStyle::diff. This could potentially come in a subsequent patch. > You might also consider putting in a FIXME with a bug number mentioning fixing up the content's location. I'll take a look at RenderBlock::StyleDidChange and RenderStyle::diff, and if it looks complex, I'll put a FIXME and a bug number for that. I'll add a FIXME comment referencing the location fix that is incoming as well. >> Source/WebCore/rendering/RenderBlock.cpp:3987 >> +#endif > > Here you could create two variables and factor out the #if ENABLES, eg > float logicalXOffset = childLogicalLeftMargin; > #if ENABLE(CSS_EXCLUSIONS) > if (shapeOutside) > logicalXOffset = 0; > #endif > setLogicalLeftForChild(childBox, floatLogicalLocation.x() + logicalXOffset); Sounds good. I'll do that for all of these cases. >> Source/WebCore/rendering/RenderBlock.h:664 >> + OwnPtr<ExclusionShape> m_shapeOutside; > > Even more so than the floating object struct, the bar for adding member variables to render block is extremely high. Most new 'members' must be maintained in a static map. See RenderBlock.cpp::gColumnInfoMap or the map maintained by ExclusionShapeInsideInfo. This actually is a member variable of the FloatingObject struct. The entry you saw earlier was in the struct used to check the size. But I'll move this variable to RenderBox.

(In reply to comment #2) > Looks like a solid patch submission. Overall, I would try to minimize the compile-time guards and factor out member variables. > As a precursor to landing this patch, you could implement ExclusionShapeOutsideInfo (perhaps inheriting from the same parent as ExclusionShapeInsideInfo). Hrm... the "view in context" made these comments disappear. Curious. Anyways, sounds like I should implement ExclusionShapeOutsideInfo and make that a member of RenderBox instead of making the ExclusionShape itself a member of RenderBox. Sound reasonable? > For the tests, if much of their content is repeating, I would use a script. The polygon and rounded rectangle tests are already doing so, as is bug 98189. Ok, I'll take a look at this.

> Source/WebCore/ChangeLog > 13 ... This patch does not implement positioning the > 14 shape outside box, so the x and y parameters are currently ignored in > 15 the specified shape. This might be a little clearer if it said that the patch "does not support positioning ..". Still wondering: can the shape extend outside of the box? > Source/WebCore/rendering/RenderBlock.h > 655 ExclusionShape* shapeOutside() const { return m_shapeOutside.get(); } This particular idiom appears to be frowned upon: [webkit-dev] On returning mutable pointers from const methods - http://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html In this case returning const ExclusionShape* should be OK. > Source/WebCore/rendering/RenderBlock.h > 654#if ENABLE(CSS_EXCLUSIONS) > 655 ExclusionShape* shapeOutside() const { return m_shapeOutside.get(); } > 656 void setShapeOutside(PassOwnPtr<ExclusionShape> shapeOutside) { m_shapeOutside = shapeOutside; } > 657#endif ... > 663#if ENABLE(CSS_EXCLUSIONS) > 664 OwnPtr<ExclusionShape> m_shapeOutside; > 665#endif The accessor methods cover m_shapeOutside so shouldn't it be private?

(In reply to comment #5) > > Source/WebCore/ChangeLog > > 13 ... This patch does not implement positioning the > > 14 shape outside box, so the x and y parameters are currently ignored in > > 15 the specified shape. > > This might be a little clearer if it said that the patch "does not support positioning ..". Ok, I'll change that. > > Still wondering: can the shape extend outside of the box? I'm not clear on which box you are referring to. The shape can extend outside of the float's margin box, if that's what you are asking. If not, can you clarify? > > > Source/WebCore/rendering/RenderBlock.h > > 655 ExclusionShape* shapeOutside() const { return m_shapeOutside.get(); } > > This particular idiom appears to be frowned upon: > > [webkit-dev] On returning mutable pointers from const methods - http://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html > > In this case returning const ExclusionShape* should be OK. That's funny, since I just saw that thread, and now have plans to change it to be that. :-) > > Source/WebCore/rendering/RenderBlock.h > > 654#if ENABLE(CSS_EXCLUSIONS) > > 655 ExclusionShape* shapeOutside() const { return m_shapeOutside.get(); } > > 656 void setShapeOutside(PassOwnPtr<ExclusionShape> shapeOutside) { m_shapeOutside = shapeOutside; } > > 657#endif > ... > > 663#if ENABLE(CSS_EXCLUSIONS) > > 664 OwnPtr<ExclusionShape> m_shapeOutside; > > 665#endif > > The accessor methods cover m_shapeOutside so shouldn't it be private? I was just following the convention of the other members, which also have accessors. Dunno why they are public. Anyways, I'm going to move this out of this object, and into RenderBox, so I'll make sure it ends up being private there. Thanks!

(In reply to comment #6) > (In reply to comment #5) > > > Source/WebCore/ChangeLog > > > 13 ... This patch does not implement positioning the > > > 14 shape outside box, so the x and y parameters are currently ignored in > > > 15 the specified shape. > > Still wondering: can the shape extend outside of the box? > > I'm not clear on which box you are referring to. The shape can extend outside of the float's margin box, if that's what you are asking. If not, can you clarify? I'd misunderstood the phrasing here. You wrote "positioning the shape outside box" which I read as: positioning the shape-outside outside the [margin] box. What's the rationale for leaving this aspect of the feature out of the implementation?

(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > > Source/WebCore/ChangeLog > > > > 13 ... This patch does not implement positioning the > > > > 14 shape outside box, so the x and y parameters are currently ignored in > > > > 15 the specified shape. > > > Still wondering: can the shape extend outside of the box? > > > > I'm not clear on which box you are referring to. The shape can extend outside of the float's margin box, if that's what you are asking. If not, can you clarify? > > I'd misunderstood the phrasing here. You wrote "positioning the shape outside box" which I read as: positioning the shape-outside outside the [margin] box. What's the rationale for leaving this aspect of the feature out of the implementation? I broke it up because I have a working implementation of the width/height, and figured that was self contained enough to push as it's own patch. I am intending to work on the positioning next, I just need to figure out how it should work. :-)

(In reply to comment #4) > (In reply to comment #2) > > For the tests, if much of their content is repeating, I would use a script. The polygon and rounded rectangle tests are already doing so, as is bug 98189. > > Ok, I'll take a look at this. I filed Bug 100699 to track this, I think it'll be simpler to do it as a separate patch.

Created attachment 171327 [details] Updated patch Updated patch

Comment on attachment 171327 [details] Updated patch Attachment 171327 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14630277

Comment on attachment 171327 [details] Updated patch Attachment 171327 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14641247

Created attachment 171330 [details] Updated patch Updated patch, fix Qt build issue

View in context: https://bugs.webkit.org/attachment.cgi?id=171327&action=review I have a couple of comments below. I think margins could get some tweaks in the specification, so I will raise that issue. I didn't have time to review all the tests, yet. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:31 > +#include "ExclusionShapeOutsideInfo.h" I think the include could go inside the following #ifdef. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:63 > + ExclusionShapeOutsideInfoMap::AddResult result = exclusionShapeOutsideInfoMap().add(box, create(box)); > + return result.iterator->value.get(); I would separate these ones into separate lines. There's too much happening in the first line. Also, you get to call create even if you are just going to throw the result away. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:74 > + // FIXME enable shape outside for non-rectangular shapes! Add a link to a bug here. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:82 > + if (!box->style() || !box->style()->shapeOutside()) > + return; Is there a reason to check for the box->style() here? You call this function from styleDidChange, so the style will already be updated to the new one and the shape might not be there anymore. I would also add an assert checking that there's no "box" in the "info map". > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:88 > + if (!m_shapeSizeDirty && logicalWidth == m_logicalWidth && logicalHeight == m_logicalHeight) m_shapeSizeDirty is not really what it sais it is. You are using it to recreate the shape when the CSS shape changes. I would rather remove that and instead use the "m_shape = 0;" as a flag that the shape needs to be recomputed. Also, I think the shape can be computed lazily. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:98 > + BasicShape* shape = m_box->style()->shapeOutside(); > + ASSERT(shape); > + > + m_shape = ExclusionShape::createExclusionShape(shape, logicalWidth, logicalHeight, m_box->style()->writingMode()); shape and m_shape have totally different types, but use the same name. I think m_shape can use a different name, something to tell that this is ready to be used (ie. computedShape?) > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:99 > + ASSERT(m_shape); This assert looks weird. If createExclusionShape is designed to return 0, then you need to treat that case. If the allocation fails, then it will crash before you get the assert, so I'm not sure what you are trying to catch here. > Source/WebCore/rendering/RenderBlock.cpp:1453 > + if (exclusionShapeOutsideInfo) > + exclusionShapeInsideInfo->computeShapeSize(logicalWidth(), computedLogicalHeight); Image can also have outside shapes and they are not RenderBlocks. Should this be in the RenderBox instead? BTW, you also recompute the size when you insert the float. Why is this needed here at all then? > Source/WebCore/rendering/RenderBlock.cpp:3805 > + logicalWidth = shapeOutside->shapeLogicalWidth(); Why are the margins ignored in this case? > Source/WebCore/rendering/RenderBlock.cpp:3986 > + ExclusionShapeOutsideInfo* shapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); > + if (shapeOutside) { > + floatLogicalLeftOffset = 0; > + floatLogicalTopOffset = 0; > + } Why do you need to do this? I would argue that you just need to update the offsets, but not make them 0. You need to position the shape inside the bounds, so I would rather subtract the minX and minY of the exclusion shape instead. > Source/WebCore/rendering/RenderBlock.cpp:4023 > + if (shapeOutside) { > + floatLogicalLeftOffset = 0; > + floatLogicalTopOffset = 0; > + } ditto > Source/WebCore/rendering/RenderBlock.cpp:4038 > + floatLogicalHeight = shapeOutside->shapeLogicalHeight(); Why do you ignore the margins? > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored.html:33 > + Requites Ahem font. Tests that the shape on a right float causes the margin on the float to have no effect on layout. I would like to keep margins for now and raise this as an issue on the spec instead. I think margins should affect the positioning relative to the other floats, but not the text.

(In reply to comment #14) > View in context: https://bugs.webkit.org/attachment.cgi?id=171327&action=review > > I have a couple of comments below. I think margins could get some tweaks in the specification, so I will raise that issue. I didn't have time to review all the tests, yet. > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:31 > > +#include "ExclusionShapeOutsideInfo.h" > > I think the include could go inside the following #ifdef. Sure. > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:63 > > + ExclusionShapeOutsideInfoMap::AddResult result = exclusionShapeOutsideInfoMap().add(box, create(box)); > > + return result.iterator->value.get(); > > I would separate these ones into separate lines. There's too much happening in the first line. Also, you get to call create even if you are just going to throw the result away. So you're saying that this should only do the create if an entry doesn't already exist, right? That works for me. > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:74 > > + // FIXME enable shape outside for non-rectangular shapes! > > Add a link to a bug here. Ok, I'll add links to bug 98672, bug 98673, bug 98674, bug 98686, and bug 100299, or should I just link to the master bug (Bug 98664)? > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:82 > > + if (!box->style() || !box->style()->shapeOutside()) > > + return; > > Is there a reason to check for the box->style() here? > > You call this function from styleDidChange, so the style will already be updated to the new one and the shape might not be there anymore. I would also add an assert checking that there's no "box" in the "info map". I'm not sure, since this check was inherited from ExclusionShapeInsideInfo, which uses it in much the same way. Is there a way to mention Bear in a comment to notify him that I'd like his feedback, or do I just have to cc him on the bug to do that? I'd like to ask him if he put that in there for any specific reason. Perhaps I should also file a bug for refactoring the code to use a common superclass. I just wanted to keep this patch simple so I didn't add a refactoring of ExclusionShapeInsideInfo. > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:88 > > + if (!m_shapeSizeDirty && logicalWidth == m_logicalWidth && logicalHeight == m_logicalHeight) > > m_shapeSizeDirty is not really what it sais it is. You are using it to recreate the shape when the CSS shape changes. I would rather remove that and instead use the "m_shape = 0;" as a flag that the shape needs to be recomputed. Also, I think the shape can be computed lazily. I don't quite follow why m_shapeSizeDirty isn't what it says it is. Isn't the whole point of a dirty flag to be able to know when you need to recompute? Also, I agree that things could be done in a lazy fashion, but is it worth the increased complexity of the code to do so? > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:98 > > + BasicShape* shape = m_box->style()->shapeOutside(); > > + ASSERT(shape); > > + > > + m_shape = ExclusionShape::createExclusionShape(shape, logicalWidth, logicalHeight, m_box->style()->writingMode()); > > shape and m_shape have totally different types, but use the same name. I think m_shape can use a different name, something to tell that this is ready to be used (ie. computedShape?) Sure. I'll do m_computedShape and I'll rename "shape" to "basicShape" to make sure there can be no confusion. > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:99 > > + ASSERT(m_shape); > > This assert looks weird. If createExclusionShape is designed to return 0, then you need to treat that case. If the allocation fails, then it will crash before you get the assert, so I'm not sure what you are trying to catch here. This is another one I will have to ask Bear about. It looks to me like that can never happen, since as far as I can tell, it can only return 0 if the passed in BasicShape is null, and we assert that it isn't on the line before. So I do think we can safely remove it. > > > Source/WebCore/rendering/RenderBlock.cpp:1453 > > + if (exclusionShapeOutsideInfo) > > + exclusionShapeInsideInfo->computeShapeSize(logicalWidth(), computedLogicalHeight); > > Image can also have outside shapes and they are not RenderBlocks. Should this be in the RenderBox instead? As far as I can tell, RenderBlock overrides RenderBox's layout() method, but never calls it. So I didn't see a reasonable place to put this call in RenderBox that would ensure it gets called. Is that understanding incorrect? I figured I would have to revisit this for floated images, and probably should have filed a bug. Also, there's a typo here, that really should be called in exclusionShapeOutsideInfo, not exclusionShapeInsideInfo. > BTW, you also recompute the size when you insert the float. Why is this needed here at all then? I'm actually pretty sure it isn't needed here at this point, I just neglected to delete it. > > > Source/WebCore/rendering/RenderBlock.cpp:3805 > > + logicalWidth = shapeOutside->shapeLogicalWidth(); > > Why are the margins ignored in this case? Because the spec says they are. > > > Source/WebCore/rendering/RenderBlock.cpp:3986 > > + ExclusionShapeOutsideInfo* shapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); > > + if (shapeOutside) { > > + floatLogicalLeftOffset = 0; > > + floatLogicalTopOffset = 0; > > + } > > Why do you need to do this? I would argue that you just need to update the offsets, but not make them 0. You need to position the shape inside the bounds, so I would rather subtract the minX and minY of the exclusion shape instead. Because I've decided to split this up into two smaller patches, one for the size and one for positioning. I wanted to get one working before I started on the other because I was messing around with positioning as well and found that I was too far off in the weeds for the moment. I will address this in bug 100399. > > > Source/WebCore/rendering/RenderBlock.cpp:4023 > > + if (shapeOutside) { > > + floatLogicalLeftOffset = 0; > > + floatLogicalTopOffset = 0; > > + } > > ditto see above. :-) > > > Source/WebCore/rendering/RenderBlock.cpp:4038 > > + floatLogicalHeight = shapeOutside->shapeLogicalHeight(); > > Why do you ignore the margins? Because the spec says so. > > > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored.html:33 > > + Requites Ahem font. Tests that the shape on a right float causes the margin on the float to have no effect on layout. > > I would like to keep margins for now and raise this as an issue on the spec instead. I think margins should affect the positioning relative to the other floats, but not the text. I think this is a much longer discussion, and would like to have it unrelated to this patch. I would like to have the patch implement what the spec says right now, and if after having a discussion about the spec, we decide that the behavior of margins should be different, then we can revise the code then. Is that agreeable? (feel free to start a www-style thread on the margins issue. :-) Thanks for the feedback!

(In reply to comment #15) > (In reply to comment #14) > > View in context: https://bugs.webkit.org/attachment.cgi?id=171327&action=review > > > > I have a couple of comments below. I think margins could get some tweaks in the specification, so I will raise that issue. I didn't have time to review all the tests, yet. > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:31 > > > +#include "ExclusionShapeOutsideInfo.h" > > > > I think the include could go inside the following #ifdef. > > Sure. > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:63 > > > + ExclusionShapeOutsideInfoMap::AddResult result = exclusionShapeOutsideInfoMap().add(box, create(box)); > > > + return result.iterator->value.get(); > > > > I would separate these ones into separate lines. There's too much happening in the first line. Also, you get to call create even if you are just going to throw the result away. > > So you're saying that this should only do the create if an entry doesn't already exist, right? That works for me. I think that adding a key that already exists will not replace the old one. The add result will just give you the old value. Anyway, you should not create it if it already exists, that is the point of caching it, right? > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:74 > > > + // FIXME enable shape outside for non-rectangular shapes! > > > > Add a link to a bug here. > > Ok, I'll add links to bug 98672, bug 98673, bug 98674, bug 98686, and bug 100299, or should I just link to the master bug (Bug 98664)? Master bug should be good enough for now. > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:82 > > > + if (!box->style() || !box->style()->shapeOutside()) > > > + return; > > > > Is there a reason to check for the box->style() here? > > > > You call this function from styleDidChange, so the style will already be updated to the new one and the shape might not be there anymore. I would also add an assert checking that there's no "box" in the "info map". > > I'm not sure, since this check was inherited from ExclusionShapeInsideInfo, which uses it in much the same way. Is there a way to mention Bear in a comment to notify him that I'd like his feedback, or do I just have to cc him on the bug to do that? I'd like to ask him if he put that in there for any specific reason. > > Perhaps I should also file a bug for refactoring the code to use a common superclass. I just wanted to keep this patch simple so I didn't add a refactoring of ExclusionShapeInsideInfo. If you want to do that, I think you can start by refactoring ExclusionShapeInsideInfo first in smaller patches. And add the new class when you are ready with that. > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:88 > > > + if (!m_shapeSizeDirty && logicalWidth == m_logicalWidth && logicalHeight == m_logicalHeight) > > > > m_shapeSizeDirty is not really what it sais it is. You are using it to recreate the shape when the CSS shape changes. I would rather remove that and instead use the "m_shape = 0;" as a flag that the shape needs to be recomputed. Also, I think the shape can be computed lazily. > > I don't quite follow why m_shapeSizeDirty isn't what it says it is. Isn't the whole point of a dirty flag to be able to know when you need to recompute? m_shapeSizeDirty says that the size is dirty, but its value is only true when the "m_shape" is dirty and needs to be recreated. That same effect can be done using "m_shape = null". Is that clear enough now? > Also, I agree that things could be done in a lazy fashion, but is it worth the increased complexity of the code to do so? It is as simple as checking that the shape != null when asking for the shape. It is not increasing the complexity, it just simplifies it. > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:98 > > > + BasicShape* shape = m_box->style()->shapeOutside(); > > > + ASSERT(shape); > > > + > > > + m_shape = ExclusionShape::createExclusionShape(shape, logicalWidth, logicalHeight, m_box->style()->writingMode()); > > > > shape and m_shape have totally different types, but use the same name. I think m_shape can use a different name, something to tell that this is ready to be used (ie. computedShape?) > > Sure. I'll do m_computedShape and I'll rename "shape" to "basicShape" to make sure there can be no confusion. Ok. > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:99 > > > + ASSERT(m_shape); > > > > This assert looks weird. If createExclusionShape is designed to return 0, then you need to treat that case. If the allocation fails, then it will crash before you get the assert, so I'm not sure what you are trying to catch here. > > This is another one I will have to ask Bear about. It looks to me like that can never happen, since as far as I can tell, it can only return 0 if the passed in BasicShape is null Add a separate bug to remove that from createExclusionShape. Usually, methods are not checking for their arguments. At most, they just use an assert. Why would anyone call createExclusionShape with a null shape anyway? > , and we assert that it isn't on the line before. So I do think we can safely remove it. Ok. > > > > > > Source/WebCore/rendering/RenderBlock.cpp:1453 > > > + if (exclusionShapeOutsideInfo) > > > + exclusionShapeInsideInfo->computeShapeSize(logicalWidth(), computedLogicalHeight); > > > > Image can also have outside shapes and they are not RenderBlocks. Should this be in the RenderBox instead? > > As far as I can tell, RenderBlock overrides RenderBox's layout() method, but never calls it. So I didn't see a reasonable place to put this call in RenderBox that would ensure it gets called. Is that understanding incorrect? Look how the transforms are recalculated at the end of most ::layout functions in WebKit. Anyway, I assume that you don't need the size at this point. So, I would better remove the code completely and do the lazy shape calculation instead when you actually got the size. (that's why doing that simplifies the patch) The shape outside changes, it will invalidate the floater's layout anyway (assuming that RenderStyle:diff returns layout in the case of shape-outside, if not fix it in this patch). Moreover, if the width/height of the floater changes it will also be during the layout, so doing it later when you actually need it, would save you a lot of trouble keeping the shape up-to-date in different places. > I figured I would have to revisit this for floated images, and probably should have filed a bug. > Also, there's a typo here, that really should be called in exclusionShapeOutsideInfo, not exclusionShapeInsideInfo. Didn't see that, nice catch. > > > BTW, you also recompute the size when you insert the float. Why is this needed here at all then? > > I'm actually pretty sure it isn't needed here at this point, I just neglected to delete it. > > > > > > Source/WebCore/rendering/RenderBlock.cpp:3805 > > > + logicalWidth = shapeOutside->shapeLogicalWidth(); > > > > Why are the margins ignored in this case? > > Because the spec says they are. It looks weird, so I've started the discussion around that. > > > > > > Source/WebCore/rendering/RenderBlock.cpp:3986 > > > + ExclusionShapeOutsideInfo* shapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); > > > + if (shapeOutside) { > > > + floatLogicalLeftOffset = 0; > > > + floatLogicalTopOffset = 0; > > > + } > > > > Why do you need to do this? I would argue that you just need to update the offsets, but not make them 0. You need to position the shape inside the bounds, so I would rather subtract the minX and minY of the exclusion shape instead. > > Because I've decided to split this up into two smaller patches, one for the size and one for positioning. I wanted to get one working before I started on the other because I was messing around with positioning as well and found that I was too far off in the weeds for the moment. I will address this in bug 100399. I think this is related to the margins. > > > > > > Source/WebCore/rendering/RenderBlock.cpp:4023 > > > + if (shapeOutside) { > > > + floatLogicalLeftOffset = 0; > > > + floatLogicalTopOffset = 0; > > > + } > > > > ditto > > see above. :-) > > > > > > Source/WebCore/rendering/RenderBlock.cpp:4038 > > > + floatLogicalHeight = shapeOutside->shapeLogicalHeight(); > > > > Why do you ignore the margins? > > Because the spec says so. > > > > > > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored.html:33 > > > + Requites Ahem font. Tests that the shape on a right float causes the margin on the float to have no effect on layout. > > > > I would like to keep margins for now and raise this as an issue on the spec instead. I think margins should affect the positioning relative to the other floats, but not the text. > > I think this is a much longer discussion, and would like to have it unrelated to this patch. I would like to have the patch implement what the spec says right now, and if after having a discussion about the spec, we decide that the behavior of margins should be different, then we can revise the code then. Is that agreeable? (feel free to start a www-style thread on the margins issue. :-) > > Thanks for the feedback!

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=171327&action=review > > > > > > I have a couple of comments below. I think margins could get some tweaks in the specification, so I will raise that issue. I didn't have time to review all the tests, yet. > > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:31 > > > > +#include "ExclusionShapeOutsideInfo.h" > > > > > > I think the include could go inside the following #ifdef. > > > > Sure. > > > > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:63 > > > > + ExclusionShapeOutsideInfoMap::AddResult result = exclusionShapeOutsideInfoMap().add(box, create(box)); > > > > + return result.iterator->value.get(); > > > > > > I would separate these ones into separate lines. There's too much happening in the first line. Also, you get to call create even if you are just going to throw the result away. > > > > So you're saying that this should only do the create if an entry doesn't already exist, right? That works for me. > I think that adding a key that already exists will not replace the old one. The add result will just give you the old value. Anyway, you should not create it if it already exists, that is the point of caching it, right? Yup. Your wording just wasn't entirely clear to me, so I wanted to make sure we were on the same page. (This is another one that should be fixed in ExclusionShapeInsideInfo as well.) > > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:74 > > > > + // FIXME enable shape outside for non-rectangular shapes! > > > > > > Add a link to a bug here. > > > > Ok, I'll add links to bug 98672, bug 98673, bug 98674, bug 98686, and bug 100299, or should I just link to the master bug (Bug 98664)? > Master bug should be good enough for now. > > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:82 > > > > + if (!box->style() || !box->style()->shapeOutside()) > > > > + return; > > > > > > Is there a reason to check for the box->style() here? > > > > > > You call this function from styleDidChange, so the style will already be updated to the new one and the shape might not be there anymore. I would also add an assert checking that there's no "box" in the "info map". > > > > I'm not sure, since this check was inherited from ExclusionShapeInsideInfo, which uses it in much the same way. Is there a way to mention Bear in a comment to notify him that I'd like his feedback, or do I just have to cc him on the bug to do that? I'd like to ask him if he put that in there for any specific reason. > > > > Perhaps I should also file a bug for refactoring the code to use a common superclass. I just wanted to keep this patch simple so I didn't add a refactoring of ExclusionShapeInsideInfo. > If you want to do that, I think you can start by refactoring ExclusionShapeInsideInfo first in smaller patches. And add the new class when you are ready with that. Yeah, I've created bug 100766 to track that. > > > > > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:88 > > > > + if (!m_shapeSizeDirty && logicalWidth == m_logicalWidth && logicalHeight == m_logicalHeight) > > > > > > m_shapeSizeDirty is not really what it sais it is. You are using it to recreate the shape when the CSS shape changes. I would rather remove that and instead use the "m_shape = 0;" as a flag that the shape needs to be recomputed. Also, I think the shape can be computed lazily. > > > > I don't quite follow why m_shapeSizeDirty isn't what it says it is. Isn't the whole point of a dirty flag to be able to know when you need to recompute? > m_shapeSizeDirty says that the size is dirty, but its value is only true when the "m_shape" is dirty and needs to be recreated. That same effect can be done using "m_shape = null". Is that clear enough now? Yeah. > > > Also, I agree that things could be done in a lazy fashion, but is it worth the increased complexity of the code to do so? > > It is as simple as checking that the shape != null when asking for the shape. It is not increasing the complexity, it just simplifies it. After sleeping on it, and reading your explanation of your reasoning on the dirty flag, I agree. > > > > > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:98 > > > > + BasicShape* shape = m_box->style()->shapeOutside(); > > > > + ASSERT(shape); > > > > + > > > > + m_shape = ExclusionShape::createExclusionShape(shape, logicalWidth, logicalHeight, m_box->style()->writingMode()); > > > > > > shape and m_shape have totally different types, but use the same name. I think m_shape can use a different name, something to tell that this is ready to be used (ie. computedShape?) > > > > Sure. I'll do m_computedShape and I'll rename "shape" to "basicShape" to make sure there can be no confusion. > Ok. > > > > > > > > > > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:99 > > > > + ASSERT(m_shape); > > > > > > This assert looks weird. If createExclusionShape is designed to return 0, then you need to treat that case. If the allocation fails, then it will crash before you get the assert, so I'm not sure what you are trying to catch here. > > > > This is another one I will have to ask Bear about. It looks to me like that can never happen, since as far as I can tell, it can only return 0 if the passed in BasicShape is null > Add a separate bug to remove that from createExclusionShape. Usually, methods are not checking for their arguments. At most, they just use an assert. Why would anyone call createExclusionShape with a null shape anyway? I agree that they shouldn't. But I've filed bug 100765 for it and will take a look at it later, since it is possible that there is some code somewhere that depends on that behavior. > > > , and we assert that it isn't on the line before. So I do think we can safely remove it. > Ok. > > > > > > > > > > Source/WebCore/rendering/RenderBlock.cpp:1453 > > > > + if (exclusionShapeOutsideInfo) > > > > + exclusionShapeInsideInfo->computeShapeSize(logicalWidth(), computedLogicalHeight); > > > > > > Image can also have outside shapes and they are not RenderBlocks. Should this be in the RenderBox instead? > > > > As far as I can tell, RenderBlock overrides RenderBox's layout() method, but never calls it. So I didn't see a reasonable place to put this call in RenderBox that would ensure it gets called. Is that understanding incorrect? > Look how the transforms are recalculated at the end of most ::layout functions in WebKit. Anyway, I assume that you don't need the size at this point. So, I would better remove the code completely and do the lazy shape calculation instead when you actually got the size. (that's why doing that simplifies the patch) > > The shape outside changes, it will invalidate the floater's layout anyway (assuming that RenderStyle:diff returns layout in the case of shape-outside, if not fix it in this patch). Moreover, if the width/height of the floater changes it will also be during the layout, so doing it later when you actually need it, would save you a lot of trouble keeping the shape up-to-date in different places. I had planned to deal with the RenderStyle::diff issue later in bug 100697, so unless you feel having to work properly in the case of style changes is integral to this patch, I'd rather fix that and write the tests as part of the patch for bug 100697. I will change it to be lazy and remove the extra code in the layout method in RenderBlock, since it isn't needed. > > > I figured I would have to revisit this for floated images, and probably should have filed a bug. > > > Also, there's a typo here, that really should be called in exclusionShapeOutsideInfo, not exclusionShapeInsideInfo. > Didn't see that, nice catch. > > > > > > BTW, you also recompute the size when you insert the float. Why is this needed here at all then? > > > > I'm actually pretty sure it isn't needed here at this point, I just neglected to delete it. > > > > > > > > > Source/WebCore/rendering/RenderBlock.cpp:3805 > > > > + logicalWidth = shapeOutside->shapeLogicalWidth(); > > > > > > Why are the margins ignored in this case? > > > > Because the spec says they are. > It looks weird, so I've started the discussion around that. > > > > > > > > > > Source/WebCore/rendering/RenderBlock.cpp:3986 > > > > + ExclusionShapeOutsideInfo* shapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); > > > > + if (shapeOutside) { > > > > + floatLogicalLeftOffset = 0; > > > > + floatLogicalTopOffset = 0; > > > > + } > > > > > > Why do you need to do this? I would argue that you just need to update the offsets, but not make them 0. You need to position the shape inside the bounds, so I would rather subtract the minX and minY of the exclusion shape instead. > > > > Because I've decided to split this up into two smaller patches, one for the size and one for positioning. I wanted to get one working before I started on the other because I was messing around with positioning as well and found that I was too far off in the weeds for the moment. I will address this in bug 100399. > I think this is related to the margins. > > > > > > > > > Source/WebCore/rendering/RenderBlock.cpp:4023 > > > > + if (shapeOutside) { > > > > + floatLogicalLeftOffset = 0; > > > > + floatLogicalTopOffset = 0; > > > > + } > > > > > > ditto > > > > see above. :-) > > > > > > > > > Source/WebCore/rendering/RenderBlock.cpp:4038 > > > > + floatLogicalHeight = shapeOutside->shapeLogicalHeight(); > > > > > > Why do you ignore the margins? > > > > Because the spec says so. > > > > > > > > > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored.html:33 > > > > + Requites Ahem font. Tests that the shape on a right float causes the margin on the float to have no effect on layout. > > > > > > I would like to keep margins for now and raise this as an issue on the spec instead. I think margins should affect the positioning relative to the other floats, but not the text. > > > > I think this is a much longer discussion, and would like to have it unrelated to this patch. I would like to have the patch implement what the spec says right now, and if after having a discussion about the spec, we decide that the behavior of margins should be different, then we can revise the code then. Is that agreeable? (feel free to start a www-style thread on the margins issue. :-) > > > > Thanks for the feedback!

Created attachment 171517 [details] Updated patch Updated patch for most recent feedback

Comment on attachment 171517 [details] Updated patch Attachment 171517 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14640629

Comment on attachment 171517 [details] Updated patch Attachment 171517 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14629652

Comment on attachment 171517 [details] Updated patch Attachment 171517 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14626722

Created attachment 171539 [details] Updated patch Updated patch, fix another Qt build issue

Comment on attachment 171539 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=171539&action=review > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:66 > + if (infoMap.contains(box)) > + return infoMap.get(box); This will make the search twice :) What about? if (ExclusionShapeOutsideInfo* shapeInfo = infoMap.get(box)) return shapeInfo; ExclusionShapeOutsideInfoMap::AddResult result = infoMap.add(box, create(box)); return result.iterator->value.get(); > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:88 > + if (!box->style() || !box->style()->shapeOutside()) > + return; I think it's safe to remove these checks as it's not doing the right thing when the style changes from a shape to none (you got a memory leak). > Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:60 > + ensureComputedShape(); > + return m_computedShape->shapeLogicalBoundingBox().x(); Why not make computedShape() do the creation magic for you? It will make the code nicer. return computedShape()->shapeLogicalBoundingBox().x() > Source/WebCore/rendering/RenderBlock.cpp:3798 > + ExclusionShapeOutsideInfo* shapeOutside = o->exclusionShapeOutsideInfo(); > + if (shapeOutside) { Add a comment saying that the specification defines that the margins are ignored. > Source/WebCore/rendering/RenderBlock.cpp:3978 > + ExclusionShapeOutsideInfo* shapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); > + if (shapeOutside) { I would repeat the comment here too, it's a different function. nit: Having the #ifdef here makes the code ugly. I wander if having a boolean outside the ifdef could simplify the code and make it breath a little. bool hasShapeOutside = false; #if ENABLE(CSS_EXCLUSIONS) ExclusionShapeOutsideInfo* shapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); hasShapeOutside = shapeOutside; #endif setLogicalLeftForChild(childBox, floatLogicalLocation.x() + (hasShapeOutside ? 0 : childLogicalLeftMargin); ... > Source/WebCore/rendering/RenderBox.cpp:140 > + ExclusionShapeOutsideInfo::removeExclusionShapeOutsideInfoForRenderBox(this); This line is too long. It really has the same information twice. I would really cut some of the keywords in the function name. > Source/WebCore/rendering/RenderBox.cpp:278 > + super nit: No need for this empty space. > Source/WebCore/rendering/RenderBox.cpp:284 > + // FIXME: A future optimization would do a deep comparison for equality. Why not now? It is just a line :) Anyway, all // FIXME: lines need to be followed by a different line with a link to bugzilla. > Source/WebCore/rendering/RenderBox.cpp:289 > + ExclusionShapeOutsideInfo* exclusionShapeOutsideInfo = ExclusionShapeOutsideInfo::ensureExclusionShapeOutsideInfoForRenderBox(this); ensureExclusionShapeOutsideInfoForRenderBox is really really long and that's because it doubles information from the class name. I know that you are following the ShapeInside, but that might need some tweaking too. What about ExclusionShapeOutsideInfo::ensureShapeInfoForRenderBox(this) instead?

Comment on attachment 171539 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=171539&action=review >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:66 >> + return infoMap.get(box); > > This will make the search twice :) > > What about? > if (ExclusionShapeOutsideInfo* shapeInfo = infoMap.get(box)) > return shapeInfo; > ExclusionShapeOutsideInfoMap::AddResult result = infoMap.add(box, create(box)); > return result.iterator->value.get(); It's a hash table lookup, that really should be really really fast, no? :-p I never much liked doing assignments in conditionals, it really smacks of trying to do too much in one statement. That being said, you're more of an authority on WebKit style than me, so I'll run with your suggestion. :-) >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:88 >> + return; > > I think it's safe to remove these checks as it's not doing the right thing when the style changes from a shape to none (you got a memory leak). Yeah, those checks are useless, it looks like HashMap::remove does the right thing (a no-op) if box isn't in the hash. >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:60 >> + return m_computedShape->shapeLogicalBoundingBox().x(); > > Why not make computedShape() do the creation magic for you? It will make the code nicer. > > return computedShape()->shapeLogicalBoundingBox().x() Sounds good to me. >> Source/WebCore/rendering/RenderBlock.cpp:3798 >> + if (shapeOutside) { > > Add a comment saying that the specification defines that the margins are ignored. ok >> Source/WebCore/rendering/RenderBlock.cpp:3978 >> + if (shapeOutside) { > > I would repeat the comment here too, it's a different function. > > nit: Having the #ifdef here makes the code ugly. I wander if having a boolean outside the ifdef could simplify the code and make it breath a little. > > bool hasShapeOutside = false; > #if ENABLE(CSS_EXCLUSIONS) > ExclusionShapeOutsideInfo* shapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); > hasShapeOutside = shapeOutside; > #endif > setLogicalLeftForChild(childBox, floatLogicalLocation.x() + (hasShapeOutside ? 0 : childLogicalLeftMargin); > ... Ok. >> Source/WebCore/rendering/RenderBox.cpp:140 >> + ExclusionShapeOutsideInfo::removeExclusionShapeOutsideInfoForRenderBox(this); > > This line is too long. It really has the same information twice. I would really cut some of the keywords in the function name. Ok, I've renamed all of the functions to be shorter, removing the redundant "ExclusionShapeOutside" from all of them. I left "Info" in because it makes them read better than without it. >> Source/WebCore/rendering/RenderBox.cpp:278 >> + > > super nit: No need for this empty space. deleted. >> Source/WebCore/rendering/RenderBox.cpp:284 >> + // FIXME: A future optimization would do a deep comparison for equality. > > Why not now? It is just a line :) > Anyway, all // FIXME: lines need to be followed by a different line with a link to bugzilla. Everything starts out as only one more line, and then next thing you know, you have a gigantic patch. Also, it's what the shape inside info was doing. :-) Bug 100811 created and added to the comment. >> Source/WebCore/rendering/RenderBox.cpp:289 >> + ExclusionShapeOutsideInfo* exclusionShapeOutsideInfo = ExclusionShapeOutsideInfo::ensureExclusionShapeOutsideInfoForRenderBox(this); > > ensureExclusionShapeOutsideInfoForRenderBox is really really long and that's because it doubles information from the class name. I know that you are following the ShapeInside, but that might need some tweaking too. > What about ExclusionShapeOutsideInfo::ensureShapeInfoForRenderBox(this) instead? I named it ExclusionShapeOutsideInfo::ensureInfoForRenderBox(this)

Created attachment 171583 [details] Updated patch Updated patch

Comment on attachment 171583 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=171583&action=review > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:36 > +#include "NotImplemented.h" Unneeded include. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:39 > +#include <wtf/OwnPtr.h> Ditto. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:40 > +#include <wtf/Vector.h> Ditto. > Source/WebCore/rendering/RenderBlock.cpp:3983 > + bool hasShapeOutside = false; > +#if ENABLE(CSS_EXCLUSIONS) > + ExclusionShapeOutsideInfo* shapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); > + hasShapeOutside = shapeOutside; > +#endif > + // The CSS Exclusions specification says that the margins are ignored > + // when a float has a shape outside. The current code could probably be simplified by caching + clearing some variable holding the margin values. > Source/WebCore/rendering/RenderBlock.cpp:4026 > + LayoutUnit floatLogicalHeight = logicalHeightForChild(childBox) + marginBeforeForChild(childBox) + marginAfterForChild(childBox); Shouldn't you also ignore the margins here? > Source/WebCore/rendering/RenderBox.cpp:141 > +#endif It really is a pita that we can't put the property into RenderBlock due to the floats.

Comment on attachment 171583 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=171583&action=review >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:36 >> +#include "NotImplemented.h" > > Unneeded include. I'll post an updated patch with these removed. >> Source/WebCore/rendering/RenderBlock.cpp:3983 >> + // when a float has a shape outside. > > The current code could probably be simplified by caching + clearing some variable holding the margin values. I was originally doing that, but it was suggested that doing it this way was clearer. Do you have a strong opinion on this? >> Source/WebCore/rendering/RenderBlock.cpp:4026 >> + LayoutUnit floatLogicalHeight = logicalHeightForChild(childBox) + marginBeforeForChild(childBox) + marginAfterForChild(childBox); > > Shouldn't you also ignore the margins here? I am, see the if (shapeOutside) below. The logical height is set from the variable. >> Source/WebCore/rendering/RenderBox.cpp:141 >> +#endif > > It really is a pita that we can't put the property into RenderBlock due to the floats. Yeah, I agree. :-)

Created attachment 174489 [details] Updated Patch Remove unused headers, update to reflect LayoutUnit refactoring.

Comment on attachment 174489 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174489&action=review > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:52 > + : m_box(box), > + m_logicalWidth(0), > + m_logicalHeight(0) Style violation, the comma should start a line, not end it. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:67 > + ExclusionShapeOutsideInfoMap& infoMap = exclusionShapeOutsideInfoMap(); > + if (ExclusionShapeOutsideInfo* shapeInfo = infoMap.get(box)) > + return shapeInfo; > + > + ExclusionShapeOutsideInfoMap::AddResult result = infoMap.add(box, create(box)); > + return result.iterator->value.get(); Note that this also involves 2 hash lookup if you don't have a cache miss. There is a pattern to do only one but it would be an overkill. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:78 > + // FIXME enable shape outside for non-rectangular shapes! (bug 98664) The usual style is: // FIXME: Enable shape outside for non-rectangular shapes! (bug 98664) Note the ':' after FIXME. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:45 > + WTF_MAKE_FAST_ALLOCATED; I think it should be WTF_MAKE_NONCOPYABLE too. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:82 > + if (m_logicalWidth != logicalWidth || m_logicalHeight != logicalHeight) { Early returns are preferred. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:83 > + dirtyShapeSize(); The pattern of basically free'ing and reallocating the computed shape seems really bad here. I would rather if we had a way of just recomputing the cached shape sizes. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:89 > + void dirtyShapeSize() { m_computedShape.clear(); } What does the dirtying and recomputing gives us? > Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:92 > + ExclusionShapeOutsideInfo(RenderBox*); explicit ExclusionShapeOutsideInfo(RenderBox*);? > Source/WebCore/rendering/RenderBlock.cpp:3801 > + LayoutUnit logicalWidth = logicalWidthForChild(o) + marginStartForChild(o) + marginEndForChild(o); > +#if ENABLE(CSS_EXCLUSIONS) > + ExclusionShapeOutsideInfo* shapeOutside = o->exclusionShapeOutsideInfo(); > + if (shapeOutside) { If you have a shapeOutside, you basically compute the margins for nothing. You could change this code to avoid that: if (shapeOutside) { .... setLogicalWidthForFloat(newObj, shapeOutside->shapeLogicalWidth()); } else #endif setLogicalWidthForFloat(newObj, logicalWidthForChild(o) + marginStartForChild(o) + marginEndForChild(o)); > Source/WebCore/rendering/RenderBlock.cpp:3983 > + ExclusionShapeOutsideInfo* shapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); > + hasShapeOutside = shapeOutside; hasShapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); > LayoutTests/ChangeLog:9 > + Tests for changing the height and width of a float with a rectangular > + shape outside. I don't understand what this means or what this adds to the whole. > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored-expected.html:26 > + <p> I would dump the bug number & title. > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored-expected.html:27 > + Requites Ahem font. Tests that the shape on a right float causes the margin on the float to have no effect on layout. Unneeded comment about Ahem: you are putting a 'font' declaration mentioning it, that should be sufficient. > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-rectangle-horizontal-larger.html:28 > + -webkit-shape-outside: rectangle(0px, -100px, 200px, 300px); I am concerned about this: it means that x / y / width / height are physical coordinates which seems very stupid and goes completely counter to the logical coordinates / directions as specified in css 3 writing modes. > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-rectangle-horizontal-larger.html:58 > + X X X X X X X X X X X X X X X I have tried understand this test but it's completely weird to me: using 31 characters in this test and allowing wrapping at every other characters (while naming this pseudo dotted line a line above). I don't know how to make it better as I can't see what it's trying to achieve: for that matter, it's never *stated* what you are testing.

Comment on attachment 174489 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174489&action=review >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:52 >> + m_logicalHeight(0) > > Style violation, the comma should start a line, not end it. Ok, will fix. >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:67 >> + return result.iterator->value.get(); > > Note that this also involves 2 hash lookup if you don't have a cache miss. There is a pattern to do only one but it would be an overkill. Do you mean that it involves 2 hash lookups when there *is* a cache miss? I can only see one lookup for a cache hit. If there are two lookups for a cache hit, can you tell me what I am missing? >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:78 >> + // FIXME enable shape outside for non-rectangular shapes! (bug 98664) > > The usual style is: > > // FIXME: Enable shape outside for non-rectangular shapes! (bug 98664) > > Note the ':' after FIXME. Ok, I'll fix it. >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:45 >> + WTF_MAKE_FAST_ALLOCATED; > > I think it should be WTF_MAKE_NONCOPYABLE too. Ok. Is there documentation (i.e. when to use them, etc) on these around somewhere? I haven't been able to find anything. >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:82 >> + if (m_logicalWidth != logicalWidth || m_logicalHeight != logicalHeight) { > > Early returns are preferred. Ok. >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:83 >> + dirtyShapeSize(); > > The pattern of basically free'ing and reallocating the computed shape seems really bad here. I would rather if we had a way of just recomputing the cached shape sizes. Can I address this in a future patch after Bug 100766 lands? That way, I can fix it for both the shape inside and shape outside case at once. >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:89 >> + void dirtyShapeSize() { m_computedShape.clear(); } > > What does the dirtying and recomputing gives us? Right now, it isn't needed, but it will be needed once I implement the ability to dynamically change the shape (Bug 100697). Then we will need to recompute. Since this class was based on the shape inside case, I kept that behavior even though I wasn't using it yet. >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:92 >> + ExclusionShapeOutsideInfo(RenderBox*); > > explicit ExclusionShapeOutsideInfo(RenderBox*);? I don't quite follow you here. Can you elaborate? >> Source/WebCore/rendering/RenderBlock.cpp:3801 >> + if (shapeOutside) { > > If you have a shapeOutside, you basically compute the margins for nothing. You could change this code to avoid that: > > if (shapeOutside) { > .... > setLogicalWidthForFloat(newObj, shapeOutside->shapeLogicalWidth()); > } else > #endif > setLogicalWidthForFloat(newObj, logicalWidthForChild(o) + marginStartForChild(o) + marginEndForChild(o)); I originally had that, but it was suggested by an informal review that I change it to the current implementation as to not have the ifdef around a if statement. I'm perfectly happy to change it back. In the case of a more complex else clause where the braces are required, do you prefer the current case where an unnecessary computation may happen, or a case like: #if if (shapeOutside) { ... } else { #endif ... #if } #endif >> Source/WebCore/rendering/RenderBlock.cpp:3983 >> + hasShapeOutside = shapeOutside; > > hasShapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo(); I feel foolish about that one. :-) >> LayoutTests/ChangeLog:9 >> + shape outside. > > I don't understand what this means or what this adds to the whole. I will give this some more thought and come up with clearer, better prose describing the tests. >> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored-expected.html:26 >> + <p> > > I would dump the bug number & title. Ok. >> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored-expected.html:27 >> + Requites Ahem font. Tests that the shape on a right float causes the margin on the float to have no effect on layout. > > Unneeded comment about Ahem: you are putting a 'font' declaration mentioning it, that should be sufficient. Ok. >> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-rectangle-horizontal-larger.html:28 >> + -webkit-shape-outside: rectangle(0px, -100px, 200px, 300px); > > I am concerned about this: it means that x / y / width / height are physical coordinates which seems very stupid and goes completely counter to the logical coordinates / directions as specified in css 3 writing modes. The current implementation for shape inside uses physical coordinates, and this uses the same approach. Can this be addressed outside of this patch? I will bring it up tomorrow and see if there is a rationale for the use of physical coordinates instead of logical for defining shapes. >> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-rectangle-horizontal-larger.html:58 >> + X X X X X X X X X X X X X X X > > I have tried understand this test but it's completely weird to me: using 31 characters in this test and allowing wrapping at every other characters (while naming this pseudo dotted line a line above). I don't know how to make it better as I can't see what it's trying to achieve: for that matter, it's never *stated* what you are testing. I am attempting to test that the inline layout respects the shape outside's bounds instead of the float's margin box bounds as it it would if there was no shape outside. (hopefully that makes sense?) I will revisit these tests and make them smaller and simpler. While you only commented on this test case, since all of the tests are in the same pattern, I'll change them all to make things clearer, and try to do a better job of explaining what the test is about.

Comment on attachment 174489 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174489&action=review >>> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-rectangle-horizontal-larger.html:28 >>> + -webkit-shape-outside: rectangle(0px, -100px, 200px, 300px); >> >> I am concerned about this: it means that x / y / width / height are physical coordinates which seems very stupid and goes completely counter to the logical coordinates / directions as specified in css 3 writing modes. > > The current implementation for shape inside uses physical coordinates, and this uses the same approach. Can this be addressed outside of this patch? I will bring it up tomorrow and see if there is a rationale for the use of physical coordinates instead of logical for defining shapes. A couple reasons for shapes to use physical coordinates instead of logical ones are: 1) When defining a block's dimensions one uses physical coordinates (top, left, width, height, etc). Since a shape needs to be sized in relation to the block it applies to, it makes sense for the shape to have physical coordinates. For example, in this rule here, it would be confusing if the width and height of the rectangle shape were dependent on the writing mode, while the width and height of the box are not. 2) A shape can affect more than just the element it is defined on (shape-inside affects children, shape-outside affects siblings), and these other elements could have different writing modes than the element that the shape is defined on.

Comment on attachment 174489 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174489&action=review >>> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:92 >>> + ExclusionShapeOutsideInfo(RenderBox*); >> >> explicit ExclusionShapeOutsideInfo(RenderBox*);? > > I don't quite follow you here. Can you elaborate? I was being dense. I get it now, and yes, that is what I want here.

Created attachment 177034 [details] Updated Patch Update for review comments.

Comment on attachment 177034 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177034&action=review > Source/WebCore/ChangeLog:16 > + Extra space. > Source/WebCore/ChangeLog:61 > + (WebCore::RenderBox::exclusionShapeOutsideInfo): Please add the details here. > Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:78 > + LayoutUnit shapeLogicalLeft() > + { > + return computedShape()->shapeLogicalBoundingBox().x(); > + } > + LayoutUnit shapeLogicalRight() > + { > + return computedShape()->shapeLogicalBoundingBox().maxX(); > + } > + LayoutUnit shapeLogicalTop() > + { > + return computedShape()->shapeLogicalBoundingBox().y(); > + } > + LayoutUnit shapeLogicalBottom() > + { > + return computedShape()->shapeLogicalBoundingBox().maxY(); > + } > + LayoutUnit shapeLogicalWidth() > + { > + return computedShape()->shapeLogicalBoundingBox().width(); > + } > + LayoutUnit shapeLogicalHeight() > + { > + return computedShape()->shapeLogicalBoundingBox().height(); > + } These functions could be const as you return a copy. To make this change, computedShape should be const and return a const which is fine as it is logically const. Hans mentioned this change and it's a good one AFAICT. > LayoutTests/ChangeLog:12 > + * fast/exclusions/resources/simple-rectangle-outside.js: Added. As a whole, the code sharing is small and this makes the test case a lot harder to follow. Sorry for hinting at the potential for code sharing and readable code but here it's not really a clear win. Would it possible to pull the old test cases with the test inside a single html? > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored-expected.html:16 > + -webkit-shape-outside: rectangle(0px, 0px, 20px, 20px); Usually you don't want expected results to depend on the feature you are trying to test and here you should be able to create a test case without any shape-outside. Ideally because it's layout based, a check-layout.js test could give us the same coverage but would be faster to run and easier to read for people.

Comment on attachment 177034 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177034&action=review >> Source/WebCore/ChangeLog:16 >> + > > Extra space. Ok >> Source/WebCore/ChangeLog:61 >> + (WebCore::RenderBox::exclusionShapeOutsideInfo): > > Please add the details here. Ok. >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:78 >> + } > > These functions could be const as you return a copy. To make this change, computedShape should be const and return a const which is fine as it is logically const. Hans mentioned this change and it's a good one AFAICT. Sure, works for me. >> LayoutTests/ChangeLog:12 >> + * fast/exclusions/resources/simple-rectangle-outside.js: Added. > > As a whole, the code sharing is small and this makes the test case a lot harder to follow. Sorry for hinting at the potential for code sharing and readable code but here it's not really a clear win. Would it possible to pull the old test cases with the test inside a single html? Yeah, that's pretty straightforward. I'll get rid of the JS generating the tests. >> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored-expected.html:16 >> + -webkit-shape-outside: rectangle(0px, 0px, 20px, 20px); > > Usually you don't want expected results to depend on the feature you are trying to test and here you should be able to create a test case without any shape-outside. > > Ideally because it's layout based, a check-layout.js test could give us the same coverage but would be faster to run and easier to read for people. Oops. I made a mistake when converting this test and destroyed the proper expectation. I've fixed that, but I'm also going to look into converting it into a check-layout.js test. I've looked into the possibility of writing a JavaScript test for this instead of a reftest, and it seems to me like it would be much less clear. In the current set of tests, I'm testing to make sure that the inline text wraps properly, not that the float is positioned properly. So in order to write a JavaScript test, I would have to check that the text is positioned in the proper location, probably by wrapping each line in a span just to be able to get the dimensions, and with all those contortions, it seems to me that the test would be much less clear as to what is happening. When I do the patch for Bug 100399, I will write the tests for the float positioning code as JS tests.

Comment on attachment 177034 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177034&action=review >> Source/WebCore/ChangeLog:16 >> + > > Extra space. Ok >> Source/WebCore/ChangeLog:61 >> + (WebCore::RenderBox::exclusionShapeOutsideInfo): > > Please add the details here. Ok. >> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:78 >> + } > > These functions could be const as you return a copy. To make this change, computedShape should be const and return a const which is fine as it is logically const. Hans mentioned this change and it's a good one AFAICT. Sure, works for me. >> LayoutTests/ChangeLog:12 >> + * fast/exclusions/resources/simple-rectangle-outside.js: Added. > > As a whole, the code sharing is small and this makes the test case a lot harder to follow. Sorry for hinting at the potential for code sharing and readable code but here it's not really a clear win. Would it possible to pull the old test cases with the test inside a single html? Yeah, that's pretty straightforward. I'll get rid of the JS generating the tests. >> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is-ignored-expected.html:16 >> + -webkit-shape-outside: rectangle(0px, 0px, 20px, 20px); > > Usually you don't want expected results to depend on the feature you are trying to test and here you should be able to create a test case without any shape-outside. > > Ideally because it's layout based, a check-layout.js test could give us the same coverage but would be faster to run and easier to read for people. Oops. I made a mistake when converting this test and destroyed the proper expectation. I've fixed that, but I'm also going to look into converting it into a check-layout.js test. I've looked into the possibility of writing a JavaScript test for this instead of a reftest, and it seems to me like it would be much less clear. In the current set of tests, I'm testing to make sure that the inline text wraps properly, not that the float is positioned properly. So in order to write a JavaScript test, I would have to check that the text is positioned in the proper location, probably by wrapping each line in a span just to be able to get the dimensions, and with all those contortions, it seems to me that the test would be much less clear as to what is happening. When I do the patch for Bug 100399, I will write the tests for the float positioning code as JS tests.

Created attachment 178027 [details] Updated Patch Update for review comments.

Comment on attachment 178027 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178027&action=review Thanks for investigating the check-layout.js possibility, I am fine with ref-tests but have some questions / comments about the tests. I am r- on the confusing behavior where the shape-outside is fully contained into the float's content box, yet we allow another float to intrude into the content box. > Source/WebCore/ChangeLog:29 > + (WebCore): Associates the ExclusionShape object for shape outside with a RenderBox. Analagous to The (WebCore): entries should be removed. Our tools are confused unfortunately. > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-rectangle-expected.html:8 > + <style> > + .test { > + font: 10px/1 Ahem, sans-serif; > + border: 1px solid black; > + } The indentation is wrong, it looks like you have some tabs in this file (which means that patch will be rejected by the svn commit-hook). > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-rectangle-horizontal-multiple.html:77 > + <h2>These tests show that floats respect the shape outside on other floats. Each test should have a red square centered in a larger rectangle.</h2> Please don't use red as a way of meaning 'success'. Red should be reserved for failure (see http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree and more importantly http://wiki.csswg.org/test/format) > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-rectangle-horizontal-multiple.html:80 > + <div class="float-right-exclusion"></div> > + <div class="float-right"></div> For float-right to be centered, it should intrude 10px into float-right-exclusion. As discussed, this is confusing and I don't really see why this can be right. > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-rectangle-percentage.html:35 > + height: 66.66666666%; > + width: 66.66666666%; That's a pretty bad choice for a percent as it will be impacted by any rounding errors. Using 50px and 50% is better (or whichever configuration you prefer as long as it's a round number).

Created attachment 179361 [details] Updated Patch Update tests. Need to talk more about overlapping floats.

Comment on attachment 179361 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179361&action=review I got an explaination why the tests are fine: shape-inside defaults to shape-outside. That's what confused me and there is a good use case for that so it shouldn't block the patch. > Source/WebCore/rendering/RenderBlock.cpp:3933 > + // The CSS Exclusions specification says that the margins are ignored > + // when a float has a shape outside. Nit: No wrapping. > Source/WebCore/rendering/RenderBlock.cpp:3965 > + // The CSS Exclusions specification says that the margins are ignored > + // when a float has a shape outside. Ditto.

Created attachment 179773 [details] Updated patch Fixed the comments to not have wrapping.

Comment on attachment 179773 [details] Updated patch Set cq+ since it has been reviewed by Julien already.

Comment on attachment 179773 [details] Updated patch Rejecting attachment 179773 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/15361851

Created attachment 179775 [details] Updated patch Add reviewer to LayoutTests changelog

Comment on attachment 179775 [details] Updated patch Clearing flags on attachment: 179775 Committed r137930: <http://trac.webkit.org/changeset/137930>

All reviewed patches have been landed. Closing bug.