|
|
DescriptionMove hardcoded extra fields to its own JSON5 file.
For extra fields in ComputedStyle, we have been hardcoding their
values in the generator script. However, this is ugly and not
sustainable. This patch moves them into its own JSON file.
This patch does not change generated code.
BUG=628043
Review-Url: https://codereview.chromium.org/2788353002
Cr-Commit-Position: refs/heads/master@{#466178}
Committed: https://chromium.googlesource.com/chromium/src/+/4187e7d068935829eb5a914b7f3f2b8144e3bb98
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Forgot file #Patch Set 4 : rebase #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Rebase #Patch Set 8 : Rebase #
Total comments: 4
Patch Set 9 : Address comments #Patch Set 10 : Address comments #Patch Set 11 : Rebase #Patch Set 12 : Rebase #Patch Set 13 : Rebase #
Depends on Patchset: Messages
Total messages: 60 (43 generated)
shend@chromium.org changed reviewers: + bugsnash@chromium.org
Hi Bugs, PTAL. I'm open to suggestions for the naming of the new JSON file.
The CQ bit was checked by shend@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
shend@chromium.org changed reviewers: - bugsnash@chromium.org
Wait, sorry, this patch is not ready yet.
Description was changed from ========== Move hardcoded nonproperties to its own JSON5 file. For nonproperty fields in ComputedStyle, we have been hardcoding their values in the generator script. Now that the code paths between property and nonproperty values are similar enough, we can move nonproperties out into a separate file and treat them the same as properties. The only nonproperty field that remains hardcoded is m_insideLink because it breaks the CSSValueID mapping generation (m_insideLink does not use real CSS keywords). A TODO has been added to fix this. This patch does not change generated code. BUG=628043 ========== to ========== Move hardcoded extra fields to its own JSON5 file. For extra fields in ComputedStyle, we have been hardcoding their values in the generator script. However, this is ugly and not sustainable. This patch moves them into its own JSON file. This patch does not change generated code. BUG=628043 ==========
shend@chromium.org changed reviewers: + bugsnash@chromium.org
Hi Bugs, second try :P PTAL
The CQ bit was checked by shend@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by shend@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...
On 2017/04/12 at 07:51:53, shend wrote: > Hi Bugs, second try :P PTAL My review waiting on https://codereview.chromium.org/2811183004 to land to resolve dependencies
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This should be ready for review now.
The CQ bit was checked by shend@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by shend@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
shend@chromium.org changed reviewers: + alancutter@chromium.org
Hi Alan, PTAL
https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 (right): https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:20: // The rest is the same as CSSProperties.json5 Full stop. Not all parameters in CSSProperties.json5 are used here, you should list the relevant ones explicitly. I'd be in favour of copy pasting them here if there's not too many. WDYT?
https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 (right): https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:20: // The rest is the same as CSSProperties.json5 On 2017/04/19 at 07:13:32, alancutter wrote: > Full stop. Done. > Not all parameters in CSSProperties.json5 are used here, you should list the relevant ones explicitly. > I'd be in favour of copy pasting them here if there's not too many. WDYT? There's about ten parameters I think, and this might grow in the future. The annoying thing is that validation information like "default" and "valid_type" have to be copied over as well. I'm leaning towards keeping it as it is, but I don't have a strong opinion as long as we don't have to copy the comments across as well. A possible compromise is to list the relevant parameters in a comment. Maybe ask for a third opinion?
Listed relevant properties in JSON file as a comment. Alan, PTAL again?
Oh whoops, I wrote this comment this morning and forgot to publish. https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 (right): https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:20: // The rest is the same as CSSProperties.json5 On 2017/04/19 at 07:38:58, shend wrote: > On 2017/04/19 at 07:13:32, alancutter wrote: > > Full stop. > > Done. > > > Not all parameters in CSSProperties.json5 are used here, you should list the relevant ones explicitly. > > I'd be in favour of copy pasting them here if there's not too many. WDYT? > > There's about ten parameters I think, and this might grow in the future. The annoying thing is that validation information like "default" and "valid_type" have to be copied over as well. I'm leaning towards keeping it as it is, but I don't have a strong opinion as long as we don't have to copy the comments across as well. A possible compromise is to list the relevant parameters in a comment. Maybe ask for a third opinion? If the list might grow in future then I would not include them all here, as this list is likely to become stale. I'm ok with giving examples or listing relevant ones
lgtm https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 (right): https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:20: // The rest is the same as CSSProperties.json5 On 2017/04/19 at 22:56:55, Bugs Nash wrote: > On 2017/04/19 at 07:38:58, shend wrote: > > On 2017/04/19 at 07:13:32, alancutter wrote: > > > Full stop. > > > > Done. > > > > > Not all parameters in CSSProperties.json5 are used here, you should list the relevant ones explicitly. > > > I'd be in favour of copy pasting them here if there's not too many. WDYT? > > > > There's about ten parameters I think, and this might grow in the future. The annoying thing is that validation information like "default" and "valid_type" have to be copied over as well. I'm leaning towards keeping it as it is, but I don't have a strong opinion as long as we don't have to copy the comments across as well. A possible compromise is to list the relevant parameters in a comment. Maybe ask for a third opinion? > > If the list might grow in future then I would not include them all here, as this list is likely to become stale. I'm ok with giving examples or listing relevant ones I share your concerns for the list becoming stale. I personally think it's better than needing the reader to trawl through generator scripts to work out the parameters used by this file though. Not perfect though by any means.
On 2017/04/20 at 03:29:58, alancutter wrote: > lgtm > > https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 (right): > > https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:20: // The rest is the same as CSSProperties.json5 > On 2017/04/19 at 22:56:55, Bugs Nash wrote: > > On 2017/04/19 at 07:38:58, shend wrote: > > > On 2017/04/19 at 07:13:32, alancutter wrote: > > > > Full stop. > > > > > > Done. > > > > > > > Not all parameters in CSSProperties.json5 are used here, you should list the relevant ones explicitly. > > > > I'd be in favour of copy pasting them here if there's not too many. WDYT? > > > > > > There's about ten parameters I think, and this might grow in the future. The annoying thing is that validation information like "default" and "valid_type" have to be copied over as well. I'm leaning towards keeping it as it is, but I don't have a strong opinion as long as we don't have to copy the comments across as well. A possible compromise is to list the relevant parameters in a comment. Maybe ask for a third opinion? > > > > If the list might grow in future then I would not include them all here, as this list is likely to become stale. I'm ok with giving examples or listing relevant ones > > I share your concerns for the list becoming stale. I personally think it's better than needing the reader to trawl through generator scripts to work out the parameters used by this file though. Not perfect though by any means. Perhaps parameters/schemas should be decoupled from the data? Something like this: // CSSProperties.json5 { schemas: ["css-properties", "fields"], data: [] } // ExtraFields.json5 { schemas: ["fields", "extra-fields"], data: [] } // css-properties.schema, fields.schema and extra-fields.schema { parameters: [] } So both CSSProperties.json5 and ExtraFields can share the same schema for stuff like field_template and field_type_path, but can add their own parameters as well?
On 2017/04/20 at 03:35:56, shend wrote: > On 2017/04/20 at 03:29:58, alancutter wrote: > > lgtm > > > > https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 (right): > > > > https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:20: // The rest is the same as CSSProperties.json5 > > On 2017/04/19 at 22:56:55, Bugs Nash wrote: > > > On 2017/04/19 at 07:38:58, shend wrote: > > > > On 2017/04/19 at 07:13:32, alancutter wrote: > > > > > Full stop. > > > > > > > > Done. > > > > > > > > > Not all parameters in CSSProperties.json5 are used here, you should list the relevant ones explicitly. > > > > > I'd be in favour of copy pasting them here if there's not too many. WDYT? > > > > > > > > There's about ten parameters I think, and this might grow in the future. The annoying thing is that validation information like "default" and "valid_type" have to be copied over as well. I'm leaning towards keeping it as it is, but I don't have a strong opinion as long as we don't have to copy the comments across as well. A possible compromise is to list the relevant parameters in a comment. Maybe ask for a third opinion? > > > > > > If the list might grow in future then I would not include them all here, as this list is likely to become stale. I'm ok with giving examples or listing relevant ones > > > > I share your concerns for the list becoming stale. I personally think it's better than needing the reader to trawl through generator scripts to work out the parameters used by this file though. Not perfect though by any means. > > Perhaps parameters/schemas should be decoupled from the data? Something like this: > > // CSSProperties.json5 > { > schemas: ["css-properties", "fields"], > data: [] > } > > // ExtraFields.json5 > { > schemas: ["fields", "extra-fields"], > data: [] > } > > // css-properties.schema, fields.schema and extra-fields.schema > { > parameters: [] > } > > So both CSSProperties.json5 and ExtraFields can share the same schema for stuff like field_template and field_type_path, but can add their own parameters as well? This is certainly a possible solution but I'm not sure it's worth the complexity. I'm willing to wait until the problem it solves is bigger than just this.
On 2017/04/20 at 03:52:59, alancutter wrote: > On 2017/04/20 at 03:35:56, shend wrote: > > On 2017/04/20 at 03:29:58, alancutter wrote: > > > lgtm > > > > > > https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 (right): > > > > > > https://codereview.chromium.org/2788353002/diff/140001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:20: // The rest is the same as CSSProperties.json5 > > > On 2017/04/19 at 22:56:55, Bugs Nash wrote: > > > > On 2017/04/19 at 07:38:58, shend wrote: > > > > > On 2017/04/19 at 07:13:32, alancutter wrote: > > > > > > Full stop. > > > > > > > > > > Done. > > > > > > > > > > > Not all parameters in CSSProperties.json5 are used here, you should list the relevant ones explicitly. > > > > > > I'd be in favour of copy pasting them here if there's not too many. WDYT? > > > > > > > > > > There's about ten parameters I think, and this might grow in the future. The annoying thing is that validation information like "default" and "valid_type" have to be copied over as well. I'm leaning towards keeping it as it is, but I don't have a strong opinion as long as we don't have to copy the comments across as well. A possible compromise is to list the relevant parameters in a comment. Maybe ask for a third opinion? > > > > > > > > If the list might grow in future then I would not include them all here, as this list is likely to become stale. I'm ok with giving examples or listing relevant ones > > > > > > I share your concerns for the list becoming stale. I personally think it's better than needing the reader to trawl through generator scripts to work out the parameters used by this file though. Not perfect though by any means. > > > > Perhaps parameters/schemas should be decoupled from the data? Something like this: > > > > // CSSProperties.json5 > > { > > schemas: ["css-properties", "fields"], > > data: [] > > } > > > > // ExtraFields.json5 > > { > > schemas: ["fields", "extra-fields"], > > data: [] > > } > > > > // css-properties.schema, fields.schema and extra-fields.schema > > { > > parameters: [] > > } > > > > So both CSSProperties.json5 and ExtraFields can share the same schema for stuff like field_template and field_type_path, but can add their own parameters as well? > > This is certainly a possible solution but I'm not sure it's worth the complexity. I'm willing to wait until the problem it solves is bigger than just this. Me too, I don't think the complexity is justifiable right now, but might be in the (far?) future.
The CQ bit was checked by shend@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...
The CQ bit was checked by shend@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shend@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shend@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2788353002/#ps240001 (title: "Rebase")
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": 240001, "attempt_start_ts": 1492729027763690, "parent_rev": "637c7353077c92bbf627b602c57f0e3604525129", "commit_rev": "4187e7d068935829eb5a914b7f3f2b8144e3bb98"}
Message was sent while issue was closed.
Description was changed from ========== Move hardcoded extra fields to its own JSON5 file. For extra fields in ComputedStyle, we have been hardcoding their values in the generator script. However, this is ugly and not sustainable. This patch moves them into its own JSON file. This patch does not change generated code. BUG=628043 ========== to ========== Move hardcoded extra fields to its own JSON5 file. For extra fields in ComputedStyle, we have been hardcoding their values in the generator script. However, this is ugly and not sustainable. This patch moves them into its own JSON file. This patch does not change generated code. BUG=628043 Review-Url: https://codereview.chromium.org/2788353002 Cr-Commit-Position: refs/heads/master@{#466178} Committed: https://chromium.googlesource.com/chromium/src/+/4187e7d068935829eb5a914b7f3f... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/4187e7d068935829eb5a914b7f3f... |