Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794853002:
Separate CSSValueID mappings generator from ComputedStyleBase generator. (Closed)
DescriptionSeparate CSSValueID mappings generator from ComputedStyleBase generator.
We currently generate mappings between CSSValueID and platform enums
inside the ComputedStyleBase generator. However, because such mappings
do not exist for nonproperty fields, it no longer makes sense for us
to generate these mappings inside make_computed_style_base.py.
This patch moves the generator for CSSValueIDMappingsGenerated to
its own file, so that it is not affected by nonproperty fields.
BUG=628043
Review-Url: https://codereview.chromium.org/2794853002
Cr-Original-Commit-Position: refs/heads/master@{#463955}
Committed: https://chromium.googlesource.com/chromium/src/+/dea75f9603af994dd10370ddd25a0dc6af7b93cd
Review-Url: https://codereview.chromium.org/2794853002
Cr-Commit-Position: refs/heads/master@{#464303}
Committed: https://chromium.googlesource.com/chromium/src/+/f6d6371a3b819d5ad6a31f25cca18e952e28d621
Patch Set 1 #Patch Set 2 : Forgot file #
Total comments: 5
Patch Set 3 : Address comments #Patch Set 4 : Fix build.gn #
Dependent Patchsets: Messages
Total messages: 45 (28 generated)
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.
shend@chromium.org changed reviewers: + bugsnash@chromium.org
Hi Bugs, PTAL :)
https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:14: enum_for_css_keyword, enum_type_name, enum_value_name, class_member_name, method_name, enum_for_css_keyword is no longer needed here https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py (right): https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:26: self._include_paths.append(property_['field_type_path'] + '.h') this population of self._include_paths didn't seem to occur previously, please update description with the purpose of this https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:36: def generate_css_value_mappings(self): this function name is no longer accurate - it doesn't generate the mappings, just gets them
Addressed comments, PTAL. https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py (right): https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:26: self._include_paths.append(property_['field_type_path'] + '.h') On 2017/04/03 at 23:58:55, Bugs Nash wrote: > this population of self._include_paths didn't seem to occur previously, please update description with the purpose of this It occurred previously, but it was being shared with the other generators. Since we're pulling this out as a separate file, the logic has to be duplicated in the new file). It's not doing anything new. https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:36: def generate_css_value_mappings(self): On 2017/04/03 at 23:58:55, Bugs Nash wrote: > this function name is no longer accurate - it doesn't generate the mappings, just gets them I moved the generation code back down to this method
On 2017/04/04 at 03:16:32, shend wrote: > Addressed comments, PTAL. > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py (right): > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:26: self._include_paths.append(property_['field_type_path'] + '.h') > On 2017/04/03 at 23:58:55, Bugs Nash wrote: > > this population of self._include_paths didn't seem to occur previously, please update description with the purpose of this > > It occurred previously, but it was being shared with the other generators. Since we're pulling this out as a separate file, the logic has to be duplicated in the new file). It's not doing anything new. Ah I see, the _get_include_paths function. This still seems like a method that can be used in the init function of the new class, could that method be pulled out into make_style_builder or something? this patch seems to messify the code a bit > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:36: def generate_css_value_mappings(self): > On 2017/04/03 at 23:58:55, Bugs Nash wrote: > > this function name is no longer accurate - it doesn't generate the mappings, just gets them > > I moved the generation code back down to this method
On 2017/04/04 at 03:35:36, bugsnash wrote: > On 2017/04/04 at 03:16:32, shend wrote: > > Addressed comments, PTAL. > > > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py (right): > > > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:26: self._include_paths.append(property_['field_type_path'] + '.h') > > On 2017/04/03 at 23:58:55, Bugs Nash wrote: > > > this population of self._include_paths didn't seem to occur previously, please update description with the purpose of this > > > > It occurred previously, but it was being shared with the other generators. Since we're pulling this out as a separate file, the logic has to be duplicated in the new file). It's not doing anything new. > > Ah I see, the _get_include_paths function. This still seems like a method that can be used in the init function of the new class, could that method be pulled out into make_style_builder or something? this patch seems to messify the code a bit > > > > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:36: def generate_css_value_mappings(self): > > On 2017/04/03 at 23:58:55, Bugs Nash wrote: > > > this function name is no longer accurate - it doesn't generate the mappings, just gets them > > > > I moved the generation code back down to this method Hmm, it's a very ComputedStyle specific thing, so I don't want to add it to make_style_builder. I could move this back into a method instead of doing it inline, which would tidying things a little bit for the cost of an extra method.
On 2017/04/04 at 03:38:08, shend wrote: > On 2017/04/04 at 03:35:36, bugsnash wrote: > > On 2017/04/04 at 03:16:32, shend wrote: > > > Addressed comments, PTAL. > > > > > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py (right): > > > > > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:26: self._include_paths.append(property_['field_type_path'] + '.h') > > > On 2017/04/03 at 23:58:55, Bugs Nash wrote: > > > > this population of self._include_paths didn't seem to occur previously, please update description with the purpose of this > > > > > > It occurred previously, but it was being shared with the other generators. Since we're pulling this out as a separate file, the logic has to be duplicated in the new file). It's not doing anything new. > > > > Ah I see, the _get_include_paths function. This still seems like a method that can be used in the init function of the new class, could that method be pulled out into make_style_builder or something? this patch seems to messify the code a bit > > > > > > > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:36: def generate_css_value_mappings(self): > > > On 2017/04/03 at 23:58:55, Bugs Nash wrote: > > > > this function name is no longer accurate - it doesn't generate the mappings, just gets them > > > > > > I moved the generation code back down to this method > > Hmm, it's a very ComputedStyle specific thing, so I don't want to add it to make_style_builder. I could move this back into a method instead of doing it inline, which would tidying things a little bit for the cost of an extra method. Do you anticipate any more moving of code around like this in future? If so it might be worth making a utils type file that both of these can draw from? if not whichever way you think makes most sense. lgtm
On 2017/04/04 at 03:50:57, bugsnash wrote: > On 2017/04/04 at 03:38:08, shend wrote: > > On 2017/04/04 at 03:35:36, bugsnash wrote: > > > On 2017/04/04 at 03:16:32, shend wrote: > > > > Addressed comments, PTAL. > > > > > > > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py (right): > > > > > > > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:26: self._include_paths.append(property_['field_type_path'] + '.h') > > > > On 2017/04/03 at 23:58:55, Bugs Nash wrote: > > > > > this population of self._include_paths didn't seem to occur previously, please update description with the purpose of this > > > > > > > > It occurred previously, but it was being shared with the other generators. Since we're pulling this out as a separate file, the logic has to be duplicated in the new file). It's not doing anything new. > > > > > > Ah I see, the _get_include_paths function. This still seems like a method that can be used in the init function of the new class, could that method be pulled out into make_style_builder or something? this patch seems to messify the code a bit > > > > > > > > > > > https://codereview.chromium.org/2794853002/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/build/scripts/make_css_value_id_mappings.py:36: def generate_css_value_mappings(self): > > > > On 2017/04/03 at 23:58:55, Bugs Nash wrote: > > > > > this function name is no longer accurate - it doesn't generate the mappings, just gets them > > > > > > > > I moved the generation code back down to this method > > > > Hmm, it's a very ComputedStyle specific thing, so I don't want to add it to make_style_builder. I could move this back into a method instead of doing it inline, which would tidying things a little bit for the cost of an extra method. > > Do you anticipate any more moving of code around like this in future? If so it might be worth making a utils type file that both of these can draw from? if not whichever way you think makes most sense. lgtm Not in the foreseeable future. But yeah, utils file sounds good when we get more of these (like 3 files), but I feel it's overkill for just 2 files.
shend@chromium.org changed reviewers: + meade@chromium.org
Hi Eddy, PTAL
lgtm
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: linux_chromium_asan_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: linux_chromium_asan_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
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": 40001, "attempt_start_ts": 1491970227079620,
"parent_rev": "3105d5e2ffe590989bbbda65e5b0e66a4a5049bd", "commit_rev":
"dea75f9603af994dd10370ddd25a0dc6af7b93cd"}
Message was sent while issue was closed.
Description was changed from ========== Separate CSSValueID mappings generator from ComputedStyleBase generator. We currently generate mappings between CSSValueID and platform enums inside the ComputedStyleBase generator. However, because such mappings do not exist for nonproperty fields, it no longer makes sense for us to generate these mappings inside make_computed_style_base.py. This patch moves the generator for CSSValueIDMappingsGenerated to its own file, so that it is not affected by nonproperty fields. BUG=628043 ========== to ========== Separate CSSValueID mappings generator from ComputedStyleBase generator. We currently generate mappings between CSSValueID and platform enums inside the ComputedStyleBase generator. However, because such mappings do not exist for nonproperty fields, it no longer makes sense for us to generate these mappings inside make_computed_style_base.py. This patch moves the generator for CSSValueIDMappingsGenerated to its own file, so that it is not affected by nonproperty fields. BUG=628043 Review-Url: https://codereview.chromium.org/2794853002 Cr-Commit-Position: refs/heads/master@{#463955} Committed: https://chromium.googlesource.com/chromium/src/+/dea75f9603af994dd10370ddd25a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/dea75f9603af994dd10370ddd25a...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2812823004/ by jdoerrie@chromium.org. The reason for reverting is: Broke compilation on Google Chrome Mac bot BUG=710778.
Message was sent while issue was closed.
Findit(https://goo.gl/kROfz5) confirmed this CL at revision 463955 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
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...
shend@chromium.org changed reviewers: + alancutter@chromium.org
Hi Alan, I'm relanding this patch, can you PTAL? The only difference is that I've added the new generator to "targets_generating_sources" in BUILD.gn. For some reason it passed the trybots before, but broke the buildbots. Perhaps the trybots were not doing a clean build? Hopefully this one works.
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
The CQ bit was checked by shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, bugsnash@chromium.org Link to the patchset: https://codereview.chromium.org/2794853002/#ps60001 (title: "Fix build.gn")
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": 60001, "attempt_start_ts": 1492058631665830,
"parent_rev": "888cf7c28286229cb62da42125ee41302d32b2de", "commit_rev":
"f6d6371a3b819d5ad6a31f25cca18e952e28d621"}
Message was sent while issue was closed.
Description was changed from ========== Separate CSSValueID mappings generator from ComputedStyleBase generator. We currently generate mappings between CSSValueID and platform enums inside the ComputedStyleBase generator. However, because such mappings do not exist for nonproperty fields, it no longer makes sense for us to generate these mappings inside make_computed_style_base.py. This patch moves the generator for CSSValueIDMappingsGenerated to its own file, so that it is not affected by nonproperty fields. BUG=628043 Review-Url: https://codereview.chromium.org/2794853002 Cr-Commit-Position: refs/heads/master@{#463955} Committed: https://chromium.googlesource.com/chromium/src/+/dea75f9603af994dd10370ddd25a... ========== to ========== Separate CSSValueID mappings generator from ComputedStyleBase generator. We currently generate mappings between CSSValueID and platform enums inside the ComputedStyleBase generator. However, because such mappings do not exist for nonproperty fields, it no longer makes sense for us to generate these mappings inside make_computed_style_base.py. This patch moves the generator for CSSValueIDMappingsGenerated to its own file, so that it is not affected by nonproperty fields. BUG=628043 Review-Url: https://codereview.chromium.org/2794853002 Cr-Original-Commit-Position: refs/heads/master@{#463955} Committed: https://chromium.googlesource.com/chromium/src/+/dea75f9603af994dd10370ddd25a... Review-Url: https://codereview.chromium.org/2794853002 Cr-Commit-Position: refs/heads/master@{#464303} Committed: https://chromium.googlesource.com/chromium/src/+/f6d6371a3b819d5ad6a31f25cca1... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f6d6371a3b819d5ad6a31f25cca1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
