Bug 100766 - [CSS Exclusions] Refactor ExclusionShapeInsideInfo to more general ExclusionShapeInfo
Summary: [CSS Exclusions] Refactor ExclusionShapeInsideInfo to more general ExclusionS...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bear Travis
URL:
Keywords:
: 100976 (view as bug list)
Depends on:
Blocks: 89256 100398
  Show dependency treegraph
 
Reported: 2012-10-30 09:37 PDT by Bem Jones-Bey
Modified: 2013-03-01 15:09 PST (History)
7 users (show)

See Also:


Attachments
Initial patch (45.30 KB, patch)
2012-11-13 14:16 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Simplifying xcode project (41.64 KB, patch)
2012-11-13 14:31 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Updating patch (40.25 KB, patch)
2012-11-14 14:48 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Factoring out code that does not require a template (42.15 KB, patch)
2012-11-14 16:11 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Updating patch (42.15 KB, patch)
2012-11-15 10:49 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Updating to current ExclusionShapeInside/OutsideInfo code (39.80 KB, patch)
2013-01-24 18:08 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Updating patch (38.25 KB, patch)
2013-01-25 11:14 PST, Bear Travis
krit: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fixing changelog and some whitespace issues (38.30 KB, patch)
2013-01-25 17:06 PST, Bear Travis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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. ***