Canvas2d: addHitRegion() should take clipping region into account.

Canvas2d: addHitRegion() should take clipping region into account.

Current implementation ignores the clipping region when calling addHitRegion().
In the past, we had no way to get the current clip path from canvas state stack
in Blink side but it's been possible since display list 2d canvas feature.

BUG=387057
Committed: https://crrev.com/da1acd5523fb34ff8c71a9777cf938e0cbbf276a
Cr-Commit-Position: refs/heads/master@{#371832}

[zino, 2016-01-25 15:01:32]

jinho.bang@samsung.com changed reviewers:
+ senorblanco@chromium.org

[zino, 2016-01-25 15:01:33]

Please take a look.

Thank you.

[Stephen White, 2016-01-25 15:37:36]

https://codereview.chromium.org/1630683002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-clip-test.html
(right):

https://codereview.chromium.org/1630683002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-clip-test.html:17:
return "This test requires eventSender";
Other tests seem to call debug() in this case. Is returning a string here really
what you want to do?

https://codereview.chromium.org/1630683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.h
(right):

https://codereview.chromium.org/1630683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.h:77:
SkPath totalClipPath(const SkPath& path) const { return
m_clipList.totalClipPath(path); }
Naming nit: I think this function needs a verb, since it's not just an accessor.
Since it also intersects its argument, perhaps intersectPathWithClip()?

[zino, 2016-01-27 17:10:30]

Thank you for review.

Please take a look again.

https://codereview.chromium.org/1630683002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-clip-test.html
(right):

https://codereview.chromium.org/1630683002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/canvas-hit-regions-clip-test.html:17:
return "This test requires eventSender";
On 2016/01/25 15:37:35, Stephen White wrote:
> Other tests seem to call debug() in this case. Is returning a string here
really
> what you want to do?

Sorry, I just copied and pasted old codes.

https://codereview.chromium.org/1630683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.h
(right):

https://codereview.chromium.org/1630683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.h:77:
SkPath totalClipPath(const SkPath& path) const { return
m_clipList.totalClipPath(path); }
On 2016/01/25 15:37:35, Stephen White wrote:
> Naming nit: I think this function needs a verb, since it's not just an
accessor.
> Since it also intersects its argument, perhaps intersectPathWithClip()?

Done.

[Stephen White, 2016-01-27 17:16:40]

LGTM

[zino, 2016-01-27 17:36:46]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2016-01-27 17:37:16]

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

[commit-bot: I haz the power, 2016-01-27 18:51:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-27 18:53:04]

Description was changed from

==========
Canvas2d: addHitRegion() should take clipping region into account.

Current implementation ignores the clipping region when calling addHitRegion().
In the past, we had no way to get the current clip path from canvas state stack
in Blink side but it's been possible since display list 2d canvas feature.

BUG=387057
==========

to

==========
Canvas2d: addHitRegion() should take clipping region into account.

Current implementation ignores the clipping region when calling addHitRegion().
In the past, we had no way to get the current clip path from canvas state stack
in Blink side but it's been possible since display list 2d canvas feature.

BUG=387057

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

[commit-bot: I haz the power, 2016-01-27 18:53:05]

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

[Justin Novosad, 2016-02-02 21:36:09]

On 2016/01/27 18:53:05, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/da1acd5523fb34ff8c71a9777cf938e0cbbf276a
> Cr-Commit-Position: refs/heads/master@{#371832}

post commit lgtm
Side note: I don't know how impactful this would be, but there is a possible
optimization that would consist in caching the cumulative clip in ClipList to
make the intersection computation O(1) instead of O(N)

[zino, 2016-02-03 03:56:37]

On 2016/02/02 21:36:09, Justin Novosad wrote:
> On 2016/01/27 18:53:05, commit-bot: I haz the power wrote:
> > Patchset 3 (id:??) landed as
> > https://crrev.com/da1acd5523fb34ff8c71a9777cf938e0cbbf276a
> > Cr-Commit-Position: refs/heads/master@{#371832}
> 
> post commit lgtm
> Side note: I don't know how impactful this would be, but there is a possible
> optimization that would consist in caching the cumulative clip in ClipList to
> make the intersection computation O(1) instead of O(N)

Thank you for input.

I finish MouseEvent(or/and Touch) and event rerouting works first and then I'll
try it asap.