Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
261243002:
Add support for [Global] / [PrimaryGlobal] IDL extended attributes (Closed)
|
Created:
6 years, 7 months ago by Inactive Modified:
6 years, 7 months ago CC:
blink-reviews, jsbell+serviceworker_chromium.org, serviceworker-reviews, arv+blink, jsbell+bindings_chromium.org, tzik, sof, kouhei+bindings_chromium.org, kinuko, nhiroki, 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, alecflett+watch_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAdd support for [Global] / [PrimaryGlobal] IDL extended attributes
Add support for [Global] / [PrimaryGlobal] IDL extended attributes as per the
latest Web IDL specification:
http://heycam.github.io/webidl/#Global
The [Global] and [PrimaryGlobal] extended attributes can be used to give a name
to one or more global interfaces, which can then be referenced by the [Exposed]
extended attribute (already supported).
One slight difference with the specification is that the values for [Global]
and [PrimaryGlobal] are '&'-separated instead of comma-separated. This is
because having comma-separated here would make IDL parsing a lot harder. This
is consistent with the [Exposed] IDL extended attribute.
There is no web exposed behavior change with this CL. It just makes our IDL
closer to the specification.
R=haraken@chromium.org, nbarth@chromium.org
BUG=369115
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173595
Patch Set 1 #Patch Set 2 : Add more comments #Patch Set 3 : Remove non-ASCII character in comment #
Total comments: 28
Patch Set 4 : Take feedback into consideration #
Total comments: 13
Patch Set 5 : Fix nits #
Total comments: 2
Patch Set 6 : Fix nit #
Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/261243002/diff/40001/Source/bindings/generate... File Source/bindings/generated_bindings.gyp (left): https://codereview.chromium.org/261243002/diff/40001/Source/bindings/generate... Source/bindings/generated_bindings.gyp:108: '<(blink_output_dir)/WorkerGlobalScopeConstructors.idl', This is split between SharedWorkerGlobalScopeConstructors.idl and DedicatedWorkerGlobalScopeConstructors.idl now. https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/utilities.py:96: match = re.search(r'^\s*\[(.*)\]\s*' Without this fix, the comments before the extended attributes in Window.idl was confusing the regex. Parsed extended attributes before: {'WillBeGarbageCollected': '', 'Custom': 'ToV8', 'ImplementedAs': 'DOMWindow', 'DoNotCheckSecurity]\n[\n PrimaryGlobal': '', 'CheckSecurity': 'Frame'} after: {'WillBeGarbageCollected': '', 'Custom': 'ToV8', 'ImplementedAs': 'DOMWindow', 'PrimaryGlobal': '', 'CheckSecurity': 'Frame'} The DoNotCheckSecurity is in a comments are shouldn't be in the parsed attributes, also, the first real extended attribute (PrimaryGlobal) was ignored because it was appended to the bad extended attribute.
LGTM, but nbarth@ might want to take another look.
On 2014/05/05 03:10:43, haraken wrote: > LGTM, but nbarth@ might want to take another look. Thanks, I'll wait for Nils to take a look as well.
Nils, you around? :)
On 2014/05/06 11:29:22, Chris Dumez wrote: > Nils, you around? :) Unfortunately Japan is national holidays until May 6. He'll show up on May 7.
On 2014/05/06 11:30:37, haraken wrote: > On 2014/05/06 11:29:22, Chris Dumez wrote: > > Nils, you around? :) > > Unfortunately Japan is national holidays until May 6. He'll show up on May 7. I see, thanks for informing me Kentaro.
Thanks Chris, and appreciate your patience! 2 substantive points, then various style points. Main substance is that I think switching one dictionary around makes it clearer: interface_name_to_global_names instead of global_name_to_interface_names. (Haven't checked completely, but seems much more direct.) Also I think we can improve the regex parsing just by swapping the order (strip comments *before* finding match), which is more correct and avoids adding a hack. https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/generate_global_constructors.py (right): https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:54: # If the [Global] or [PrimaryGlobal] extended attribute is declared with an Could you put this explanation in a docstring instead of comment? (We're not very consistent on this, but this sort of explanation is perfect for a docstring.) https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:60: for extended_attribute in ['Global', 'PrimaryGlobal']: |key| is fine for the name here https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:61: global_value = extended_attributes.get(extended_attribute) Could you flatten this logic? I generally prefer the following form: if key not in extended_attributes: continue global_value = extended_attributes[key] if global_value: # FIXME: In spec names are comma-separated, but that makes parsing very # difficult (https://www.w3.org/Bugs/Public/show_bug.cgi?id=24959). return global_value.split('&') return [interface_name] Note official style points: * Use the "implicit" false if at all possible. http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#True/False_eva... * Don't use else after return http://www.chromium.org/developers/coding-style (Checking key instead of using dict.get() is personal preference, and avoids test for implicit None.) https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:162: interface_name_to_idl_filename = {args[i]: args[i + 1] Need to use dict(...) instead of {...: ...}: dictionary comprehensions are Python 2.7, and we're using Python 2.6. (Mac OS X and Windows) Also, I'd prefer this to be a list of tuples, since it's just used for one loop: dict just makes the loop more complicated. (Detailed below.) https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:168: # An interface can have multiple global names (e.g. This is really confusing; I think it's because of a dictionary being in the backwards order. I think you want interface_name_to_global_names instead of global_name_to_interface_names. Ultimately we're writing IDL files with lists of constructors, right? (...and these IDL files have one-to-one correspondence with interface names, like WindowConstructors.idl: partial interface Window { ... }; ...so can use interface name as key.) So what we want is a dictionary (aka, map) interface_name_to_constructors, as you've done. However, it's easier to compose maps if you don't have to invert, so clearer is: interface name -> global names and global name -> constructors. ...and then interface_name_to_constructors is very simple to compute. ...and in fact can be computed inline in the next loop, so we don't need a separate loop. Does this make sense? https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:174: assert global_name in global_name_to_interface_names, \ This assert is not needed, since it'll trigger an error on the next line. If you'd like to catch the error, just use try:... except KeyError:... More generally, asserts should *not* be used for bad input, only for logic errors (incorrect assumptions). (Among other reasons, b/c asserts can be turned off.) https://wiki.python.org/moin/UsingAssertionsEffectively https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:183: for interface_name, constructors in interface_name_to_constructors.iteritems(): for interface_name, idl_filename in interface_name_idl_filename: https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:186: interface_name_to_idl_filename[interface_name], How about: interface_name, idl_filename, interface_name_to_constructors[interface_name], (Avoids an iteritems) https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/utilities.py:96: match = re.search(r'^\s*\[(.*)\]\s*' On 2014/05/04 21:40:39, Chris Dumez wrote: > Without this fix, the comments before the extended attributes in Window.idl was > confusing the regex. Hmm...could you instead try putting the # Strip comments section *before* this main match? That's more correct and less obscure. (Properly should strip comments from whole file first, not just from match.) https://codereview.chromium.org/261243002/diff/40001/Source/core/frame/Window... File Source/core/frame/Window.idl (right): https://codereview.chromium.org/261243002/diff/40001/Source/core/frame/Window... Source/core/frame/Window.idl:31: PrimaryGlobal, Alphabetical order please. https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Dedi... File Source/core/workers/DedicatedWorkerGlobalScope.idl (right): https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Dedi... Source/core/workers/DedicatedWorkerGlobalScope.idl:32: Global=Worker&DedicatedWorker, Alphabetical order. https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Shar... File Source/core/workers/SharedWorkerGlobalScope.idl (right): https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Shar... Source/core/workers/SharedWorkerGlobalScope.idl:32: Global=Worker&SharedWorker, Ditto. https://codereview.chromium.org/261243002/diff/40001/Source/modules/servicewo... File Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl (right): https://codereview.chromium.org/261243002/diff/40001/Source/modules/servicewo... Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl:31: Global=ServiceWorker, Ditto.
On 2014/05/07 01:55:56, Nils Barth wrote: > Thanks Chris, and appreciate your patience! > > 2 substantive points, then various style points. > > Main substance is that I think switching one dictionary around makes it clearer: > interface_name_to_global_names instead of > global_name_to_interface_names. > (Haven't checked completely, but seems much more direct.) > > Also I think we can improve the regex parsing just by swapping the order (strip > comments *before* finding match), which is more correct and avoids adding a > hack. > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > File Source/bindings/scripts/generate_global_constructors.py (right): > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > Source/bindings/scripts/generate_global_constructors.py:54: # If the [Global] or > [PrimaryGlobal] extended attribute is declared with an > Could you put this explanation in a docstring instead of comment? > (We're not very consistent on this, but this sort of explanation > is perfect for a docstring.) > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > Source/bindings/scripts/generate_global_constructors.py:60: for > extended_attribute in ['Global', 'PrimaryGlobal']: > |key| is fine for the name here > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > Source/bindings/scripts/generate_global_constructors.py:61: global_value = > extended_attributes.get(extended_attribute) > Could you flatten this logic? > > I generally prefer the following form: > if key not in extended_attributes: > continue > global_value = extended_attributes[key] > if global_value: > # FIXME: In spec names are comma-separated, but that makes parsing very > # difficult (https://www.w3.org/Bugs/Public/show_bug.cgi?id=24959). > return global_value.split('&') > return [interface_name] > > > Note official style points: > * Use the "implicit" false if at all possible. > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#True/False_eva... > * Don't use else after return > http://www.chromium.org/developers/coding-style > > (Checking key instead of using dict.get() is personal preference, and avoids > test for implicit None.) > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > Source/bindings/scripts/generate_global_constructors.py:162: > interface_name_to_idl_filename = {args[i]: args[i + 1] > Need to use dict(...) instead of {...: ...}: > dictionary comprehensions are Python 2.7, and we're using Python 2.6. > (Mac OS X and Windows) > Also, I'd prefer this to be a list of tuples, since it's just used for one loop: > dict just makes the loop more complicated. > (Detailed below.) > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > Source/bindings/scripts/generate_global_constructors.py:168: # An interface can > have multiple global names (e.g. > This is really confusing; I think it's because of a dictionary being in the > backwards order. > I think you want > interface_name_to_global_names instead of > global_name_to_interface_names. > > Ultimately we're writing IDL files with lists of constructors, right? > (...and these IDL files have one-to-one correspondence with interface names, > like > WindowConstructors.idl: partial interface Window { ... }; > ...so can use interface name as key.) > > So what we want is a dictionary (aka, map) > interface_name_to_constructors, as you've done. > However, it's easier to compose maps if you don't have to invert, > so clearer is: > interface name -> global names > and > global name -> constructors. > ...and then interface_name_to_constructors is very simple to compute. > > ...and in fact can be computed inline in the next loop, so we don't need a > separate loop. > > Does this make sense? > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > Source/bindings/scripts/generate_global_constructors.py:174: assert global_name > in global_name_to_interface_names, \ > This assert is not needed, since it'll trigger an error on the next line. > If you'd like to catch the error, just use try:... except KeyError:... > > More generally, asserts should *not* be used for bad input, > only for logic errors (incorrect assumptions). > (Among other reasons, b/c asserts can be turned off.) > https://wiki.python.org/moin/UsingAssertionsEffectively > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > Source/bindings/scripts/generate_global_constructors.py:183: for interface_name, > constructors in interface_name_to_constructors.iteritems(): > for interface_name, idl_filename in interface_name_idl_filename: > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > Source/bindings/scripts/generate_global_constructors.py:186: > interface_name_to_idl_filename[interface_name], > How about: > interface_name, > idl_filename, > interface_name_to_constructors[interface_name], > > (Avoids an iteritems) > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > File Source/bindings/scripts/utilities.py (right): > > https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... > Source/bindings/scripts/utilities.py:96: match = re.search(r'^\s*\[(.*)\]\s*' > On 2014/05/04 21:40:39, Chris Dumez wrote: > > Without this fix, the comments before the extended attributes in Window.idl > was > > confusing the regex. > > Hmm...could you instead try putting the # Strip comments > section *before* this main match? > That's more correct and less obscure. > (Properly should strip comments from whole file first, not just from match.) > > https://codereview.chromium.org/261243002/diff/40001/Source/core/frame/Window... > File Source/core/frame/Window.idl (right): > > https://codereview.chromium.org/261243002/diff/40001/Source/core/frame/Window... > Source/core/frame/Window.idl:31: PrimaryGlobal, > Alphabetical order please. > > https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Dedi... > File Source/core/workers/DedicatedWorkerGlobalScope.idl (right): > > https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Dedi... > Source/core/workers/DedicatedWorkerGlobalScope.idl:32: > Global=Worker&DedicatedWorker, > Alphabetical order. > > https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Shar... > File Source/core/workers/SharedWorkerGlobalScope.idl (right): > > https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Shar... > Source/core/workers/SharedWorkerGlobalScope.idl:32: Global=Worker&SharedWorker, > Ditto. > > https://codereview.chromium.org/261243002/diff/40001/Source/modules/servicewo... > File Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl (right): > > https://codereview.chromium.org/261243002/diff/40001/Source/modules/servicewo... > Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl:31: > Global=ServiceWorker, > Ditto. Thanks for the review Nils. It has been a long day and these don't look like minor changes so I will take care of this tomorrow.
On 2014/05/07 02:20:04, Chris Dumez wrote: > Thanks for the review Nils. It has been a long day and these don't look like > minor changes so I will take care of this tomorrow. No worries; agreed, some are a bit involved. As we say in Japanese, o-tsukare-sama! (You must be tired [from working so hard]!)
https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/generate_global_constructors.py (right): https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:54: # If the [Global] or [PrimaryGlobal] extended attribute is declared with an On 2014/05/07 01:55:57, Nils Barth wrote: > Could you put this explanation in a docstring instead of comment? > (We're not very consistent on this, but this sort of explanation > is perfect for a docstring.) Done. https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:60: for extended_attribute in ['Global', 'PrimaryGlobal']: On 2014/05/07 01:55:57, Nils Barth wrote: > |key| is fine for the name here Done. https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:61: global_value = extended_attributes.get(extended_attribute) On 2014/05/07 01:55:57, Nils Barth wrote: > Could you flatten this logic? > > I generally prefer the following form: > if key not in extended_attributes: > continue > global_value = extended_attributes[key] > if global_value: > # FIXME: In spec names are comma-separated, but that makes parsing very > # difficult (https://www.w3.org/Bugs/Public/show_bug.cgi?id=24959). > return global_value.split('&') > return [interface_name] > > > Note official style points: > * Use the "implicit" false if at all possible. > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#True/False_eva... > * Don't use else after return > http://www.chromium.org/developers/coding-style > > (Checking key instead of using dict.get() is personal preference, and avoids > test for implicit None.) Done. https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:162: interface_name_to_idl_filename = {args[i]: args[i + 1] On 2014/05/07 01:55:57, Nils Barth wrote: > Need to use dict(...) instead of {...: ...}: > dictionary comprehensions are Python 2.7, and we're using Python 2.6. > (Mac OS X and Windows) > Also, I'd prefer this to be a list of tuples, since it's just used for one loop: > dict just makes the loop more complicated. > (Detailed below.) Done. https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:168: # An interface can have multiple global names (e.g. On 2014/05/07 01:55:57, Nils Barth wrote: > This is really confusing; I think it's because of a dictionary being in the > backwards order. > I think you want > interface_name_to_global_names instead of > global_name_to_interface_names. > > Ultimately we're writing IDL files with lists of constructors, right? > (...and these IDL files have one-to-one correspondence with interface names, > like > WindowConstructors.idl: partial interface Window { ... }; > ...so can use interface name as key.) > > So what we want is a dictionary (aka, map) > interface_name_to_constructors, as you've done. > However, it's easier to compose maps if you don't have to invert, > so clearer is: > interface name -> global names > and > global name -> constructors. > ...and then interface_name_to_constructors is very simple to compute. > > ...and in fact can be computed inline in the next loop, so we don't need a > separate loop. > > Does this make sense? Done. https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:174: assert global_name in global_name_to_interface_names, \ On 2014/05/07 01:55:57, Nils Barth wrote: > This assert is not needed, since it'll trigger an error on the next line. > If you'd like to catch the error, just use try:... except KeyError:... > > More generally, asserts should *not* be used for bad input, > only for logic errors (incorrect assumptions). > (Among other reasons, b/c asserts can be turned off.) > https://wiki.python.org/moin/UsingAssertionsEffectively Done. https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:183: for interface_name, constructors in interface_name_to_constructors.iteritems(): On 2014/05/07 01:55:57, Nils Barth wrote: > for interface_name, idl_filename in interface_name_idl_filename: Done. https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:186: interface_name_to_idl_filename[interface_name], On 2014/05/07 01:55:57, Nils Barth wrote: > How about: > interface_name, > idl_filename, > interface_name_to_constructors[interface_name], > > (Avoids an iteritems) Done. https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/utilities.py (right): https://codereview.chromium.org/261243002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/utilities.py:96: match = re.search(r'^\s*\[(.*)\]\s*' On 2014/05/07 01:55:57, Nils Barth wrote: > On 2014/05/04 21:40:39, Chris Dumez wrote: > > Without this fix, the comments before the extended attributes in Window.idl > was > > confusing the regex. > > Hmm...could you instead try putting the # Strip comments > section *before* this main match? > That's more correct and less obscure. > (Properly should strip comments from whole file first, not just from match.) Done. https://codereview.chromium.org/261243002/diff/40001/Source/core/frame/Window... File Source/core/frame/Window.idl (right): https://codereview.chromium.org/261243002/diff/40001/Source/core/frame/Window... Source/core/frame/Window.idl:31: PrimaryGlobal, On 2014/05/07 01:55:57, Nils Barth wrote: > Alphabetical order please. Done. https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Dedi... File Source/core/workers/DedicatedWorkerGlobalScope.idl (right): https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Dedi... Source/core/workers/DedicatedWorkerGlobalScope.idl:32: Global=Worker&DedicatedWorker, On 2014/05/07 01:55:57, Nils Barth wrote: > Alphabetical order. Done. https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Shar... File Source/core/workers/SharedWorkerGlobalScope.idl (right): https://codereview.chromium.org/261243002/diff/40001/Source/core/workers/Shar... Source/core/workers/SharedWorkerGlobalScope.idl:32: Global=Worker&SharedWorker, On 2014/05/07 01:55:57, Nils Barth wrote: > Ditto. Done. https://codereview.chromium.org/261243002/diff/40001/Source/modules/servicewo... File Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl (right): https://codereview.chromium.org/261243002/diff/40001/Source/modules/servicewo... Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl:31: Global=ServiceWorker, On 2014/05/07 01:55:57, Nils Barth wrote: > Ditto. Done.
LGTM with nits - thanks for the revisions! https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... File Source/bindings/scripts/generate_global_constructors.py (right): https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:56: """ Could you start the docstring with a 1-line summary? """Returns global names, if any, for an interface name. If the [Global] ... """ http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen... https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:75: """ Summary eg: """Returns constructors for an interface.""" (Don't need details of implementation in docstring.) https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:80: constructors = [] A little slicker is an auxiliary flattening function (as you use below): def flatten_list(*iterables): return list(itertools.chain(*iterables)) ...which then lets us have: return flatten_list(global_name_to_constructors[global_name] for global_name in global_names) https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:180: # Print warnings from bad [Exposed] / [Global] mapping. Shouldn't we throw an exception instead? This should be a fatal error, right? https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:182: for global_name in global_name_to_constructors: A bit simpler as a set difference, no? https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:184: print 'WARNING: "%s" used in [Exposed=xxx] but does not match any'\ No backslash line continuation; use parentheses and implicit continuation instead. http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Line_l... https://docs.python.org/2/howto/doanddont.html#using-backslash-to-continue-st...
https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... File Source/bindings/scripts/generate_global_constructors.py (right): https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:56: """ On 2014/05/08 00:37:03, Nils Barth wrote: > Could you start the docstring with a 1-line summary? > """Returns global names, if any, for an interface name. > > If the [Global] ... > """ > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen... Done. https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:75: """ On 2014/05/08 00:37:03, Nils Barth wrote: > Summary eg: > """Returns constructors for an interface.""" > > (Don't need details of implementation in docstring.) Done. https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:80: constructors = [] On 2014/05/08 00:37:03, Nils Barth wrote: > A little slicker is an auxiliary flattening function > (as you use below): > > def flatten_list(*iterables): > return list(itertools.chain(*iterables)) > > > ...which then lets us have: > return flatten_list(global_name_to_constructors[global_name] > for global_name in global_names) Done. But used itertools.chain.from_iterable() instead as I think it leads to slightly more readable code. https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:180: # Print warnings from bad [Exposed] / [Global] mapping. On 2014/05/08 00:37:03, Nils Barth wrote: > Shouldn't we throw an exception instead? > This should be a fatal error, right? Done. https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:182: for global_name in global_name_to_constructors: On 2014/05/08 00:37:03, Nils Barth wrote: > A bit simpler as a set difference, no? Done. https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:184: print 'WARNING: "%s" used in [Exposed=xxx] but does not match any'\ On 2014/05/08 00:37:03, Nils Barth wrote: > No backslash line continuation; use parentheses and implicit continuation > instead. > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Line_l... > https://docs.python.org/2/howto/doanddont.html#using-backslash-to-continue-st... Done.
One nit. https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... File Source/bindings/scripts/generate_global_constructors.py (right): https://codereview.chromium.org/261243002/diff/60001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:80: constructors = [] On 2014/05/08 01:09:29, Chris Dumez wrote: > Done. But used itertools.chain.from_iterable() instead as I think it leads to > slightly more readable code. That's fine too, and agreed, a bit clearer. https://codereview.chromium.org/261243002/diff/80001/Source/bindings/scripts/... File Source/bindings/scripts/generate_global_constructors.py (right): https://codereview.chromium.org/261243002/diff/80001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:183: if len(unknown_global_names) > 0: Nit: use implicit true: if unknown_global_names: ... http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=True/F...
https://codereview.chromium.org/261243002/diff/80001/Source/bindings/scripts/... File Source/bindings/scripts/generate_global_constructors.py (right): https://codereview.chromium.org/261243002/diff/80001/Source/bindings/scripts/... Source/bindings/scripts/generate_global_constructors.py:183: if len(unknown_global_names) > 0: On 2014/05/08 01:13:54, Nils Barth wrote: > Nit: use implicit true: > if unknown_global_names: > ... > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=True/F... Done. Sorry I suck at using the Google coding style.
The CQ bit was checked by ch.dumez@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/261243002/100001
On 2014/05/08 01:17:02, Chris Dumez wrote: > Done. Sorry I suck at using the Google coding style. N/p -- I'm sure Samsung coding style would take some getting used to for me ;)
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_blink_rel on tryserver.blink
Message was sent while issue was closed.
Change committed as 173595 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
