[S.P.] Fix cull rects for InlineTextBox to account for vertical text.

[S.P.] Fix cull rects for InlineTextBox to account for vertical text.

Also, simplifies the code in InlineTextBox::paint to use the same rect for painting,
early out and cull rects for slimming paint.

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

[chrishtr, 2015-03-06 23:03:40]

chrishtr@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[Xianzhu, 2015-03-06 23:17:06]

https://codereview.chromium.org/988903002/diff/1/Source/core/paint/InlineText...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/988903002/diff/1/Source/core/paint/InlineText...
Source/core/paint/InlineTextBoxPainter.cpp:60: FloatRect boxRect(boxOrigin,
FloatSize(m_inlineTextBox.logicalWidth(), m_inlineTextBox.logicalHeight()));
It seems that we used overflowRect to check for early return, now we use
location/width/height. Is this change intended?

https://codereview.chromium.org/988903002/diff/1/Source/core/paint/InlineText...
Source/core/paint/InlineTextBoxPainter.cpp:98: drawingRecorder = adoptPtr(new
DrawingRecorder(paintInfo.context, m_inlineTextBox.displayItemClient(),
DisplayItem::paintPhaseToDrawingType(paintInfo.phase), boxRect));
Ditto.

[chrishtr, 2015-03-06 23:19:22]

https://codereview.chromium.org/988903002/diff/1/Source/core/paint/InlineText...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/988903002/diff/1/Source/core/paint/InlineText...
Source/core/paint/InlineTextBoxPainter.cpp:60: FloatRect boxRect(boxOrigin,
FloatSize(m_inlineTextBox.logicalWidth(), m_inlineTextBox.logicalHeight()));
On 2015/03/06 at 23:17:06, Xianzhu wrote:
> It seems that we used overflowRect to check for early return, now we use
location/width/height. Is this change intended?

Yes. Part of this CL is unifying that code, since boxRect should be the same as
overflowRect was previously. The one exception is when it's adjusted for text
combine on line 71, which I also did on purpose to make early-outing more
precise in that use case and further unify the code.

[chrishtr, 2015-03-06 23:20:37]

It was a bit tricky to verify the arithmetic works correctly for vertical text.
I'm assuming that the flipped location is the same as the overflowRect location,
which does seem to be the case logically and in my tests.

[Xianzhu, 2015-03-06 23:31:33]

https://codereview.chromium.org/988903002/diff/1/Source/core/paint/InlineText...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/988903002/diff/1/Source/core/paint/InlineText...
Source/core/paint/InlineTextBoxPainter.cpp:60: FloatRect boxRect(boxOrigin,
FloatSize(m_inlineTextBox.logicalWidth(), m_inlineTextBox.logicalHeight()));
On 2015/03/06 23:19:22, chrishtr wrote:
> On 2015/03/06 at 23:17:06, Xianzhu wrote:
> > It seems that we used overflowRect to check for early return, now we use
> location/width/height. Is this change intended?
> 
> Yes. Part of this CL is unifying that code, since boxRect should be the same
as
> overflowRect was previously. The one exception is when it's adjusted for text
> combine on line 71, which I also did on purpose to make early-outing more
> precise in that use case and further unify the code.

Based on InlineBox.cpp, it seems that
logicalFrameRect/logicalWidth/logicalHeight (from which boxRect is calculated)
and logicalOverflowRect are different. The former values seem not to include
overflow.

[chrishtr, 2015-03-06 23:35:35]

This broke some layout tests, in particular the ones referred to in
crbug.com/460198. Reverting.

[chrishtr, 2015-03-06 23:35:54]

On 2015/03/06 at 23:35:35, chrishtr wrote:
> This broke some layout tests, in particular the ones referred to in
crbug.com/460198. Reverting.

Oops, wrong CL to post that comment on. Please ignore.

[chrishtr, 2015-03-07 00:52:35]

https://codereview.chromium.org/988903002/diff/1/Source/core/paint/InlineText...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/988903002/diff/1/Source/core/paint/InlineText...
Source/core/paint/InlineTextBoxPainter.cpp:60: FloatRect boxRect(boxOrigin,
FloatSize(m_inlineTextBox.logicalWidth(), m_inlineTextBox.logicalHeight()));
On 2015/03/06 at 23:31:33, Xianzhu wrote:
> On 2015/03/06 23:19:22, chrishtr wrote:
> > On 2015/03/06 at 23:17:06, Xianzhu wrote:
> > > It seems that we used overflowRect to check for early return, now we use
> > location/width/height. Is this change intended?
> > 
> > Yes. Part of this CL is unifying that code, since boxRect should be the same
as
> > overflowRect was previously. The one exception is when it's adjusted for
text
> > combine on line 71, which I also did on purpose to make early-outing more
> > precise in that use case and further unify the code.
> 
> Based on InlineBox.cpp, it seems that
logicalFrameRect/logicalWidth/logicalHeight (from which boxRect is calculated)
and logicalOverflowRect are different. The former values seem not to include
overflow.

Sigh. Yes it does. Oh well. Going to make this CL less aggressive.

[chrishtr, 2015-03-09 04:52:22]

PTAL. Fixed instead by adding a visualRectToPhysicalRect method, which is
similar to but not the same as one by the same name for block layout.

[Xianzhu, 2015-03-09 16:09:45]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-03-09 16:09:45]

lgtm

[commit-bot: I haz the power, 2015-03-09 16:10:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/988903002/80001

[commit-bot: I haz the power, 2015-03-09 17:40:46]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191564