Simplify computation of text selection top/bottom.

Simplify computation of text selection top/bottom.

Leads to a functionality change wherein we will now see
selection highlight overlap in some cases where previously
we avoided it. We've already gone down this path in other
cases, see for example http://crrev.com/1727113007.

This change fixes two issues:

1. A paint invalidation bug that could lead to missing
   glyph bits.

2. A paint issue where text selection rect could be shrunk
   to avoid overlap with preceding line resulting in
   partially invisible glyph portions painted outside of
   the selection rect.

It also removes logic around fiddling selection location,
when floats are present, depending on line offset of
previous/next lines. This could be a relic from selection
gap days. Without problematic specific test cases to
consider, we may as well remove said logic.

BUG=657325
Committed: https://crrev.com/e1ac96c0207dd18057fc1f385362b1bee52bb26f
Cr-Commit-Position: refs/heads/master@{#436446}

[wkorman, 2016-12-01 00:08:09]

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

[commit-bot: I haz the power, 2016-12-01 00:08:37]

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

[wkorman, 2016-12-01 00:12:20]

wkorman@chromium.org changed reviewers:
+ pdr@chromium.org, schenney@chromium.org

[wkorman, 2016-12-01 00:17:33]

Description was changed from

==========
Simplify computation of text selection top/bottom.

BUG=657325
==========

to

==========
Simplify computation of text selection top/bottom.

Fixes two issues:

1. paint invalidation bug that could lead to missing glyph bits

2. paint issue where text selection rect could be shrunk
to avoid overlap with preceding line resulting in partially
invisible glyph portions painted outside of the selection
rect.

BUG=657325
==========

[wkorman, 2016-12-01 00:27:35]

Description was changed from

==========
Simplify computation of text selection top/bottom.

Fixes two issues:

1. paint invalidation bug that could lead to missing glyph bits

2. paint issue where text selection rect could be shrunk
to avoid overlap with preceding line resulting in partially
invisible glyph portions painted outside of the selection
rect.

BUG=657325
==========

to

==========
Simplify computation of text selection top/bottom.

Leads to a functionality change wherein we will now see
selection highlight overlap in some cases where previously
we avoided it. We've already gone down this path in other
cases, see for example http://crrev.com/1727113007.

This change fixes two issues:

1. A paint invalidation bug that could lead to missing
   glyph bits.

2. A paint issue where text selection rect could be shrunk
   to avoid overlap with preceding line resulting in
   partially invisible glyph portions painted outside of
   the selection rect.

BUG=657325
==========

[wkorman, 2016-12-01 00:34:07]

Description was changed from

==========
Simplify computation of text selection top/bottom.

Leads to a functionality change wherein we will now see
selection highlight overlap in some cases where previously
we avoided it. We've already gone down this path in other
cases, see for example http://crrev.com/1727113007.

This change fixes two issues:

1. A paint invalidation bug that could lead to missing
   glyph bits.

2. A paint issue where text selection rect could be shrunk
   to avoid overlap with preceding line resulting in
   partially invisible glyph portions painted outside of
   the selection rect.

BUG=657325
==========

to

==========
Simplify computation of text selection top/bottom.

Leads to a functionality change wherein we will now see
selection highlight overlap in some cases where previously
we avoided it. We've already gone down this path in other
cases, see for example http://crrev.com/1727113007.

This change fixes two issues:

1. A paint invalidation bug that could lead to missing
   glyph bits.

2. A paint issue where text selection rect could be shrunk
   to avoid overlap with preceding line resulting in
   partially invisible glyph portions painted outside of
   the selection rect.

It also removes logic around fiddling selection location,
when floats are present, depending on line offset of
previous/next lines. This could be a relic from selection
gap days. Without problematic specific test cases to
consider, we may as well remove said logic.

BUG=657325
==========

[wkorman, 2016-12-01 01:05:30]

wkorman@chromium.org changed reviewers:
+ eae@chromium.org

[wkorman, 2016-12-01 01:05:31]

Note this patch knowingly regresses behavior of
fast/text/selection-with-inline-padding.html which
was added in http://crrev.com/331303004.

Open to thoughts, from in person review with two other
reviewers we think this change is still beneficial.

We could explore a subsequent patch to avoid
incorporating y-padding, but we need to not break the
test case included in this change in doing so.

Fixing the bug linked to this patch seems higher
priority than regressing behavior of selection with
y-padding on an inline.

It's also not clear (i.e. there is surely something I
don't know) why we have a no-quirks mode check around
line/selection bottom in RootInlineBox::selectionBottom().

[commit-bot: I haz the power, 2016-12-01 01:40:14]

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

[commit-bot: I haz the power, 2016-12-01 01:40:14]

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, 2016-12-01 05:45:50]

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

[commit-bot: I haz the power, 2016-12-01 05:46:09]

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, 2016-12-01 07:13:21]

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

[commit-bot: I haz the power, 2016-12-01 07:13:22]

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, 2016-12-01 20:50:27]

On 2016/12/01 01:05:31, wkorman wrote:
> Note this patch knowingly regresses behavior of
> fast/text/selection-with-inline-padding.html which
> was added in http://crrev.com/331303004.
> 
> Open to thoughts, from in person review with two other
> reviewers we think this change is still beneficial.
> 
> We could explore a subsequent patch to avoid
> incorporating y-padding, but we need to not break the
> test case included in this change in doing so.
> 
> Fixing the bug linked to this patch seems higher
> priority than regressing behavior of selection with
> y-padding on an inline.
> 
> It's also not clear (i.e. there is surely something I
> don't know) why we have a no-quirks mode check around
> line/selection bottom in RootInlineBox::selectionBottom().

I spent more time this morning looking at
fast/text/selection-with-inline-padding.html exploring
how to reconcile with what we want for
paint/text/selection-no-clip-text.html and came away
with no good answer.

I continue to believe landing this patch as-is is at
minimum a reasonable interim step.

[pdr., 2016-12-01 21:54:48]

I think this patch is an improvement. LGTM.

https://codereview.chromium.org/2546473003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/RootInlineBox.cpp (left):

https://codereview.chromium.org/2546473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/RootInlineBox.cpp:404: // This line
has actually been moved further down, probably from a large
I traced this code back 12 years and found it was added for selection gaps,
which we have removed:
https://trac.webkit.org/changeset/7766/trunk/WebCore/khtml/rendering/render_l...

[eae, 2016-12-02 10:12:30]

LGTM

https://codereview.chromium.org/2546473003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/RootInlineBox.cpp (left):

https://codereview.chromium.org/2546473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/RootInlineBox.cpp:404: // This line
has actually been moved further down, probably from a large
On 2016/12/01 21:54:48, pdr. wrote:
> I traced this code back 12 years and found it was added for selection gaps,
> which we have removed:
>
https://trac.webkit.org/changeset/7766/trunk/WebCore/khtml/rendering/render_l...

Nice detective work there pdr!

[wkorman, 2016-12-05 21:36:01]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-12-05 21:36:01]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2546473003/#ps80001
(title: "Add add'l test expectations.")

[commit-bot: I haz the power, 2016-12-05 21:36:39]

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, 2016-12-05 23:25:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480973761258030,
"parent_rev": "c050a33a6de8d78058be1fd677da06219ac7fc71", "commit_rev":
"76bb2336e0fad3c83f3f38dd9d829c916d792f1c"}

[commit-bot: I haz the power, 2016-12-05 23:25:34]

Description was changed from

==========
Simplify computation of text selection top/bottom.

Leads to a functionality change wherein we will now see
selection highlight overlap in some cases where previously
we avoided it. We've already gone down this path in other
cases, see for example http://crrev.com/1727113007.

This change fixes two issues:

1. A paint invalidation bug that could lead to missing
   glyph bits.

2. A paint issue where text selection rect could be shrunk
   to avoid overlap with preceding line resulting in
   partially invisible glyph portions painted outside of
   the selection rect.

It also removes logic around fiddling selection location,
when floats are present, depending on line offset of
previous/next lines. This could be a relic from selection
gap days. Without problematic specific test cases to
consider, we may as well remove said logic.

BUG=657325
==========

to

==========
Simplify computation of text selection top/bottom.

Leads to a functionality change wherein we will now see
selection highlight overlap in some cases where previously
we avoided it. We've already gone down this path in other
cases, see for example http://crrev.com/1727113007.

This change fixes two issues:

1. A paint invalidation bug that could lead to missing
   glyph bits.

2. A paint issue where text selection rect could be shrunk
   to avoid overlap with preceding line resulting in
   partially invisible glyph portions painted outside of
   the selection rect.

It also removes logic around fiddling selection location,
when floats are present, depending on line offset of
previous/next lines. This could be a relic from selection
gap days. Without problematic specific test cases to
consider, we may as well remove said logic.

BUG=657325
==========

[commit-bot: I haz the power, 2016-12-05 23:25:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-05 23:27:43]

Description was changed from

==========
Simplify computation of text selection top/bottom.

Leads to a functionality change wherein we will now see
selection highlight overlap in some cases where previously
we avoided it. We've already gone down this path in other
cases, see for example http://crrev.com/1727113007.

This change fixes two issues:

1. A paint invalidation bug that could lead to missing
   glyph bits.

2. A paint issue where text selection rect could be shrunk
   to avoid overlap with preceding line resulting in
   partially invisible glyph portions painted outside of
   the selection rect.

It also removes logic around fiddling selection location,
when floats are present, depending on line offset of
previous/next lines. This could be a relic from selection
gap days. Without problematic specific test cases to
consider, we may as well remove said logic.

BUG=657325
==========

to

==========
Simplify computation of text selection top/bottom.

Leads to a functionality change wherein we will now see
selection highlight overlap in some cases where previously
we avoided it. We've already gone down this path in other
cases, see for example http://crrev.com/1727113007.

This change fixes two issues:

1. A paint invalidation bug that could lead to missing
   glyph bits.

2. A paint issue where text selection rect could be shrunk
   to avoid overlap with preceding line resulting in
   partially invisible glyph portions painted outside of
   the selection rect.

It also removes logic around fiddling selection location,
when floats are present, depending on line offset of
previous/next lines. This could be a relic from selection
gap days. Without problematic specific test cases to
consider, we may as well remove said logic.

BUG=657325

Committed: https://crrev.com/e1ac96c0207dd18057fc1f385362b1bee52bb26f
Cr-Commit-Position: refs/heads/master@{#436446}
==========

[commit-bot: I haz the power, 2016-12-05 23:27:44]

Patchset 5 (id:??) landed as
https://crrev.com/e1ac96c0207dd18057fc1f385362b1bee52bb26f
Cr-Commit-Position: refs/heads/master@{#436446}