Bug 109330 – [CSS Filters] Refactor filter outsets into a class

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Max Vujovic]

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.

[Max Vujovic]

Created attachment 187389 [details]
Patch for Review

[Early Warning System Bot]

Comment on attachment 187389 [details]
Patch for Review

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

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Max Vujovic]

Created attachment 187618 [details]
Patch for Review

[Max Vujovic]

Comment on attachment 187618 [details]
Patch for Review

Bots are looking good, setting r?.

[Dean Jackson]

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.

[Max Vujovic]

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 :)

[Dean Jackson]

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.

[Max Vujovic]

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.

[Max Vujovic]

Comment on attachment 188177 [details]
Patch for Landing

Bots are looking good. Setting cq+.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 188177 [details]
Patch for Landing

Clearing flags on attachment: 188177

Committed r142823: <http://trac.webkit.org/changeset/142823>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.