Parse input argument types and store the argument types in CSSPaintDefinition.

Parse input argument types and store the argument types in CSSPaintDefinition.

BUG=672647
Review-Url: https://codereview.chromium.org/2561773003
Cr-Commit-Position: refs/heads/master@{#444657}
Committed: https://chromium.googlesource.com/chromium/src/+/5d90cabdbd8ea0727b661c2f4d5be5e13151e94c

[renjieliu1, 2016-12-09 07:36:14]

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

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

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

[renjieliu1, 2016-12-09 07:36:45]

Description was changed from

==========
Parse input argument types and store the argument types in CSSPaintDefinition.

BUG=672647
==========

to

==========
Parse input argument types and store the argument types in CSSPaintDefinition.

BUG=672647
==========

[renjieliu1, 2016-12-09 07:36:45]

renjieliu@chromium.org changed reviewers:
+ ikilpatrick@chromium.org, meade@chromium.org, timloh@chromium.org

[commit-bot: I haz the power, 2016-12-09 09:39:52]

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

[commit-bot: I haz the power, 2016-12-09 09:39:52]

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2016-12-12 09:49:26]

It's better to pick one reviewer at a time, or we'll all think that someone else
will do it! I'm out this week, so it looks like Ian might be the best person to
start with for this bit of code. Once you have a first LGTM, then you can add
the OWNERS reviewer.

[renjieliu1, 2016-12-12 23:00:03]

On 2016/12/12 09:49:26, Eddy (OOO until 19 Dec) wrote:
> It's better to pick one reviewer at a time, or we'll all think that someone
else
> will do it! I'm out this week, so it looks like Ian might be the best person
to
> start with for this bit of code. Once you have a first LGTM, then you can add
> the OWNERS reviewer.

Thank you for the advice! I will let Ian take the first pass. PS: it's pretty
cool that OOO shows on the reviewer :)

[renjieliu1, 2016-12-12 23:00:18]

renjieliu@chromium.org changed reviewers:
- meade@chromium.org, timloh@chromium.org

[renjieliu1, 2017-01-09 05:10:04]

Issue 2550183006 is checked in :)

[renjieliu1, 2017-01-10 00:56:50]

renjieliu@chromium.org changed reviewers:
+ nainar@chromium.org

[ikilpatrick, 2017-01-12 05:36:04]

Thanks for working on this again :)

This looks really good!

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:123: if
(!syntaxDescriptor.isValid())
It would be good to throw a TypeError here.

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp (right):

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:16: #include
"testing/gmock/include/gmock/gmock.h"
not needed?

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:93: }
instead of these tests, can you write LayoutTests instead?

see:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/csspaint/...

[ikilpatrick, 2017-01-12 18:17:03]

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:108:
v8::Local<v8::Value> inputArgumentTypeValues;
we should also only attempt to parse this if the runtime flag is enabled.

[renjieliu1, 2017-01-13 04:16:13]

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

[renjieliu1, 2017-01-13 04:17:50]

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

[renjieliu1, 2017-01-13 04:18:23]

thank you! :D

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
(right):

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:108:
v8::Local<v8::Value> inputArgumentTypeValues;
On 2017/01/12 18:17:03, ikilpatrick wrote:
> we should also only attempt to parse this if the runtime flag is enabled.

Done.

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp:123: if
(!syntaxDescriptor.isValid())
On 2017/01/12 05:36:04, ikilpatrick wrote:
> It would be good to throw a TypeError here.

Done.

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp (right):

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:16: #include
"testing/gmock/include/gmock/gmock.h"
On 2017/01/12 05:36:04, ikilpatrick wrote:
> not needed?

'EXPECT_THAT' is actually from gmock.h

https://codereview.chromium.org/2561773003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:93: }
On 2017/01/12 05:36:04, ikilpatrick wrote:
> instead of these tests, can you write LayoutTests instead?
> 
> see:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/csspaint/...

Thank you! layout test added!:)

[commit-bot: I haz the power, 2017-01-13 04:18:30]

Dry run: 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, 2017-01-13 06:13:00]

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

[commit-bot: I haz the power, 2017-01-13 06:13:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ikilpatrick, 2017-01-13 18:50:18]

https://codereview.chromium.org/2561773003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/csspaint/parse-input-arguments.html (right):

https://codereview.chromium.org/2561773003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/csspaint/parse-input-arguments.html:11:
tests.reduce(function(chain, obj) {
can you pull this out into a common js file?

https://codereview.chromium.org/2561773003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp (right):

https://codereview.chromium.org/2561773003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:16: #include
"testing/gmock/include/gmock/gmock.h"
oh, just use EXPECT_EQ in gtest.h and remove this depedency since we aren't
mocking anything.

https://codereview.chromium.org/2561773003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:81:
EXPECT_THAT(definition->inputArgumentTypes().size(), 1);
So these tests doesn't add that much, it would be better if it also checked the
parsed inputArgumentTypes where of the correct type.

If not, i would delete.

[renjieliu1, 2017-01-15 05:05:12]

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

[renjieliu1, 2017-01-15 05:05:20]

https://codereview.chromium.org/2561773003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/csspaint/parse-input-arguments.html (right):

https://codereview.chromium.org/2561773003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/csspaint/parse-input-arguments.html:11:
tests.reduce(function(chain, obj) {
On 2017/01/13 18:50:18, ikilpatrick wrote:
> can you pull this out into a common js file?

Done.

https://codereview.chromium.org/2561773003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp (right):

https://codereview.chromium.org/2561773003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:16: #include
"testing/gmock/include/gmock/gmock.h"
On 2017/01/13 18:50:18, ikilpatrick wrote:
> oh, just use EXPECT_EQ in gtest.h and remove this depedency since we aren't
> mocking anything.

Done.

https://codereview.chromium.org/2561773003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp:81:
EXPECT_THAT(definition->inputArgumentTypes().size(), 1);
On 2017/01/13 18:50:18, ikilpatrick wrote:
> So these tests doesn't add that much, it would be better if it also checked
the
> parsed inputArgumentTypes where of the correct type.
> 
> If not, i would delete.

CSSSyntaxDescriptor hasn't implemented a compare method, so it's hard to check
the type for now. I will delete the two tests.

[commit-bot: I haz the power, 2017-01-15 05:05:25]

Dry run: 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, 2017-01-15 06:31:47]

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

[commit-bot: I haz the power, 2017-01-15 06:31:48]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ikilpatrick, 2017-01-18 01:32:32]

Thanks! This looks great. lgtm

https://codereview.chromium.org/2561773003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h (right):

https://codereview.chromium.org/2561773003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h:22: // the
author.
If you like this comment could be expanded now that this class is growing, an
explanation of the things it holds onto here and what they are used for. 

Or in a follow up patch if you like :)

[renjieliu1, 2017-01-18 02:52:34]

On 2017/01/18 01:32:32, ikilpatrick wrote:
> Thanks! This looks great. lgtm
> 
>
https://codereview.chromium.org/2561773003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h (right):
> 
>
https://codereview.chromium.org/2561773003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h:22: // the
> author.
> If you like this comment could be expanded now that this class is growing, an
> explanation of the things it holds onto here and what they are used for. 
> 
> Or in a follow up patch if you like :)

Added more comments. :)

[renjieliu1, 2017-01-18 02:56:11]

renjieliu@chromium.org changed reviewers:
+ timloh@chromium.org

[renjieliu1, 2017-01-18 02:59:38]

renjieliu@chromium.org changed reviewers:
+ sashab@chromium.org
- timloh@chromium.org

[renjieliu1, 2017-01-18 03:00:29]

Add Sasha for owner review

[sashab, 2017-01-18 04:15:46]

sashab@chromium.org changed reviewers:
+ meade@chromium.org

[sashab, 2017-01-18 04:15:47]

core/ rs LGTM

[renjieliu1, 2017-01-18 23:14:15]

The CQ bit was checked by renjieliu@chromium.org

[renjieliu1, 2017-01-18 23:14:15]

The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org,
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2561773003/#ps100001
(title: "Add comments.")

[commit-bot: I haz the power, 2017-01-18 23:15:09]

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, 2017-01-19 01:09:09]

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

[commit-bot: I haz the power, 2017-01-19 01:09:10]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[renjieliu1, 2017-01-19 03:29:45]

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

[commit-bot: I haz the power, 2017-01-19 03:30:19]

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

[renjieliu1, 2017-01-19 03:32:20]

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

[commit-bot: I haz the power, 2017-01-19 03:33:05]

Dry run: 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, 2017-01-19 04:55:39]

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

[commit-bot: I haz the power, 2017-01-19 04:55:40]

Dry run: This issue passed the CQ dry run.

[renjieliu1, 2017-01-19 05:32:43]

The CQ bit was checked by renjieliu@chromium.org

[renjieliu1, 2017-01-19 05:32:43]

The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org,
ikilpatrick@chromium.org, sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/2561773003/#ps120001
(title: "update tests")

[commit-bot: I haz the power, 2017-01-19 05:33:10]

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, 2017-01-19 05:39:12]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484803963338620,
"parent_rev": "a6d7142534246d479668c7ad7122d7864979e2b8", "commit_rev":
"5d90cabdbd8ea0727b661c2f4d5be5e13151e94c"}

[commit-bot: I haz the power, 2017-01-19 05:39:42]

Description was changed from

==========
Parse input argument types and store the argument types in CSSPaintDefinition.

BUG=672647
==========

to

==========
Parse input argument types and store the argument types in CSSPaintDefinition.

BUG=672647

Review-Url: https://codereview.chromium.org/2561773003
Cr-Commit-Position: refs/heads/master@{#444657}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d90cabdbd8ea0727b661c2f4d5b...
==========

[commit-bot: I haz the power, 2017-01-19 05:39:43]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/5d90cabdbd8ea0727b661c2f4d5b...