Summary: | [CSS Filters] Refactor filter outsets into a class | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Max Vujovic <mvujovic> | ||||||||
Component: | Layout and Rendering | Assignee: | 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
Max Vujovic
2013-02-08 15:51:06 PST
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. |