Bug 100766

Created attachment 173986 [details] Initial patch

Created attachment 173989 [details] Simplifying xcode project

Comment on attachment 173989 [details] Simplifying xcode project View in context: https://bugs.webkit.org/attachment.cgi?id=173989&action=review > Source/WebCore/rendering/ExclusionShapeInfo.cpp:55 > + return shape && (shape->type() == BasicShape::BASIC_SHAPE_RECTANGLE || shape->type() == BasicShape::BASIC_SHAPE_POLYGON); This check against the type() should be different for shapeInside and shapeOutside (for example, shapeOutside only supports rectangles at the moment). > Source/WebCore/rendering/ExclusionShapeInfo.cpp:98 > +BasicShape* ExclusionShapeInfo<RenderBox, CSSPropertyWebkitShapeOutside>::basicShapeForRenderer(const RenderBox* renderer) { return renderer->style()->shapeOutside(); } It seems to me like this would be a lot clearer if we did this with subclasses (ExclusionShapeInsideInfo, ExclusionShapeOutsideInfo) and overriding instead of overloading, but I guess that you're doing it this way for efficiency?

Created attachment 174266 [details] Updating patch Attempting to simplify out the static helper methods, and remove as many repetitions of the template setup as possible.

Comment on attachment 174266 [details] Updating patch View in context: https://bugs.webkit.org/attachment.cgi?id=174266&action=review > Source/WebCore/rendering/ExclusionShapeInfo.h:57 > + return shape && (shape->type() == BasicShape::BASIC_SHAPE_RECTANGLE || shape->type() == BasicShape::BASIC_SHAPE_POLYGON); I'm still concerned that this line needs to be different in the shapeInside and shapeOutside case. How are we going to handle that?

Created attachment 174280 [details] Factoring out code that does not require a template Created ExclusionShapeInfoPartial class with code that does not need to be templatized. Also slightly altering the isInfoEnabledForRenderer method so that it can be specialized for ShapeOutside.

Comment on attachment 174280 [details] Factoring out code that does not require a template Attachment 174280 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14846165 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

Created attachment 174488 [details] Updating patch

Created attachment 184625 [details] Updating to current ExclusionShapeInside/OutsideInfo code Now that ExclusionShapeOutsideInfo is also present, the patch needed to be updated. There is also the possibility of moving ExclusionShapeInsideInfo into RenderBlockRareData, so I have simplified the MappedInfo class for now.

I like it. Just FYI, in my patch for Bug 106927, I have added a method (shapeLogicalOffset) to ExclusiomShapeOutsideInfo. I don't think it makes sense for Shape Inside, so it'll probably stay in ExclusionShapeOutsideInfo.

Created attachment 184777 [details] Updating patch Removing some extraneous diffs, and running by the bots again.

Comment on attachment 184777 [details] Updating patch LGTM. r=me

Comment on attachment 184777 [details] Updating patch Rejecting attachment 184777 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'validate-changelog', '--non-interactive', 184777, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/16121573

Created attachment 184841 [details] Fixing changelog and some whitespace issues Accidentally removed the blank reviewer line.

Comment on attachment 184841 [details] Fixing changelog and some whitespace issues Clearing flags on attachment: 184841 Committed r140978: <http://trac.webkit.org/changeset/140978>

All reviewed patches have been landed. Closing bug.

*** Bug 100976 has been marked as a duplicate of this bug. ***