Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2796803002:
Refactored out the need to use CSSPropertyID to parse shadow properties. (Closed)
DescriptionRefactored out the need to use CSSPropertyID to parse shadow properties.
This is pre work to allow the parseSingleValue method of the
CSSPropertyTextShadow and CSSPropertyBoxShadow properties to be
implemented in separate APIs.
This patch also makes the arguments passed to consumeShadow consistent
with those passed to parseSingleShadow and removes property specific
naming in those arguments.
BUG=668012
Review-Url: https://codereview.chromium.org/2796803002
Cr-Commit-Position: refs/heads/master@{#462808}
Committed: https://chromium.googlesource.com/chromium/src/+/7586cc3f2ec274b36870e7b70e3acc034f0dbd2f
Patch Set 1 #Patch Set 2 : converted double bool arguments to single bool argument #Messages
Total messages: 26 (15 generated)
The CQ bit was checked by bugsnash@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bugsnash@chromium.org changed reviewers: + ericwilligers@chromium.org
Hey Eric, PTAL for first round The bools passed to consumeShadow here are a bit strange. The method is never actually called with 2 different bools. I am open to merging them into 1 argument instead of splitting the consumeShadow arguments into 2, but I would like it to be consistent between the 2 methods. I thought this way made the code more readable. Thanks!
bugsnash@chromium.org changed reviewers: + suzyh@chromium.org - ericwilligers@chromium.org
On 2017/04/04 at 05:08:12, Bugs Nash wrote: > Hey Eric, PTAL for first round > > The bools passed to consumeShadow here are a bit strange. The method is never actually called with 2 different bools. I am open to merging them into 1 argument instead of splitting the consumeShadow arguments into 2, but I would like it to be consistent between the 2 methods. I thought this way made the code more readable. > > Thanks! Changing first pass reviewer to Suzy as Eric is off sick
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
On 2017/04/04 at 05:27:51, bugsnash wrote: > On 2017/04/04 at 05:08:12, Bugs Nash wrote: > > Hey Eric, PTAL for first round > > > > The bools passed to consumeShadow here are a bit strange. The method is never actually called with 2 different bools. I am open to merging them into 1 argument instead of splitting the consumeShadow arguments into 2, but I would like it to be consistent between the 2 methods. I thought this way made the code more readable. > > > > Thanks! > > Changing first pass reviewer to Suzy as Eric is off sick lgtm I take it parseSingleShadow can be called with two different bool values? I'm happy for consumeShadow to take two bools as well (yes, it does seem more readable). If you want to keep it a little tighter, consider adding an assertion in consumeShadow that the values of the two bools are equal.
On 2017/04/04 at 23:43:30, suzyh wrote: > On 2017/04/04 at 05:27:51, bugsnash wrote: > > On 2017/04/04 at 05:08:12, Bugs Nash wrote: > > > Hey Eric, PTAL for first round > > > > > > The bools passed to consumeShadow here are a bit strange. The method is never actually called with 2 different bools. I am open to merging them into 1 argument instead of splitting the consumeShadow arguments into 2, but I would like it to be consistent between the 2 methods. I thought this way made the code more readable. > > > > > > Thanks! > > > > Changing first pass reviewer to Suzy as Eric is off sick > > lgtm > > I take it parseSingleShadow can be called with two different bool values? > I'm happy for consumeShadow to take two bools as well (yes, it does seem more readable). If you want to keep it a little tighter, consider adding an assertion in consumeShadow that the values of the two bools are equal. parseSingleShadow takes 2 boolean arguments, but is never called anywhere in the code base with 2 different bools. Which is what I meant about the code being strange. So I *could* change the arguments to parseSingleShadow to take 1 bool called 'isBoxShadowProperty' instead to make these consistent, but I would like to avoid property specific logic (or even the implication of property specific logic). Alternatively I could call it 'allowInsetAndSpread' and have this 1 bool passed to both parseSingleShadow and consumeShadow. Since no property actually allows one but not the other. That avoids an extra argument but might be less readable? Which do you think is better?
On 2017/04/05 at 01:16:19, bugsnash wrote: > On 2017/04/04 at 23:43:30, suzyh wrote: > > On 2017/04/04 at 05:27:51, bugsnash wrote: > > > On 2017/04/04 at 05:08:12, Bugs Nash wrote: > > > > Hey Eric, PTAL for first round > > > > > > > > The bools passed to consumeShadow here are a bit strange. The method is never actually called with 2 different bools. I am open to merging them into 1 argument instead of splitting the consumeShadow arguments into 2, but I would like it to be consistent between the 2 methods. I thought this way made the code more readable. > > > > > > > > Thanks! > > > > > > Changing first pass reviewer to Suzy as Eric is off sick > > > > lgtm > > > > I take it parseSingleShadow can be called with two different bool values? > > I'm happy for consumeShadow to take two bools as well (yes, it does seem more readable). If you want to keep it a little tighter, consider adding an assertion in consumeShadow that the values of the two bools are equal. > > parseSingleShadow takes 2 boolean arguments, but is never called anywhere in the code base with 2 different bools. Which is what I meant about the code being strange. So I *could* change the arguments to parseSingleShadow to take 1 bool called 'isBoxShadowProperty' instead to make these consistent, but I would like to avoid property specific logic (or even the implication of property specific logic). Alternatively I could call it 'allowInsetAndSpread' and have this 1 bool passed to both parseSingleShadow and consumeShadow. Since no property actually allows one but not the other. That avoids an extra argument but might be less readable? > > Which do you think is better? Ahh. I definitely agree on avoiding 'isBoxShadowProperty'. I'm happy with either 'allowInsetAndSpread' or two bools with an assertion that they're equal (probably in parseSingleShadow, but maybe in both methods). I suggest asking Shane which makes sense from a spec perspective, because I don't have a sense of what these bools mean in the grand scheme of things.
The CQ bit was checked by bugsnash@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bugsnash@chromium.org changed reviewers: + alancutter@chromium.org
On 2017/04/05 at 01:22:00, suzyh wrote: > On 2017/04/05 at 01:16:19, bugsnash wrote: > > On 2017/04/04 at 23:43:30, suzyh wrote: > > > On 2017/04/04 at 05:27:51, bugsnash wrote: > > > > On 2017/04/04 at 05:08:12, Bugs Nash wrote: > > > > > Hey Eric, PTAL for first round > > > > > > > > > > The bools passed to consumeShadow here are a bit strange. The method is never actually called with 2 different bools. I am open to merging them into 1 argument instead of splitting the consumeShadow arguments into 2, but I would like it to be consistent between the 2 methods. I thought this way made the code more readable. > > > > > > > > > > Thanks! > > > > > > > > Changing first pass reviewer to Suzy as Eric is off sick > > > > > > lgtm > > > > > > I take it parseSingleShadow can be called with two different bool values? > > > I'm happy for consumeShadow to take two bools as well (yes, it does seem more readable). If you want to keep it a little tighter, consider adding an assertion in consumeShadow that the values of the two bools are equal. > > > > parseSingleShadow takes 2 boolean arguments, but is never called anywhere in the code base with 2 different bools. Which is what I meant about the code being strange. So I *could* change the arguments to parseSingleShadow to take 1 bool called 'isBoxShadowProperty' instead to make these consistent, but I would like to avoid property specific logic (or even the implication of property specific logic). Alternatively I could call it 'allowInsetAndSpread' and have this 1 bool passed to both parseSingleShadow and consumeShadow. Since no property actually allows one but not the other. That avoids an extra argument but might be less readable? > > > > Which do you think is better? > > Ahh. I definitely agree on avoiding 'isBoxShadowProperty'. I'm happy with either 'allowInsetAndSpread' or two bools with an assertion that they're equal (probably in parseSingleShadow, but maybe in both methods). I suggest asking Shane which makes sense from a spec perspective, because I don't have a sense of what these bools mean in the grand scheme of things. Converted to single bool named allowInsetAndSpread after consulting with Shane. +alancutter for owners review
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. The description should mention the change to the parameters.
On 2017/04/07 at 01:31:21, alancutter wrote: > lgtm. > The description should mention the change to the parameters. It does: "This patch also makes the arguments passed to consumeShadow consistent with those passed to parseSingleShadow and removes property specific naming in those arguments." Is that sufficient?
On 2017/04/07 at 01:33:33, bugsnash wrote: > On 2017/04/07 at 01:31:21, alancutter wrote: > > lgtm. > > The description should mention the change to the parameters. > > It does: "This patch also makes the arguments passed to consumeShadow consistent > with those passed to parseSingleShadow and removes property specific > naming in those arguments." > > Is that sufficient? Yes.
The CQ bit was checked by bugsnash@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from suzyh@chromium.org Link to the patchset: https://codereview.chromium.org/2796803002/#ps20001 (title: "converted double bool arguments to single bool argument")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491547718154730,
"parent_rev": "67161fd023615beeb90c01c878220b5485cd057c", "commit_rev":
"7586cc3f2ec274b36870e7b70e3acc034f0dbd2f"}
Message was sent while issue was closed.
Description was changed from ========== Refactored out the need to use CSSPropertyID to parse shadow properties. This is pre work to allow the parseSingleValue method of the CSSPropertyTextShadow and CSSPropertyBoxShadow properties to be implemented in separate APIs. This patch also makes the arguments passed to consumeShadow consistent with those passed to parseSingleShadow and removes property specific naming in those arguments. BUG=668012 ========== to ========== Refactored out the need to use CSSPropertyID to parse shadow properties. This is pre work to allow the parseSingleValue method of the CSSPropertyTextShadow and CSSPropertyBoxShadow properties to be implemented in separate APIs. This patch also makes the arguments passed to consumeShadow consistent with those passed to parseSingleShadow and removes property specific naming in those arguments. BUG=668012 Review-Url: https://codereview.chromium.org/2796803002 Cr-Commit-Position: refs/heads/master@{#462808} Committed: https://chromium.googlesource.com/chromium/src/+/7586cc3f2ec274b36870e7b70e3a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7586cc3f2ec274b36870e7b70e3a... |
