Don't use selection bounds to paint document marker

Don't use selection bounds to paint document marker

If there is no selection, selection bounds may exceed the paint
invalidation rect.

The original reason of using selection bounds to paint document
marker "when the selection and this highlight are on the same word
there are no pieces sticking out" no longer applies because we don't
allow selection and document markers at the same time.

BUG=619633
Committed: https://crrev.com/889581a0249d4ccddcb7d85d3c45e54c2af93a48
Cr-Commit-Position: refs/heads/master@{#400080}

[Xianzhu, 2016-06-14 19:01:42]

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

[Xianzhu, 2016-06-14 19:01:56]

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

[jbroman, 2016-06-15 08:32:17]

Description was changed from

==========
Don't use selection bounds to paint document marker

If there is no selection, selection bounds may exceed the paint
invalidation rect.

The original reason of using seleciton bounds to paint document
marker "when the selection and this highlight are on the same word
there are no pieces sticking out" no longer applies because we don't
allow selection and document markers at the same time.

BUG=619633
==========

to

==========
Don't use selection bounds to paint document marker

If there is no selection, selection bounds may exceed the paint
invalidation rect.

The original reason of using selection bounds to paint document
marker "when the selection and this highlight are on the same word
there are no pieces sticking out" no longer applies because we don't
allow selection and document markers at the same time.

BUG=619633
==========

[jbroman, 2016-06-15 08:37:10]

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/text/text-match-highlights-big-line-height.html
(right):

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/text/text-match-highlights-big-line-height.html:20:
onload = function() {
Is the execution of the onload handler guaranteed to beat the taking of the
screenshot? It might be safer to use testRunner.waitUntilDone/notifyDOne.

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:842: int height =
m_inlineTextBox.logicalHeight();
Why coerce this to int, but not the other dimensions?

[Xianzhu, 2016-06-15 16:40:48]

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/text/text-match-highlights-big-line-height.html
(right):

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/text/text-match-highlights-big-line-height.html:20:
onload = function() {
On 2016/06/15 08:37:10, jbroman wrote:
> Is the execution of the onload handler guaranteed to beat the taking of the
> screenshot? It might be safer to use testRunner.waitUntilDone/notifyDOne.

Yes, layout test wait for finish of onload and take snapshot.

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:842: int height =
m_inlineTextBox.logicalHeight();
On 2016/06/15 08:37:10, jbroman wrote:
> Why coerce this to int, but not the other dimensions?

This is because GraphicsContext::drawHighlightForText accepts int for the 4th
parameter.

[jbroman, 2016-06-15 18:19:47]

lgtm

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/text/text-match-highlights-big-line-height.html
(right):

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/text/text-match-highlights-big-line-height.html:20:
onload = function() {
On 2016/06/15 at 16:40:48, Xianzhu wrote:
> On 2016/06/15 08:37:10, jbroman wrote:
> > Is the execution of the onload handler guaranteed to beat the taking of the
> > screenshot? It might be safer to use testRunner.waitUntilDone/notifyDOne.
> 
> Yes, layout test wait for finish of onload and take snapshot.

Ah yes, of course. I must have forgotten that it gets dispatched synchronously.
My bad. :)

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:842: int height =
m_inlineTextBox.logicalHeight();
On 2016/06/15 at 16:40:48, Xianzhu wrote:
> On 2016/06/15 08:37:10, jbroman wrote:
> > Why coerce this to int, but not the other dimensions?
> 
> This is because GraphicsContext::drawHighlightForText accepts int for the 4th
parameter.

For consistency, would you mind matching the way this is done elsewhere in this
file, i.e. using a "boxRect" constructed as:

LayoutRect boxRect(boxOrigin, LayoutSize(m_inlineTextBox.logicalWidth(),
m_inlineTextBox.logicalHeight()));

This would make it more obvious (to me, at least) that this is similar to the
other highlight-drawing code that calls drawHighlightForText in this file. (It
would be ideal to share more code, but at least making it parallel makes it more
obvious to me what's going on.)

[Xianzhu, 2016-06-15 19:40:56]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-06-15 19:40:56]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2065723002/#ps40001
(title: "Don't use selection bounds to paint document marker")

[Xianzhu, 2016-06-15 19:41:11]

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2065723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:842: int height =
m_inlineTextBox.logicalHeight();
On 2016/06/15 18:19:47, jbroman wrote:
> On 2016/06/15 at 16:40:48, Xianzhu wrote:
> > On 2016/06/15 08:37:10, jbroman wrote:
> > > Why coerce this to int, but not the other dimensions?
> > 
> > This is because GraphicsContext::drawHighlightForText accepts int for the
4th
> parameter.
> 
> For consistency, would you mind matching the way this is done elsewhere in
this
> file, i.e. using a "boxRect" constructed as:
> 
> LayoutRect boxRect(boxOrigin, LayoutSize(m_inlineTextBox.logicalWidth(),
> m_inlineTextBox.logicalHeight()));
> 
> This would make it more obvious (to me, at least) that this is similar to the
> other highlight-drawing code that calls drawHighlightForText in this file. (It
> would be ideal to share more code, but at least making it parallel makes it
more
> obvious to me what's going on.)

Done.

[commit-bot: I haz the power, 2016-06-15 19:41:22]

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

[commit-bot: I haz the power, 2016-06-15 23:24:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 23:24:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2016-06-16 01:31:32]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-06-16 01:32:15]

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

[commit-bot: I haz the power, 2016-06-16 03:21:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-16 03:25:22]

Description was changed from

==========
Don't use selection bounds to paint document marker

If there is no selection, selection bounds may exceed the paint
invalidation rect.

The original reason of using selection bounds to paint document
marker "when the selection and this highlight are on the same word
there are no pieces sticking out" no longer applies because we don't
allow selection and document markers at the same time.

BUG=619633
==========

to

==========
Don't use selection bounds to paint document marker

If there is no selection, selection bounds may exceed the paint
invalidation rect.

The original reason of using selection bounds to paint document
marker "when the selection and this highlight are on the same word
there are no pieces sticking out" no longer applies because we don't
allow selection and document markers at the same time.

BUG=619633

Committed: https://crrev.com/889581a0249d4ccddcb7d85d3c45e54c2af93a48
Cr-Commit-Position: refs/heads/master@{#400080}
==========

[commit-bot: I haz the power, 2016-06-16 03:25:23]

Patchset 3 (id:??) landed as
https://crrev.com/889581a0249d4ccddcb7d85d3c45e54c2af93a48
Cr-Commit-Position: refs/heads/master@{#400080}