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

Issue 270573005: Move modules-dependent IDL statements out of core by supporting 'implements' in RHS interface (Closed)

Created:
6 years, 7 months ago by c.shu
Modified:
6 years, 7 months ago
CC:
blink-reviews, arv+blink, jsbell+bindings_chromium.org, sof, kouhei+bindings_chromium.org, abarth-chromium, falken, marja+watch_chromium.org, blink-reviews-bindings_chromium.org, horo+watch_chromium.org, kinuko+watch, Nate Chapin, watchdog-blink-watchlist_google.com
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

IDLs in core have 'implements' statements that depend on interfaces in modules, i.e.: CoreFoo implements ModulesBar; To avoid layer violation, these statements are moved to the implemented (RHS) IDL files. The code generator is updated to support this. BUG=358074 BUG=360435 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173599

Patch Set 1 #

Total comments: 5

Patch Set 2 : Addressed review comments. #

Total comments: 6

Patch Set 3 : Refine utility function. #

Total comments: 4

Patch Set 4 : Addressed reviewer comments. #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -12 lines) Patch
M Source/bindings/scripts/compute_interfaces_info.py View 1 2 3 3 chunks +8 lines, -2 lines 0 comments Download
M Source/bindings/scripts/utilities.py View 1 2 3 2 chunks +12 lines, -8 lines 4 comments Download
M Source/core/frame/Window.idl View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/workers/WorkerGlobalScope.idl View 1 chunk +0 lines, -1 line 0 comments Download
M Source/modules/imagebitmap/ImageBitmapFactories.idl View 1 chunk +4 lines, -0 lines 1 comment Download

Messages

Total messages: 30 (0 generated)
c.shu
6 years, 7 months ago (2014-05-07 19:16:58 UTC) #1
Inactive
I'd like haraken to give his opinion on this as I seem to remember he ...
6 years, 7 months ago (2014-05-07 20:00:27 UTC) #2
Inactive
https://codereview.chromium.org/270573005/diff/1/Source/bindings/scripts/compute_interfaces_info.py File Source/bindings/scripts/compute_interfaces_info.py (right): https://codereview.chromium.org/270573005/diff/1/Source/bindings/scripts/compute_interfaces_info.py#newcode194 Source/bindings/scripts/compute_interfaces_info.py:194: source_implements_interfaces = get_source_interfaces_from_idl(idl_file_contents, interface_name) Please add a comment to ...
6 years, 7 months ago (2014-05-07 20:08:06 UTC) #3
c.shu
https://codereview.chromium.org/270573005/diff/1/Source/modules/imagebitmap/ImageBitmapFactories.idl File Source/modules/imagebitmap/ImageBitmapFactories.idl (right): https://codereview.chromium.org/270573005/diff/1/Source/modules/imagebitmap/ImageBitmapFactories.idl#newcode59 Source/modules/imagebitmap/ImageBitmapFactories.idl:59: Window implements ImageBitmapFactories; We may have to support both ...
6 years, 7 months ago (2014-05-07 20:14:29 UTC) #4
Inactive
https://codereview.chromium.org/270573005/diff/1/Source/modules/imagebitmap/ImageBitmapFactories.idl File Source/modules/imagebitmap/ImageBitmapFactories.idl (right): https://codereview.chromium.org/270573005/diff/1/Source/modules/imagebitmap/ImageBitmapFactories.idl#newcode59 Source/modules/imagebitmap/ImageBitmapFactories.idl:59: Window implements ImageBitmapFactories; On 2014/05/07 20:14:29, c.shu wrote: > ...
6 years, 7 months ago (2014-05-07 20:21:09 UTC) #5
c.shu
Merged two functions and added explanations.
6 years, 7 months ago (2014-05-07 21:01:49 UTC) #6
Inactive
https://codereview.chromium.org/270573005/diff/20001/Source/bindings/scripts/compute_interfaces_info.py File Source/bindings/scripts/compute_interfaces_info.py (right): https://codereview.chromium.org/270573005/diff/20001/Source/bindings/scripts/compute_interfaces_info.py#newcode175 Source/bindings/scripts/compute_interfaces_info.py:175: how about something like: implements_statements = get_implements_from_idl(idl_file_content, interface_name) // ...
6 years, 7 months ago (2014-05-07 21:10:09 UTC) #7
c.shu
6 years, 7 months ago (2014-05-07 21:10:14 UTC) #8
c.shu
Addressed reviewer comments. https://codereview.chromium.org/270573005/diff/20001/Source/bindings/scripts/compute_interfaces_info.py File Source/bindings/scripts/compute_interfaces_info.py (right): https://codereview.chromium.org/270573005/diff/20001/Source/bindings/scripts/compute_interfaces_info.py#newcode175 Source/bindings/scripts/compute_interfaces_info.py:175: On 2014/05/07 21:10:09, Chris Dumez wrote: ...
6 years, 7 months ago (2014-05-07 21:42:00 UTC) #9
Inactive
https://codereview.chromium.org/270573005/diff/30001/Source/bindings/scripts/compute_interfaces_info.py File Source/bindings/scripts/compute_interfaces_info.py (right): https://codereview.chromium.org/270573005/diff/30001/Source/bindings/scripts/compute_interfaces_info.py#newcode176 Source/bindings/scripts/compute_interfaces_info.py:176: [left_interfaces, right_interfaces] = get_implements_from_idl(idl_file_contents, interface_name) left_interfaces, right_interfaces = ...
6 years, 7 months ago (2014-05-07 21:45:05 UTC) #10
c.shu
https://codereview.chromium.org/270573005/diff/30001/Source/bindings/scripts/compute_interfaces_info.py File Source/bindings/scripts/compute_interfaces_info.py (right): https://codereview.chromium.org/270573005/diff/30001/Source/bindings/scripts/compute_interfaces_info.py#newcode176 Source/bindings/scripts/compute_interfaces_info.py:176: [left_interfaces, right_interfaces] = get_implements_from_idl(idl_file_contents, interface_name) On 2014/05/07 21:45:05, Chris ...
6 years, 7 months ago (2014-05-07 21:52:40 UTC) #11
Inactive
lgtm but please let haraken and Nils review as well.
6 years, 7 months ago (2014-05-08 00:31:26 UTC) #12
haraken
LGTM but nbarth@ should check.
6 years, 7 months ago (2014-05-08 00:58:32 UTC) #13
Inactive
cshu: By the way, what does 'RHS' mean? I am not a big fan of ...
6 years, 7 months ago (2014-05-08 01:13:01 UTC) #14
Nils Barth (inactive)
On 2014/05/08 01:13:01, Chris Dumez wrote: > cshu: By the way, what does 'RHS' mean? ...
6 years, 7 months ago (2014-05-08 01:37:35 UTC) #15
Inactive
On 2014/05/08 01:37:35, Nils Barth wrote: > On 2014/05/08 01:13:01, Chris Dumez wrote: > > ...
6 years, 7 months ago (2014-05-08 01:41:36 UTC) #16
c.shu
On 2014/05/08 01:41:36, Chris Dumez wrote: > On 2014/05/08 01:37:35, Nils Barth wrote: > > ...
6 years, 7 months ago (2014-05-08 02:00:12 UTC) #17
Inactive
On 2014/05/08 02:00:12, c.shu wrote: > On 2014/05/08 01:41:36, Chris Dumez wrote: > > On ...
6 years, 7 months ago (2014-05-08 02:09:14 UTC) #18
c.shu
On 2014/05/08 02:09:14, Chris Dumez wrote: > On 2014/05/08 02:00:12, c.shu wrote: > > On ...
6 years, 7 months ago (2014-05-08 02:16:16 UTC) #19
Nils Barth (inactive)
Thanks so much for doing this Chang! This is correct and necessary for componentization to ...
6 years, 7 months ago (2014-05-08 02:37:39 UTC) #20
Nils Barth (inactive)
LGTM with nits. https://codereview.chromium.org/270573005/diff/50001/Source/bindings/scripts/utilities.py File Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/270573005/diff/50001/Source/bindings/scripts/utilities.py#newcode64 Source/bindings/scripts/utilities.py:64: # Returns two lists of interfaces ...
6 years, 7 months ago (2014-05-08 02:46:46 UTC) #21
c.shu
On 2014/05/08 02:37:39, Nils Barth wrote: > Thanks so much for doing this Chang! > ...
6 years, 7 months ago (2014-05-08 02:48:34 UTC) #22
Nils Barth (inactive)
A note on *where* to put the "implements" statement. Specs vary, and use both. Option ...
6 years, 7 months ago (2014-05-08 02:54:22 UTC) #23
c.shu
The CQ bit was checked by c.shu@samsung.com
6 years, 7 months ago (2014-05-08 02:57:20 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/c.shu@samsung.com/270573005/50001
6 years, 7 months ago (2014-05-08 02:58:08 UTC) #25
commit-bot: I haz the power
Change committed as 173599
6 years, 7 months ago (2014-05-08 04:08:52 UTC) #26
Nils Barth (inactive)
On 2014/05/08 02:57:20, c.shu wrote: > The CQ bit was checked by mailto:c.shu@samsung.com BTW Chang, ...
6 years, 7 months ago (2014-05-08 06:37:03 UTC) #27
Nils Barth (inactive)
This caused a run-bindings-test failure, and the logic was fragile: we need to handle these ...
6 years, 7 months ago (2014-05-08 06:40:25 UTC) #28
c.shu
On 2014/05/08 06:37:03, Nils Barth wrote: > On 2014/05/08 02:57:20, c.shu wrote: > > The ...
6 years, 7 months ago (2014-05-08 14:26:51 UTC) #29
Nils Barth (inactive)
6 years, 7 months ago (2014-05-09 00:40:50 UTC) #30
Message was sent while issue was closed.
On 2014/05/08 14:26:51, c.shu wrote:
> Really sorry about this. I didn't realize there were comments in your post. I
> made a post almost at the same time and your comment was collapsed. I just saw
> the green line. :)

N/p -- I was just a bit puzzled (^.-)b

Powered by Google App Engine
This is Rietveld 408576698