support function in custom paint input arguments

support function as parameter in custom paint input arguments.

Example: paint(myPaintFunc, Calc(1px + 2px), rgb(10, 10, 10)) can be parsed and used.


BUG=672647
Review-Url: https://codereview.chromium.org/2698083003
Cr-Commit-Position: refs/heads/master@{#453767}
Committed: https://chromium.googlesource.com/chromium/src/+/6d5b46f03cec4b48aa5c6915a82952a600fe550c

[renjieliu1, 2017-02-16 00:20:02]

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

[renjieliu1, 2017-02-16 00:20:23]

Description was changed from

==========
support function in inputarguments


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

to

==========
support function in inputarguments


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

[renjieliu1, 2017-02-16 00:20:23]

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

[commit-bot: I haz the power, 2017-02-16 00:21:41]

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

[meade_UTC10, 2017-02-16 00:58:35]

Please give this CL a better description.

[commit-bot: I haz the power, 2017-02-16 01:21:00]

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

[commit-bot: I haz the power, 2017-02-16 01:21:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[renjieliu1, 2017-02-16 02:08:57]

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

[commit-bot: I haz the power, 2017-02-16 02:10:12]

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

[renjieliu1, 2017-02-16 02:11:32]

Description was changed from

==========
support function in inputarguments


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

to

==========
support function as parameter in custom paint input arguments.

Example: paint(myPaintFunc, Calc(1px + 2px), rgb(10, 10, 10)) can be parsed and
used.


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

[renjieliu1, 2017-02-16 02:12:34]

On 2017/02/16 00:58:35, Eddy wrote:
> Please give this CL a better description.

Add an example for clarification. Thank you! :)

[meade_UTC10, 2017-02-16 02:44:59]

https://codereview.chromium.org/2698083003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2698083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1169: //
like function calls.
Sounds like this comment needs to be updated.

https://codereview.chromium.org/2698083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1176: if
(args.peek().getBlockType() != CSSParserToken::BlockStart) {
There's a lot of nested ifs in this while loop now. Would it be better to split
this out into two functions? Something like this (incorrect) idea:

while(!args.atEnd()) {
  if (args.peek().getBlockType() != CSSParserToken::BlockStart) {
    // Handle function input
  } else {
    // Handle list input
  }
}

This might make it clearer where the function name is getting parsed - it's not
clear to me now

https://codereview.chromium.org/2698083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1180:
argumentTokens.push_back(args.peek());
So this pushes an open brace, then the contents of the block, then a closing
brace?

Why not

CSSParserTokenRange contents = args.consumeBlock();
argumentTokens.push_back(LeftBracketToken);
while (!contents.end())
  argumentTokens.push_back(contents.consume()
argumentTokens.push_back(RightBracketToken);

https://codereview.chromium.org/2698083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1185: //
Assume it is a function.
This comment is confusing, suggest removing it.

[renjieliu1, 2017-02-16 04:38:42]

thank you for the review!

https://codereview.chromium.org/2698083003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2698083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1169: //
like function calls.
On 2017/02/16 02:44:59, Eddy wrote:
> Sounds like this comment needs to be updated.

Done.

https://codereview.chromium.org/2698083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1176: if
(args.peek().getBlockType() != CSSParserToken::BlockStart) {
On 2017/02/16 02:44:59, Eddy wrote:
> There's a lot of nested ifs in this while loop now. Would it be better to
split
> this out into two functions? Something like this (incorrect) idea:
> 
> while(!args.atEnd()) {
>   if (args.peek().getBlockType() != CSSParserToken::BlockStart) {
>     // Handle function input
>   } else {
>     // Handle list input
>   }
> }
> 
> This might make it clearer where the function name is getting parsed - it's
not
> clear to me now

Done.

https://codereview.chromium.org/2698083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1180:
argumentTokens.push_back(args.peek());
On 2017/02/16 02:44:59, Eddy wrote:
> So this pushes an open brace, then the contents of the block, then a closing
> brace?
> 
> Why not
> 
> CSSParserTokenRange contents = args.consumeBlock();
> argumentTokens.push_back(LeftBracketToken);
> while (!contents.end())
>   argumentTokens.push_back(contents.consume()
> argumentTokens.push_back(RightBracketToken);
I totally agree that's what we should do, however, CSSParserToken treats
function name along with parenthesis as a single token.

Example rgb(1, 2, 3) => rgb(1, 2, 3 after the consumeBlock function.

https://codereview.chromium.org/2698083003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1185: //
Assume it is a function.
On 2017/02/16 02:44:59, Eddy wrote:
> This comment is confusing, suggest removing it.

Done.

[meade_UTC10, 2017-02-17 06:40:04]

https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:49: if
(args.peek().getBlockType() != CSSParserToken::BlockStart) {
Let's flip the if statement order - It's usually clearer to do

if (condition) {
  // conditionalFoo
} else {
  // notConditonalFoo
}

than

if (!condition) {
  // NotConditionalFoo
} else {
  // conditionalFoo
}

https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:52: //
Encounter function block.
Incidentally, what does it mean to have got here, but not be encountering a
function block?

https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:53:
argumentTokens.push_back(args.peek());
Perhaps, then, a comment to explain this?

if (args.peek().getBlockType() == CSSParserToken::BlockStart) {
  // Function block.
  // Push the function name and initial brace.
  argumentTokens.push_back(args.peek());
  CSSParserTokenRange contents = args.consumeBlock();
  while (!contents.atEnd()) {
     // Push function arguments
     argumentTokens.push_back(contents.consume());
  }
  argumentTokens.push_back(
      CSSParserToken(RightParenthesisToken, CSSParserToken::BlockEnd));
} else {
...
}

[renjieliu1, 2017-02-19 04:57:58]

thank you for the review!

https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:49: if
(args.peek().getBlockType() != CSSParserToken::BlockStart) {
On 2017/02/17 06:40:04, Eddy wrote:
> Let's flip the if statement order - It's usually clearer to do
> 
> if (condition) {
>   // conditionalFoo
> } else {
>   // notConditonalFoo
> }
> 
> than
> 
> if (!condition) {
>   // NotConditionalFoo
> } else {
>   // conditionalFoo
> }

Done.

https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:52: //
Encounter function block.
On 2017/02/17 06:40:04, Eddy wrote:
> Incidentally, what does it mean to have got here, but not be encountering a
> function block?

I think if it is not a function but got here, in the variable resolving phase,
it will fail, but since we don't have knowledge about the argument types here,
we can't check anything here

https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:53:
argumentTokens.push_back(args.peek());
On 2017/02/17 06:40:04, Eddy wrote:
> Perhaps, then, a comment to explain this?
> 
> if (args.peek().getBlockType() == CSSParserToken::BlockStart) {
>   // Function block.
>   // Push the function name and initial brace.
>   argumentTokens.push_back(args.peek());
>   CSSParserTokenRange contents = args.consumeBlock();
>   while (!contents.atEnd()) {
>      // Push function arguments
>      argumentTokens.push_back(contents.consume());
>   }
>   argumentTokens.push_back(
>       CSSParserToken(RightParenthesisToken, CSSParserToken::BlockEnd));
> } else {
> ...
> }

thank you for the suggestion!

[meade_UTC10, 2017-02-22 06:56:22]

lgtm

https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:52: //
Encounter function block.
On 2017/02/19 04:57:58, renjieliu1 wrote:
> On 2017/02/17 06:40:04, Eddy wrote:
> > Incidentally, what does it mean to have got here, but not be encountering a
> > function block?
> 
> I think if it is not a function but got here, in the variable resolving phase,
> it will fail, but since we don't have knowledge about the argument types here,
> we can't check anything here
Could you please add that as a comment? It still seems a bit mysterious if
you're just reading the code :)

[renjieliu1, 2017-02-23 02:34:42]

On 2017/02/22 06:56:22, Eddy wrote:
> lgtm
> 
>
https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
> (right):
> 
>
https://codereview.chromium.org/2698083003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:52: //
> Encounter function block.
> On 2017/02/19 04:57:58, renjieliu1 wrote:
> > On 2017/02/17 06:40:04, Eddy wrote:
> > > Incidentally, what does it mean to have got here, but not be encountering
a
> > > function block?
> > 
> > I think if it is not a function but got here, in the variable resolving
phase,
> > it will fail, but since we don't have knowledge about the argument types
here,
> > we can't check anything here
> Could you please add that as a comment? It still seems a bit mysterious if
> you're just reading the code :)

sure thing!

[renjieliu1, 2017-02-23 02:36:15]

thank you for the review!

[renjieliu1, 2017-02-28 09:44:41]

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

[commit-bot: I haz the power, 2017-02-28 09:44:53]

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-02-28 11:07:50]

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

[commit-bot: I haz the power, 2017-02-28 11:07:50]

Dry run: This issue passed the CQ dry run.

[renjieliu1, 2017-02-28 11:25:10]

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

[ikilpatrick, 2017-02-28 23:33:31]

lgtm

[renjieliu1, 2017-03-01 00:09:45]

The CQ bit was checked by renjieliu@chromium.org

[renjieliu1, 2017-03-01 00:09:46]

The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org
Link to the patchset: https://codereview.chromium.org/2698083003/#ps120001
(title: "add comment")

[commit-bot: I haz the power, 2017-03-01 00:10: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-03-01 00:22:26]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488326985927610,
"parent_rev": "245d300c64b99ed80af0fc2556bdd41f654d9371", "commit_rev":
"6d5b46f03cec4b48aa5c6915a82952a600fe550c"}

[commit-bot: I haz the power, 2017-03-01 00:23:40]

Description was changed from

==========
support function as parameter in custom paint input arguments.

Example: paint(myPaintFunc, Calc(1px + 2px), rgb(10, 10, 10)) can be parsed and
used.


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

to

==========
support function as parameter in custom paint input arguments.

Example: paint(myPaintFunc, Calc(1px + 2px), rgb(10, 10, 10)) can be parsed and
used.


BUG=672647

Review-Url: https://codereview.chromium.org/2698083003
Cr-Commit-Position: refs/heads/master@{#453767}
Committed:
https://chromium.googlesource.com/chromium/src/+/6d5b46f03cec4b48aa5c6915a829...
==========

[commit-bot: I haz the power, 2017-03-01 00:23:41]

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