Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(169)

Issue 2756993003: [WebAgentsAPI]: Remove OverloadSetAdapter. (Closed)

Created:
3 years, 9 months ago by dglazkov
Modified:
3 years, 9 months ago
Reviewers:
haraken, bashi, Yuki
CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove OverloadSetAdapter. Despite potential promise of code reuse, the effective overload set algorithm isn't useful for C++-based bindings: the primary purpose of the algorithm is to build a set that is later used by the overload resolution algorithm: https://heycam.github.io/webidl/#dfn-overload-resolution-algorithm The purpose of this algorithm is to identify the overloaded operation based on the arguments given. In case of C++, the compiler does the work of identification and the task is diminished to generating sufficient number of overloads to express the intent in WebIDL. That work is something that is more efficiently done with a different algorithm. Also added another unit test based on the spec example. R=bashi@chromium.org,yukishiino@chromium.org BUG=683743 Review-Url: https://codereview.chromium.org/2756993003 Cr-Commit-Position: refs/heads/master@{#457997} Committed: https://chromium.googlesource.com/chromium/src/+/8ad9fcbb9107bf9c945c8eaa6a0124124a4956f7

Patch Set 1 #

Patch Set 2 : Added a note about diverging from spec. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+131 lines, -59 lines) Patch
M third_party/WebKit/Source/bindings/scripts/overload_set_algorithm.py View 3 chunks +6 lines, -43 lines 0 comments Download
M third_party/WebKit/Source/bindings/scripts/overload_set_algorithm_test.py View 1 3 chunks +125 lines, -16 lines 0 comments Download

Messages

Total messages: 15 (10 generated)
dglazkov
Added a note about diverging from spec.
3 years, 9 months ago (2017-03-18 00:00:59 UTC) #3
dglazkov
PTAL.
3 years, 9 months ago (2017-03-19 03:51:16 UTC) #9
haraken
LGTM
3 years, 9 months ago (2017-03-19 04:14:38 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2756993003/20001
3 years, 9 months ago (2017-03-19 16:13:42 UTC) #12
commit-bot: I haz the power
3 years, 9 months ago (2017-03-19 17:13:16 UTC) #15
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8ad9fcbb9107bf9c945c8eaa6a01...

Powered by Google App Engine
This is Rietveld 408576698