Bug 100766

Summary: [CSS Exclusions] Refactor ExclusionShapeInsideInfo to more general ExclusionShapeInfo
Product: WebKit Reporter: Bem Jones-Bey <bjonesbe>
Component: CSSAssignee: Bear Travis <betravis>
Status: RESOLVED FIXED    
Severity: Normal CC: betravis, dglazkov, eric, gyuyoung.kim, ojan.autocc, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 89256, 100398    
Attachments:
Description Flags
Initial patch
none
Simplifying xcode project
none
Updating patch
none
Factoring out code that does not require a template
none
Updating patch
none
Updating to current ExclusionShapeInside/OutsideInfo code
none
Updating patch
krit: review+, webkit.review.bot: commit-queue-
Fixing changelog and some whitespace issues none

Description Bem Jones-Bey 2012-10-30 09:37:41 PDT
Create a common superclass, and also there are fixes in ExclusionShapeOutsideInfo that may need to be copied to ExclusionShapeInsideInfo. See bug 100398 for a bit more info.
Comment 1 Bear Travis 2012-11-13 14:16:50 PST
Created attachment 173986 [details]
Initial patch
Comment 2 Bear Travis 2012-11-13 14:31:23 PST
Created attachment 173989 [details]
Simplifying xcode project
Comment 3 Bem Jones-Bey 2012-11-13 19:21:01 PST
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?
Comment 4 Bear Travis 2012-11-14 14:48:07 PST
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 5 Bem Jones-Bey 2012-11-14 14:57:51 PST
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?
Comment 6 Bear Travis 2012-11-14 16:11:55 PST
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 7 WebKit Review Bot 2012-11-14 17:08:52 PST
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
Comment 8 Bear Travis 2012-11-15 10:49:53 PST
Created attachment 174488 [details]
Updating patch
Comment 9 Bear Travis 2013-01-24 18:08:47 PST
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.
Comment 10 Bem Jones-Bey 2013-01-25 09:26:40 PST
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.
Comment 11 Bear Travis 2013-01-25 11:14:20 PST
Created attachment 184777 [details]
Updating patch

Removing some extraneous diffs, and running by the bots again.
Comment 12 Dirk Schulze 2013-01-25 15:10:07 PST
Comment on attachment 184777 [details]
Updating patch

LGTM. r=me
Comment 13 WebKit Review Bot 2013-01-25 15:22:56 PST
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
Comment 14 Bear Travis 2013-01-25 17:06:43 PST
Created attachment 184841 [details]
Fixing changelog and some whitespace issues

Accidentally removed the blank reviewer line.
Comment 15 WebKit Review Bot 2013-01-28 10:38:01 PST
Comment on attachment 184841 [details]
Fixing changelog and some whitespace issues

Clearing flags on attachment: 184841

Committed r140978: <http://trac.webkit.org/changeset/140978>
Comment 16 WebKit Review Bot 2013-01-28 10:38:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Bear Travis 2013-03-01 15:09:02 PST
*** Bug 100976 has been marked as a duplicate of this bug. ***