Implement ConsumeShorthandViaLonghand to parse shorthand by calling longhand ParseSingleValue

Implement ConsumeShorthandViaLonghand to parse shorthand by calling longhand ParseSingleValue

Consume4Values, Consume2Values and ConsumeShorthandGreedily all call 
shorthand's constituent longhands' ParseSingleValue. This cl merges 
these functions by implementing ConsumeShorthandViaLonghand, which 
will be called by the three existing "Consume*" functions.

Later, we will move ConsumeShorthandViaLonghand to CSSPropertyParserHelpers 
class so that this function will call property API's parseSingleValue. 
At the moment, we are only merging the logic and will continue to call 
CSSPropertyParsers' ParseSingleValue.

BUG=668012

[Jia, 2017-06-23 04:18:14]

Description was changed from

==========
Merge Consume4Values, Consume2Values and ConsumeShorthandGreedily by
implementing one funtion that performs all logic

BUG=
==========

to

==========
Implement ConsumeShorthandViaLonghand to parse shorthand by calling longhand
ParseSingleValue

Consume4Values, Consume2Values and ConsumeShorthandGreedily all call 
shorthand's constituent longhands' ParseSingleValue. This cl merges 
these functions by implementing ConsumeShorthandViaLonghand, which 
will be called by the three existing "Consume*" functions.

Later, we will move ConsumeShorthandViaLonghand to CSSPropertyParserHelpers 
class so that this function will call property API's parseSingleValue. 
At the moment, we are only merging the logic and will continue to call 
CSSPropertyParsers' ParseSingleValue.

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

[Jia, 2017-06-23 04:18:43]

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

[Jia, 2017-06-23 04:18:50]

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

[commit-bot: I haz the power, 2017-06-23 04:19:00]

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-06-23 04:35:31]

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

[commit-bot: I haz the power, 2017-06-23 04:35:31]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Jia, 2017-06-23 05:38:32]

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

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

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-06-23 05:53:35]

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

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

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Jia, 2017-06-25 23:38:57]

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

[commit-bot: I haz the power, 2017-06-25 23:39:07]

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-06-26 01:35:40]

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

[commit-bot: I haz the power, 2017-06-26 01:35:40]

Dry run: This issue passed the CQ dry run.

[Bugs Nash, 2017-06-26 05:23:50]

I find this new general method confusing, while the existing methods were clear
to me. I don't think it makes sense to merge them together, as they each do
distinct things (especially the ParseShorthandGreedily method).

Is this refactor necessary to move parsing logic into property APIs? It doesn't
seem like it would be to me.

[Jia, 2017-06-26 05:35:56]

On 2017/06/26 05:23:50, Bugs Nash wrote:
> I find this new general method confusing, while the existing methods were
clear
> to me. I don't think it makes sense to merge them together, as they each do
> distinct things (especially the ParseShorthandGreedily method).
> 
> Is this refactor necessary to move parsing logic into property APIs? It
doesn't
> seem like it would be to me.

Thanks for the comment! As I understand it, project ribbon aims to impl APIs as
well as refactor/clean up the code. I see there's a lot of common logic between
the three methods and so duplicated code (e.g. they all call longhands parsing
methods), hence I think it's appropriate to refactor them. An important benefit
is to see more clearly which shorthands get parsed via longhand parsing methods.

[Bugs Nash, 2017-06-26 06:03:06]

On 2017/06/26 at 05:35:56, jiameng wrote:
> On 2017/06/26 05:23:50, Bugs Nash wrote:
> > I find this new general method confusing, while the existing methods were
clear
> > to me. I don't think it makes sense to merge them together, as they each do
> > distinct things (especially the ParseShorthandGreedily method).
> > 
> > Is this refactor necessary to move parsing logic into property APIs? It
doesn't
> > seem like it would be to me.
> 
> Thanks for the comment! As I understand it, project ribbon aims to impl APIs
as well as refactor/clean up the code. I see there's a lot of common logic
between the three methods and so duplicated code (e.g. they all call longhands
parsing methods), hence I think it's appropriate to refactor them. An important
benefit is to see more clearly which shorthands get parsed via longhand parsing
methods.

Thanks for explaining :) I still think that the code is clearer as is, I don't
think that duplicating calls to ParseSingleValue is a problem since this is a 1
line call. Adding other Ribbon devs to this review for second/third opinions.

More generally, I don't think that Ribbon's goal explicitly includes doing
ancillary code clean ups (although of course cleaning up code is good no matter
what you're working on!)

[Bugs Nash, 2017-06-26 06:04:05]

bugsnash@chromium.org changed reviewers:
+ meade@chromium.org, rjwright@chromium.org

[Bugs Nash, 2017-06-26 06:04:05]

+rjwright and meade for opinions on whether this is a code health improvement or
not

[Bugs Nash, 2017-06-26 06:06:09]

On 2017/06/26 at 05:35:56, jiameng wrote:
> On 2017/06/26 05:23:50, Bugs Nash wrote:
> > I find this new general method confusing, while the existing methods were
clear
> > to me. I don't think it makes sense to merge them together, as they each do
> > distinct things (especially the ParseShorthandGreedily method).
> > 
> > Is this refactor necessary to move parsing logic into property APIs? It
doesn't
> > seem like it would be to me.
> 
> Thanks for the comment! As I understand it, project ribbon aims to impl APIs
as well as refactor/clean up the code. I see there's a lot of common logic
between the three methods and so duplicated code (e.g. they all call longhands
parsing methods), hence I think it's appropriate to refactor them. An important
benefit is to see more clearly which shorthands get parsed via longhand parsing
methods.


Oh, if you think that it should be more clear when shorthand parsing calls
through to longhand parsing, might I suggest changing the method names to
ConsumeLonghandsGreedily, Consume2Longhands, and Consume4Longhands or something
to that effect?

[Jia, 2017-06-26 06:10:41]

On 2017/06/26 06:06:09, Bugs Nash wrote:
> On 2017/06/26 at 05:35:56, jiameng wrote:
> > On 2017/06/26 05:23:50, Bugs Nash wrote:
> > > I find this new general method confusing, while the existing methods were
> clear
> > > to me. I don't think it makes sense to merge them together, as they each
do
> > > distinct things (especially the ParseShorthandGreedily method).
> > > 
> > > Is this refactor necessary to move parsing logic into property APIs? It
> doesn't
> > > seem like it would be to me.
> > 
> > Thanks for the comment! As I understand it, project ribbon aims to impl APIs
> as well as refactor/clean up the code. I see there's a lot of common logic
> between the three methods and so duplicated code (e.g. they all call longhands
> parsing methods), hence I think it's appropriate to refactor them. An
important
> benefit is to see more clearly which shorthands get parsed via longhand
parsing
> methods.
> 
> 
> Oh, if you think that it should be more clear when shorthand parsing calls
> through to longhand parsing, might I suggest changing the method names to
> ConsumeLonghandsGreedily, Consume2Longhands, and Consume4Longhands or
something
> to that effect?

Thanks again for the comment! These names sound good. I still think we should
refactor the code to remove duplicates when possible. :) Let's see what rjwright
and meade think.

[meade_UTC10, 2017-06-28 07:29:02]

On 2017/06/26 06:10:41, Jia wrote:
> On 2017/06/26 06:06:09, Bugs Nash wrote:
> > On 2017/06/26 at 05:35:56, jiameng wrote:
> > > On 2017/06/26 05:23:50, Bugs Nash wrote:
> > > > I find this new general method confusing, while the existing methods
were
> > clear
> > > > to me. I don't think it makes sense to merge them together, as they each
> do
> > > > distinct things (especially the ParseShorthandGreedily method).
> > > > 
> > > > Is this refactor necessary to move parsing logic into property APIs? It
> > doesn't
> > > > seem like it would be to me.
> > > 
> > > Thanks for the comment! As I understand it, project ribbon aims to impl
APIs
> > as well as refactor/clean up the code. I see there's a lot of common logic
> > between the three methods and so duplicated code (e.g. they all call
longhands
> > parsing methods), hence I think it's appropriate to refactor them. An
> important
> > benefit is to see more clearly which shorthands get parsed via longhand
> parsing
> > methods.
> > 
> > 
> > Oh, if you think that it should be more clear when shorthand parsing calls
> > through to longhand parsing, might I suggest changing the method names to
> > ConsumeLonghandsGreedily, Consume2Longhands, and Consume4Longhands or
> something
> > to that effect?
> 
> Thanks again for the comment! These names sound good. I still think we should
> refactor the code to remove duplicates when possible. :) Let's see what
rjwright
> and meade think.

hmm, I agree with bugs - the new function has a lot more branching in it, which
is much harder to understand than the fairly straightforward ConsumeNValues
methods, even if they did contain some pretty similar logic. Also, I think this
loses some of the DCHECKs around the lengths of the shorthands given to the
functions. Sorry :/

[Jia, 2017-06-29 04:43:56]

On 2017/06/28 07:29:02, meade_UTC10 wrote:
> On 2017/06/26 06:10:41, Jia wrote:
> > On 2017/06/26 06:06:09, Bugs Nash wrote:
> > > On 2017/06/26 at 05:35:56, jiameng wrote:
> > > > On 2017/06/26 05:23:50, Bugs Nash wrote:
> > > > > I find this new general method confusing, while the existing methods
> were
> > > clear
> > > > > to me. I don't think it makes sense to merge them together, as they
each
> > do
> > > > > distinct things (especially the ParseShorthandGreedily method).
> > > > > 
> > > > > Is this refactor necessary to move parsing logic into property APIs?
It
> > > doesn't
> > > > > seem like it would be to me.
> > > > 
> > > > Thanks for the comment! As I understand it, project ribbon aims to impl
> APIs
> > > as well as refactor/clean up the code. I see there's a lot of common logic
> > > between the three methods and so duplicated code (e.g. they all call
> longhands
> > > parsing methods), hence I think it's appropriate to refactor them. An
> > important
> > > benefit is to see more clearly which shorthands get parsed via longhand
> > parsing
> > > methods.
> > > 
> > > 
> > > Oh, if you think that it should be more clear when shorthand parsing calls
> > > through to longhand parsing, might I suggest changing the method names to
> > > ConsumeLonghandsGreedily, Consume2Longhands, and Consume4Longhands or
> > something
> > > to that effect?
> > 
> > Thanks again for the comment! These names sound good. I still think we
should
> > refactor the code to remove duplicates when possible. :) Let's see what
> rjwright
> > and meade think.
> 
> hmm, I agree with bugs - the new function has a lot more branching in it,
which
> is much harder to understand than the fairly straightforward ConsumeNValues
> methods, even if they did contain some pretty similar logic. Also, I think
this
> loses some of the DCHECKs around the lengths of the shorthands given to the
> functions. Sorry :/

Thanks for the comments! I will keep ConsumeShorthandGreedily as it is (no
merging). 
Re Consume4Values and Consume2Values, we could either keep them separate (to
keep the
logic crystal clear) or merge them into one (to avoid duplicate). It seems
keeping 
logic clear has higher priority,  then I think we should keep them separate.
WTY?

[meade_UTC10, 2017-06-29 04:55:36]

On 2017/06/29 04:43:56, Jia wrote:
> On 2017/06/28 07:29:02, meade_UTC10 wrote:
> > On 2017/06/26 06:10:41, Jia wrote:
> > > On 2017/06/26 06:06:09, Bugs Nash wrote:
> > > > On 2017/06/26 at 05:35:56, jiameng wrote:
> > > > > On 2017/06/26 05:23:50, Bugs Nash wrote:
> > > > > > I find this new general method confusing, while the existing methods
> > were
> > > > clear
> > > > > > to me. I don't think it makes sense to merge them together, as they
> each
> > > do
> > > > > > distinct things (especially the ParseShorthandGreedily method).
> > > > > > 
> > > > > > Is this refactor necessary to move parsing logic into property APIs?
> It
> > > > doesn't
> > > > > > seem like it would be to me.
> > > > > 
> > > > > Thanks for the comment! As I understand it, project ribbon aims to
impl
> > APIs
> > > > as well as refactor/clean up the code. I see there's a lot of common
logic
> > > > between the three methods and so duplicated code (e.g. they all call
> > longhands
> > > > parsing methods), hence I think it's appropriate to refactor them. An
> > > important
> > > > benefit is to see more clearly which shorthands get parsed via longhand
> > > parsing
> > > > methods.
> > > > 
> > > > 
> > > > Oh, if you think that it should be more clear when shorthand parsing
calls
> > > > through to longhand parsing, might I suggest changing the method names
to
> > > > ConsumeLonghandsGreedily, Consume2Longhands, and Consume4Longhands or
> > > something
> > > > to that effect?
> > > 
> > > Thanks again for the comment! These names sound good. I still think we
> should
> > > refactor the code to remove duplicates when possible. :) Let's see what
> > rjwright
> > > and meade think.
> > 
> > hmm, I agree with bugs - the new function has a lot more branching in it,
> which
> > is much harder to understand than the fairly straightforward ConsumeNValues
> > methods, even if they did contain some pretty similar logic. Also, I think
> this
> > loses some of the DCHECKs around the lengths of the shorthands given to the
> > functions. Sorry :/
> 
> Thanks for the comments! I will keep ConsumeShorthandGreedily as it is (no
> merging). 
> Re Consume4Values and Consume2Values, we could either keep them separate (to
> keep the
> logic crystal clear) or merge them into one (to avoid duplicate). It seems
> keeping 
> logic clear has higher priority,  then I think we should keep them separate.
> WTY?

Separate SGTM.

[Bugs Nash, 2017-06-29 23:29:38]

On 2017/06/29 at 04:55:36, meade wrote:
> On 2017/06/29 04:43:56, Jia wrote:
> > On 2017/06/28 07:29:02, meade_UTC10 wrote:
> > > On 2017/06/26 06:10:41, Jia wrote:
> > > > On 2017/06/26 06:06:09, Bugs Nash wrote:
> > > > > On 2017/06/26 at 05:35:56, jiameng wrote:
> > > > > > On 2017/06/26 05:23:50, Bugs Nash wrote:
> > > > > > > I find this new general method confusing, while the existing
methods
> > > were
> > > > > clear
> > > > > > > to me. I don't think it makes sense to merge them together, as
they
> > each
> > > > do
> > > > > > > distinct things (especially the ParseShorthandGreedily method).
> > > > > > > 
> > > > > > > Is this refactor necessary to move parsing logic into property
APIs?
> > It
> > > > > doesn't
> > > > > > > seem like it would be to me.
> > > > > > 
> > > > > > Thanks for the comment! As I understand it, project ribbon aims to
impl
> > > APIs
> > > > > as well as refactor/clean up the code. I see there's a lot of common
logic
> > > > > between the three methods and so duplicated code (e.g. they all call
> > > longhands
> > > > > parsing methods), hence I think it's appropriate to refactor them. An
> > > > important
> > > > > benefit is to see more clearly which shorthands get parsed via
longhand
> > > > parsing
> > > > > methods.
> > > > > 
> > > > > 
> > > > > Oh, if you think that it should be more clear when shorthand parsing
calls
> > > > > through to longhand parsing, might I suggest changing the method names
to
> > > > > ConsumeLonghandsGreedily, Consume2Longhands, and Consume4Longhands or
> > > > something
> > > > > to that effect?
> > > > 
> > > > Thanks again for the comment! These names sound good. I still think we
> > should
> > > > refactor the code to remove duplicates when possible. :) Let's see what
> > > rjwright
> > > > and meade think.
> > > 
> > > hmm, I agree with bugs - the new function has a lot more branching in it,
> > which
> > > is much harder to understand than the fairly straightforward
ConsumeNValues
> > > methods, even if they did contain some pretty similar logic. Also, I think
> > this
> > > loses some of the DCHECKs around the lengths of the shorthands given to
the
> > > functions. Sorry :/
> > 
> > Thanks for the comments! I will keep ConsumeShorthandGreedily as it is (no
> > merging). 
> > Re Consume4Values and Consume2Values, we could either keep them separate (to
> > keep the
> > logic crystal clear) or merge them into one (to avoid duplicate). It seems
> > keeping 
> > logic clear has higher priority,  then I think we should keep them separate.
> > WTY?
> 
> Separate SGTM.

I'd also prefer them separate as they make it clear that they are consuming
'start' and 'end' for 2 values and 'top', 'bottom', 'left', 'right' for 4
values, as well as making it clear how the default values cascade for these
values.

[rjwright, 2017-06-30 02:27:39]

On 2017/06/29 23:29:38, Bugs Nash wrote:
> On 2017/06/29 at 04:55:36, meade wrote:
> > On 2017/06/29 04:43:56, Jia wrote:
> > > On 2017/06/28 07:29:02, meade_UTC10 wrote:
> > > > On 2017/06/26 06:10:41, Jia wrote:
> > > > > On 2017/06/26 06:06:09, Bugs Nash wrote:
> > > > > > On 2017/06/26 at 05:35:56, jiameng wrote:
> > > > > > > On 2017/06/26 05:23:50, Bugs Nash wrote:
> > > > > > > > I find this new general method confusing, while the existing
> methods
> > > > were
> > > > > > clear
> > > > > > > > to me. I don't think it makes sense to merge them together, as
> they
> > > each
> > > > > do
> > > > > > > > distinct things (especially the ParseShorthandGreedily method).
> > > > > > > > 
> > > > > > > > Is this refactor necessary to move parsing logic into property
> APIs?
> > > It
> > > > > > doesn't
> > > > > > > > seem like it would be to me.
> > > > > > > 
> > > > > > > Thanks for the comment! As I understand it, project ribbon aims to
> impl
> > > > APIs
> > > > > > as well as refactor/clean up the code. I see there's a lot of common
> logic
> > > > > > between the three methods and so duplicated code (e.g. they all call
> > > > longhands
> > > > > > parsing methods), hence I think it's appropriate to refactor them.
An
> > > > > important
> > > > > > benefit is to see more clearly which shorthands get parsed via
> longhand
> > > > > parsing
> > > > > > methods.
> > > > > > 
> > > > > > 
> > > > > > Oh, if you think that it should be more clear when shorthand parsing
> calls
> > > > > > through to longhand parsing, might I suggest changing the method
names
> to
> > > > > > ConsumeLonghandsGreedily, Consume2Longhands, and Consume4Longhands
or
> > > > > something
> > > > > > to that effect?
> > > > > 
> > > > > Thanks again for the comment! These names sound good. I still think we
> > > should
> > > > > refactor the code to remove duplicates when possible. :) Let's see
what
> > > > rjwright
> > > > > and meade think.
> > > > 
> > > > hmm, I agree with bugs - the new function has a lot more branching in
it,
> > > which
> > > > is much harder to understand than the fairly straightforward
> ConsumeNValues
> > > > methods, even if they did contain some pretty similar logic. Also, I
think
> > > this
> > > > loses some of the DCHECKs around the lengths of the shorthands given to
> the
> > > > functions. Sorry :/
> > > 
> > > Thanks for the comments! I will keep ConsumeShorthandGreedily as it is (no
> > > merging). 
> > > Re Consume4Values and Consume2Values, we could either keep them separate
(to
> > > keep the
> > > logic crystal clear) or merge them into one (to avoid duplicate). It seems
> > > keeping 
> > > logic clear has higher priority,  then I think we should keep them
separate.
> > > WTY?
> > 
> > Separate SGTM.
> 
> I'd also prefer them separate as they make it clear that they are consuming
> 'start' and 'end' for 2 values and 'top', 'bottom', 'left', 'right' for 4
> values, as well as making it clear how the default values cascade for these
> values.


Possibly redundant at this point, but I have read this patch and agree that it
was simpler before.

[meade_UTC10, 2017-07-13 01:41:41]

meade@chromium.org changed reviewers:
- meade@chromium.org

[Jia, 2017-07-14 02:37:26]

Thanks for all your comments! I've kept them separate in the refactoring
process. See crrev.com/c/560920, crrev.com/c/564875 and crrev.com/c/565662

[Jia, 2017-07-14 02:37:46]

I'm closing this issue now. Thanks.

[Jia, 2017-07-14 02:37:47]

I'm closing this issue now. Thanks.