Refactored out property specific logic in ConsumeBorderImageSlice.

Refactored out property specific logic in ConsumeBorderImageSlice.

Replaced property specific logic in ConsumeBorderImageSlice with a bool
arg specifying whether the property has fill set by default. This
removes the need to pass property to the method, moving the
property specific logic to the calling method, which can later be
separated out into property APIs.

BUG=668012
Review-Url: https://codereview.chromium.org/2860283003
Cr-Commit-Position: refs/heads/master@{#470256}
Committed: https://chromium.googlesource.com/chromium/src/+/2bef7b29ce7529f8512e76226f8690d67e2a0b97

[Bugs Nash, 2017-05-05 04:02:05]

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

[commit-bot: I haz the power, 2017-05-05 04:02:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bugs Nash, 2017-05-05 04:04:18]

bugsnash@chromium.org changed reviewers:
+ jiameng@chromium.org

[Bugs Nash, 2017-05-05 04:04:19]

Note that I've deleted the property specific FIXME comments from
ConsumeBorderImageSlice because they didn't seem actionable. I'm open to putting
them back in, but I would like to merge them and either reword to remove
property specific language or move them to the calling methods where property
specific logic has been moved.

[Jia, 2017-05-05 05:10:24]

On 2017/05/05 04:04:19, Bugs Nash wrote:
> Note that I've deleted the property specific FIXME comments from
> ConsumeBorderImageSlice because they didn't seem actionable. I'm open to
putting
> them back in, but I would like to merge them and either reword to remove
> property specific language or move them to the calling methods where property
> specific logic has been moved.

LGTM with one comment.

[Jia, 2017-05-05 05:10:49]

https://codereview.chromium.org/2860283003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2860283003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2495: bool
default_fill,
Having both "property" and "default_fill" seems redundant because "default_fill"
is a function of "property". If the plan for the next step is to remove
"property" from the param list and expose "default_fill" only, then please add a
TODO so that "property" will be removed in some later cl.

[Bugs Nash, 2017-05-05 05:18:05]

addressed review

[Bugs Nash, 2017-05-05 05:18:37]

bugsnash@chromium.org changed reviewers:
+ suzyh@chromium.org

[Bugs Nash, 2017-05-05 05:18:37]

+suzyh for owners

https://codereview.chromium.org/2860283003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2860283003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2495: bool
default_fill,
On 2017/05/05 at 05:10:49, Jia wrote:
> Having both "property" and "default_fill" seems redundant because
"default_fill" is a function of "property". If the plan for the next step is to
remove "property" from the param list and expose "default_fill" only, then
please add a TODO so that "property" will be removed in some later cl.

Done

[suzyh_UTC10 (ex-contributor), 2017-05-05 05:47:12]

lgtm with nits

https://codereview.chromium.org/2860283003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2860283003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:966: repeat,
true))
I'd suggest following this sort of protocol here:

...   repeat, true /* default_fill */))

Or use a named constant instead of literal "true".

Likewise for the other literal bools.

https://codereview.chromium.org/2860283003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2494: //
TODO(bugsnash): refactor out property specific logic from this method
Consider referring to a particular bug in the TODO.

[Bugs Nash, 2017-05-05 06:23:41]

The CQ bit was checked by bugsnash@chromium.org

[Bugs Nash, 2017-05-05 06:23:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
jiameng@chromium.org, suzyh@chromium.org
Link to the patchset: https://codereview.chromium.org/2860283003/#ps40001
(title: "addressed review")

[commit-bot: I haz the power, 2017-05-05 06:23:53]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bugs Nash, 2017-05-05 06:23:55]

https://codereview.chromium.org/2860283003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2860283003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:966: repeat,
true))
On 2017/05/05 at 05:47:12, suzyh_UTC10 wrote:
> I'd suggest following this sort of protocol here:
> 
> ...   repeat, true /* default_fill */))
> 
> Or use a named constant instead of literal "true".
> 
> Likewise for the other literal bools.

Done

https://codereview.chromium.org/2860283003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2494: //
TODO(bugsnash): refactor out property specific logic from this method
On 2017/05/05 at 05:47:12, suzyh_UTC10 wrote:
> Consider referring to a particular bug in the TODO.

Done

[commit-bot: I haz the power, 2017-05-05 08:12:32]

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

[commit-bot: I haz the power, 2017-05-05 08:12:33]

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_...)

[Bugs Nash, 2017-05-08 00:20:30]

fixed bug - argument order

[Bugs Nash, 2017-05-08 00:20:40]

The CQ bit was checked by bugsnash@chromium.org

[Bugs Nash, 2017-05-08 00:20:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
jiameng@chromium.org, suzyh@chromium.org
Link to the patchset: https://codereview.chromium.org/2860283003/#ps60001
(title: "fixed bug - argument order")

[commit-bot: I haz the power, 2017-05-08 00:20:51]

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-05-08 01:39:17]

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

[commit-bot: I haz the power, 2017-05-08 01:39:18]

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

[Bugs Nash, 2017-05-08 02:57:57]

The CQ bit was checked by bugsnash@chromium.org

[commit-bot: I haz the power, 2017-05-08 02:58:16]

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-05-08 04:14:15]

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

[commit-bot: I haz the power, 2017-05-08 04:14:16]

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

[Bugs Nash, 2017-05-08 04:27:44]

The CQ bit was checked by bugsnash@chromium.org

[commit-bot: I haz the power, 2017-05-08 04:28:14]

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-05-08 06:00:28]

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

[commit-bot: I haz the power, 2017-05-08 06:00:29]

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

[Bugs Nash, 2017-05-09 04:25:11]

The CQ bit was checked by bugsnash@chromium.org

[commit-bot: I haz the power, 2017-05-09 04:26:33]

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-05-09 06:54:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494303911373190,
"parent_rev": "8044572fec6d8b9f339507db22c9e33c18bcde04", "commit_rev":
"2bef7b29ce7529f8512e76226f8690d67e2a0b97"}

[commit-bot: I haz the power, 2017-05-09 06:54:28]

Description was changed from

==========
Refactored out property specific logic in ConsumeBorderImageSlice.

Replaced property specific logic in ConsumeBorderImageSlice with a bool
arg specifying whether the property has fill set by default. This
removes the need to pass property to the method, moving the
property specific logic to the calling method, which can later be
separated out into property APIs.

BUG=668012
==========

to

==========
Refactored out property specific logic in ConsumeBorderImageSlice.

Replaced property specific logic in ConsumeBorderImageSlice with a bool
arg specifying whether the property has fill set by default. This
removes the need to pass property to the method, moving the
property specific logic to the calling method, which can later be
separated out into property APIs.

BUG=668012

Review-Url: https://codereview.chromium.org/2860283003
Cr-Commit-Position: refs/heads/master@{#470256}
Committed:
https://chromium.googlesource.com/chromium/src/+/2bef7b29ce7529f8512e76226f86...
==========

[commit-bot: I haz the power, 2017-05-09 06:54:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2bef7b29ce7529f8512e76226f86...