Canvas2d: The control in hit region should be a canvas fallback element.

Canvas2d: The control in hit region should be a canvas fallback element.

According to the hit region spec[1], the control parameter should be a supported
interactive canvas fallback element.

[1] https://html.spec.whatwg.org/multipage/scripting.html#supported-interactive-canvas-fallback-element

BUG=328961
Committed: https://crrev.com/bd95f3ca29d7e11a5c11def6b2a46ee0a23084de
Cr-Commit-Position: refs/heads/master@{#375312}

[zino, 2016-02-12 14:17:40]

Description was changed from

==========
Canvas2d: The control in hit region should be a canvas fallback element.

According to the hit region spec[1], the control parameter should be a supported
interactive canvas fallback element.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#supported-interactive-c...

BUG=328961
==========

to

==========
Canvas2d: The control in hit region should be a canvas fallback element.

According to the hit region spec[1], the control parameter should be a supported
interactive canvas fallback element.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#supported-interactive-c...

BUG=328961
==========

[zino, 2016-02-12 14:17:40]

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

[zino, 2016-02-12 14:17:53]

Please take a look.

Thank you!

[zino, 2016-02-12 15:04:08]

Patchset #3 (id:40001) has been deleted

[Justin Novosad, 2016-02-12 15:10:46]

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1045: if
(element.hasTagName(HTMLNames::aTag))
You should use the helpers (isHTMLAnchorElement) instead of calling hasTagName
directly. Same for other instances below.

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1046: return
!Traversal<HTMLImageElement>::firstWithin(element);
Please add a comment to explain this.

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1048: if
(element.hasTagName(HTMLNames::buttonTag))
Could be "else if". Same thing for "if" statements below.

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1054: ||
(inputElement.isTextButton() && !inputElement.isImage()))
Why do you need to check !inputElement.isImage()? Can an inputElement be a text
button and an image at the same time? It does not look that way from reading the
code of the various InputType classes.

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1066: if
(selectElement.multiple() || selectElement.size() > 1)
Add a comment to explain this condition.

[zino, 2016-02-12 16:40:34]

Thank you for review.

Could you review again?

Thank you.

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1045: if
(element.hasTagName(HTMLNames::aTag))
On 2016/02/12 15:10:45, Justin Novosad wrote:
> You should use the helpers (isHTMLAnchorElement) instead of calling hasTagName
> directly. Same for other instances below.

Done.

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1046: return
!Traversal<HTMLImageElement>::firstWithin(element);
On 2016/02/12 15:10:45, Justin Novosad wrote:
> Please add a comment to explain this.

Done.

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1048: if
(element.hasTagName(HTMLNames::buttonTag))
On 2016/02/12 15:10:45, Justin Novosad wrote:
> Could be "else if". Same thing for "if" statements below.

As far as I know, we don't use "else if" in Blink.
I'm not sure but the blink coding style[1] is still saying like that.

[1] Line breaking-"3":
https://www.chromium.org/blink/coding-style#TOC-Line-breaking

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1054: ||
(inputElement.isTextButton() && !inputElement.isImage()))
On 2016/02/12 15:10:45, Justin Novosad wrote:
> Why do you need to check !inputElement.isImage()? Can an inputElement be a
text
> button and an image at the same time? It does not look that way from reading
the
> code of the various InputType classes.

Done.

https://codereview.chromium.org/1692183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1066: if
(selectElement.multiple() || selectElement.size() > 1)
On 2016/02/12 15:10:45, Justin Novosad wrote:
> Add a comment to explain this condition.

Done.

[Justin Novosad, 2016-02-12 17:47:42]

lgtm

[zino, 2016-02-12 22:08:52]

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

[commit-bot: I haz the power, 2016-02-12 22:09:22]

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

[commit-bot: I haz the power, 2016-02-12 22:16:18]

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

[commit-bot: I haz the power, 2016-02-12 22:16:18]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[zino, 2016-02-12 23:59:34]

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

[commit-bot: I haz the power, 2016-02-13 00:00:29]

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

[commit-bot: I haz the power, 2016-02-13 01:18:06]

Description was changed from

==========
Canvas2d: The control in hit region should be a canvas fallback element.

According to the hit region spec[1], the control parameter should be a supported
interactive canvas fallback element.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#supported-interactive-c...

BUG=328961
==========

to

==========
Canvas2d: The control in hit region should be a canvas fallback element.

According to the hit region spec[1], the control parameter should be a supported
interactive canvas fallback element.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#supported-interactive-c...

BUG=328961
==========

[commit-bot: I haz the power, 2016-02-13 01:18:06]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-16 22:46:34]

Description was changed from

==========
Canvas2d: The control in hit region should be a canvas fallback element.

According to the hit region spec[1], the control parameter should be a supported
interactive canvas fallback element.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#supported-interactive-c...

BUG=328961
==========

to

==========
Canvas2d: The control in hit region should be a canvas fallback element.

According to the hit region spec[1], the control parameter should be a supported
interactive canvas fallback element.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#supported-interactive-c...

BUG=328961

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

[commit-bot: I haz the power, 2016-02-16 22:46:35]

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