Fix clip-path reference box/coordinate space setup for hit-testing

Fix clip-path reference box/coordinate space setup for hit-testing

Synchronize hit-testing code with rendering code with regards to how the
coordinate space is handled (for userSpaceOnUse <clipPath>s), and how
the reference boxes is computed.

Also "straighten" out code-flow a bit while here.

BUG=418484
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/9b55e0544cd1f6770d3e2664cef25fadca00487c
Cr-Commit-Position: refs/heads/master@{#414730}

[fs, 2016-08-26 12:08:00]

Description was changed from

==========
Fix clip-path reference box/coordinate space setup for hit-testing

Synchronize hit-testing code with rendering code with regards to how the
coordinate space is handled (for userSpaceOnUse <clipPath>s), and how
the reference boxes is computed.

Also "straighten" out code-flow a bit while here.

BUG=418484
==========

to

==========
Fix clip-path reference box/coordinate space setup for hit-testing

Synchronize hit-testing code with rendering code with regards to how the
coordinate space is handled (for userSpaceOnUse <clipPath>s), and how
the reference boxes is computed.

Also "straighten" out code-flow a bit while here.

BUG=418484
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-08-26 12:08:25]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 12:08:47]

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

[fs, 2016-08-26 12:15:22]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 12:15:27]

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

[fs, 2016-08-26 13:10:59]

fs@opera.com changed reviewers:
+ schenney@chromium.org

[commit-bot: I haz the power, 2016-08-26 13:38:32]

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

[commit-bot: I haz the power, 2016-08-26 13:38:34]

Dry run: This issue passed the CQ dry run.

[Stephen Chennney, 2016-08-26 14:19:28]

No significant problem, but I found a couple of things to be less clear than
they could be. Thoughts?

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

https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2076:
convertToLayerCoords(rootLayer, offsetToRootLayer);
I infer that this method can handle a null rootLayer, or rootLayer is never
null. If the latter, we should DCHECK it.

https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:376: LayoutRect
computeReferenceBox() const;
I think this should be named boxForReferenceHitTesting or something like that.
boxForReferenceClipsAndMasks?

[fs, 2016-08-26 14:41:51]

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

https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2076:
convertToLayerCoords(rootLayer, offsetToRootLayer);
On 2016/08/26 at 14:19:28, Stephen Chennney wrote:
> I infer that this method can handle a null rootLayer, or rootLayer is never
null. If the latter, we should DCHECK it.

Yeah, my assumption is the latter (corroborated by codesearch; it always starts
out as 'this' AFAICS.)

https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:376: LayoutRect
computeReferenceBox() const;
On 2016/08/26 at 14:19:28, Stephen Chennney wrote:
> I think this should be named boxForReferenceHitTesting or something like that.
boxForReferenceClipsAndMasks?

We use it when painting too, so I'd go with something more like the latter -
maybe referenceBoxForClipPaths? (Having 'box' as a prefix makes it difficult to
"land" well and still get 'reference' in there - boxForClipPath would be an
alternative I guess.)

[Stephen Chennney, 2016-08-26 15:02:59]

On 2016/08/26 14:41:51, fs wrote:
>
https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):
> 
>
https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayer.cpp:2076:
> convertToLayerCoords(rootLayer, offsetToRootLayer);
> On 2016/08/26 at 14:19:28, Stephen Chennney wrote:
> > I infer that this method can handle a null rootLayer, or rootLayer is never
> null. If the latter, we should DCHECK it.
> 
> Yeah, my assumption is the latter (corroborated by codesearch; it always
starts
> out as 'this' AFAICS.)

Go with the DCHECK then. I hate leaving it open to future mistakes and the
fuzzer bugs we would have to deal with.
 
>
https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintLayer.h (right):
> 
>
https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayer.h:376: LayoutRect
> computeReferenceBox() const;
> On 2016/08/26 at 14:19:28, Stephen Chennney wrote:
> > I think this should be named boxForReferenceHitTesting or something like
that.
> boxForReferenceClipsAndMasks?
> 
> We use it when painting too, so I'd go with something more like the latter -
> maybe referenceBoxForClipPaths? (Having 'box' as a prefix makes it difficult
to
> "land" well and still get 'reference' in there - boxForClipPath would be an
> alternative I guess.)

I'm liking boxForClipPath. Let's go with that.

[fs, 2016-08-26 15:09:05]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[fs, 2016-08-26 15:09:09]

On 2016/08/26 at 15:02:59, schenney wrote:
> On 2016/08/26 14:41:51, fs wrote:
> >
https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):
> > 
> >
https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintLayer.cpp:2076:
> > convertToLayerCoords(rootLayer, offsetToRootLayer);
> > On 2016/08/26 at 14:19:28, Stephen Chennney wrote:
> > > I infer that this method can handle a null rootLayer, or rootLayer is
never
> > null. If the latter, we should DCHECK it.
> > 
> > Yeah, my assumption is the latter (corroborated by codesearch; it always
starts
> > out as 'this' AFAICS.)
> 
> Go with the DCHECK then. I hate leaving it open to future mistakes and the
fuzzer bugs we would have to deal with.

Done, and...

> >
https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintLayer.h (right):
> > 
> >
https://codereview.chromium.org/2280963002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintLayer.h:376: LayoutRect
> > computeReferenceBox() const;
> > On 2016/08/26 at 14:19:28, Stephen Chennney wrote:
> > > I think this should be named boxForReferenceHitTesting or something like
that.
> > boxForReferenceClipsAndMasks?
> > 
> > We use it when painting too, so I'd go with something more like the latter -
> > maybe referenceBoxForClipPaths? (Having 'box' as a prefix makes it difficult
to
> > "land" well and still get 'reference' in there - boxForClipPath would be an
> > alternative I guess.)
> 
> I'm liking boxForClipPath. Let's go with that.

...done.

[commit-bot: I haz the power, 2016-08-26 15:09:18]

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

[Stephen Chennney, 2016-08-26 15:27:13]

Thanks. LGTM.

[fs, 2016-08-26 15:48:47]

The CQ bit was unchecked by fs@opera.com

[fs, 2016-08-26 15:48:51]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-08-26 15:49:12]

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-08-26 16:44:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-26 16:46:06]

Description was changed from

==========
Fix clip-path reference box/coordinate space setup for hit-testing

Synchronize hit-testing code with rendering code with regards to how the
coordinate space is handled (for userSpaceOnUse <clipPath>s), and how
the reference boxes is computed.

Also "straighten" out code-flow a bit while here.

BUG=418484
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix clip-path reference box/coordinate space setup for hit-testing

Synchronize hit-testing code with rendering code with regards to how the
coordinate space is handled (for userSpaceOnUse <clipPath>s), and how
the reference boxes is computed.

Also "straighten" out code-flow a bit while here.

BUG=418484
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/9b55e0544cd1f6770d3e2664cef25fadca00487c
Cr-Commit-Position: refs/heads/master@{#414730}
==========

[commit-bot: I haz the power, 2016-08-26 16:46:07]

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