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.
Created attachment 187389 [details] Patch for Review
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 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 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 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 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 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
Created attachment 187618 [details] Patch for Review
Comment on attachment 187618 [details] Patch for Review Bots are looking good, setting r?.
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 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 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.
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 on attachment 188177 [details] Patch for Landing Bots are looking good. Setting cq+.
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 on attachment 188177 [details] Patch for Landing Clearing flags on attachment: 188177 Committed r142823: <http://trac.webkit.org/changeset/142823>
All reviewed patches have been landed. Closing bug.