|
|
Description[WebAgentsAPI]: Start adding support for method arguments.
To make the C++ API more natural/elegant to use,
I decided to rely on method overloads to handle
union and optional types, thus generating more than
one C++ function for each IDL operation.
Includes:
* Basic handling of optional and union types in arguments.
* Plumbing for template contexts to include arguments.
* Minor reshufflings of related code.
BUG=683743
R=bashi,yuki,haraken
Review-Url: https://codereview.chromium.org/2703653004
Cr-Commit-Position: refs/heads/master@{#458057}
Committed: https://chromium.googlesource.com/chromium/src/+/912f7ad6383cc7aa987c66cee8bb23c75c45685c
Patch Set 1 #Patch Set 2 : Rebased. #
Total comments: 4
Patch Set 3 : Comments addressed. #
Messages
Total messages: 34 (18 generated)
The CQ bit was checked by dglazkov@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...
Description was changed from ========== [WebAgentsAPI]: Add support for method arguments. To make the C++ more natural and elegant to use, I decided to rely on method overloads to handle union and optional types, thus generating more than one C++ functions for each IDL operation. Includes: * Basic handling of optional and union types in arguments. * Plumbing for template contexts to include arguments. * Minor reshufflings of related code. BUG=683743 R=bashi,yuki,haraken ========== to ========== [WebAgentsAPI]: Add support for method arguments. To make the C++ API more natural/elegant to use, I decided to rely on method overloads to handle union and optional types, thus generating more than one C++ functions for each IDL operation. Includes: * Basic handling of optional and union types in arguments. * Plumbing for template contexts to include arguments. * Minor reshufflings of related code. BUG=683743 R=bashi,yuki,haraken ==========
Description was changed from ========== [WebAgentsAPI]: Add support for method arguments. To make the C++ API more natural/elegant to use, I decided to rely on method overloads to handle union and optional types, thus generating more than one C++ functions for each IDL operation. Includes: * Basic handling of optional and union types in arguments. * Plumbing for template contexts to include arguments. * Minor reshufflings of related code. BUG=683743 R=bashi,yuki,haraken ========== to ========== [WebAgentsAPI]: Add support for method arguments. To make the C++ API more natural/elegant to use, I decided to rely on method overloads to handle union and optional types, thus generating more than one C++ function for each IDL operation. Includes: * Basic handling of optional and union types in arguments. * Plumbing for template contexts to include arguments. * Minor reshufflings of related code. BUG=683743 R=bashi,yuki,haraken ==========
Description was changed from ========== [WebAgentsAPI]: Add support for method arguments. To make the C++ API more natural/elegant to use, I decided to rely on method overloads to handle union and optional types, thus generating more than one C++ function for each IDL operation. Includes: * Basic handling of optional and union types in arguments. * Plumbing for template contexts to include arguments. * Minor reshufflings of related code. BUG=683743 R=bashi,yuki,haraken ========== to ========== [WebAgentsAPI]: Start adding support for method arguments. To make the C++ API more natural/elegant to use, I decided to rely on method overloads to handle union and optional types, thus generating more than one C++ function for each IDL operation. Includes: * Basic handling of optional and union types in arguments. * Plumbing for template contexts to include arguments. * Minor reshufflings of related code. BUG=683743 R=bashi,yuki,haraken ==========
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_...)
PTAL.
bashi, yukishiino: Should we probably reuse the "overload resolution algorithm" for web agent APIs as well? Or would it be an overkill (i.e., this CL is fine)?
On 2017/02/19 02:32:24, haraken wrote: > bashi, yukishiino: Should we probably reuse the "overload resolution algorithm" > for web agent APIs as well? Or would it be an overkill (i.e., this CL is fine)? FWIW, the overload resolution algorithm is defined in [1] and implemented in [2]. [1] https://heycam.github.io/webidl/#es-overloads [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip...
On 2017/02/19 at 02:34:26, haraken wrote: > On 2017/02/19 02:32:24, haraken wrote: > > bashi, yukishiino: Should we probably reuse the "overload resolution algorithm" > > for web agent APIs as well? Or would it be an overkill (i.e., this CL is fine)? > > FWIW, the overload resolution algorithm is defined in [1] and implemented in [2]. > > [1] https://heycam.github.io/webidl/#es-overloads > [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip... ZOMG, totally missed it. Let me study it. WDYT if I factor this out into its own class and reuse? With tests? :) :DG<
On 2017/02/19 04:30:12, dglazkov wrote: > On 2017/02/19 at 02:34:26, haraken wrote: > > On 2017/02/19 02:32:24, haraken wrote: > > > bashi, yukishiino: Should we probably reuse the "overload resolution > algorithm" > > > for web agent APIs as well? Or would it be an overkill (i.e., this CL is > fine)? > > > > FWIW, the overload resolution algorithm is defined in [1] and implemented in > [2]. > > > > [1] https://heycam.github.io/webidl/#es-overloads > > [2] > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip... > > ZOMG, totally missed it. Let me study it. WDYT if I factor this out into its own > class and reuse? With tests? :) That'd be definitely better :)
On 2017/02/19 04:51:54, haraken wrote: > On 2017/02/19 04:30:12, dglazkov wrote: > > On 2017/02/19 at 02:34:26, haraken wrote: > > > On 2017/02/19 02:32:24, haraken wrote: > > > > bashi, yukishiino: Should we probably reuse the "overload resolution > > algorithm" > > > > for web agent APIs as well? Or would it be an overkill (i.e., this CL is > > fine)? > > > > > > FWIW, the overload resolution algorithm is defined in [1] and implemented in > > [2]. > > > > > > [1] https://heycam.github.io/webidl/#es-overloads > > > [2] > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip... > > > > ZOMG, totally missed it. Let me study it. WDYT if I factor this out into its > own > > class and reuse? With tests? :) > > That'd be definitely better :) Yes! That's great if we can factor out the overload resolution logic.
On 2017/02/19 23:00:48, bashi wrote: > On 2017/02/19 04:51:54, haraken wrote: > > On 2017/02/19 04:30:12, dglazkov wrote: > > > On 2017/02/19 at 02:34:26, haraken wrote: > > > > On 2017/02/19 02:32:24, haraken wrote: > > > > > bashi, yukishiino: Should we probably reuse the "overload resolution > > > algorithm" > > > > > for web agent APIs as well? Or would it be an overkill (i.e., this CL is > > > fine)? > > > > > > > > FWIW, the overload resolution algorithm is defined in [1] and implemented > in > > > [2]. > > > > > > > > [1] https://heycam.github.io/webidl/#es-overloads > > > > [2] > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip... > > > > > > ZOMG, totally missed it. Let me study it. WDYT if I factor this out into its > > own > > > class and reuse? With tests? :) > > > > That'd be definitely better :) > > Yes! That's great if we can factor out the overload resolution logic. Sounds nice to me, too. By the way, our implementation would be (partially) out of date. IIRC, it's one of our todos to catch up to the latest spec.
I accept this yak shave.
On 2017/02/20 at 02:58:20, dglazkov wrote: > I accept this yak shave. 3 weeks later... :) I am not sure the effective_overload_set algorithm is the right fit for C++ bindings. I tried to describe my thinking in https://codereview.chromium.org/2756993003. I am now leaning to using the algorithm in this patch, rather than the one in spec -- it's just too different from what I need. For example, I would like to expand union types into separate overloads, so that the API is easier to write code against (no need to create a union class). WDYT?
Rebased.
The CQ bit was checked by dglazkov@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.
> I am not sure the effective_overload_set algorithm is the right fit for C++ > bindings. I tried to describe my thinking in > https://codereview.chromium.org/2756993003. > > I am now leaning to using the algorithm in this patch, rather than the one in > spec -- it's just too different from what I need. For example, I would like to > expand union types into separate overloads, so that the API is easier to write > code against (no need to create a union class). > > WDYT? Sounds reasonable. The complicated overload resolution algorithm is needed to let the IDL compiler automatically map JS values to overloaded C++ methods. In case of the Web Agents, however, the IDL compiler can just generate the overloaded C++ methods and ask developers to implement a Web Agent (in C++) so that the agent calls a proper C++ method (this sounds like a reasonable assumption). LGTM. https://codereview.chromium.org/2703653004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/code_generator_web_agent_api.py (right): https://codereview.chromium.org/2703653004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/code_generator_web_agent_api.py:98: lists. This is the where the actual splitting into overloads happens. the place where https://codereview.chromium.org/2703653004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/code_generator_web_agent_api.py:127: def split_into_overloads(self): It would be helpful to describe a concrete example of the list of the list in a comment.
Comments addressed.
The CQ bit was checked by dglazkov@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/2703653004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/scripts/code_generator_web_agent_api.py (right): https://codereview.chromium.org/2703653004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/code_generator_web_agent_api.py:98: lists. This is the where the actual splitting into overloads happens. On 2017/03/18 at 16:30:17, haraken wrote: > the place where Fixed. Thanks! :) https://codereview.chromium.org/2703653004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/scripts/code_generator_web_agent_api.py:127: def split_into_overloads(self): On 2017/03/18 at 16:30:17, haraken wrote: > It would be helpful to describe a concrete example of the list of the list in a comment. Sure thing! I added an explanation with an example and a unit test to show how that example works.
LGTM
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 dglazkov@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": 1490019683162640, "parent_rev": "e0454c1a4c0df693e36938fd644315909a40f09b", "commit_rev": "912f7ad6383cc7aa987c66cee8bb23c75c45685c"}
Message was sent while issue was closed.
Description was changed from ========== [WebAgentsAPI]: Start adding support for method arguments. To make the C++ API more natural/elegant to use, I decided to rely on method overloads to handle union and optional types, thus generating more than one C++ function for each IDL operation. Includes: * Basic handling of optional and union types in arguments. * Plumbing for template contexts to include arguments. * Minor reshufflings of related code. BUG=683743 R=bashi,yuki,haraken ========== to ========== [WebAgentsAPI]: Start adding support for method arguments. To make the C++ API more natural/elegant to use, I decided to rely on method overloads to handle union and optional types, thus generating more than one C++ function for each IDL operation. Includes: * Basic handling of optional and union types in arguments. * Plumbing for template contexts to include arguments. * Minor reshufflings of related code. BUG=683743 R=bashi,yuki,haraken Review-Url: https://codereview.chromium.org/2703653004 Cr-Commit-Position: refs/heads/master@{#458057} Committed: https://chromium.googlesource.com/chromium/src/+/912f7ad6383cc7aa987c66cee8bb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/912f7ad6383cc7aa987c66cee8bb... |