Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24257003:
IDL compiler: Types (Closed)
|
Created:
7 years, 3 months ago by Nils Barth (inactive) Modified:
7 years, 2 months ago Reviewers:
haraken CC:
blink-reviews, kojih, jsbell+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, haraken, Nate Chapin, do-not-use, kouhei (in TOK) Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionIDL compiler: Types
This is a basic CL for the compiler, doing various type handling and
conversion.
It's rather long (600 lines), but primarily data tables, and the logic
is very flat.
BUG=239771
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158239
Patch Set 1 #Patch Set 2 : Fix callback interfaces #
Total comments: 110
Patch Set 3 : Fix callback interface note #Patch Set 4 : Revised, add tests #Patch Set 5 : Revised #
Total comments: 36
Patch Set 6 : Revised, final #Patch Set 7 : Spit and finish #Patch Set 8 : Fix copyright + license #
Messages
Total messages: 14 (0 generated)
Here's the type handling. (With tweaks so existing code handles it.) It's long but flat, and mostly data, and I've broken it up into sections. Could you PTAL?
50% reviewed. Let me take a look later. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... File Source/bindings/scripts/unstable/v8_types.py (right): https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:73: # PRIMITIVE_TYPES doesn't contain Promise. Drop this comment, since it doesn't make sense at this point. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:96: 'DOMString', # FIXME: ok to include here? What does this comment mean? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:102: 'NodeFilter', Why is NodeFilter not a wrapper? (Actually, I don't have to ask this kind of question nor you don't have to study code to answer the question, if you incrementally move real IDL files. That's the reason why we want to incrementally add necessary features to the new compiler as we move real IDL files.) Given that NON_WRAPPER_TYPES is for special cases, let's remove this from this CL. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:106: TYPED_ARRAYS = { Ditto. Let's add TYPED_ARRAYS when you need to support TypedArrays. Since you already have an entire WIP CL, I'm OK with adding a couple of unnecessary things to each CL (split from the WIP CL). However, we don't want to add unnecessary things without knowing whether it's correct or not. For example, I'd prefer: 'Uint8Array': ('cpp_type': 'unsigned char', 'v8_type': 'v8::kExternalUnsignedByteArray'), but I'm not sure until I see real generated code for typed arrays. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:193: 'SVG' in idl_type)) Remove these uncertain things for now, and revisit the issue when supporting real IDL files that need them. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:203: 'int', # FIXME: int is not an IDL type Shall we remove? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:218: CPP_TYPE_SPECIAL_CONVERSION_RULES = { Remove these special cases until you support IDL files that need them. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:233: def cpp_type(idl_type, extended_attributes=None, used_as_parameter=False): used_as_parameter => used_as_argument ? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:235: # Perl: GetNativeType; WIP: get_native_type Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:236: # FIXME: support used_as_parameter for all types I don't think we need to support used_as_parameter for all types. Only a part of types will require special handling when used as parameters. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:238: svg_cpp_type = get_svg_type_needing_tear_off(idl_type) Remove. Let's revisit the SVG issue after completing all other parts. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:274: # FIXME: code_generator_idl_reader dependency Add this when implementing [ImplementedAs]. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:278: if ref_ptr_type(idl_type) and not used_as_parameter: Shouldn't this be: if ref_ptr_type(idl_type): if used_as_parameter: return cpp_template('PassRefPtr', idl_type) return cpp_template('RefPtr', idl_type) ? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:284: def cpp_template(template, inner_type): cpp_template => cpp_template_type ? "template" is confusing with jinja's template. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:294: # FIXME: check polarity on used_as_parameter Do you mean we want to support [TreatReturnedNullAs=xxx] etc? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:305: # FIXME: add case: extended_attributes.get('TreatUndefinedAs') == 'NullString') I think you can drop this. No real IDL files require this. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:325: # Perl: SkipIncludeHeader Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:333: # Perl: AddIncludesForType Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:349: # Perl: HeaderFilesForInterface in perl Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:355: # return list(includes_for_cpp_class(cpp_class_name, relative_dir_posix)) Remove this for now, and add this when you implement it. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:366: 'float': 'static_cast<{idl_type}>({v8_value}->NumberValue())', Isn't idl_type always 'float' ? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:367: 'double': 'static_cast<{idl_type}>({v8_value}->NumberValue())', Isn't idl_type always 'double' ? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:376: # Specific types Remove this until you need it. In the future, I think you should implement toXXX for all these special XXX and remove these special handling. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:628: ################################################################################ Drop SVG for now. At this point, it's not clear how we refactor SVG code.
100% reviewed. It's hard to review this CL because there are a lot of code that isn't yet used by any IDL file. (I had to look up the Perl generator to understand how each code is used.) From the next CL, please implement only one small feature in one CL. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... File Source/bindings/scripts/unstable/v8_types.py (right): https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:385: V8_VALUE_TO_CPP_VALUE_AND_INCLUDES = { It looks like that defining this macro doesn't improve readability. I'd inline the contents in v8_value_to_cpp_value. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:397: V8_VALUE_TO_CPP_VALUE_ARRAY_OR_SEQUENCE = 'toNativeArray<{cpp_type}>({v8_value}, {isolate})' Ditto for other V8_VALUE_CPP_VALUE_XXX. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:408: # Perl: JSValueToNative Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:441: # Perl: part of JSValueToNative Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:454: V8_VALUE_TO_CPP_VALUE_STATEMENT_STRING = 'V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID({cpp_type}, {variable_name}, {cpp_value});' Ditto. Inline the contents to v8_value_to_cpp_value_statement. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:460: # Perl: JSValueToNativeStatement Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:461: if idl_type == 'unsigned long' and 'IsIndex' in extended_attributes: We no longer have [IsIndex]. In any event, I'm not sure if this branch is needed. Please add it when you support IDL files that actually need it. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:468: if not this_cpp_type.startswith('V8StringResource'): This check looks unnecessary (You're just checking whether the implementation of cpp_type() is correct). Remove it. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:487: def preprocess_type_and_value(idl_type, cpp_value, extended_attributes): You can split this to preprocess_idl_type and preprocess_cpp_value. Then you can merge line 413-416 into preprocess_idl_type. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:511: def v8_conversion_type_and_includes(idl_type): v8_convertion_type => v8_type https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:517: # Perl: part of NativeToJSValue Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:524: if this_cpp_type in ['int', 'unsigned', 'ScriptValue']: This branch looks strange. All of these branches should be 'if idl_type in ...'. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:532: conversion_type = simple_conversion_type() You can inline simple_conversion_type() here. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:550: # FIXME: Use safe handles Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:551: includes.add('wtf/GetPtr.h') I'm not sure if we need to use GetPtr in generated code (I know we're using GetPtr in the current generated code, but I'm not sure if it's really needed). Remove it until you need it. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:565: 'primitive': 'v8SetReturnValue({callback_info}, {cpp_value});', Remove this until you need it. It's dangerous if this kind of wildcard method is mis-used for wrong types. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:570: # FIXME: callback_info (and isolate?) should be required I think it's required. Why is this FIXME? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:573: # Perl: part of NativeToJSValue Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:577: if for_main_world_suffix == 'ForMainWorld': I guess 'if for_main_world_suffix' should come before 'if not script_wrappable'. (I'm not sure because there is no actually generated code...) https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:584: if conversion_type in ['array', 'Date', 'ScriptValue', 'SerializedScriptValue']: It looks strange why these types are handled specially. In particular, it's strange that you're treating ScriptValue as 'primitive' here but converting 'primitive' to 'v8::Number::New' in CPP_VALUE_TO_V8_VALUE. Remove this branch until you need it. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:600: 'DOMString_no_isolate': 'v8String({cpp_value})', Where is this used? Remove it until you need it. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:607: # General Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:609: 'primitive': 'v8::Number::New({cpp_value})', I'd rename 'primitive' to 'number'. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:616: # Perl: NativeToJSValue Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:618: # FIXME: support union types (in WIP) Remove. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:622: conversion_type = 'DOMString_no_isolate' We don't want to have this branch. Remove it until you need it.
Thanks for detailed review! See revised CL, with comments. For next CL, I'll implement just enough to support basic writable attributes (setters, and any necessary changes to getters etc.), so it should be reasonably short, and then we can incrementally land other features like TypedArrays, CallWith, etc., etc. 2 general points 1. Test cases We can’t test on *full* real IDL files, because there aren’t enough simple ones (many attributes only appear in large IDL files). What we can do is test on specific attributes actually used in real IDL files (there are ~2,600 attributes, in about 260 types). I’ve added a TestObjectPython.idl file so we can incrementally add attributes to it as we support them. (For example, this CL doesn't support DOMString, since it's not primitive but not a wrapper type. Easily fix, but better in a follow-up.) 2. Changes to output I’d like to match the Perl output exactly, otherwise I can’t automatically compare. If we want to change the output, we can change the Perl too, but that slows us down. Alternatively, we can add a FIXME when we think something is unnecessary. (Specifically this is regarding “wtf/GetPtr.h”.) https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... File Source/bindings/scripts/unstable/v8_types.py (right): https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:73: # PRIMITIVE_TYPES doesn't contain Promise. On 2013/09/19 10:51:42, haraken wrote: > > Drop this comment, since it doesn't make sense at this point. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:96: 'DOMString', # FIXME: ok to include here? On 2013/09/19 10:51:42, haraken wrote: > What does this comment mean? Cruft from refactoring; deleted as part of deleting this variable. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:102: 'NodeFilter', On 2013/09/19 10:51:42, haraken wrote: > Given that NON_WRAPPER_TYPES is for special cases, let's remove this from this > CL. Removed. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:106: TYPED_ARRAYS = { On 2013/09/19 10:51:42, haraken wrote: > Ditto. Let's add TYPED_ARRAYS when you need to support TypedArrays. Removed. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:193: 'SVG' in idl_type)) On 2013/09/19 10:51:42, haraken wrote: > Remove these uncertain things for now, and revisit the issue when supporting > real IDL files that need them. I've removed these, and made wrapper_type just a function returning ref_ptr_type. We know we'll need this function in future, but for not including special cases. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:203: 'int', # FIXME: int is not an IDL type On 2013/09/19 10:51:42, haraken wrote: > Shall we remove? Yup, turns out it's only used in Inspector IDLS, which aren't legit IDLs generally (mixed with C++ code). https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:218: CPP_TYPE_SPECIAL_CONVERSION_RULES = { On 2013/09/19 10:51:42, haraken wrote: > Remove these special cases until you support IDL files that need them. Removed. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:233: def cpp_type(idl_type, extended_attributes=None, used_as_parameter=False): On 2013/09/19 10:51:42, haraken wrote: > > used_as_parameter => used_as_argument ? Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:235: # Perl: GetNativeType; WIP: get_native_type On 2013/09/19 10:51:42, haraken wrote: > Remove. Done, removed throughout. (Useful for reference in my notes, but doesn't need to be in tree.) https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:236: # FIXME: support used_as_parameter for all types On 2013/09/19 10:51:42, haraken wrote: > > I don't think we need to support used_as_parameter for all types. Only a part of > types will require special handling when used as parameters. Got it; this was a FIXME in the WIP. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:238: svg_cpp_type = get_svg_type_needing_tear_off(idl_type) On 2013/09/19 10:51:42, haraken wrote: > Remove. Let's revisit the SVG issue after completing all other parts. Got it, done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:274: # FIXME: code_generator_idl_reader dependency On 2013/09/19 10:51:42, haraken wrote: > > Add this when implementing [ImplementedAs]. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:278: if ref_ptr_type(idl_type) and not used_as_parameter: On 2013/09/19 10:51:42, haraken wrote: > > Shouldn't this be: > > if ref_ptr_type(idl_type): > if used_as_parameter: > return cpp_template('PassRefPtr', idl_type) > return cpp_template('RefPtr', idl_type) > > ? That sounds sensible, though the Perl code (in GetNativeType) reads: return "RefPtr<${type}>" if IsRefPtrType($type) and not $isParameter; Shall I fix the Perl code in a separate CL? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:284: def cpp_template(template, inner_type): On 2013/09/19 10:51:42, haraken wrote: > cpp_template => cpp_template_type ? > > "template" is confusing with jinja's template. Yeah, unfortunate name clash. Renamed, should be clearer. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:294: # FIXME: check polarity on used_as_parameter On 2013/09/19 10:51:42, haraken wrote: > Do you mean we want to support [TreatReturnedNullAs=xxx] etc? Oh, that's what it meant! (Comment from WIP) Added note to explicitly say "support TreatReturnedNullStringAs". https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:305: # FIXME: add case: extended_attributes.get('TreatUndefinedAs') == 'NullString') On 2013/09/19 10:51:42, haraken wrote: > I think you can drop this. No real IDL files require this. Got it; replaced this with raising an exception in case anyone tries to do so in future. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:325: # Perl: SkipIncludeHeader On 2013/09/19 10:51:42, haraken wrote: > > Remove. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:333: # Perl: AddIncludesForType On 2013/09/19 10:51:42, haraken wrote: > > Remove. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:349: # Perl: HeaderFilesForInterface in perl On 2013/09/19 10:51:42, haraken wrote: > > Remove. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:355: # return list(includes_for_cpp_class(cpp_class_name, relative_dir_posix)) On 2013/09/19 10:51:42, haraken wrote: > > Remove this for now, and add this when you implement it. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:366: 'float': 'static_cast<{idl_type}>({v8_value}->NumberValue())', On 2013/09/19 10:51:42, haraken wrote: > Isn't idl_type always 'float' ? Good point! Originally 'float' and 'double' shared a format string, so 'idl_type' could be either, but since they're now in a lookup table, we can just fill it in. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:367: 'double': 'static_cast<{idl_type}>({v8_value}->NumberValue())', On 2013/09/19 10:51:42, haraken wrote: > > Isn't idl_type always 'double' ? Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:376: # Specific types On 2013/09/19 10:51:42, haraken wrote: > Remove this until you need it. Done. > In the future, I think you should implement toXXX for all these special XXX and > remove these special handling. Makes sense; added to my to-do-later. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:385: V8_VALUE_TO_CPP_VALUE_AND_INCLUDES = { On 2013/09/19 12:04:12, haraken wrote: > It looks like that defining this macro doesn't improve readability. I'd inline > the contents in v8_value_to_cpp_value. I'd really prefer to keep this as a table; it's analogous to V8_VALUE_TO_CPP_VALUE, but with includes, and means that v8_value_to_cpp_value is a *very* simple function. I'm ok with inlining specific format strings (the other constants), since that's removes a bit of indirection, but I'd like to have lookup tables instead of long if/elif statements. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:397: V8_VALUE_TO_CPP_VALUE_ARRAY_OR_SEQUENCE = 'toNativeArray<{cpp_type}>({v8_value}, {isolate})' On 2013/09/19 12:04:12, haraken wrote: > Ditto for other V8_VALUE_CPP_VALUE_XXX. Done (wanted to keep all format strings together, but fine to inline individual cases). https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:408: # Perl: JSValueToNative On 2013/09/19 12:04:12, haraken wrote: > > Remove. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:441: # Perl: part of JSValueToNative On 2013/09/19 12:04:12, haraken wrote: > > Remove. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:454: V8_VALUE_TO_CPP_VALUE_STATEMENT_STRING = 'V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID({cpp_type}, {variable_name}, {cpp_value});' On 2013/09/19 12:04:12, haraken wrote: > Ditto. Inline the contents to v8_value_to_cpp_value_statement. Done. Wanted to keep data and logic clearly separated, but in some cases that just adds excess indirection. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:460: # Perl: JSValueToNativeStatement On 2013/09/19 12:04:12, haraken wrote: > > Remove. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:461: if idl_type == 'unsigned long' and 'IsIndex' in extended_attributes: On 2013/09/19 12:04:12, haraken wrote: > We no longer have [IsIndex]. Got it, removing cruft. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:468: if not this_cpp_type.startswith('V8StringResource'): On 2013/09/19 12:04:12, haraken wrote: > This check looks unnecessary (You're just checking whether the implementation of > cpp_type() is correct). Remove it. Agreed, removed. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:487: def preprocess_type_and_value(idl_type, cpp_value, extended_attributes): On 2013/09/19 12:04:12, haraken wrote: > You can split this to preprocess_idl_type and preprocess_cpp_value. I don't think I can clearly separate them, as the casting to double changes type and value simultaneously, so it's clearer to have them together. I would only be able to remove one line from this function (the idl_type change one), and then I'd need another function just to do the idl_type conversion. > Then you can merge line 413-416 into preprocess_idl_type. Those lines are for V8 -> C++ (idl_type is input); this function is C++ -> V8 (idl_type is output). https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:511: def v8_conversion_type_and_includes(idl_type): On 2013/09/19 12:04:12, haraken wrote: > v8_convertion_type => v8_type The 'conversion_type' is an auxiliary variable, and isn't actually a V8/JS type: it can be IDL, C++, or just 'for_main_world' etc. This is so the type handling can be factored out of v8_set_return_value and cpp_value_to_v8_value (in Perl it's merged in a big mess with a separate branch for each case, selecting v8SetReturn or not). I've elaborated the comment to make this clearer. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:517: # Perl: part of NativeToJSValue On 2013/09/19 12:04:12, haraken wrote: > > Remove. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:524: if this_cpp_type in ['int', 'unsigned', 'ScriptValue']: On 2013/09/19 12:04:12, haraken wrote: > This branch looks strange. All of these branches should be 'if idl_type in ...'. It is a bit funny, but makes sense: for these types, conversion only depends on the C++ type, not which IDL type (all unsigned types are treated the same), and so we call cpp_type to not duplicate that logic here. I've added a comment to explain. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:532: conversion_type = simple_conversion_type() On 2013/09/19 12:04:12, haraken wrote: > You can inline simple_conversion_type() here. Done. I wanted to clearly separate type from includes (so only a single set() for "no includes"), but that adds several extra lines, and it's clearer inline. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:550: # FIXME: Use safe handles On 2013/09/19 12:04:12, haraken wrote: > > Remove. Done. The comment is in the Perl CG, hence copied over. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:551: includes.add('wtf/GetPtr.h') On 2013/09/19 12:04:12, haraken wrote: > I'm not sure if we need to use GetPtr in generated code (I know we're using > GetPtr in the current generated code, but I'm not sure if it's really needed). Understood. However, so I can check that we're generating the same code, it's important to generate *exactly* the same code as the Perl, so for now I'd like to leave it in. I've added a FIXME so we won't forget about it. If you'd like, I could try removing it from the Perl CG, but that might take a while to investigate. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:565: 'primitive': 'v8SetReturnValue({callback_info}, {cpp_value});', On 2013/09/19 12:04:12, haraken wrote: > Remove this until you need it. It's dangerous if this kind of wildcard method is > mis-used for wrong types. Do you mean 'default'? 'primitive' is used for float, double, array, Date, ScriptValue, and SerializedScriptValue, so we do need it now. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:570: # FIXME: callback_info (and isolate?) should be required On 2013/09/19 12:04:12, haraken wrote: > I think it's required. Why is this FIXME? Fixed this by making them required and changing call sites: the function declaration left them as optional arguments, but the function body assumed they existed, hence the FIXME. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:577: if for_main_world_suffix == 'ForMainWorld': On 2013/09/19 12:04:12, haraken wrote: > I guess 'if for_main_world_suffix' should come before 'if not script_wrappable'. > (I'm not sure because there is no actually generated code...) This is from the end of NativeToJSValue in Perl. We'll see this in [PerWorldBindings], right? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:584: if conversion_type in ['array', 'Date', 'ScriptValue', 'SerializedScriptValue']: On 2013/09/19 12:04:12, haraken wrote: > It looks strange why these types are handled specially. It would be cleaner if these had specific v8SetReturnValue* functions; should I add a FIXME here and put this in my to-do list? > In particular, it's strange that you're treating ScriptValue as > 'primitive' here but converting 'primitive' to 'v8::Number::New' > in CPP_VALUE_TO_V8_VALUE. Agreed that the naming is a bit confusing; I could duplicate the 'primitive' line in V8_SET_RETURN_VALUE with another entry called 'converted' or something; however, they use the same v8SetReturnValue function. (I've done so, renaming to 'number' and 'converted'.) 'primitive' is double and float, hence why it's passed to v8::Number::New it C++ -> V8. For setting return value, v8SetReturnValue is used for these numbers, and also for these converted values, hence the overlap. > Remove this branch until you need it. We do need it already, for Date attribute getters. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:600: 'DOMString_no_isolate': 'v8String({cpp_value})', On 2013/09/19 12:04:12, haraken wrote: > > Where is this used? Remove it until you need it. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:607: # General On 2013/09/19 12:04:12, haraken wrote: > Remove. Above this line are actual types; below this line are general-use conversion types. I wanted to keep these clearly separate; some different wording? https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:609: 'primitive': 'v8::Number::New({cpp_value})', On 2013/09/19 12:04:12, haraken wrote: > > I'd rename 'primitive' to 'number'. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:616: # Perl: NativeToJSValue On 2013/09/19 12:04:12, haraken wrote: > > Remove. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:618: # FIXME: support union types (in WIP) On 2013/09/19 12:04:12, haraken wrote: > > Remove. Done. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:622: conversion_type = 'DOMString_no_isolate' On 2013/09/19 12:04:12, haraken wrote: > > We don't want to have this branch. Remove it until you need it. Done. (Compared to Perl/WIP, this removed a long function to handle arguments; until we run into a DOMString w/o an isolate, we can omit.) https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:628: ################################################################################ On 2013/09/19 10:51:42, haraken wrote: > Drop SVG for now. At this point, it's not clear how we refactor SVG code. Got it. WIP has lots of SVG cruft, but we can ignore it until we reach that point, at which point it'll hopefully be somewhat cleaner!
A thought about reading the Perl CG: in principle, reviewers should not have to read the Perl CG, just the Python CG and the generated code. If you ever feel a need to refer to the Perl to understand the new CG, please feel free to push back and say "this should be clear from the Python!". (This'll also help clarify the logic!) In some cases we might have to have temporary hacks to match the Perl, or (if easy) change the Perl so we don't need the hack, but this should always be explained and clear from the Python code. This is particularly important since once the Perl is gone, there won't be anything to refer to, and it'll just be "mysterious legacy cruft", which is exactly what we want to *avoid*!
Second round of comments. Let me take another close look at the latest CL later. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... File Source/bindings/scripts/unstable/v8_types.py (right): https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:278: if ref_ptr_type(idl_type) and not used_as_parameter: > That sounds sensible, though the Perl code (in GetNativeType) reads: > return "RefPtr<${type}>" if IsRefPtrType($type) and not $isParameter; > Shall I fix the Perl code in a separate CL? In the same CL please. As mentioned before, the reason why we're doing the rewrite in the trunk is to make it easier for you to apply changes to the Perl generator and the Python generator at the same time. Repeatedly, what we want to do is not just translating Perl into Python. We do want to fix a lot of existing bugs and dead branches in the current Perl generator in the process of rewriting. That's the exact reason why we want to rewrite it _incrementally_, verifying that which code is needed or unneeded. A FIXME is OK if the fix to the Perl generator is not trivial, but please fix it otherwise. > If you ever feel a need to refer to the Perl to understand the new CG, please > feel free to push back and say "this should be clear from the Python!". The reason I have to look up the Perl generator a lot of times is not that your code is unclear but that you're just doing 1:1 translation from Perl to Python. Without understanding the details of the Perl generator, I cannot understand whether the new Python generator is well-refactored. Hence, please do try to make changes incrementally from next CLs. > For next CL, I'll implement just enough to support basic writable attributes (setters, and any necessary changes to getters etc.) Supporting writable DOM attributes sounds too big. I'd suggest implementing extended IDL attributes and all return types used in readonly DOM attributes. After completing all readonly DOM attributes, you can start implementing writable DOM attributes. > We can’t test on *full* real IDL files, because there aren’t enough simple ones > (many attributes only appear in large IDL files). > What we can do is test on specific attributes actually used in real IDL files > (there are ~2,600 attributes, in about 260 types). This makes sense. Then, let's just say in each CL that "this CL supports XXX". For example, XXX is "ScriptValue as a return type", "[CallWith] extended attribute" etc. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:385: V8_VALUE_TO_CPP_VALUE_AND_INCLUDES = { On 2013/09/20 03:43:18, Nils Barth wrote: > On 2013/09/19 12:04:12, haraken wrote: > > It looks like that defining this macro doesn't improve readability. I'd inline > > the contents in v8_value_to_cpp_value. > > I'd really prefer to keep this as a table; > it's analogous to V8_VALUE_TO_CPP_VALUE, but with includes, > and means that v8_value_to_cpp_value is a *very* simple function. > > I'm ok with inlining specific format strings (the other constants), since that's > removes a bit of indirection, but I'd like to have lookup tables instead of long > if/elif statements. I don't much like the macros because: - The macros are using {v8_value}, {isolate} etc, which are defined in v8_value_to_cpp_value. Inlining will make the connection more readable. - You're sometimes handling includes in v8_value_to_cpp and sometimes in the macros. It's inconsistent. - In general, having one macro for one string just increases indirection and makes the code less readable. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:397: V8_VALUE_TO_CPP_VALUE_ARRAY_OR_SEQUENCE = 'toNativeArray<{cpp_type}>({v8_value}, {isolate})' On 2013/09/20 03:43:18, Nils Barth wrote: > On 2013/09/19 12:04:12, haraken wrote: > > Ditto for other V8_VALUE_CPP_VALUE_XXX. > > Done (wanted to keep all format strings together, but fine to inline individual > cases). Ditto. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:487: def preprocess_type_and_value(idl_type, cpp_value, extended_attributes): On 2013/09/20 03:43:18, Nils Barth wrote: > On 2013/09/19 12:04:12, haraken wrote: > > You can split this to preprocess_idl_type and preprocess_cpp_value. > > I don't think I can clearly separate them, as the casting to double changes type > and value simultaneously, so it's clearer to have them together. > I would only be able to remove one line from this function (the idl_type change > one), and then I'd need another function just to do the idl_type conversion. > > > Then you can merge line 413-416 into preprocess_idl_type. > > Those lines are for V8 -> C++ (idl_type is input); > this function is C++ -> V8 (idl_type is output). Makes sense. Thanks for the clarification. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:511: def v8_conversion_type_and_includes(idl_type): On 2013/09/20 03:43:18, Nils Barth wrote: > On 2013/09/19 12:04:12, haraken wrote: > > v8_convertion_type => v8_type > > The 'conversion_type' is an auxiliary variable, and isn't actually a V8/JS type: > it can be IDL, C++, or just 'for_main_world' etc. > This is so the type handling can be factored out of v8_set_return_value and > cpp_value_to_v8_value (in Perl it's merged in a big mess with a separate branch > for each case, selecting v8SetReturn or not). > I've elaborated the comment to make this clearer. Makes sense. Then we might want to rename it to v8_return_value_type https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:524: if this_cpp_type in ['int', 'unsigned', 'ScriptValue']: On 2013/09/20 03:43:18, Nils Barth wrote: > On 2013/09/19 12:04:12, haraken wrote: > > This branch looks strange. All of these branches should be 'if idl_type in > ...'. > > It is a bit funny, but makes sense: for these types, conversion only depends on > the C++ type, not which IDL type (all unsigned types are treated the same), and > so we call cpp_type to not duplicate that logic here. > I've added a comment to explain. Then, should we use 'if cpp_type in ...' for all branches? Mixing 'if idl_type in ...' and 'if cpp_type in ...' are not exclusive and inconsistent. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:577: if for_main_world_suffix == 'ForMainWorld': On 2013/09/20 03:43:18, Nils Barth wrote: > On 2013/09/19 12:04:12, haraken wrote: > > I guess 'if for_main_world_suffix' should come before 'if not > script_wrappable'. > > (I'm not sure because there is no actually generated code...) > > This is from the end of NativeToJSValue in Perl. > We'll see this in [PerWorldBindings], right? Isn't it a bug of the Perl generator? Given that the for_main_world conversion doesn't require script_wrappable, it doesn't make sense to check 'if not script_wrappable' before 'if for_main_world_suffix'. Anyway, remove it for now and implement it when you support [PerWorldBindings].
On 2013/09/20 04:47:28, haraken wrote:
> Second round of comments. Let me take another close look at the latest CL
later.
Thanks; some quick replies below, and I'll revise.
> > That sounds sensible, though the Perl code (in GetNativeType) reads:
> > return "RefPtr<${type}>" if IsRefPtrType($type) and not $isParameter;
> > Shall I fix the Perl code in a separate CL?
>
> In the same CL please. As mentioned before, the reason why we're doing the
> rewrite in the trunk is to make it easier for you to apply changes to the Perl
> generator and the Python generator at the same time.
For this change (which is a one-line fix), I'd prefer to do it in a separate
Perl-only fix,
since we haven't yet implemented this in Python.
> Repeatedly, what we want to do is not just translating Perl into Python. We do
> want to fix a lot of existing bugs and dead branches in the current Perl
> generator in the process of rewriting.
Got it; we're fixing things as we run into them, which takes longer initially
(instead of cleaning up afterwards), but gives a better CG when we're done.
> A FIXME is OK if the fix to the Perl generator is not trivial, but please fix
it
> otherwise.
Understood.
In general, I think it's easier for other contributors if we fix problem in the
Perl generator in a separate CL, so it's not mixed in with new Python code, and
it's much easier to identify problems. As we rewrite, we *discover* problems
with the Perl, but we can fix these separately.
> > If you ever feel a need to refer to the Perl to understand the new CG,
please
> > feel free to push back and say "this should be clear from the Python!".
>
> The reason I have to look up the Perl generator a lot of times is not that
your
> code is unclear but that you're just doing 1:1 translation from Perl to
Python.
> Without understanding the details of the Perl generator, I cannot understand
> whether the new Python generator is well-refactored.
Ok, got it; the Perl is the reference for the logic.
> > For next CL, I'll implement just enough to support basic writable attributes
> (setters, and any necessary changes to getters etc.)
>
> Supporting writable DOM attributes sounds too big. I'd suggest implementing
> extended IDL attributes and all return types used in readonly DOM attributes.
> After completing all readonly DOM attributes, you can start implementing
> writable DOM attributes.
Ok, that should give us a number of smaller CLs before another big CL, which
should break it up
between big CLs.
> Then, let's just say in each CL that "this CL supports XXX".
> For example, XXX is "ScriptValue as a return type", "[CallWith] extended
> attribute" etc.
Exactly -- and it'll be added to the test file, so we'll know we have good test
coverage.
We've got 35-40 extended attributes for attributes, each of which should be a
quick CL.
(Quick revisions, per comments.) https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... File Source/bindings/scripts/unstable/v8_types.py (right): https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:385: V8_VALUE_TO_CPP_VALUE_AND_INCLUDES = { On 2013/09/20 04:47:29, haraken wrote: > On 2013/09/20 03:43:18, Nils Barth wrote: > > On 2013/09/19 12:04:12, haraken wrote: > > > It looks like that defining this macro doesn't improve readability. I'd > inline > > > the contents in v8_value_to_cpp_value. > > > > I'd really prefer to keep this as a table; > > it's analogous to V8_VALUE_TO_CPP_VALUE, but with includes, > > and means that v8_value_to_cpp_value is a *very* simple function. > > > > I'm ok with inlining specific format strings (the other constants), since > that's > > removes a bit of indirection, but I'd like to have lookup tables instead of > long > > if/elif statements. > > I don't much like the macros because: Wording nit: these are 'constants' (there are no macros in Python). > - The macros are using {v8_value}, {isolate} etc, which are defined in > v8_value_to_cpp_value. Inlining will make the connection more readable. Other than {arguments} (which has a special case), these are just arguments passed to the function, so I don't think it's confusing. To help with the connection, I've put them right before (instead of at the top of the file), and the tables and function all fit on one screen (the overall length would almost double if we inlined, due to needing separate cases for each type). It's also much easier to compare the format strings if they're next to each other (toInt8, toUInt8, etc.) instead of spread out through the code. > - You're sometimes handling includes in v8_value_to_cpp and sometimes in the > macros. It's inconsistent. All the literal includes are in the lookup-table; the generic includes (includes_for_type + 'V8Foo.h') are in the next case. There's 3 cases: 1. Simple conversion (no include) 2. Special cases (literal includes) 3. General case (generic includes) This would become 16 cases if we inlined them, which is less clear. > - In general, having one macro for one string just increases indirection and > makes the code less readable. Agreed: I've removed the single-use constants. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:511: def v8_conversion_type_and_includes(idl_type): On 2013/09/20 04:47:29, haraken wrote: > Makes sense. Then we might want to rename it to v8_return_value_type No problem. It's used both for explicit conversion (e.g., for parameters), and the implicit conversion of the return value, so 'return value type' is too restrictive. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:524: if this_cpp_type in ['int', 'unsigned', 'ScriptValue']: On 2013/09/20 04:47:29, haraken wrote: > Then, should we use 'if cpp_type in ...' for all branches? Mixing 'if idl_type > in ...' and 'if cpp_type in ...' are not exclusive and inconsistent. It really just depends on idl_type, which is why we're using that generally. In revised one I've rewritten this as: # For these types, the specific IDL type doesn't matter, only the # C++ type. Call cpp_type to avoid duplication. this_cpp_type = cpp_type(idl_type) if this_cpp_type in ['int', 'unsigned', 'ScriptValue']: return this_cpp_type, set() If we'd rather explicitly check idl_type (and have a bit of code duplication), this becomes a few more lines: if idl_type in CPP_INT_TYPES: return 'int', set() if idl_type in CPP_UNSIGNED_TYPES: return 'unsigned', set() if callback_function_type(idl_type): return 'ScriptValue', set() This is a bit more explicit, so I'll change it to this. https://codereview.chromium.org/24257003/diff/3001/Source/bindings/scripts/un... Source/bindings/scripts/unstable/v8_types.py:577: if for_main_world_suffix == 'ForMainWorld': On 2013/09/20 04:47:29, haraken wrote: > Isn't it a bug of the Perl generator? Might be; anyways, removing for now and we'll investigate when we get to [PerWorldBindings].
Thank you very much for the iterative updates! This CL looks much nicer. LGTM with comments. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... File Source/bindings/scripts/unstable/v8_types.py (right): https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:131: idl_type in ['any', 'DOMString']) Maybe you can put 'any' to NON_WRAPPER_TYPES, and write this like: idl_type in NON_WRAPPER_TYPES ? https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:157: CPP_INT_TYPES = set([ Nit: CPP_INT_TYPES => CPP_SIGNED_TYPES https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:210: # FIXME: Support [TreatReturnedNullStringAs] I begin to think that this FIXME isn't needed. If an idl return type is DOMString, we will always use String as a C++ return type. Whether [TreatReturnedNullStringAs] or not won't matter. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:212: # FIXME: Rename NullString to EmptyString (here and in .idl files), per spec This is not a naming problem but an implementation problem. We're intentionally using NullString instead of EmptyString, because it's faster and the difference doesn't matter for JS semantics. Let's revert back to the original FIXME. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:258: def header_files_for_interface(interface_name, cpp_class_name): header_files_for_interface => includes_for_interface ? Either way, remove this method until you need it. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:281: V8_VALUE_TO_CPP_VALUE_AND_INCLUDES = { You can merge V8_VALUE_TO_CPP_VALUE and V8_VALUE_TO_CPP_VALUE_AND_INCLUDES if you want. e.g., 'boolean': ('{v8_value}->BooleanValue()', set()), I don't have a strong opinion here though. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:300: if callback_function_type(idl_type): Shall we implement preprocess_type_and_value for the V8=>C++ conversion as well? Then you can put other conversions (e.g., enums) in the method. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:391: return 'int', set() int => signed https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:415: # FIXME: are these necessary? RefPtr.h is necessary, but I don't think GetPtr.h is necessary. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:418: return 'pointer', includes pointer => DOMWrapper https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:437: def pointer_conversion_type(): pointer_conversion_type => dom_wrapper_conversion_type https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:439: return 'default' default => DOMWrapperDefault https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:440: return 'fast' fast => DOMWrapperFast https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:443: conversion_type, includes = v8_conversion_type_and_includes(idl_type) conversion_type => v8_conversion_type https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:449: conversion_type = 'converted' The presence of 'converted' looks strange. The keys in V8_SET_RETURN_VALUE should be exactly the same as the keys in CPP_VALUE_TO_V8_VALUE (except for DOMWrapperFast and DOMWrapperDefault, which are performance hacks). I'm OK with duplicating entries in V8_SET_RETURN_VALUE like this: 'Date': 'v8SetReturnValue({callback_info}, {cpp_value});', 'ScriptValue': 'v8SetReturnValue({callback_info}, {cpp_value});', 'SerializedScriptValue': 'v8SetReturnValue({callback_info}, {cpp_value});', It would be more important that the keys in these hashes correspond to v8_conversion_types (except for performance hacks for DOMWrapper). https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:477: conversion_type, includes = v8_conversion_type_and_includes(idl_type) conversion_type => v8_conversion_type
Thanks for comments; various naming/nit replies, and 2 substantive questions (we can address in future CLs once actually implementing these, but quick answers if any come to mind). 1. Are RefPtr types all wrapper types? (In Perl CG union types are an exception: is this a bug?) 2. NullString vs. EmptyString in TreatNullAs Will commit revised CL and then post next (much shorter) one. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... File Source/bindings/scripts/unstable/v8_types.py (right): https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:131: idl_type in ['any', 'DOMString']) On 2013/09/20 15:21:35, haraken wrote: > > Maybe you can put 'any' to NON_WRAPPER_TYPES, and write this like: > > idl_type in NON_WRAPPER_TYPES > > ? This is ref_ptr_type, not wrapper_type: 'any'(/'Promise') and 'DOMString' are the only exceptions in ref_ptr_type, but there are many more in wrapper_type. We could have a NON_REF_PTR_TYPE with just 'any' and 'DOMString' (and 'Promise', when we implement that), but it seems easier to inline. (Anyway I'm doing non-wrapper types in the next CL.) I just noticed something, maybe a bug in the Perl CG: Union types are not RefPtr types, but are wrapper types. This is the only such case: otherwise wrapper types are also RefPtr types. Do you know if this is correct, or a bug? (Maybe it's obvious, otherwise we can handle when we implement union types.) Reference: IsRefPtrType https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... IsWrapperType https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... %nonWrapperTypes https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... v8_types (attributes WIP): https://codereview.chromium.org/24028003/diff/20001/Source/bindings/scripts/u... https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:157: CPP_INT_TYPES = set([ On 2013/09/20 15:21:35, haraken wrote: > Nit: CPP_INT_TYPES => CPP_SIGNED_TYPES That's nicely symmetric, but we use 'int' in the generated code (not 'signed' or 'signed int'), so it seems more consistent to use INT here; note the below: if idl_type in CPP_INT_TYPES: return 'int' if idl_type in CPP_UNSIGNED_TYPES: return 'unsigned' https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:210: # FIXME: Support [TreatReturnedNullStringAs] On 2013/09/20 15:21:35, haraken wrote: > I begin to think that this FIXME isn't needed. If an idl return type is > DOMString, we will always use String as a C++ return type. Whether > [TreatReturnedNullStringAs] or not won't matter. Got it, removed (one other place too). (This is handled by cpp_value_to_v8_value; in Perl NativeToJSValue.) https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:212: # FIXME: Rename NullString to EmptyString (here and in .idl files), per spec On 2013/09/20 15:21:35, haraken wrote: > > This is not a naming problem but an implementation problem. We're intentionally > using NullString instead of EmptyString, because it's faster and the difference > doesn't matter for JS semantics. Let's revert back to the original FIXME. (Removing this function for now; will add and address these when review CL implementing these extended attributes.) I'm confused; do we want to fix something (if so, what?), or are we just documenting non-spec behavior that we don't plan to change? (We currently don't implement spec, which requires EmptyString and Null, and instead implement our own NullString.) For reference: For TreatUndefinedAs, the spec allows both EmptyString (string with value "") and Null (value null) (and Missing for operations), and there is no such thing as NullString (null is a value, the only value of the Null type). For TreatNullAs, the spec allows only EmptyString (required identifier). http://www.w3.org/TR/WebIDL/#TreatNullAs http://www.w3.org/TR/WebIDL/#TreatUndefinedAs https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:258: def header_files_for_interface(interface_name, cpp_class_name): On 2013/09/20 15:21:35, haraken wrote: > header_files_for_interface => includes_for_interface ? > > Either way, remove this method until you need it. Good point both ways; done. (Removed, will rename when actually implement.) https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:281: V8_VALUE_TO_CPP_VALUE_AND_INCLUDES = { On 2013/09/20 15:21:35, haraken wrote: > You can merge V8_VALUE_TO_CPP_VALUE and V8_VALUE_TO_CPP_VALUE_AND_INCLUDES if > you want. > > e.g., 'boolean': ('{v8_value}->BooleanValue()', set()), > > I don't have a strong opinion here though. I actually did that at first! It reduces us to 2 cases (in table or default) instead of 3 (no includes, includes, default), but the extra (,) and set() made it a lot less readable, so I thought it clearer to separate them. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:300: if callback_function_type(idl_type): On 2013/09/20 15:21:35, haraken wrote: > Shall we implement preprocess_type_and_value for the V8=>C++ conversion as well? > Then you can put other conversions (e.g., enums) in the method. I thought about it, but it's just two cases (this and enum), and only used in this one place, so I thought it clearer to inline it. (The C++=>V8 conversion is more complex and used 2 places, which is why I made it a function.) https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:391: return 'int', set() On 2013/09/20 15:21:35, haraken wrote: > int => signed See above (we use 'int' internally, like v8SetReturnValueInt) https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:415: # FIXME: are these necessary? On 2013/09/20 15:21:35, haraken wrote: > RefPtr.h is necessary, but I don't think GetPtr.h is necessary. Got it, thanks! Reworded and moved FIXME to GetPtr only. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:418: return 'pointer', includes On 2013/09/20 15:21:35, haraken wrote: > pointer => DOMWrapper Done. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:437: def pointer_conversion_type(): On 2013/09/20 15:21:35, haraken wrote: > > pointer_conversion_type => dom_wrapper_conversion_type Done. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:439: return 'default' On 2013/09/20 15:21:35, haraken wrote: > > default => DOMWrapperDefault Done. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:440: return 'fast' On 2013/09/20 15:21:35, haraken wrote: > > fast => DOMWrapperFast Done. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:443: conversion_type, includes = v8_conversion_type_and_includes(idl_type) On 2013/09/20 15:21:35, haraken wrote: > > conversion_type => v8_conversion_type Done. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:449: conversion_type = 'converted' On 2013/09/20 15:21:35, haraken wrote: > > The presence of 'converted' looks strange. > > The keys in V8_SET_RETURN_VALUE should be exactly the same as the keys in > CPP_VALUE_TO_V8_VALUE (except for DOMWrapperFast and DOMWrapperDefault, which > are performance hacks). I'm OK with duplicating entries in V8_SET_RETURN_VALUE > like this: > > 'Date': 'v8SetReturnValue({callback_info}, {cpp_value});', > 'ScriptValue': 'v8SetReturnValue({callback_info}, {cpp_value});', > 'SerializedScriptValue': 'v8SetReturnValue({callback_info}, {cpp_value});', > > It would be more important that the keys in these hashes correspond to > v8_conversion_types (except for performance hacks for DOMWrapper). Got it; that makes the code clearer (less indirection) at a minor cost of duplication. Using the IDL types as keys as much as possible helps make it clearer. I've also replace 'number' (for 'float'/'double') with just 'float' and 'double' directly, for the same reason. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:477: conversion_type, includes = v8_conversion_type_and_includes(idl_type) On 2013/09/20 15:21:35, haraken wrote: > > conversion_type => v8_conversion_type Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nbarth@chromium.org/24257003/28001
Message was sent while issue was closed.
Change committed as 158239
Message was sent while issue was closed.
https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... File Source/bindings/scripts/unstable/v8_types.py (right): https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:131: idl_type in ['any', 'DOMString']) > This is ref_ptr_type, not wrapper_type: > 'any'(/'Promise') and 'DOMString' are the only exceptions in ref_ptr_type, but > there are many more in wrapper_type. We could have a NON_REF_PTR_TYPE with just > 'any' and 'DOMString' (and 'Promise', when we implement that), but it seems > easier to inline. > > (Anyway I'm doing non-wrapper types in the next CL.) > > I just noticed something, maybe a bug in the Perl CG: > Union types are not RefPtr types, but are wrapper types. > This is the only such case: otherwise wrapper types are also RefPtr types. > Do you know if this is correct, or a bug? > (Maybe it's obvious, otherwise we can handle when we implement union types.) Actually I cannot answer without investigation. However, basically, I think there are three types: - Primitive types speced in Web IDL (int, any, DOMString, ...) - Wrapper types (Node, XMLHttpRequest, ...) - Special casing (SerializedScriptValue, Promise, EventListener, ...) In this categorization, I think IsRefPtr = IsWrapperType + some cases in the special casing. https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:212: # FIXME: Rename NullString to EmptyString (here and in .idl files), per spec > I'm confused; do we want to fix something (if so, what?), or are we just > documenting non-spec behavior that we don't plan to change? > > (We currently don't implement spec, which requires EmptyString and Null, and > instead implement our own NullString.) Sorry for not being clear. This shouldn't be a FIXME but should be a comment. We're using NullString on purpose for performance.
Message was sent while issue was closed.
Thanks for clarifications; will bear in mind for future CLs! https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... File Source/bindings/scripts/unstable/v8_types.py (right): https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:131: idl_type in ['any', 'DOMString']) On 2013/09/24 05:36:57, haraken wrote: > However, basically, I think there are three types: > > - Primitive types speced in Web IDL (int, any, DOMString, ...) "Primitive type" is a technical term, and only includes certain Web IDL types: http://www.w3.org/TR/WebIDL/#dfn-primitive-type "The following types are known as primitive types: boolean, the integer types, float, unresticted float, double and unrestricted double." Note that 'int' is not a Web IDL type (you need to specify 'short' or 'long'), and 'any' and 'DOMString' are Web IDL types but are not primitive. > - Wrapper types (Node, XMLHttpRequest, ...) > - Special casing (SerializedScriptValue, Promise, EventListener, ...) > > In this categorization, I think IsRefPtr = IsWrapperType + some cases in the > special casing. Thanks for clarifying! In Web IDL spec terms, what we implement as "wrapper types" sound like general "interface types" (i.e., type corresponding to an interface, not something built-in like DOMString). https://codereview.chromium.org/24257003/diff/18001/Source/bindings/scripts/u... Source/bindings/scripts/unstable/v8_types.py:212: # FIXME: Rename NullString to EmptyString (here and in .idl files), per spec On 2013/09/24 05:36:57, haraken wrote: > > I'm confused; do we want to fix something (if so, what?), or are we just > > documenting non-spec behavior that we don't plan to change? > > > > (We currently don't implement spec, which requires EmptyString and Null, and > > instead implement our own NullString.) > > Sorry for not being clear. This shouldn't be a FIXME but should be a comment. > We're using NullString on purpose for performance. Got it, thanks! I've documented at: https://sites.google.com/a/chromium.org/dev/blink/webidl/blink-idl-extended-a... ...and will add a comment in the code when we implement. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
