Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2970003002:
Add code generation for ConditionalFeatures bindings code (Closed)
DescriptionAdd code generation for ConditionalFeatures bindings code
This replaces the handwritten ConditionalFeaturesFor{Core,Modules}.cpp files with a script which reads IDL files looking for the appropriate extended attributes (just OriginTrialEnabled currently) and generates the required code to install those features at runtime.
BUG=615060
Review-Url: https://codereview.chromium.org/2970003002
Cr-Commit-Position: refs/heads/master@{#487917}
Committed: https://chromium.googlesource.com/chromium/src/+/d25397270d7f955de0988221f919a0b67f91f159
Patch Set 1 #Patch Set 2 : Fix imports #Patch Set 3 : Guard against unused variables #Patch Set 4 : Read origin trial attributes directly from IDL files #Patch Set 5 : Normalize info file path #Patch Set 6 : Rebase #Patch Set 7 : Add awful dep from core to modules to get it to compile from clean #Patch Set 8 : Clean up #
Total comments: 21
Patch Set 9 : Clean up templates and build rules #Patch Set 10 : Removing hacks, adding TODOs #Patch Set 11 : Move awkward dependency into scripts.gni with its counterpart #
Total comments: 20
Patch Set 12 : Addressing style issues, refactor single-use class into functions #Patch Set 13 : Make code generation testable, add to bindings tests #
Total comments: 10
Patch Set 14 : Style fixes #
Total comments: 8
Patch Set 15 : Readability fixes #Messages
Total messages: 87 (64 generated)
The CQ bit was checked by iclelland@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...)
The CQ bit was checked by iclelland@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 iclelland@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_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by iclelland@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by iclelland@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by iclelland@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 iclelland@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 iclelland@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...
iclelland@chromium.org changed reviewers: + chasej@chromium.org
+r chasej -- There are still a couple of things I'm not 100% happy with here: - There's a dependency from core to modules in the build that should be fixed - There's a hack for ServiceWorkerGlobalScope in place, because it's the only global which is defined in modules/ Otherwise, WDYT?
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_...)
Aside from the hacks mentioned, it looks pretty good! Thanks for figuring this all out. Comments below. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:33: "condition": 'OriginTrials::%sEnabled' % uncapitalize(feature_name), I'm wondering if it would be better to have the code bits (condition check, install method name) defined in the template with the rest of the code? Instead could just define the type of conditional feature here (i.e. only origin trial for now). I'm thinking about someone trying to figure out the generated code, and having to trace back to this script file to find out where names are coming from. Perhaps not worrying about, since anyone changing the generation will have to understand the scripts anyways? Just something to consider. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:3: // found in the LICENSE file. Should this copyright be replaced by an include? Looks like other files use: {% include 'copyright_block.txt' %} https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:20: void InstallConditionalFeaturesCore(const WrapperTypeInfo* wrapper_type_info, Should be "...ForCore" to be consistent with file name (and modules version)? https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:47: {% for installer in interface.installers %} Will this code generation properly handle global objects? Specifically, it doesn't declare/get the instance object, like in the Modules file below. Probably best addressed by de-duplicating code (as in my comment below). https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:58: void InstallPendingConditionalFeatureCore(const String& feature, Ditto. Should be "...ForCore" to be consistent with file name (and modules version)? https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl:3: // found in the LICENSE file. Ditto about copyright include. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl:3: // found in the LICENSE file. Ditto about copyright include. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl:21: void InstallConditionalFeaturesForModules( This method seems very similar to the Core version, other than the name and some specifics for ContextEnabled. Maybe the body should be extracted into a macro/helper to avoid duplication? This applies to the other methods and definitions in the file as well. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.h.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.h.tmpl:3: // found in the LICENSE file. Ditto about copyright include.
The CQ bit was checked by iclelland@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...
https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:33: "condition": 'OriginTrials::%sEnabled' % uncapitalize(feature_name), On 2017/07/11 20:12:31, chasej wrote: > I'm wondering if it would be better to have the code bits (condition check, > install method name) defined in the template with the rest of the code? Instead > could just define the type of conditional feature here (i.e. only origin trial > for now). I was definitely thinking about other kinds of conditionals here -- FP, or ContextFeatures, etc. This method would eventually return the right condition to check for each one, specifically to avoid any kind of logic creeping into the templates. > I'm thinking about someone trying to figure out the generated code, > and having to trace back to this script file to find out where names are coming > from. Perhaps not worrying about, since anyone changing the generation will have > to understand the scripts anyways? Just something to consider. I also want to add comments to the generated code, explaining where to look to see where the different pieces come from. This piece in particular should be just a matter of tracing from generated code -> template -> context dict in this file. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:3: // found in the LICENSE file. On 2017/07/11 20:12:32, chasej wrote: > Should this copyright be replaced by an include? Looks like other files use: > {% include 'copyright_block.txt' %} Done. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:20: void InstallConditionalFeaturesCore(const WrapperTypeInfo* wrapper_type_info, On 2017/07/11 20:12:32, chasej wrote: > Should be "...ForCore" to be consistent with file name (and modules version)? Done. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:47: {% for installer in interface.installers %} On 2017/07/11 20:12:31, chasej wrote: > Will this code generation properly handle global objects? Specifically, it > doesn't declare/get the instance object, like in the Modules file below. > Probably best addressed by de-duplicating code (as in my comment below). That's a good point, we've never defined a trial interface directly on the core members of a global, but it's a possibility that we should be prepared to handle. I'll dedupe as much as possible between the two files. It should even be possible to have a common core that both of them import, once the ContextFeature code is removed. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:58: void InstallPendingConditionalFeatureCore(const String& feature, On 2017/07/11 20:12:32, chasej wrote: > Ditto. Should be "...ForCore" to be consistent with file name (and modules > version)? Yes, this was a holdover from the handwritten code. Thanks. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl:3: // found in the LICENSE file. On 2017/07/11 20:12:32, chasej wrote: > Ditto about copyright include. Done. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl:3: // found in the LICENSE file. On 2017/07/11 20:12:32, chasej wrote: > Ditto about copyright include. Done. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl:21: void InstallConditionalFeaturesForModules( On 2017/07/11 20:12:32, chasej wrote: > This method seems very similar to the Core version, other than the name and some > specifics for ContextEnabled. Maybe the body should be extracted into a > macro/helper to avoid duplication? > > This applies to the other methods and definitions in the file as well. I've made them identical now, except that the Core version has to have some special case code for handling the ContextEnabledFeatures, until I refactor that to make it just another type of autogenerated binding. I've added a todo to actually extract the method bodies into a template once that is taken care of. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.h.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.h.tmpl:3: // found in the LICENSE file. On 2017/07/11 20:12:32, chasej wrote: > Ditto about copyright include. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Looks good, with a minor comment/question. I'm guessing final l-g-t-m should wait until the two hacks are addressed (core -> modules dependency and SW hack). https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:47: {% for installer in interface.installers %} On 2017/07/12 17:23:20, iclelland wrote: > On 2017/07/11 20:12:31, chasej wrote: > > Will this code generation properly handle global objects? Specifically, it > > doesn't declare/get the instance object, like in the Modules file below. > > Probably best addressed by de-duplicating code (as in my comment below). > > That's a good point, we've never defined a trial interface directly on the core > members of a global, but it's a possibility that we should be prepared to > handle. I'll dedupe as much as possible between the two files. It should even be > possible to have a common core that both of them import, once the ContextFeature > code is removed. Actually, the Long Task Observer trial required installing things on the Window instance: https://chromium.googlesource.com/chromium/src/+/82e2114b9d4b41b5d619e0a401b7... I don't remember what was installed, but just pointing out this scenario is not a hypothetical. If interested, could go back to see what the IDL looked like for Long Task Observer. Regardless, this code will now handle the scenario, so we're good. https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl:21: void InstallConditionalFeaturesForModules( On 2017/07/12 17:23:20, iclelland wrote: > On 2017/07/11 20:12:32, chasej wrote: > > This method seems very similar to the Core version, other than the name and > some > > specifics for ContextEnabled. Maybe the body should be extracted into a > > macro/helper to avoid duplication? > > > > This applies to the other methods and definitions in the file as well. > > I've made them identical now, except that the Core version has to have some > special case code for handling the ContextEnabledFeatures, until I refactor that > to make it just another type of autogenerated binding. > > I've added a todo to actually extract the method bodies into a template once > that is taken care of. Is that newly-added todo in this CL? I'm not seeing it (although I easily could have missed it). I see the todo about unifying ContextFeatureSettings in ConditionalFeaturesForCore.cpp.tmpl. I'm assuming that handling ContextFeatureSettings, and de-duplicating method bodies will be later CLs. So, I just want to clarify where the todo will be. Looking at these templates more, maybe there could be just one template, and plug in "Core" or "Modules" as necessary? The only material difference could be that Register..ForModules()_calls Register...ForCore() (and the resulting includes). I'm working on another CL that should make the InstallConditionalFeaturesOnWindow() method obsolete.
The CQ bit was checked by iclelland@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 iclelland@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...
iclelland@chromium.org changed reviewers: + jbroman@chromium.org
+r jbroman -- could you take a look? Especially at the dep in scripts.gni on interfaces_info. The same issue is still present for the idl_compiler template, and I'm not certain what the intended path forward is for that. Thanks! https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl (right): https://codereview.chromium.org/2970003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl:21: void InstallConditionalFeaturesForModules( On 2017/07/12 19:53:41, chasej wrote: > On 2017/07/12 17:23:20, iclelland wrote: > > On 2017/07/11 20:12:32, chasej wrote: > > > This method seems very similar to the Core version, other than the name and > > some > > > specifics for ContextEnabled. Maybe the body should be extracted into a > > > macro/helper to avoid duplication? > > > > > > This applies to the other methods and definitions in the file as well. > > > > I've made them identical now, except that the Core version has to have some > > special case code for handling the ContextEnabledFeatures, until I refactor > that > > to make it just another type of autogenerated binding. > > > > I've added a todo to actually extract the method bodies into a template once > > that is taken care of. > > Is that newly-added todo in this CL? I'm not seeing it (although I easily could > have missed it). I see the todo about unifying ContextFeatureSettings in > ConditionalFeaturesForCore.cpp.tmpl. > > I'm assuming that handling ContextFeatureSettings, and de-duplicating method > bodies will be later CLs. So, I just want to clarify where the todo will be. Oops -- missed it, thanks for catching. > Looking at these templates more, maybe there could be just one template, and > plug in "Core" or "Modules" as necessary? The only material difference could be > that Register..ForModules()_calls Register...ForCore() (and the resulting > includes). I'm working on another CL that should make the > InstallConditionalFeaturesOnWindow() method obsolete. That would be great, too :) At that point, I think that we should look at having either a single template, or two very lightweight templates that rely heavily on template inheritance to insert most of the actual code from blocks defined in a common parent.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
iclelland@chromium.org changed reviewers: + haraken@chromium.org - jbroman@chromium.org
Ahh, looks like jbroman is OOO for a bit. haraken@, do you know whether the Scripts->Modules DEP is intentional, or if there is a better way to structure the build files here? (It looks like you closed crbug.com/358074, but the FIXME in scripts.gni is still there)
haraken@chromium.org changed reviewers: + bashi@chromium.org, tasak@google.com, yukishiino@chromium.org
> haraken@, do you know whether the Scripts->Modules DEP is intentional, or if > there is a better way to structure the build files here? > > (It looks like you closed crbug.com/358074, but the FIXME in scripts.gni is > still there) tasak@ and/or bashi@ would know. (I guess the FIXME is still needed to resolve partial interfaces defined in modules but I'm not sure.) yukishiino@: Would you mind reviewing this CL overall? https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/scripts.gni (right): https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/scripts.gni:487: # http://crbug.com/358074 tasak, bashi: The question is whether this TODO is still valid or not. The TODO is copied from line 307.
https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/scripts.gni (right): https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/scripts.gni:487: # http://crbug.com/358074 On 2017/07/14 01:23:30, haraken wrote: > > tasak, bashi: The question is whether this TODO is still valid or not. The TODO > is copied from line 307. > It depends on what information generate_conditional_features.py needs. If it doesn't need information from modules when generating code for core, you don't need this dep. I didn't take closer look at the CL but I feel you may not need this dep.
On 2017/07/14 02:15:17, bashi wrote: > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/scripts/scripts.gni (right): > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/scripts/scripts.gni:487: # > http://crbug.com/358074 > On 2017/07/14 01:23:30, haraken wrote: > > > > tasak, bashi: The question is whether this TODO is still valid or not. The > TODO > > is copied from line 307. > > > > It depends on what information generate_conditional_features.py needs. If it > doesn't need information from modules when generating code for core, you don't > need this dep. I didn't take closer look at the CL but I feel you may not need > this dep. This code certainly doesn't need any info from modules when generating the core code -- the issue is that create_component_info_provider_core in utilities.py (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip...) requires InterfacesInfoOverall.pickle, which is part of modules.
Could you add support of "third_party/WebKit/Tools/Scripts/run-bindings-tests" ? If you can add a test .idl file in bindings/tests/idls/ and we can see the output in bindings/tests/results/, then it's great because not only we can test it but also we can review with an example output. Overall looks good. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/code_generator.py (right): https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/code_generator.py:146: class IDLCodeGeneratorBase(CodeGeneratorBase): Is this just a refactoring? Does this make sense? Is |info_provider| independent from IDLs? |render_template| seems using only self.generator_name as a member. CodeGeneratorBase won't use any of info_provider, jinja_env, nor output_dir. Does it make sense to have CodeGeneratorBase class at all? https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py (right): https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:36: "class": interface_info[1], s/class/v8_class/ v8_interface.py name them "v8_class" and "v8_class_or_partial". Let's be consistent. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:39: install_functions.append(install_function) nit: You can use the list-comprehension style, and it may be simpler in this case. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:107: "is_global": interface_info[0] in self.global_objects, You can tell if an interface is global or not by looking at whether the interface has [Global] or [PrimaryGlobal] extended attribute. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:108: "class": interface_info[1], nit: s/class/v8_class/ We have two kind of class names, e.g. "DOMWindow" and "V8Window". So "class" is ambiguous, or "class" is interpreted as "DOMWindow" rather than "V8Window". Also "class" is a keyword in many languages. I'm afraid that this name could cause trouble in future. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl (right): https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:1: {% include 'copyright_block.txt' %} Could you follow the "format_blink_cpp_source_code" magic used in other *.tmpl? {% filter format_blink_cpp_source_code %} {% include 'copyright_block.txt' %} // ... {% endfilter %}{# format_blink_cpp_source_code #} https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:6: #include "{{ include }}" nit: We usually don't put spaces inside {{ and }}. s/{{ var_name }}/{{var_name}}/g Ditto for below. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:56: interface_object); nit: Why break a line between prototype_object and interface_object? https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl (right): https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl:1: {% include 'copyright_block.txt' %} Could you follow the "format_blink_cpp_source_code" magic used in other *.tmpl? {% filter format_blink_cpp_source_code %} {% include 'copyright_block.txt' %} // ... {% endfilter %}{# format_blink_cpp_source_code #}
The CQ bit was checked by iclelland@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...
Thanks, Yuki -- I've addressed a number of your comments. I'm still looking at adding this code to the bindings tests, and removing the dependency on info_provider. info_provider, though, really seems to be the best source for the mapping between partial interfaces and their parent interfaces. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/code_generator.py (right): https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/code_generator.py:146: class IDLCodeGeneratorBase(CodeGeneratorBase): On 2017/07/14 03:30:07, Yuki wrote: > Is this just a refactoring? I wanted to reuse some of the functionality from CodeGeneratorBase, although it was too specifically targetted towards just generating code for a single interface. Specifically, I wanted: -- Access to info_provider for a global view of the interfaces -- The jinja templating environment and I didn't want the parts that assumed a single IDL type. So I split it into two, and based the conditional features generator on the root base class. > Does this make sense? That's a legitimate question :) It did when I started, but I had a slightly different approach at the time (I was annotating the interfaces_info_overall.pickle file with the origin_trials information). I will revisit that and see what it looks like if I avoid using the CodeGenerator classes at all. > > Is |info_provider| independent from IDLs? I'm not sure what you mean by that. Are you asking whether the same information appears in both? I suppose so, since |info_provider| provides the information that ultimately comes from the IDL files. It's already been processed though -- partial interfaces have had their dependencies resolved, etc. Again, maybe that's a dependency that I don't need anymore, so I'll see if I can live without it. > > |render_template| seems using only self.generator_name as a member. > CodeGeneratorBase won't use any of info_provider, jinja_env, nor output_dir. > Does it make sense to have CodeGeneratorBase class at all? https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py (right): https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:36: "class": interface_info[1], On 2017/07/14 03:30:07, Yuki wrote: > s/class/v8_class/ > > v8_interface.py name them "v8_class" and "v8_class_or_partial". > Let's be consistent. Done. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:39: install_functions.append(install_function) On 2017/07/14 03:30:07, Yuki wrote: > nit: You can use the list-comprehension style, and it may be simpler in this > case. Done. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:107: "is_global": interface_info[0] in self.global_objects, On 2017/07/14 03:30:07, Yuki wrote: > You can tell if an interface is global or not by looking at whether the > interface has [Global] or [PrimaryGlobal] extended attribute. The trouble here is that in many cases, the origin trial attributes are defined on a partial interface (see third_party/WebKit/Source/modules/vr/NavigatorVR.idl or WindowModulesConstructors.idl), but I actually need to tell whether the parent interface is a global or not. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:108: "class": interface_info[1], On 2017/07/14 03:30:07, Yuki wrote: > nit: s/class/v8_class/ > > We have two kind of class names, e.g. "DOMWindow" and "V8Window". So "class" is > ambiguous, or "class" is interpreted as "DOMWindow" rather than "V8Window". > > Also "class" is a keyword in many languages. I'm afraid that this name could > cause trouble in future. Fair point. Done. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl (right): https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:1: {% include 'copyright_block.txt' %} On 2017/07/14 03:30:08, Yuki wrote: > Could you follow the "format_blink_cpp_source_code" magic used in other *.tmpl? > > {% filter format_blink_cpp_source_code %} > > {% include 'copyright_block.txt' %} > > // ... > > {% endfilter %}{# format_blink_cpp_source_code #} Done -- that's a great filter, thanks! https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:6: #include "{{ include }}" On 2017/07/14 03:30:07, Yuki wrote: > nit: We usually don't put spaces inside {{ and }}. > > s/{{ var_name }}/{{var_name}}/g > > Ditto for below. /me goes and reads the *chromium* jinja style guide again :) Thanks, Done. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:56: interface_object); On 2017/07/14 03:30:08, Yuki wrote: > nit: Why break a line between prototype_object and interface_object? Not necessary, especially if the code is being formatted automatically now. It matched the original handwritten code. I was attempting to minimize the differences between the two. https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl (right): https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl:1: {% include 'copyright_block.txt' %} On 2017/07/14 03:30:08, Yuki wrote: > Could you follow the "format_blink_cpp_source_code" magic used in other *.tmpl? > > {% filter format_blink_cpp_source_code %} > > {% include 'copyright_block.txt' %} > > // ... > > {% endfilter %}{# format_blink_cpp_source_code #} Done.
The CQ bit was checked by iclelland@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/07/14 18:25:05, iclelland wrote: > Thanks, Yuki -- I've addressed a number of your comments. I'm still looking at > adding this code to the bindings tests, and removing the dependency on > info_provider. info_provider, though, really seems to be the best source for the > mapping between partial interfaces and their parent interfaces. > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/scripts/code_generator.py (right): > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/scripts/code_generator.py:146: class > IDLCodeGeneratorBase(CodeGeneratorBase): > On 2017/07/14 03:30:07, Yuki wrote: > > Is this just a refactoring? > > I wanted to reuse some of the functionality from CodeGeneratorBase, although it > was too specifically targetted towards just generating code for a single > interface. Specifically, I wanted: > -- Access to info_provider for a global view of the interfaces > -- The jinja templating environment > and I didn't want the parts that assumed a single IDL type. So I split it into > two, and based the conditional features generator on the root base class. > > > > Does this make sense? > > That's a legitimate question :) It did when I started, but I had a slightly > different approach at the time (I was annotating the > interfaces_info_overall.pickle file with the origin_trials information). I will > revisit that and see what it looks like if I avoid using the CodeGenerator > classes at all. > > > > > Is |info_provider| independent from IDLs? > > I'm not sure what you mean by that. Are you asking whether the same information > appears in both? I suppose so, since |info_provider| provides the information > that ultimately comes from the IDL files. It's already been processed though -- > partial interfaces have had their dependencies resolved, etc. > > Again, maybe that's a dependency that I don't need anymore, so I'll see if I can > live without it. > > > > > |render_template| seems using only self.generator_name as a member. > > CodeGeneratorBase won't use any of info_provider, jinja_env, nor output_dir. > > Does it make sense to have CodeGeneratorBase class at all? > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py > (right): > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:36: > "class": interface_info[1], > On 2017/07/14 03:30:07, Yuki wrote: > > s/class/v8_class/ > > > > v8_interface.py name them "v8_class" and "v8_class_or_partial". > > Let's be consistent. > > Done. > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:39: > install_functions.append(install_function) > On 2017/07/14 03:30:07, Yuki wrote: > > nit: You can use the list-comprehension style, and it may be simpler in this > > case. > > Done. > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:107: > "is_global": interface_info[0] in self.global_objects, > On 2017/07/14 03:30:07, Yuki wrote: > > You can tell if an interface is global or not by looking at whether the > > interface has [Global] or [PrimaryGlobal] extended attribute. > > The trouble here is that in many cases, the origin trial attributes are defined > on a partial interface (see third_party/WebKit/Source/modules/vr/NavigatorVR.idl > or WindowModulesConstructors.idl), but I actually need to tell whether the > parent interface is a global or not. > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:108: > "class": interface_info[1], > On 2017/07/14 03:30:07, Yuki wrote: > > nit: s/class/v8_class/ > > > > We have two kind of class names, e.g. "DOMWindow" and "V8Window". So "class" > is > > ambiguous, or "class" is interpreted as "DOMWindow" rather than "V8Window". > > > > Also "class" is a keyword in many languages. I'm afraid that this name could > > cause trouble in future. > > Fair point. Done. > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl > (right): > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:1: > {% include 'copyright_block.txt' %} > On 2017/07/14 03:30:08, Yuki wrote: > > Could you follow the "format_blink_cpp_source_code" magic used in other > *.tmpl? > > > > {% filter format_blink_cpp_source_code %} > > > > {% include 'copyright_block.txt' %} > > > > // ... > > > > {% endfilter %}{# format_blink_cpp_source_code #} > > Done -- that's a great filter, thanks! > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:6: > #include "{{ include }}" > On 2017/07/14 03:30:07, Yuki wrote: > > nit: We usually don't put spaces inside {{ and }}. > > > > s/{{ var_name }}/{{var_name}}/g > > > > Ditto for below. > > /me goes and reads the *chromium* jinja style guide again :) > > Thanks, Done. > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl:56: > interface_object); > On 2017/07/14 03:30:08, Yuki wrote: > > nit: Why break a line between prototype_object and interface_object? > > Not necessary, especially if the code is being formatted automatically now. It > matched the original handwritten code. I was attempting to minimize the > differences between the two. > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl > (right): > > https://codereview.chromium.org/2970003002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl:1: > {% include 'copyright_block.txt' %} > On 2017/07/14 03:30:08, Yuki wrote: > > Could you follow the "format_blink_cpp_source_code" magic used in other > *.tmpl? > > > > {% filter format_blink_cpp_source_code %} > > > > {% include 'copyright_block.txt' %} > > > > // ... > > > > {% endfilter %}{# format_blink_cpp_source_code #} > > Done. This is now testable, and tested. I've removed the dependency on a global object list file, by making better use of info_provider. info_provider itself is still required, but IDLReader depends on it, so I don't think I'm getting rid of that dependency. Yuki, can you take another look? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with style nits. https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py (right): https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:19: from code_generator import initialize_jinja_env, normalize_and_sort_includes, render_template nit: 80 columns rule https://google.github.io/styleguide/pyguide.html?showone=Line_length#Line_length ditto for all lines > 80 columns. https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:62: # Reads a set of IDL files and compiles the mapping between interfaces and the Good comment. Thanks. Could you follow the pydoc style? https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/scripts.gni (right): https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/scripts.gni:442: # component = component to generate conditional feature bindings for ("Core" or "Modules") nit: 80 columns? https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.h.tmpl (right): https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.h.tmpl:1: {% include 'copyright_block.txt' %} nit: {% filter format_blink_cpp_source_code %} https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py (right): https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py:349: [filename for filename in idl_filenames if filename not in dictionary_impl_filenames]) nit: 80 columns?
Just FYI, You may have already noticed it, but just in case, https://chromium-review.googlesource.com/c/508328/ seems going to make a change on ConditionalFeaturesForCore.{h,cpp}. Please be careful not to discard that change because you're removing those files.
On 2017/07/15 12:24:04, Yuki wrote: > Just FYI, > > You may have already noticed it, but just in case, > https://chromium-review.googlesource.com/c/508328/ seems going to make a change > on ConditionalFeaturesForCore.{h,cpp}. Please be careful not to discard that > change because you're removing those files. Yes, thank you Yuki. Jason and I sat down together yesterday to discuss how we can coordinate the changes to the code generation so that we don't step on each other's work.
The CQ bit was checked by iclelland@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...
iclelland@chromium.org changed reviewers: + tkent@chromium.org
haraken, can you approve the changes in these build files? -- third_party/WebKit/Source/core/BUILD.gn -- third_party/WebKit/Source/modules/BUILD.gn tkent, can you approve the change to hird_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py ? Thanks! https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py (right): https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:19: from code_generator import initialize_jinja_env, normalize_and_sort_includes, render_template On 2017/07/15 12:09:19, Yuki wrote: > nit: 80 columns rule > https://google.github.io/styleguide/pyguide.html?showone=Line_length#Line_length > > ditto for all lines > 80 columns. Done, thanks. Ran the whole file through autopep8 for good measure. https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:62: # Reads a set of IDL files and compiles the mapping between interfaces and the On 2017/07/15 12:09:19, Yuki wrote: > Good comment. Thanks. > > Could you follow the pydoc style? Done. https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/scripts.gni (right): https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/scripts.gni:442: # component = component to generate conditional feature bindings for ("Core" or "Modules") On 2017/07/15 12:09:19, Yuki wrote: > nit: 80 columns? Sure. I didn't see anything in the GN style guide about line lengths, but consistency is good, and it's easy to do for comments. https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.h.tmpl (right): https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.h.tmpl:1: {% include 'copyright_block.txt' %} On 2017/07/15 12:09:19, Yuki wrote: > nit: {% filter format_blink_cpp_source_code %} Thanks -- missed saving this one. https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py (right): https://codereview.chromium.org/2970003002/diff/240001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/bindings/bindings_tests.py:349: [filename for filename in idl_filenames if filename not in dictionary_impl_filenames]) On 2017/07/15 12:09:19, Yuki wrote: > nit: 80 columns? Done.
Description was changed from
==========
Add initial ConditionalFeatures code generator
BUG=615060
==========
to
==========
Add code generation for ConditionalFeatures bindings code
This replaces the handwritten ConditionalFeaturesFor{Core,Modules}.cpp files
with a script which reads IDL files looking for the appropriate extended
attributes (just OriginTrialEnabled currently) and generates the required code
to install those features at runtime.
BUG=615060
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is nice! LGTM. https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py (right): https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:36: "v8_class_or_partial": interface_info[2]} What is [1] and [2]? Is there any more readable way to access the entry? https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:60: return interfaces.items()[0][1] Ditto. https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:144: "v8_class": interface_info[1], Ditto. https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl (right): https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl:19: CORE_EXPORT void InstallConditionalFeaturesOnWindow(const ScriptState*); (Not related to this CL but) InstallConditionalFeaturesOnWindow is called by LocalWindowProxy::initialize only for the main world. Is this simply because OriginTrials is supported only on the main world?
The CQ bit was checked by iclelland@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...
https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py (right): https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:36: "v8_class_or_partial": interface_info[2]} On 2017/07/17 07:28:17, haraken wrote: > > What is [1] and [2]? Is there any more readable way to access the entry? > I created a namedtuple subclass to allow these items to be indexed by name instead. (I would have used a desructuring bind, but that makes the code around line 144 much more complicated.) https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:60: return interfaces.items()[0][1] On 2017/07/17 07:28:17, haraken wrote: > > Ditto. > Changed this to values(), since we don't actually care about the key. Added a comment that we're just returning the first (and only) value from the dictionary. https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py:144: "v8_class": interface_info[1], On 2017/07/17 07:28:17, haraken wrote: > > Ditto. Namedtuple makes this more readable now. https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl (right): https://codereview.chromium.org/2970003002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.h.tmpl:19: CORE_EXPORT void InstallConditionalFeaturesOnWindow(const ScriptState*); On 2017/07/17 07:28:17, haraken wrote: > > (Not related to this CL but) InstallConditionalFeaturesOnWindow is called by > LocalWindowProxy::initialize only for the main world. Is this simply because > OriginTrials is supported only on the main world? > That is correct, OriginTrials are only supported in the main world.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
tkent, ping :)
lgtm
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2970003002/#ps280001 (title: "Readability fixes")
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": 280001, "attempt_start_ts": 1500476500211570,
"parent_rev": "71f161e3bd95827876f67c656168f936d88a6e3a", "commit_rev":
"d25397270d7f955de0988221f919a0b67f91f159"}
Message was sent while issue was closed.
Description was changed from
==========
Add code generation for ConditionalFeatures bindings code
This replaces the handwritten ConditionalFeaturesFor{Core,Modules}.cpp files
with a script which reads IDL files looking for the appropriate extended
attributes (just OriginTrialEnabled currently) and generates the required code
to install those features at runtime.
BUG=615060
==========
to
==========
Add code generation for ConditionalFeatures bindings code
This replaces the handwritten ConditionalFeaturesFor{Core,Modules}.cpp files
with a script which reads IDL files looking for the appropriate extended
attributes (just OriginTrialEnabled currently) and generates the required code
to install those features at runtime.
BUG=615060
Review-Url: https://codereview.chromium.org/2970003002
Cr-Commit-Position: refs/heads/master@{#487917}
Committed:
https://chromium.googlesource.com/chromium/src/+/d25397270d7f955de0988221f919...
==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/d25397270d7f955de0988221f919... |
