Refactor RenderBox::addVisualEffectOverflow()

Refactor RenderBox::addVisualEffectOverflow()

Extracted computeVisualEffectOverflow(). It will be used in later
changes about invalidation of visual effect overflow.

BUG=380472
TEST=none (refactor only)

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177368

[Xianzhu, 2014-06-26 23:29:31]

This change depends on https://codereview.chromium.org/347393007/. Actually this
CL will overwrite all changes of that CL.

Hope this would be more convenient for your review. If you'd like, I can also
put them back together.

[Xianzhu, 2014-07-01 17:15:05]

ping...

[Julien - ping for review, 2014-07-01 23:00:16]

lgtm

Don't we have a master bug for this changes? If so, we should use BUG= to link
to it.

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4074: LayoutRect visualEffectOverflow =
borderBoxRect();
We have a helper function for "no overflow" that should be used here:
noOverflowRect()

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4081: if (!hasVisualEffectOverflow())
Nobody calls this without a visual effect overflow so we should just have an
ASSERT.

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4089: style()->getBoxShadowExtent(top,
right, bottom, left);
While it's a no-op to call getBoxShadowExtent without box-shadow, I would rather
have it gated by style->boxShadow() for consistency.

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4093: left = -left;
I wonder if we couldn't make the code more consistent here (LayoutBoxExtent is
an extent for which negative values would mean intruding IMO).

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:228: bool hasVisualEffectOverflow() const {
return style()->boxShadow() || style()->hasBorderImageOutsets() ||
style()->hasOutline(); }
https://codereview.chromium.org/204843002 landed which adds this function to
RenderStyle (where it belongs) so you will need to rebaseline the change.

[Xianzhu, 2014-07-02 00:02:04]

Added bug reference.

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4074: LayoutRect visualEffectOverflow =
borderBoxRect();
On 2014/07/01 23:00:16, Julien Chaffraix - PST wrote:
> We have a helper function for "no overflow" that should be used here:
> noOverflowRect()

noOverflowRect seems incorrect to me:
- the current code always uses borderBoxRect as the default value and starting
point of visualOverflowRect;
- noOverflowRect is in content coordinates, while borderBoxRect's location is
always (0,0);
- noOverflowRect flips according to content writing-mode, while borderBoxRect
doesn't.

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4081: if (!hasVisualEffectOverflow())
On 2014/07/01 23:00:15, Julien Chaffraix - PST wrote:
> Nobody calls this without a visual effect overflow so we should just have an
> ASSERT.

Done.

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4089: style()->getBoxShadowExtent(top,
right, bottom, left);
On 2014/07/01 23:00:16, Julien Chaffraix - PST wrote:
> While it's a no-op to call getBoxShadowExtent without box-shadow, I would
rather
> have it gated by style->boxShadow() for consistency.

Done.

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4093: left = -left;
On 2014/07/01 23:00:15, Julien Chaffraix - PST wrote:
> I wonder if we couldn't make the code more consistent here (LayoutBoxExtent is
> an extent for which negative values would mean intruding IMO).

I tried that several days ago but haven't finished yet because it needs much
care not to bring bug into existing code, especially in areas that I'm not
familiar with.

May continue when have time :)

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:228: bool hasVisualEffectOverflow() const {
return style()->boxShadow() || style()->hasBorderImageOutsets() ||
style()->hasOutline(); }
On 2014/07/01 23:00:16, Julien Chaffraix - PST wrote:
> https://codereview.chromium.org/204843002 landed which adds this function to
> RenderStyle (where it belongs) so you will need to rebaseline the change.

Done.

[Xianzhu, 2014-07-02 00:05:23]

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/337473008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:4074: LayoutRect visualEffectOverflow =
borderBoxRect();
On 2014/07/02 00:02:03, Xianzhu wrote:
> - noOverflowRect is in content coordinates

Correction: noOverflowRect is context-box in border-box coordinates, so it is in
the same coordinates as borderBoxRect, but the sizes of them are different.

[Xianzhu, 2014-07-02 03:48:02]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-07-02 03:48:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/337473008/40001

[commit-bot: I haz the power, 2014-07-02 12:55:37]

Change committed as 177368