Refactor OverlayScrollbarSizeRelevancy into OverlayScrollbarClipBehavior

Refactor OverlayScrollbarSizeRelevancy into OverlayScrollbarClipBehavior

OverlayScrollbarSizeRelevancy was added in [1] and took chrishtr and me
quite a bit of time to understand in [2]. For example, it was not clear
what "relevancy" meant, nor that including overlay scrollbar size is
needed for hit testing. When creating the new LayoutViewItem API, this
code was incorrectly exposed even though it wasn't needed.

This patch does the following renames and adds a comment explaining why:
OverlayScrollbarSizeRelevancy -> OverlayScrollbarClipBehavior
IncludeOverlayScrollbarSize -> IncludeOverlayScrollbarSizeForHitTesting

Additionally, LayoutViewItem has been updated to no longer use the enum.

[1] https://chromium.googlesource.com/chromium/src/+/9477b0e3afbc92ba1e75e235b0f09fe231853770
[2] https://codereview.chromium.org/1897753002/

BUG=597946
Committed: https://crrev.com/0c45146717e9d715483d89fa7314ed7fcc5831bd
Cr-Commit-Position: refs/heads/master@{#389345}

[pdr., 2016-04-22 05:00:04]

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

[commit-bot: I haz the power, 2016-04-22 05:00:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1907213002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1907213002/1

[pdr., 2016-04-22 06:05:29]

Description was changed from

==========
Refactor OverlayScrollbarSizeRelevancy into OverlayScrollbarClipBehavior

OverlayScrollbarSizeRelevancy was added in [1] and took chrishtr and me
quite a bit of time to understand in [2]. For example, it was not clear
what "relevancy" meant, nor why including overlay scrollbar size is
needed for hit testing. When creating the new LayoutViewItem API, this
code was incorrectly exposed even though it wasn't needed.

This patch does the following renames and adds a comment to explain it:
OverlayScrollbarSizeRelevancy -> OverlayScrollbarClipBehavior
IncludeOverlayScrollbarSize -> IncludeOverlayScrollbarSizeForHitTesting

Additionally, LayoutViewItem has been updated to no longer use the enum.

[1]
https://chromium.googlesource.com/chromium/src/+/9477b0e3afbc92ba1e75e235b0f0...
[2] https://codereview.chromium.org/1897753002/

BUG=597946
==========

to

==========
Refactor OverlayScrollbarSizeRelevancy into OverlayScrollbarClipBehavior

OverlayScrollbarSizeRelevancy was added in [1] and took chrishtr and me
quite a bit of time to understand in [2]. For example, it was not clear
what "relevancy" meant, nor that including overlay scrollbar size is
needed for hit testing. When creating the new LayoutViewItem API, this
code was incorrectly exposed even though it wasn't needed.

This patch does the following renames and adds a comment explaining why:
OverlayScrollbarSizeRelevancy -> OverlayScrollbarClipBehavior
IncludeOverlayScrollbarSize -> IncludeOverlayScrollbarSizeForHitTesting

Additionally, LayoutViewItem has been updated to no longer use the enum.

[1]
https://chromium.googlesource.com/chromium/src/+/9477b0e3afbc92ba1e75e235b0f0...
[2] https://codereview.chromium.org/1897753002/

BUG=597946
==========

[pdr., 2016-04-22 06:05:35]

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

[commit-bot: I haz the power, 2016-04-22 06:13:46]

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

[commit-bot: I haz the power, 2016-04-22 06:13:47]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2016-04-22 16:05:04]

https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:45: enum
OverlayScrollbarClipBehavior { IgnoreOverlayScrollbarSize,
IncludeOverlayScrollbarSizeForHitTesting };
Exclude, not Include, right?

[pdr., 2016-04-22 17:27:32]

On 2016/04/22 at 16:05:04, chrishtr wrote:
>
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> 
>
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBox.h:45: enum
OverlayScrollbarClipBehavior { IgnoreOverlayScrollbarSize,
IncludeOverlayScrollbarSizeForHitTesting };
> Exclude, not Include, right?

We had talked about that but after looking at this more, I think include makes
sense after all. It's (sort of) the opposite of "ignore" from the other enum
value. These terms are confusing because we're talking about clipping something
out--so whether it's excluded from the clip, or excluded from the clipped result
is ambiguous.

[chrishtr, 2016-04-22 17:31:16]

On 2016/04/22 at 17:27:32, pdr wrote:
> On 2016/04/22 at 16:05:04, chrishtr wrote:
> >
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > 
> >
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/LayoutBox.h:45: enum
OverlayScrollbarClipBehavior { IgnoreOverlayScrollbarSize,
IncludeOverlayScrollbarSizeForHitTesting };
> > Exclude, not Include, right?
> 
> We had talked about that but after looking at this more, I think include makes
sense after all. It's (sort of) the opposite of "ignore" from the other enum
value. These terms are confusing because we're talking about clipping something
out--so whether it's excluded from the clip, or excluded from the clipped result
is ambiguous.

It makes sense when passed as the argument to excludeScrollbars, but not to
overflowClipRect. When passed there it makes it sound like it's being included
in the overflow clip rect, whereas it's really
being omitted.

[pdr., 2016-04-22 18:19:02]

On 2016/04/22 at 17:31:16, chrishtr wrote:
> On 2016/04/22 at 17:27:32, pdr wrote:
> > On 2016/04/22 at 16:05:04, chrishtr wrote:
> > >
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > > 
> > >
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/layout/LayoutBox.h:45: enum
OverlayScrollbarClipBehavior { IgnoreOverlayScrollbarSize,
IncludeOverlayScrollbarSizeForHitTesting };
> > > Exclude, not Include, right?
> > 
> > We had talked about that but after looking at this more, I think include
makes sense after all. It's (sort of) the opposite of "ignore" from the other
enum value. These terms are confusing because we're talking about clipping
something out--so whether it's excluded from the clip, or excluded from the
clipped result is ambiguous.
> 
> It makes sense when passed as the argument to excludeScrollbars, but not to
overflowClipRect. When passed there it makes it sound like it's being included
in the overflow clip rect, whereas it's really
> being omitted.

That's a fair point. What do you suggest we do? We could introduce a second
enum.

[pdr., 2016-04-22 18:25:59]

On 2016/04/22 at 18:19:02, pdr wrote:
> On 2016/04/22 at 17:31:16, chrishtr wrote:
> > On 2016/04/22 at 17:27:32, pdr wrote:
> > > On 2016/04/22 at 16:05:04, chrishtr wrote:
> > > >
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> > > > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > > > 
> > > >
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/layout/LayoutBox.h:45: enum
OverlayScrollbarClipBehavior { IgnoreOverlayScrollbarSize,
IncludeOverlayScrollbarSizeForHitTesting };
> > > > Exclude, not Include, right?
> > > 
> > > We had talked about that but after looking at this more, I think include
makes sense after all. It's (sort of) the opposite of "ignore" from the other
enum value. These terms are confusing because we're talking about clipping
something out--so whether it's excluded from the clip, or excluded from the
clipped result is ambiguous.
> > 
> > It makes sense when passed as the argument to excludeScrollbars, but not to
overflowClipRect. When passed there it makes it sound like it's being included
in the overflow clip rect, whereas it's really
> > being omitted.
> 
> That's a fair point. What do you suggest we do? We could introduce a second
enum.

If we could design the system from scratch, I think it would be better to always
clip the scrollbar and then re-add the overflow ones just for hit testing.
Unfortunately that is hard to do cleanly now :/

[chrishtr, 2016-04-22 18:32:53]

On 2016/04/22 at 18:25:59, pdr wrote:
> On 2016/04/22 at 18:19:02, pdr wrote:
> > On 2016/04/22 at 17:31:16, chrishtr wrote:
> > > On 2016/04/22 at 17:27:32, pdr wrote:
> > > > On 2016/04/22 at 16:05:04, chrishtr wrote:
> > > > >
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> > > > > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > > > > 
> > > > >
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> > > > > third_party/WebKit/Source/core/layout/LayoutBox.h:45: enum
OverlayScrollbarClipBehavior { IgnoreOverlayScrollbarSize,
IncludeOverlayScrollbarSizeForHitTesting };
> > > > > Exclude, not Include, right?
> > > > 
> > > > We had talked about that but after looking at this more, I think include
makes sense after all. It's (sort of) the opposite of "ignore" from the other
enum value. These terms are confusing because we're talking about clipping
something out--so whether it's excluded from the clip, or excluded from the
clipped result is ambiguous.
> > > 
> > > It makes sense when passed as the argument to excludeScrollbars, but not
to overflowClipRect. When passed there it makes it sound like it's being
included in the overflow clip rect, whereas it's really
> > > being omitted.
> > 
> > That's a fair point. What do you suggest we do? We could introduce a second
enum.
> 
> If we could design the system from scratch, I think it would be better to
always clip the scrollbar and then re-add the overflow ones just for hit
testing. Unfortunately that is hard to do cleanly now :/

I suggest a second enum. Or skip the enum for excludeScrollbars and use a bool,
since it's a private helper method.

[pdr., 2016-04-22 18:43:07]

On 2016/04/22 at 18:32:53, chrishtr wrote:
> On 2016/04/22 at 18:25:59, pdr wrote:
> > On 2016/04/22 at 18:19:02, pdr wrote:
> > > On 2016/04/22 at 17:31:16, chrishtr wrote:
> > > > On 2016/04/22 at 17:27:32, pdr wrote:
> > > > > On 2016/04/22 at 16:05:04, chrishtr wrote:
> > > > > >
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> > > > > > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > > > > > 
> > > > > >
https://codereview.chromium.org/1907213002/diff/1/third_party/WebKit/Source/c...
> > > > > > third_party/WebKit/Source/core/layout/LayoutBox.h:45: enum
OverlayScrollbarClipBehavior { IgnoreOverlayScrollbarSize,
IncludeOverlayScrollbarSizeForHitTesting };
> > > > > > Exclude, not Include, right?
> > > > > 
> > > > > We had talked about that but after looking at this more, I think
include makes sense after all. It's (sort of) the opposite of "ignore" from the
other enum value. These terms are confusing because we're talking about clipping
something out--so whether it's excluded from the clip, or excluded from the
clipped result is ambiguous.
> > > > 
> > > > It makes sense when passed as the argument to excludeScrollbars, but not
to overflowClipRect. When passed there it makes it sound like it's being
included in the overflow clip rect, whereas it's really
> > > > being omitted.
> > > 
> > > That's a fair point. What do you suggest we do? We could introduce a
second enum.
> > 
> > If we could design the system from scratch, I think it would be better to
always clip the scrollbar and then re-add the overflow ones just for hit
testing. Unfortunately that is hard to do cleanly now :/
> 
> I suggest a second enum. Or skip the enum for excludeScrollbars and use a
bool, since it's a private helper method.

I'm flip flopping on my original objection. I now agree with you and think
ExcludeOverlayScrollbarSizeForHitTesting looks good after all.

PTAL

Note that I left LayoutBox::excludeScrollbars unchanged from the previous patch.
Adding a second enum (or bool) just for the protected excludeScrollbars seems
like one too many overlay scrollbar enums so I'd like to keep reusing
OverlayScrollbarClipBehavior there.

[chrishtr, 2016-04-22 22:33:26]

https://codereview.chromium.org/1907213002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1907213002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:45: enum
OverlayScrollbarClipBehavior { IgnoreOverlayScrollbarSize,
ExcludeOverlayScrollbarSizeForHitTesting };
Why have the ForHitTesting suffix?

[pdr., 2016-04-22 22:39:00]

On 2016/04/22 at 22:33:26, chrishtr wrote:
>
https://codereview.chromium.org/1907213002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> 
>
https://codereview.chromium.org/1907213002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.h:45: enum
OverlayScrollbarClipBehavior { IgnoreOverlayScrollbarSize,
ExcludeOverlayScrollbarSizeForHitTesting };
> Why have the ForHitTesting suffix?

This will make it very difficult to get non-hit testing uses of this function
past reviewers. If you look at the code in
PaintLayerScrollableArea::verticalScrollbarWidth, the
ExcludeOverlayScrollbarSizeForHitTesting codepath truly is only for hit testing
and will not work in the general case.

[chrishtr, 2016-04-22 22:44:23]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-04-22 22:44:24]

lgtm

[chrishtr, 2016-04-22 22:44:28]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-04-22 22:44:29]

lgtm

[commit-bot: I haz the power, 2016-04-22 22:45:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1907213002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1907213002/20001

[commit-bot: I haz the power, 2016-04-23 02:33:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-23 02:35:51]

Description was changed from

==========
Refactor OverlayScrollbarSizeRelevancy into OverlayScrollbarClipBehavior

OverlayScrollbarSizeRelevancy was added in [1] and took chrishtr and me
quite a bit of time to understand in [2]. For example, it was not clear
what "relevancy" meant, nor that including overlay scrollbar size is
needed for hit testing. When creating the new LayoutViewItem API, this
code was incorrectly exposed even though it wasn't needed.

This patch does the following renames and adds a comment explaining why:
OverlayScrollbarSizeRelevancy -> OverlayScrollbarClipBehavior
IncludeOverlayScrollbarSize -> IncludeOverlayScrollbarSizeForHitTesting

Additionally, LayoutViewItem has been updated to no longer use the enum.

[1]
https://chromium.googlesource.com/chromium/src/+/9477b0e3afbc92ba1e75e235b0f0...
[2] https://codereview.chromium.org/1897753002/

BUG=597946
==========

to

==========
Refactor OverlayScrollbarSizeRelevancy into OverlayScrollbarClipBehavior

OverlayScrollbarSizeRelevancy was added in [1] and took chrishtr and me
quite a bit of time to understand in [2]. For example, it was not clear
what "relevancy" meant, nor that including overlay scrollbar size is
needed for hit testing. When creating the new LayoutViewItem API, this
code was incorrectly exposed even though it wasn't needed.

This patch does the following renames and adds a comment explaining why:
OverlayScrollbarSizeRelevancy -> OverlayScrollbarClipBehavior
IncludeOverlayScrollbarSize -> IncludeOverlayScrollbarSizeForHitTesting

Additionally, LayoutViewItem has been updated to no longer use the enum.

[1]
https://chromium.googlesource.com/chromium/src/+/9477b0e3afbc92ba1e75e235b0f0...
[2] https://codereview.chromium.org/1897753002/

BUG=597946

Committed: https://crrev.com/0c45146717e9d715483d89fa7314ed7fcc5831bd
Cr-Commit-Position: refs/heads/master@{#389345}
==========

[commit-bot: I haz the power, 2016-04-23 02:35:52]

Patchset 2 (id:??) landed as
https://crrev.com/0c45146717e9d715483d89fa7314ed7fcc5831bd
Cr-Commit-Position: refs/heads/master@{#389345}