Remove clips where we can show that they are unnecessary.

Remove block content clips where we can show that they are unnecessary.

This CL:
1. Modifies RenderBlock::computeOverflow to track, in addition to the visual overflow of the element itself, a bounding rect for the visual overflow of its contents. All call sites of addVisualOverflow are modified to declare whether the visual overflow is affected by the contents clip or not.
2. Modifies RenderBox::pushContentsClip to use this rect to determine whether the contents are visually contained by the clip, in which case it omits the clip.

See measurements in my comment #3 on the bug: https://code.google.com/p/chromium/issues/detail?id=238732#c3

BUG=238732
R=jchaffraix@chromium.org

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

[jbroman, 2013-06-18 13:44:19]

ping for review

[Julien - ping for review, 2013-06-19 19:42:58]

https://codereview.chromium.org/16402019/diff/33001/Source/core/platform/grap...
File Source/core/platform/graphics/RoundedRect.cpp (right):

https://codereview.chromium.org/16402019/diff/33001/Source/core/platform/grap...
Source/core/platform/graphics/RoundedRect.cpp:83: IntRect
RoundedRect::enclosedRect() const
This will return a smaller rectangle that what enclosed rect would be expected
to return AFAICT. If you look at
http://www.w3.org/TR/css3-background/#border-radius, you are returning the rect
formed by all the ellipses centers.

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1767: LayoutRect rectToApply =
LayoutRect(clientRect.x() + min<LayoutUnit>(0, textIndent), clientRect.y(),
clientRect.width() - min<LayoutUnit>(0, textIndent), clientRect.height());
Removing the min seems wrong: textIndent is negative in that branch.

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1815: if
(positionedObject->style()->position() != FixedPosition) {
I am not sure I see the point of this change: you are basically still ignoring
fixed positioned objects in addOverflowFromChild as you explicitly ignore them
for layout overflow and they have a self-painting layer. However to arrive at
the same result, you do a lot more computations.

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1720:
m_rareData->m_contentsVisualOverflowRect.uniteIfNonZero(rect);
Why isn't this in RenderOverflow as it is its reason to be?

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1749:
m_rareData->m_contentsVisualOverflowRect = LayoutRect();
You are now forcing all RenderBlocks with an overflow clip to have a rare data
as computeOverflow is called for each layout. And that is regardless of whether
you will have a need for it.

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:2918: }
This should probably be shared with the logic in RenderBlock::paintCaret as they
are very similar.

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.h:191: virtual void addVisualOverflow(const
LayoutRect&, VisualOverflowClipBehavior);
Making this virtual seems to go in the wrong direction. This is required mainly
because you are storing the new rect in RenderBlock where any RenderBox can have
an overflow clip but not all are blocks (e.g. RenderListMarkers).

[jbroman, 2013-08-02 14:28:16]

PTAL.

Major refactoring, moving this to RenderOverflow and trimming the CL to be
smaller and cleaner. Made possible after layout overflow tracking was fixed.

https://codereview.chromium.org/16402019/diff/33001/Source/core/platform/grap...
File Source/core/platform/graphics/RoundedRect.cpp (right):

https://codereview.chromium.org/16402019/diff/33001/Source/core/platform/grap...
Source/core/platform/graphics/RoundedRect.cpp:83: IntRect
RoundedRect::enclosedRect() const
On 2013/06/19 19:42:58, Julien Chaffraix wrote:
> This will return a smaller rectangle that what enclosed rect would be expected
> to return AFAICT. If you look at
> http://www.w3.org/TR/css3-background/#border-radius, you are returning the
rect
> formed by all the ellipses centers.

What I want is "quickly give me a rect which is enclosed by this rounded rect"
(the idea being to be able to say that if some geometry is enclosed by
rrect.enclosedRect(), then it is enclosed by rrect).

Using the ellipse centres seemed like a reasonable way of computing such a rect.
In the common case, the radii are all a few pixels and the returned rect is just
inside the outer rect (i.e. the one returned by RoundedRect::rect).

What do you suggest? A name change? A comment?

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1767: LayoutRect rectToApply =
LayoutRect(clientRect.x() + min<LayoutUnit>(0, textIndent), clientRect.y(),
clientRect.width() - min<LayoutUnit>(0, textIndent), clientRect.height());
On 2013/06/19 19:42:58, Julien Chaffraix wrote:
> Removing the min seems wrong: textIndent is negative in that branch.

min returns the lesser of its arguments. If textIndent < 0, then the lesser of 0
and textIndent is necessarily 0, no?

That said, removing it from this CL to make the diff nicer.

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1815: if
(positionedObject->style()->position() != FixedPosition) {
On 2013/06/19 19:42:58, Julien Chaffraix wrote:
> I am not sure I see the point of this change: you are basically still ignoring
> fixed positioned objects in addOverflowFromChild as you explicitly ignore them
> for layout overflow and they have a self-painting layer. However to arrive at
> the same result, you do a lot more computations.

Good point. Reverted.

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1720:
m_rareData->m_contentsVisualOverflowRect.uniteIfNonZero(rect);
On 2013/06/19 19:42:58, Julien Chaffraix wrote:
> Why isn't this in RenderOverflow as it is its reason to be?

Moved as requested.

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1749:
m_rareData->m_contentsVisualOverflowRect = LayoutRect();
On 2013/06/19 19:42:58, Julien Chaffraix wrote:
> You are now forcing all RenderBlocks with an overflow clip to have a rare data
> as computeOverflow is called for each layout. And that is regardless of
whether
> you will have a need for it.

Changed to RenderOverflow. Note that in practice most elements with overflow
clips will have *some*  contents and will get a RenderOverflow to store their
contents visual bounding rect anyhow. This could be possibly avoided for
"overflow: hidden" but not for "overflow: scroll".

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:2918: }
On 2013/06/19 19:42:58, Julien Chaffraix wrote:
> This should probably be shared with the logic in RenderBlock::paintCaret as
they
> are very similar.

They diverged and then grew together again. Created RenderBlock::hasCaret to
deduplicate.

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.h:191: virtual void addVisualOverflow(const
LayoutRect&, VisualOverflowClipBehavior);
On 2013/06/19 19:42:58, Julien Chaffraix wrote:
> Making this virtual seems to go in the wrong direction. This is required
mainly
> because you are storing the new rect in RenderBlock where any RenderBox can
have
> an overflow clip but not all are blocks (e.g. RenderListMarkers).

I don't see how any other kind of box can have an overflow clip (certainly I
can't think of a way to apply an overflow clip to a list marker).

In particular, the only implementation of hasOverflowClip() is in RenderObject,
which queries the bit fields. The only invocation of setHasOverflowClip where
the argument can be true is in RenderBox::updateFromStyle, where it is guarded
by the condition isRenderBlock().

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Equivalently, the eponymous "overflow" CSS property applies only to block
container elements (which always have renderers which subclass RenderBlock,
AFAICT).

http://www.w3.org/TR/CSS2/visufx.html#overflow

I did originally have the rect in RenderBox, but it was only used for
RenderBlocks, so that seemed silly to penalize ALL boxes everywhere with the
size (including, as you note, list markers, as well as blocks with no overflow
clip).

[jbroman, 2013-08-02 15:02:20]

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1767: LayoutRect rectToApply =
LayoutRect(clientRect.x() + min<LayoutUnit>(0, textIndent), clientRect.y(),
clientRect.width() - min<LayoutUnit>(0, textIndent), clientRect.height());
On 2013/08/02 14:28:17, jbroman wrote:
> On 2013/06/19 19:42:58, Julien Chaffraix wrote:
> > Removing the min seems wrong: textIndent is negative in that branch.
> 
> min returns the lesser of its arguments. If textIndent < 0, then the lesser of
0
> and textIndent is necessarily 0, no?
> 
> That said, removing it from this CL to make the diff nicer.

Err, I meant "is necessarily textIndent". The change was correct; my
sentence-forming abilities are questionable.

[Julien - ping for review, 2013-08-07 00:17:08]

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/16402019/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1767: LayoutRect rectToApply =
LayoutRect(clientRect.x() + min<LayoutUnit>(0, textIndent), clientRect.y(),
clientRect.width() - min<LayoutUnit>(0, textIndent), clientRect.height());
On 2013/08/02 15:02:20, jbroman wrote:
> On 2013/08/02 14:28:17, jbroman wrote:
> > On 2013/06/19 19:42:58, Julien Chaffraix wrote:
> > > Removing the min seems wrong: textIndent is negative in that branch.
> > 
> > min returns the lesser of its arguments. If textIndent < 0, then the lesser
of
> 0
> > and textIndent is necessarily 0, no?
> > 
> > That said, removing it from this CL to make the diff nicer.
> 
> Err, I meant "is necessarily textIndent". The change was correct; my
> sentence-forming abilities are questionable.

Yeah, I misread the condition and your code was right. My bad!

https://codereview.chromium.org/16402019/diff/63001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/16402019/diff/63001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:703: crbug.com/238732
fast/forms/placeholder-position.html [ NeedsRebaseline ]
My problem with not including these baselines is that I can't double check that
what you do is OK and thus have to err on the side of it-is-not-fine.

https://codereview.chromium.org/16402019/diff/63001/Source/core/platform/grap...
File Source/core/platform/graphics/RoundedRectTest.cpp (right):

https://codereview.chromium.org/16402019/diff/63001/Source/core/platform/grap...
Source/core/platform/graphics/RoundedRectTest.cpp:66: IntRect(100, 200, 300,
400),
Breaking before this line looks awkward as Blink doesn't have the 80 lines rule.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:1533: RoundedRect clipRRect(0, 0, 0, 0);
Let's not abbreviate this variable. On top of that you have |clipRect| and
|clipRRect|, recipe for mixing one for the other.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:1561: if (style()->hasBorderRadius())
This could reuse hasBorderRadius.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:4313: if (rect.isEmpty())
Is that very frequent? (merely interesting as it seems weird to call
addVisualOverflow without an empty rect)

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:4338: if (!hasVisualOverflow() &&
contentsVisualOverflowRect().isEmpty()) {
When do we clear contentsVisualOverflowRect()?

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.h:195: void addVisualOverflow(const LayoutRect&,
VisualOverflowClipBehavior);
I would rather see 2 functions instead of adding a mandatory parameter. That
would yield to better code.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
File Source/core/rendering/RenderOverflowTest.cpp (right):

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderOverflowTest.cpp:75: // could probably just use
LayoutRect::unite).
No clue what this comment means.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderOverflowTest.cpp:76:
overflow.addLayoutOverflow(LayoutRect(200, 200, 0, 0));
This seems like a dumb case (and arguable in how we should handle it). Can we
either replace this test with something not 0 sized? If we really need to test
this case, it should be its own test and not mixed with other testing.

Your unit tests are also not very unitary: you test multiple operations and they
would be better off split into several more focused test case (including some
common set-up method as you repeat the overflow initialization 5 times). For
example, this test checks 3 cases:
- adding some "top" overflow should expand the layout overflow.
- adding some overflow contained in the existing one shouldn't modify the layout
overflow.
- adding a 0 sized rectangle expands the layout overflow.

On top of that, you check that none of the previous cases change the visual
overflow.

[jbroman, 2013-08-08 14:35:39]

https://codereview.chromium.org/16402019/diff/63001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/16402019/diff/63001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:703: crbug.com/238732
fast/forms/placeholder-position.html [ NeedsRebaseline ]
On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> My problem with not including these baselines is that I can't double check
that
> what you do is OK and thus have to err on the side of it-is-not-fine.

The number of rebaselines needed has been drastically reduced since the font
rendering on Mac changed. As for the remaining test expectations:

I'm not sure how to get a baseline from some platforms (win, mac-lion,
mac-snowleopard) until the change lands and the bots have a chance to try it. Am
I missing something?

I can show you the updated baselines for the platforms I do have (linux, mac).
The other platform changes should be analogous.
https://codereview.chromium.org/22632002/

What do you think?

https://codereview.chromium.org/16402019/diff/63001/Source/core/platform/grap...
File Source/core/platform/graphics/RoundedRectTest.cpp (right):

https://codereview.chromium.org/16402019/diff/63001/Source/core/platform/grap...
Source/core/platform/graphics/RoundedRectTest.cpp:66: IntRect(100, 200, 300,
400),
On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> Breaking before this line looks awkward as Blink doesn't have the 80 lines
rule.

Re-wrapped as one (116-column) line, here and below.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:1533: RoundedRect clipRRect(0, 0, 0, 0);
On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> Let's not abbreviate this variable. On top of that you have |clipRect| and
> |clipRRect|, recipe for mixing one for the other.

Changed to |clipRoundedRect|.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:1561: if (style()->hasBorderRadius())
On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> This could reuse hasBorderRadius.

And now it does. :) Done.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:4313: if (rect.isEmpty())
On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> Is that very frequent? (merely interesting as it seems weird to call
> addVisualOverflow without an empty rect)

I suspect not, for the reason you mention; was preserving the check from before.
Would you like it removed, or should I investigate further?

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:4338: if (!hasVisualOverflow() &&
contentsVisualOverflowRect().isEmpty()) {
On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> When do we clear contentsVisualOverflowRect()?

The same time we clear the rest of the overflow. For blocks, that's the first
line of RenderBlock::computeOverflow.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.h:195: void addVisualOverflow(const LayoutRect&,
VisualOverflowClipBehavior);
On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> I would rather see 2 functions instead of adding a mandatory parameter. That
> would yield to better code.

Done.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
File Source/core/rendering/RenderOverflowTest.cpp (right):

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderOverflowTest.cpp:75: // could probably just use
LayoutRect::unite).
On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> No clue what this comment means.

Removed. It didn't add much value anyhow.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderOverflowTest.cpp:76:
overflow.addLayoutOverflow(LayoutRect(200, 200, 0, 0));
On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> This seems like a dumb case (and arguable in how we should handle it). Can we
> either replace this test with something not 0 sized? If we really need to test
> this case, it should be its own test and not mixed with other testing.

I agree that it's a dumb case, and the previous comment was more or less a
(poorly phrased) comment on the same. I do think there's value in testing what
it does do, so that if we do change it in the future, we do so knowingly and
intentionally. Changing the test at the same time would be easy.

> Your unit tests are also not very unitary: you test multiple operations and
they
> would be better off split into several more focused test case (including some
> common set-up method as you repeat the overflow initialization 5 times). For
> example, this test checks 3 cases:
> - adding some "top" overflow should expand the layout overflow.
> - adding some overflow contained in the existing one shouldn't modify the
layout
> overflow.
> - adding a 0 sized rectangle expands the layout overflow.
> 
> On top of that, you check that none of the previous cases change the visual
> overflow.

If you want unitary test cases, then unitary test cases shall you get.

[Julien - ping for review, 2013-08-08 21:36:49]

lgtm

https://codereview.chromium.org/16402019/diff/63001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/16402019/diff/63001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:703: crbug.com/238732
fast/forms/placeholder-position.html [ NeedsRebaseline ]
On 2013/08/08 14:35:39, jbroman wrote:
> On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> > My problem with not including these baselines is that I can't double check
> that
> > what you do is OK and thus have to err on the side of it-is-not-fine.
> 
> The number of rebaselines needed has been drastically reduced since the font
> rendering on Mac changed. As for the remaining test expectations:
> 
> I'm not sure how to get a baseline from some platforms (win, mac-lion,
> mac-snowleopard) until the change lands and the bots have a chance to try it.
Am
> I missing something?

I wasn't asking for *all* baselines but for updating one platform (which you
prefer). With the new baseline, it's possible to say if the change creates some
regressions.

> I can show you the updated baselines for the platforms I do have (linux, mac).
> The other platform changes should be analogous.
> https://codereview.chromium.org/22632002/
> 
> What do you think?

That's pretty much what I am asking, ideally this should be part of this change
for easier review.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:4313: if (rect.isEmpty())
On 2013/08/08 14:35:39, jbroman wrote:
> On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> > Is that very frequent? (merely interesting as it seems weird to call
> > addVisualOverflow without an empty rect)
> 
> I suspect not, for the reason you mention; was preserving the check from
before.
> Would you like it removed, or should I investigate further?

Removed, it seems like a corner case.

https://codereview.chromium.org/16402019/diff/88001/Source/core/platform/grap...
File Source/core/platform/graphics/RoundedRect.h (right):

https://codereview.chromium.org/16402019/diff/88001/Source/core/platform/grap...
Source/core/platform/graphics/RoundedRect.h:86: IntRect enclosedRect() const;
I really think we should specify what we return as it is not the enclosed rect
with the maximum perimeter (the one we would expect). Alternative names:
ellipseCenterRect(), radiusCenterRect(), radiusCenterEnclosedRect().

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.h:889: bool hasCaret(CaretType);
Those functions should be const.

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.h:43: enum ContentsClipBehavior {
ContentsClipRequired, ContentsClipAutomatic };
Those names could use some improvement: "automatic" is a very undefined word.

Here are some better names:
* ForceContentsClip, SkipContentClipIfPossible
* ContentsClipForced, ContentClipMayBeIgnored

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.h:480: bool pushContentsClip(PaintInfo&, const
LayoutPoint& accumulatedOffset, ContentsClipBehavior = ContentsClipRequired);
There are 3 callers, let's avoid the default parameter.

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
File Source/core/rendering/RenderOverflowTest.cpp (right):

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
Source/core/rendering/RenderOverflowTest.cpp:43: void PrintTo(const LayoutRect&
rect, ::std::ostream* os)
AFAICT we don't put :: before std though both versions are correct.

[jbroman, 2013-08-09 15:34:34]

https://codereview.chromium.org/16402019/diff/63001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/16402019/diff/63001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:703: crbug.com/238732
fast/forms/placeholder-position.html [ NeedsRebaseline ]
On 2013/08/08 21:36:49, Julien Chaffraix wrote:
> On 2013/08/08 14:35:39, jbroman wrote:
> > On 2013/08/07 00:17:08, Julien Chaffraix wrote:
> > > My problem with not including these baselines is that I can't double check
> > that
> > > what you do is OK and thus have to err on the side of it-is-not-fine.
> > 
> > The number of rebaselines needed has been drastically reduced since the font
> > rendering on Mac changed. As for the remaining test expectations:
> > 
> > I'm not sure how to get a baseline from some platforms (win, mac-lion,
> > mac-snowleopard) until the change lands and the bots have a chance to try
it.
> Am
> > I missing something?
> 
> I wasn't asking for *all* baselines but for updating one platform (which you
> prefer). With the new baseline, it's possible to say if the change creates
some
> regressions.
> 
> > I can show you the updated baselines for the platforms I do have (linux,
mac).
> > The other platform changes should be analogous.
> > https://codereview.chromium.org/22632002/
> > 
> > What do you think?
> 
> That's pretty much what I am asking, ideally this should be part of this
change
> for easier review.

Noted for future reference. It's mildly annoying since trybots/CQ can't handle
binary diffs.

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/16402019/diff/63001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:4313: if (rect.isEmpty())
Done.

https://codereview.chromium.org/16402019/diff/88001/Source/core/platform/grap...
File Source/core/platform/graphics/RoundedRect.h (right):

https://codereview.chromium.org/16402019/diff/88001/Source/core/platform/grap...
Source/core/platform/graphics/RoundedRect.h:86: IntRect enclosedRect() const;
On 2013/08/08 21:36:50, Julien Chaffraix wrote:
> I really think we should specify what we return as it is not the enclosed rect
> with the maximum perimeter (the one we would expect). Alternative names:
> ellipseCenterRect(), radiusCenterRect(), radiusCenterEnclosedRect().

Changed to radiusCenterRect().

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.h:889: bool hasCaret(CaretType);
On 2013/08/08 21:36:50, Julien Chaffraix wrote:
> Those functions should be const.

Done.

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.h:43: enum ContentsClipBehavior {
ContentsClipRequired, ContentsClipAutomatic };
On 2013/08/08 21:36:50, Julien Chaffraix wrote:
> Those names could use some improvement: "automatic" is a very undefined word.
> 
> Here are some better names:
> * ForceContentsClip, SkipContentClipIfPossible
> * ContentsClipForced, ContentClipMayBeIgnored

Took your first suggestion.

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.h:480: bool pushContentsClip(PaintInfo&, const
LayoutPoint& accumulatedOffset, ContentsClipBehavior = ContentsClipRequired);
On 2013/08/08 21:36:50, Julien Chaffraix wrote:
> There are 3 callers, let's avoid the default parameter.

Done.

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
File Source/core/rendering/RenderOverflowTest.cpp (right):

https://codereview.chromium.org/16402019/diff/88001/Source/core/rendering/Ren...
Source/core/rendering/RenderOverflowTest.cpp:43: void PrintTo(const LayoutRect&
rect, ::std::ostream* os)
On 2013/08/08 21:36:50, Julien Chaffraix wrote:
> AFAICT we don't put :: before std though both versions are correct.

Changed here and in RoundedRectTest.

[jbroman, 2013-08-09 17:43:47]

CQ isn't responding; going to dcommit this (release trybots are all green)

[jbroman, 2013-08-09 17:47:06]

Committed patchset #15 manually as r155847 (presubmit successful).