Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2495033003:
Stop using v8_interface.interface_context in Web modules bindings. (Closed)
DescriptionStop using v8_interface.interface_context in Web modules bindings.
We should build our own instead of reusing, let's do that.
Also, let's start generating something for the header files.
R=yukishiino,bashi
BUG=654129
NOTRY=true
Committed: https://crrev.com/f7340622bc0a7c7d613a8ec6e8c63aea934b3503
Cr-Commit-Position: refs/heads/master@{#433435}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Feedback addressed. #
Total comments: 3
Patch Set 3 : Removed the comment. #
Messages
Total messages: 25 (9 generated)
PTAL.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py (right): https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:34: def includes_for_type(idl_type): Would you help me understand why you need to implement a customized includes_for_type for web modules? Doesn't includes_for_type in v8_types.py work? https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:49: methods.append(Method.create(idl_operation)) bashi@, yukishiino@: Is this an expected way to build up meta information of attributes and methods? I'm asking this just because we're not using the Attribute/Method.create pattern in other generators in the IDL compiler. https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:59: class Attribute(object): object is unused https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:73: class Method(object): Ditto. https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:106: if interface.is_callback and interface.is_partial: and => or ? https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:107: return None Or you can raise an exception when you hit something you don't expect. https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:112: h_template = self.get_template('h') header_template https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:114: h_text = h_template.render(template_context) header_text
Feedback addressed.
https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py (right): https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:34: def includes_for_type(idl_type): On 2016/11/12 at 08:01:37, haraken wrote: > Would you help me understand why you need to implement a customized includes_for_type for web modules? Doesn't includes_for_type in v8_types.py work? Sure! `v8_types.includes_for_type` returns very specific paths and names that don't apply in webmodules. For example, if I need to depend on NodeList, I probably don't need `bindings/core/v8/V8NodeList.h` in my includes (see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip...). https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:49: methods.append(Method.create(idl_operation)) On 2016/11/12 at 08:01:37, haraken wrote: > bashi@, yukishiino@: Is this an expected way to build up meta information of attributes and methods? I'm asking this just because we're not using the Attribute/Method.create pattern in other generators in the IDL compiler. Factories seem like a good thing. We use them elsewhere in blink :) https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:59: class Attribute(object): On 2016/11/12 at 08:01:37, haraken wrote: > object is unused What do you mean? The Attribute.create returns an instance. https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:73: class Method(object): On 2016/11/12 at 08:01:37, haraken wrote: > Ditto. Same here. https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:106: if interface.is_callback and interface.is_partial: On 2016/11/12 at 08:01:37, haraken wrote: > and => or ? Oops. Thanks! Fixed. https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:107: return None On 2016/11/12 at 08:01:37, haraken wrote: > Or you can raise an exception when you hit something you don't expect. That's a great idea! Fixed. https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:112: h_template = self.get_template('h') On 2016/11/12 at 08:01:37, haraken wrote: > header_template Fixed. https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:114: h_text = h_template.render(template_context) On 2016/11/12 at 08:01:37, haraken wrote: > header_text Fixed.
bashi@chromium.org changed reviewers: + bashi@chromium.org
lgtm on my side. https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py (right): https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:49: methods.append(Method.create(idl_operation)) On 2016/11/13 00:11:45, dglazkov wrote: > On 2016/11/12 at 08:01:37, haraken wrote: > > bashi@, yukishiino@: Is this an expected way to build up meta information of > attributes and methods? I'm asking this just because we're not using the > Attribute/Method.create pattern in other generators in the IDL compiler. > > Factories seem like a good thing. We use them elsewhere in blink :) We haven't used factories but I'm fine with using them. I like classes over dicts. yukishiino@: what do you think? https://codereview.chromium.org/2495033003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py (right): https://codereview.chromium.org/2495033003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:72: # idl_definitions.TypedObject sub-types, so maybe use inheritance? I think it's a good idea to separate them from TypedObject until we want to access a lot of information which are located in TypedObject. Less dependency allows us to modify TypedObject (if needed) easily. https://codereview.chromium.org/2495033003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/web_module_interface.h.tmpl (right): https://codereview.chromium.org/2495033003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/web_module_interface.h.tmpl:4: // clang-format on Maybe it's better to add a way to controll clang-format {on,off} rather than just overriding it?
LGTM https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py (right): https://codereview.chromium.org/2495033003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:49: methods.append(Method.create(idl_operation)) On 2016/11/13 23:33:58, bashi1 wrote: > On 2016/11/13 00:11:45, dglazkov wrote: > > On 2016/11/12 at 08:01:37, haraken wrote: > > > bashi@, yukishiino@: Is this an expected way to build up meta information of > > attributes and methods? I'm asking this just because we're not using the > > Attribute/Method.create pattern in other generators in the IDL compiler. > > > > Factories seem like a good thing. We use them elsewhere in blink :) > > We haven't used factories but I'm fine with using them. I like classes over > dicts. Yeah, other places are using attribute_context, method_context etc, which return a dictionary. Also this CL is using interface_context. I don't care as long as they will get consistent in the future :) > > yukishiino@: what do you think?
Description was changed from ========== Stop using v8_interface.interface_context in Web modules bindings. We should build our own instead of reusing, let's do that. Also, let's start generating something for the header files. R=yukishiino,bashi BUG=654129 NOTRY=true ========== to ========== Stop using v8_interface.interface_context in Web modules bindings. We should build our own instead of reusing, let's do that. Also, let's start generating something for the header files. R=yukishiino,bashi BUG=654129 NOTRY=true ==========
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org - yukishiino@chromium.or
lgtm
On 2016/11/13 at 23:33:58, bashi wrote: > lgtm on my side. > > https://codereview.chromium.org/2495033003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/templates/web_module_interface.h.tmpl:4: // clang-format on > Maybe it's better to add a way to controll clang-format {on,off} rather than just overriding it? Tell me more. I would love to find a way to do this more elegantly.
https://codereview.chromium.org/2495033003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py (right): https://codereview.chromium.org/2495033003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:72: # idl_definitions.TypedObject sub-types, so maybe use inheritance? On 2016/11/13 at 23:33:58, bashi1 wrote: > I think it's a good idea to separate them from TypedObject until we want to access a lot of information which are located in TypedObject. Less dependency allows us to modify TypedObject (if needed) easily. SG. Will remove comment before landing.
Removed the comment.
The CQ bit was checked by dglazkov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, bashi@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2495033003/#ps40001 (title: "Removed the comment.")
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": 1479593810917440,
"parent_rev": ["261ec779657568c261b5f3b8c35bc435e9be588d", null], "commit_rev":
["f995cc26caeed3ee3501565424fd93720ba3bdac", null]}
Message was sent while issue was closed.
Description was changed from ========== Stop using v8_interface.interface_context in Web modules bindings. We should build our own instead of reusing, let's do that. Also, let's start generating something for the header files. R=yukishiino,bashi BUG=654129 NOTRY=true ========== to ========== Stop using v8_interface.interface_context in Web modules bindings. We should build our own instead of reusing, let's do that. Also, let's start generating something for the header files. R=yukishiino,bashi BUG=654129 NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Stop using v8_interface.interface_context in Web modules bindings. We should build our own instead of reusing, let's do that. Also, let's start generating something for the header files. R=yukishiino,bashi BUG=654129 NOTRY=true ========== to ========== Stop using v8_interface.interface_context in Web modules bindings. We should build our own instead of reusing, let's do that. Also, let's start generating something for the header files. R=yukishiino,bashi BUG=654129 NOTRY=true Committed: https://crrev.com/f7340622bc0a7c7d613a8ec6e8c63aea934b3503 Cr-Commit-Position: refs/heads/master@{#433435} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f7340622bc0a7c7d613a8ec6e8c63aea934b3503 Cr-Commit-Position: refs/heads/master@{#433435}
Message was sent while issue was closed.
On 2016/11/19 22:13:10, dglazkov (OOO until Nov 28) wrote: > On 2016/11/13 at 23:33:58, bashi wrote: > > lgtm on my side. > > > > > https://codereview.chromium.org/2495033003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/templates/web_module_interface.h.tmpl:4: // > clang-format on > > Maybe it's better to add a way to controll clang-format {on,off} rather than > just overriding it? > > Tell me more. I would love to find a way to do this more elegantly. We will need to add a flag (e.g. clang_format_mode) to the template context, then use it in copyright_block.txt: // clang-format {% clang_format_mode %} I'll do that :)
Message was sent while issue was closed.
On 2016/11/24 02:01:47, bashi1 wrote: > On 2016/11/19 22:13:10, dglazkov (OOO until Nov 28) wrote: > > On 2016/11/13 at 23:33:58, bashi wrote: > > > lgtm on my side. > > > > > > > > > https://codereview.chromium.org/2495033003/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/bindings/templates/web_module_interface.h.tmpl:4: > // > > clang-format on > > > Maybe it's better to add a way to controll clang-format {on,off} rather than > > just overriding it? > > > > Tell me more. I would love to find a way to do this more elegantly. > > We will need to add a flag (e.g. clang_format_mode) to the template context, > then use it in copyright_block.txt: > > // clang-format {% clang_format_mode %} > > I'll do that :) I have a concern on turning on clang-format, because a change in an IDL file can re-generate all generated .cpp/.h files. The reasons why I think so are 1) Currently we have dependency from "all IDL files" to "all generated files" 2) Code generator compares generated code and existing .cpp|.h files to decide if they should be updated. 3) clang-format runs on generated files Am I wrong? If I'm wrong, then I think we can remove "clang-format off" from the template. Anyway, I think we don't need to make it configurable; always on or off.
Message was sent while issue was closed.
On 2016/11/24 02:43:28, peria wrote: > On 2016/11/24 02:01:47, bashi1 wrote: > > On 2016/11/19 22:13:10, dglazkov (OOO until Nov 28) wrote: > > > On 2016/11/13 at 23:33:58, bashi wrote: > > > > lgtm on my side. > > > > > > > > > > > > > > https://codereview.chromium.org/2495033003/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/bindings/templates/web_module_interface.h.tmpl:4: > > // > > > clang-format on > > > > Maybe it's better to add a way to controll clang-format {on,off} rather > than > > > just overriding it? > > > > > > Tell me more. I would love to find a way to do this more elegantly. > > > > We will need to add a flag (e.g. clang_format_mode) to the template context, > > then use it in copyright_block.txt: > > > > // clang-format {% clang_format_mode %} > > > > I'll do that :) > > I have a concern on turning on clang-format, because a change in an IDL file can > re-generate all generated .cpp/.h files. > > The reasons why I think so are > 1) Currently we have dependency from "all IDL files" to "all generated files" > 2) Code generator compares generated code and existing .cpp|.h files to decide > if they should be updated. > 3) clang-format runs on generated files > Am I wrong? > > If I'm wrong, then I think we can remove "clang-format off" from the template. > Anyway, I think we don't need to make it configurable; always on or off. Thanks peria-san. IIUC it means that when we turn on clang-format, we will compile generated code every time even there is no logic change. I'd prefer to turn off clang-format as a short-term fix then. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
