Reduce rectangle merges when doing region-based hit test on text boxes

Reduce rectangle merges when doing region-based hit test on text boxes

Only unite the box rect with the hit test result if it intersects the hit
test rect. That is sufficient because the only use for the united rect
is to check that it contains the location rect for early termination of
testing.

Patch based on work by oicqchen@gmail.com.

R=wkorman@chromium.org
BUG=676764
Review-Url: https://codereview.chromium.org/2616483002
Cr-Commit-Position: refs/heads/master@{#443684}
Committed: https://chromium.googlesource.com/chromium/src/+/4f24093ff9c2a18dcddb8728edf40be941b8aadc

[Stephen Chennney, 2017-01-03 21:25:43]

Description was changed from

==========
Reduce rectangle merges when doing region-based hit test on text boxes

Only unite the box rect with the hit test result if it intersects the hit
test rect. That is sufficient because the only use for the united rect
is to check that it contains the location rect for early termination of
testing.

R=wkorman@chromium.org
BUG=676764
==========

to

==========
Reduce rectangle merges when doing region-based hit test on text boxes

Only unite the box rect with the hit test result if it intersects the hit
test rect. That is sufficient because the only use for the united rect
is to check that it contains the location rect for early termination of
testing.

Patch based on work by oicqchen@gmail.com.

R=wkorman@chromium.org
BUG=676764
==========

[Stephen Chennney, 2017-01-03 21:26:14]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 21:26:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 00:03:54]

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

[commit-bot: I haz the power, 2017-01-04 00:03:54]

Dry run: This issue passed the CQ dry run.

[wkorman, 2017-01-04 00:36:35]

What about adding a unit test, or a performance test, or both?

Not sure how hard the latter would be to implement, I know we have some but
unsure if we've existing for hit testing in particular.

On the former, there is a LayoutInlineTest, could perhaps use a RenderingTest
and then call hitTestCulledInline()

https://codereview.chromium.org/2616483002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2616483002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:880: m_intersected =
m_intersected || intersects;
nit: could use |= here and below.

[Stephen Chennney, 2017-01-04 17:11:59]

On 2017/01/04 00:36:35, wkorman wrote:
> What about adding a unit test, or a performance test, or both?
> 
> Not sure how hard the latter would be to implement, I know we have some but
> unsure if we've existing for hit testing in particular.
> 
> On the former, there is a LayoutInlineTest, could perhaps use a RenderingTest
> and then call hitTestCulledInline()

I'll go with a unit test to verify correctness, somehow.
 
>
https://codereview.chromium.org/2616483002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):
> 
>
https://codereview.chromium.org/2616483002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutInline.cpp:880: m_intersected =
> m_intersected || intersects;
> nit: could use |= here and below.

Yep. For some reason I think |= is buggy on Windows, buts that's probably old
news circa 2007.

[Stephen Chennney, 2017-01-13 15:38:23]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 15:38:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2017-01-13 15:45:47]

I've modified the logic a bit and have a unit test to confirm it works. I spent
2 days trying to get a perf test that hits the code and isolates performance,
but it's not possible (it seems) due to hit testing optimizations that avoid
work when hit testing the same position, and workarounds for that introduce
costs that mask the hit testing. It also seems that the test environment cannot
reproduce the conditions that exist in mobile Chrome, where it's trivial to
touch the code.

I'm a bit worried that this is not actually more efficient. I'm going to hack up
a standalone benchmark to just stress this code and I'll attach it to the bug
report.

[commit-bot: I haz the power, 2017-01-13 17:37:03]

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

[commit-bot: I haz the power, 2017-01-13 17:37:04]

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

[wkorman, 2017-01-13 17:39:03]

LGTM if we'd like to land this and observe

[Stephen Chennney, 2017-01-13 18:41:38]

On 2017/01/13 17:39:03, wkorman wrote:
> LGTM if we'd like to land this and observe

Apparently Mac has different font metrics in unit tests. Investigating.

[Stephen Chennney, 2017-01-13 19:43:01]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-01-13 19:43:02]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2616483002/#ps40001
(title: "Improve test to be platform agnostic.")

[commit-bot: I haz the power, 2017-01-13 19:44:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 21:46:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484336581689470,
"parent_rev": "643c475ca25c4d5191516af0fb7ac3b87177a89c", "commit_rev":
"4f24093ff9c2a18dcddb8728edf40be941b8aadc"}

[commit-bot: I haz the power, 2017-01-13 21:47:25]

Description was changed from

==========
Reduce rectangle merges when doing region-based hit test on text boxes

Only unite the box rect with the hit test result if it intersects the hit
test rect. That is sufficient because the only use for the united rect
is to check that it contains the location rect for early termination of
testing.

Patch based on work by oicqchen@gmail.com.

R=wkorman@chromium.org
BUG=676764
==========

to

==========
Reduce rectangle merges when doing region-based hit test on text boxes

Only unite the box rect with the hit test result if it intersects the hit
test rect. That is sufficient because the only use for the united rect
is to check that it contains the location rect for early termination of
testing.

Patch based on work by oicqchen@gmail.com.

R=wkorman@chromium.org
BUG=676764

Review-Url: https://codereview.chromium.org/2616483002
Cr-Commit-Position: refs/heads/master@{#443684}
Committed:
https://chromium.googlesource.com/chromium/src/+/4f24093ff9c2a18dcddb8728edf4...
==========

[commit-bot: I haz the power, 2017-01-13 21:47:27]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/4f24093ff9c2a18dcddb8728edf4...