add HitTestCanvasResult Class instend of std::pair<Element*, String>

add HitTestCanvasResult Class instend of std::pair<Element*, String>

HitRegion is need to many information to expand(ie. cursor, parent_id, label, etc) in canvas2d.
"std::pair<element*, string>" can't have a lot of information.
so HitTestCanvasResult was added to expend.

BUG=592947
Committed: https://crrev.com/3192926ec460259b66e2d6a6bc07e05049105eed
Cr-Commit-Position: refs/heads/master@{#413330}

[Hwanseung Lee, 2016-08-09 15:57:16]

Description was changed from

==========
add HitTestCanvasResult Class instend of std::pair<Element*, String>

HitRegion is need to many information to expand(ie. cursor, parent_id, label,
etc) in canvas2d.
"std::pair<element*, string>" can't have a lot of information.
so HitTestCanvasResult was added to expend.

BUG=592947
==========

to

==========
add HitTestCanvasResult Class instend of std::pair<Element*, String>

HitRegion is need to many information to expand(ie. cursor, parent_id, label,
etc) in canvas2d.
"std::pair<element*, string>" can't have a lot of information.
so HitTestCanvasResult was added to expend.

BUG=592947
==========

[Hwanseung Lee, 2016-08-09 15:57:16]

hs1217.lee@samsung.com changed reviewers:
+ jinho.bang@samsung.com

[Hwanseung Lee, 2016-08-09 15:58:53]

could you review this commit?

[zino, 2016-08-19 13:34:10]

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

[zino, 2016-08-19 13:45:43]

Dear Justin,

This patch is not perfect yet but

I think it's better to move the hit region related files to core directory.

Many informations(id, control, cursor and other things) are used in core/event.

So, if the feature is in core directory, we can avoid unnecessary copies.

Also, you suggested opinion to move the related methods to canvas element
(instead of canvas2d context). -
https://github.com/whatwg/html/issues/1030#issuecomment-208954414

What do you think?

[Justin Novosad, 2016-08-19 14:03:41]

On 2016/08/19 13:45:43, zino wrote:
> Dear Justin,
> 
> This patch is not perfect yet but
> 
> I think it's better to move the hit region related files to core directory.
> 
> Many informations(id, control, cursor and other things) are used in
core/event.
> 
> So, if the feature is in core directory, we can avoid unnecessary copies.
> 
> Also, you suggested opinion to move the related methods to canvas element
> (instead of canvas2d context). -
> https://github.com/whatwg/html/issues/1030#issuecomment-208954414
> 
> What do you think?

This patch is good as is.  As long as it does not break any tests

lgtm

[zino, 2016-08-19 15:09:21]

On 2016/08/19 14:03:41, Justin Novosad wrote:
> This patch is good as is.  As long as it does not break any tests
> 

Thank you for answer.

My opinion was that it's better to move the hit region related files to core
directory for performance reason. (but follow-up CL)
Current implementation only takes 'id' and 'control' but hit regions can expand
more.
(such as cursor: Hwanseung is already working on another CL)
Then, it can be heavy to copy the hit region object from module to core.
For example, if JS author uses mousemove event rerouting, its performance will
be poor.

Could you please answer my opinion?

[Justin Novosad, 2016-08-19 16:51:41]

On 2016/08/19 15:09:21, zino wrote:
> On 2016/08/19 14:03:41, Justin Novosad wrote:
> > This patch is good as is.  As long as it does not break any tests
> > 
> 
> Thank you for answer.
> 
> My opinion was that it's better to move the hit region related files to core
> directory for performance reason. (but follow-up CL)
> Current implementation only takes 'id' and 'control' but hit regions can
expand
> more.
> (such as cursor: Hwanseung is already working on another CL)
> Then, it can be heavy to copy the hit region object from module to core.
> For example, if JS author uses mousemove event rerouting, its performance will
> be poor.
> 
> Could you please answer my opinion?

I agree with that. But one small step at a time is preferable.  Large patches
are harder to deal with.

[zino, 2016-08-19 17:17:35]

On 2016/08/19 16:51:41, Justin Novosad wrote:
> I agree with that. But one small step at a time is preferable.  Large patches
> are harder to deal with.

Sure, I agree with you.

Thanks!

lgtm

[zino, 2016-08-19 17:41:08]

Sorry, not lgtm. (to prevent CQ)

I think you should fix minor problems in this CL before landing.

I'll leave some comments.. Sorry Hwanseung.

[zino, 2016-08-19 18:00:07]

https://codereview.chromium.org/2229743003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2229743003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:3185:
result.setHitCanvasRegion(hitTestCanvasResult);
Do we really need this? HitTestResult only has setter in your patch.

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

https://codereview.chromium.org/2229743003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/HitTestResult.cpp:229: void
HitTestResult::setHitCanvasRegion(HitTestCanvasResult* n)
You are saving HitTestCanvasResult to HitTestResult but it can't be used because
there is no getter.
Also, you are not using HitTestResult directly in this patch. So, You can remove
this?

[Hwanseung Lee, 2016-08-20 06:03:54]

@zino, i remove some code follow your guide.
could you review again?

https://codereview.chromium.org/2229743003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2229743003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:3185:
result.setHitCanvasRegion(hitTestCanvasResult);
On 2016/08/19 18:00:07, zino wrote:
> Do we really need this? HitTestResult only has setter in your patch.

Done.

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

https://codereview.chromium.org/2229743003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/HitTestResult.cpp:229: void
HitTestResult::setHitCanvasRegion(HitTestCanvasResult* n)
On 2016/08/19 18:00:07, zino wrote:
> You are saving HitTestCanvasResult to HitTestResult but it can't be used
because
> there is no getter.
> Also, you are not using HitTestResult directly in this patch. So, You can
remove
> this?

Done.

[zino, 2016-08-20 06:12:52]

lgtm!

[Hwanseung Lee, 2016-08-20 06:13:20]

The CQ bit was checked by hs1217.lee@samsung.com

[Hwanseung Lee, 2016-08-20 06:13:20]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2229743003/#ps20001
(title: "add HitTestCanvasResult Class instend of std::pair<Element*, String>")

[commit-bot: I haz the power, 2016-08-20 06:13:35]

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-20 07:37:21]

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

[commit-bot: I haz the power, 2016-08-20 07:37:22]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Hwanseung Lee, 2016-08-20 07:51:10]

The CQ bit was checked by hs1217.lee@samsung.com

[commit-bot: I haz the power, 2016-08-20 07:51:23]

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-20 08:29:50]

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

[commit-bot: I haz the power, 2016-08-20 08:29:52]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Hwanseung Lee, 2016-08-20 08:30:53]

The CQ bit was checked by hs1217.lee@samsung.com

[commit-bot: I haz the power, 2016-08-20 08:31:03]

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-20 09:57:28]

Description was changed from

==========
add HitTestCanvasResult Class instend of std::pair<Element*, String>

HitRegion is need to many information to expand(ie. cursor, parent_id, label,
etc) in canvas2d.
"std::pair<element*, string>" can't have a lot of information.
so HitTestCanvasResult was added to expend.

BUG=592947
==========

to

==========
add HitTestCanvasResult Class instend of std::pair<Element*, String>

HitRegion is need to many information to expand(ie. cursor, parent_id, label,
etc) in canvas2d.
"std::pair<element*, string>" can't have a lot of information.
so HitTestCanvasResult was added to expend.

BUG=592947
==========

[commit-bot: I haz the power, 2016-08-20 09:57:30]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-20 09:58:37]

Description was changed from

==========
add HitTestCanvasResult Class instend of std::pair<Element*, String>

HitRegion is need to many information to expand(ie. cursor, parent_id, label,
etc) in canvas2d.
"std::pair<element*, string>" can't have a lot of information.
so HitTestCanvasResult was added to expend.

BUG=592947
==========

to

==========
add HitTestCanvasResult Class instend of std::pair<Element*, String>

HitRegion is need to many information to expand(ie. cursor, parent_id, label,
etc) in canvas2d.
"std::pair<element*, string>" can't have a lot of information.
so HitTestCanvasResult was added to expend.

BUG=592947

Committed: https://crrev.com/3192926ec460259b66e2d6a6bc07e05049105eed
Cr-Commit-Position: refs/heads/master@{#413330}
==========

[commit-bot: I haz the power, 2016-08-20 09:58:39]

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