Bug 109330

Summary: [CSS Filters] Refactor filter outsets into a class
Product: WebKit Reporter: Max Vujovic <mvujovic>
Component: Layout and RenderingAssignee: Max Vujovic <mvujovic>
Status: RESOLVED FIXED    
Severity: Normal CC: achicu, buildbot, cmarcelo, dglazkov, dino, eric, luiz, michelangelo, noam, ojan.autocc, peter+ews, philn, rniwa, simon.fraser, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 109098    
Attachments:
Description Flags
Patch for Review
mvujovic: commit-queue-
Patch for Review
dino: review+, mvujovic: commit-queue-
Patch for Landing none

Description Max Vujovic 2013-02-08 15:51:06 PST
In filters related code, we're often operating on 4 ints representing the top, right, bottom, and left outsets from a filter like blur or drop-shadow.

Here are some signs that we should make a class to hold those 4 ints:
1) In RenderLayer.cpp, we have a expandRectForFilterOutsets function, which looks like feature envy.
2) RenderStyle (and other classes) have getFilterOutsets methods which set 4 ints by reference. The calling code has to define 4 ints, which looks bloated. 
3) To fix bug 109098, we will need to check if filter outsets changed, which sounds like a nice job for an inequality operator. 

If we had a class for filter outsets called IntRectOutsets, for example, we could clean all these situations up. LayoutBoxExtent is somewhat similar to the proposed class, but it's more catered to layout concepts like logical vs. physical dimensions, and it holds LayoutUnits instead of ints.
Comment 1 Max Vujovic 2013-02-08 17:13:29 PST
Created attachment 187389 [details]
Patch for Review
Comment 2 Early Warning System Bot 2013-02-08 17:20:38 PST
Comment on attachment 187389 [details]
Patch for Review

Attachment 187389 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16431879
Comment 3 Early Warning System Bot 2013-02-08 17:23:00 PST
Comment on attachment 187389 [details]
Patch for Review

Attachment 187389 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16443909
Comment 4 WebKit Review Bot 2013-02-08 17:49:36 PST
Comment on attachment 187389 [details]
Patch for Review

Attachment 187389 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16466023
Comment 5 Peter Beverloo (cr-android ews) 2013-02-08 18:03:06 PST
Comment on attachment 187389 [details]
Patch for Review

Attachment 187389 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16428946
Comment 6 WebKit Review Bot 2013-02-08 23:23:35 PST
Comment on attachment 187389 [details]
Patch for Review

Attachment 187389 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16476100
Comment 7 Build Bot 2013-02-09 14:24:41 PST
Comment on attachment 187389 [details]
Patch for Review

Attachment 187389 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16469565

New failing tests:
compositing/checkerboard.html
accessibility/anchor-linked-anonymous-block-crash.html
compositing/absolute-inside-out-of-view-fixed.html
animations/3d/matrix-transform-type-animation.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/cache/history-only-cached-subresource-loads.html
compositing/bounds-in-flipped-writing-mode.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/iframe-304-crash.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
Comment 8 Max Vujovic 2013-02-11 11:09:37 PST
Created attachment 187618 [details]
Patch for Review
Comment 9 Max Vujovic 2013-02-11 14:03:11 PST
Comment on attachment 187618 [details]
Patch for Review

Bots are looking good, setting r?.
Comment 10 Dean Jackson 2013-02-12 10:32:28 PST
Comment on attachment 187618 [details]
Patch for Review

View in context: https://bugs.webkit.org/attachment.cgi?id=187618&action=review

> Source/WebCore/platform/graphics/IntRectOutsets.h:16
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDER âAS ISâ AND ANY

Not sure if this is a error in the review tool or the patch (the cruft around AS IS)

> Source/WebCore/platform/graphics/IntRectOutsets.h:39
> +class IntRectOutsets {

I wonder if this should be called OutsetsIntRect? Our other rect classes are of the form <Type>Rect (e.g. IntRect) and <Usecase>Rect (e.g. LayoutRect). So it makes sense to be called <Usecase><Type>Rect.
Comment 11 Max Vujovic 2013-02-12 12:02:40 PST
Comment on attachment 187618 [details]
Patch for Review

Thanks very much for the review, Dean!

View in context: https://bugs.webkit.org/attachment.cgi?id=187618&action=review

>> Source/WebCore/platform/graphics/IntRectOutsets.h:16
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDER âAS ISâ AND ANY
> 
> Not sure if this is a error in the review tool or the patch (the cruft around AS IS)

I think it's the review tool. Those are actually slanted quotes. They look fine in my text editor (Sublime Text 2) and the patch looks fine when viewed in the browser as plain text. I copy/pasted this header from some other file already in WebKit.

>> Source/WebCore/platform/graphics/IntRectOutsets.h:39
>> +class IntRectOutsets {
> 
> I wonder if this should be called OutsetsIntRect? Our other rect classes are of the form <Type>Rect (e.g. IntRect) and <Usecase>Rect (e.g. LayoutRect). So it makes sense to be called <Usecase><Type>Rect.

I see your point. However, I'm concerned that "OutsetsIntRect" sounds like it represents a rect instead of the outsets of a rect. I suppose one could think of the outsets as a rect, though.

To me, outsets represent the modification of a rect. Outsets are the same concept as margins or padding.

For RenderStyle margins, we use "LengthBox". I don't especially like this name because it implies an actual box, not a modification of a box. However, if one interprets "box" as referring to the CSS box model, it can make sense.
In other code, we use "LayoutBoxExtent". I like this name because it's implying a modification of a box.

We use the terms "outsets" and "extent" somewhat interchangeably in the codebase. We haven't used "outsets" in a class name, though. For example:

RenderBox.cpp:4014: 
LayoutBoxExtent borderOutsets = style()->borderImageOutsets();

I like the name "IntRectExtent" best. This one implies the modification of a rect, and it looks like LayoutBoxExtent.

Some other ideas I had were:
IntBoxExtent - This one implies it's the extent of an IntBox, but we don't have IntBoxes. We only have IntRects.
IntRectOutsets - This one uses the term "outsets", which we haven't used in class names yet.

Whichever name we choose, I can typedef the name to FilterOutsets.

What do you think? I'm fine with any choice, including your suggestion, since they all make sense in their own way :)
Comment 12 Dean Jackson 2013-02-12 14:46:53 PST
Comment on attachment 187618 [details]
Patch for Review

View in context: https://bugs.webkit.org/attachment.cgi?id=187618&action=review

>>> Source/WebCore/platform/graphics/IntRectOutsets.h:39
>>> +class IntRectOutsets {
>> 
>> I wonder if this should be called OutsetsIntRect? Our other rect classes are of the form <Type>Rect (e.g. IntRect) and <Usecase>Rect (e.g. LayoutRect). So it makes sense to be called <Usecase><Type>Rect.
> 
> I see your point. However, I'm concerned that "OutsetsIntRect" sounds like it represents a rect instead of the outsets of a rect. I suppose one could think of the outsets as a rect, though.
> 
> To me, outsets represent the modification of a rect. Outsets are the same concept as margins or padding.
> 
> For RenderStyle margins, we use "LengthBox". I don't especially like this name because it implies an actual box, not a modification of a box. However, if one interprets "box" as referring to the CSS box model, it can make sense.
> In other code, we use "LayoutBoxExtent". I like this name because it's implying a modification of a box.
> 
> We use the terms "outsets" and "extent" somewhat interchangeably in the codebase. We haven't used "outsets" in a class name, though. For example:
> 
> RenderBox.cpp:4014: 
> LayoutBoxExtent borderOutsets = style()->borderImageOutsets();
> 
> I like the name "IntRectExtent" best. This one implies the modification of a rect, and it looks like LayoutBoxExtent.
> 
> Some other ideas I had were:
> IntBoxExtent - This one implies it's the extent of an IntBox, but we don't have IntBoxes. We only have IntRects.
> IntRectOutsets - This one uses the term "outsets", which we haven't used in class names yet.
> 
> Whichever name we choose, I can typedef the name to FilterOutsets.
> 
> What do you think? I'm fine with any choice, including your suggestion, since they all make sense in their own way :)

I don't have a strong opinion. IntRectExtent is fine.
Comment 13 Max Vujovic 2013-02-13 14:09:02 PST
Created attachment 188177 [details]
Patch for Landing

Renamed IntRectOutsets to IntRectExtent. Created a FilterOutsets typedef. Running the bots again to make sure the refactor went ok.
Comment 14 Max Vujovic 2013-02-13 15:36:55 PST
Comment on attachment 188177 [details]
Patch for Landing

Bots are looking good. Setting cq+.
Comment 15 WebKit Review Bot 2013-02-13 17:01:03 PST
Comment on attachment 188177 [details]
Patch for Landing

Rejecting attachment 188177 [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', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 188177, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 13634 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
53>At revision 13634.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/16536358
Comment 16 WebKit Review Bot 2013-02-13 17:01:08 PST
Comment on attachment 188177 [details]
Patch for Landing

Clearing flags on attachment: 188177

Committed r142823: <http://trac.webkit.org/changeset/142823>
Comment 17 WebKit Review Bot 2013-02-13 17:01:15 PST
All reviewed patches have been landed.  Closing bug.