Bug 131596

<rdar://problem/16603443>

Created attachment 229243 [details] WIP - can parse and typecheck 6.0 and 7.0 combined protocols

Created attachment 229624 [details] WIP: generates InspectorWebBackendCommands.js and InspectorWebBackendDispatchers.h

Can you post example output files?

Created attachment 229774 [details] WIP: generates InspectorWebBackendCommands.js and InspectorWebBackendDispatchers.h,cpp

Newest version (3rd WIP) has the following changes from the 2nd WIP: * tests that don't fail will produce one big concatenated file, to reduce the number of results files needed. * generator knowledge of how to represent types is no longer in the Type subclasses, it's provided by static methods that do instanceof checks. * generates the backend dispatcher implementations. * added some important tests for async commands, domain sizes/include guards, and parameter passing modes for all the various configurations (in/out, optional, object/array/primitive). I think the architecture is pretty well defined now, so you could start pre-reviewing the models.py and the existing generators.

Created attachment 229776 [details] current generated output for actual InspectorJS/Web.json files

Created attachment 229782 [details] WIP 4: generates all except type builders

Created attachment 232019 [details] patch, with windows fixes

Created attachment 232025 [details] changes to turn CMake/GCC happy

Created attachment 232043 [details] PFR v1

Attachment 232043 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:63: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:115: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:66: [FrontendDispatcherImplementationGenerator.generate_output] Instance of 'FrontendDispatcherImplementationGenerator' has no 'domains_to_generate' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:251: whitespace before ')' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:66: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:124: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:143: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:160: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:57: [TypeBuilderImplementationGenerator.generate_output] Instance of 'TypeBuilderImplementationGenerator' has no 'calculate_types_requiring_shape_assertions' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:87: [TypeBuilderImplementationGenerator._generate_enum_mapping] Instance of 'TypeBuilderImplementationGenerator' has no 'assigned_enum_values' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:98: [TypeBuilderImplementationGenerator._generate_open_field_names] Instance of 'TypeBuilderImplementationGenerator' has no 'domains_to_generate' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:108: [TypeBuilderImplementationGenerator._generate_builders_for_domain.<lambda>] Instance of 'TypeBuilderImplementationGenerator' has no 'type_needs_shape_assertions' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:75: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:91: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:102: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:111: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:71: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:100: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:108: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:144: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:184: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:74: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'domains_to_generate' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:115: [BackendDispatcherHeaderGenerator._generate_anonymous_enum_for_parameter] Instance of 'BackendDispatcherHeaderGenerator' has no 'encoding_for_enum_value' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:66: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:82: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:117: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:127: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:133: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:168: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:189: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:202: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:216: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:242: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:69: [BackendDispatcherImplementationGenerator.generate_output] Instance of 'BackendDispatcherImplementationGenerator' has no 'domains_to_generate' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:70: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:86: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:112: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:73: [FrontendDispatcherHeaderGenerator.generate_output] Instance of 'FrontendDispatcherHeaderGenerator' has no 'domains_to_generate' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:93: [FrontendDispatcherHeaderGenerator._generate_anonymous_enum_for_parameter] Instance of 'FrontendDispatcherHeaderGenerator' has no 'encoding_for_enum_value' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:42: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:72: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:83: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:212: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:61: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:150: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:150: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:63: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:78: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:287: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:310: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:175: whitespace before '}' [pep8/E202] [5] Total errors found: 51 in 81 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 232043 [details] PFR v1 View in context: https://bugs.webkit.org/attachment.cgi?id=232043&action=review This is looking really really great! I reviewed the build files, models, generator, and runner scripts. I have yet to look at the individual generators and tests, so still more review work. > Source/JavaScriptCore/CMakeLists.txt:882 > + DEPENDS ${JavaScriptCore_INSPECTOR_PROTOCOL_SCIRPTS} Typo: "SCIRPTS" => "SCRIPTS" > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:5251 > + name = codegen; Nit: ideally this would be "path = codegen" instead of name. So when adding a file to this directory, Xcode will automatically open a file picker for this directory instead of the root. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:5254 > + C4703CC1192844A40013FBEA /* codegen */ = { Nit: This section seems duplicated It has the same UUID, so it seems accidental? > Source/JavaScriptCore/inspector/InspectorTypeBuilder.h:198 > + static void assertCorrectValue(InspectorValue*); I never liked assertCorrectValue. Really this is just checking if the value is the appropriate type. How about "assertTypeOfValue" or "assertValueHasExpectedTypes"? > Source/JavaScriptCore/inspector/InspectorTypeBuilder.h:203 > +template<typename T, InspectorValue::Type TYPE> > +struct PrimitiveBindingTraits { Looks like template type "T" is unused. You should be able to remove it. > Source/JavaScriptCore/inspector/InspectorTypeBuilder.h:247 > +template<> struct BindingTraits<InspectorArray> : public PrimitiveBindingTraits<InspectorArray, InspectorValue::Type::Array> { }; Which simplifies these: - template<> struct BindingTraits<InspectorArray> : public PrimitiveBindingTraits<InspectorArray, InspectorValue::Type::Array> { }; + template<> struct BindingTraits<InspectorArray> : public PrimitiveBindingTraits<InspectorValue::Type::Array> { }; > Source/JavaScriptCore/inspector/InspectorValues.h:123 > + , m_doubleValue((double)value) { } Nit: static_cast<double>(value) > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:39 > +# Use a global logger, which normally only logs errors. > +# It can be configured to log debug messages from the CLI. > +logging.basicConfig(format='%(levelname)s: %(message)s', level=logging.ERROR) > +log = logging.getLogger('global') This is at the top of each file with mixed comments. I think you can standardize with no comment. > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:48 > + def __init__(self, model, input_filepath): > + self._model = model > + self._input_filepath = input_filepath > + > + def model(self): > + return self._model This is duplicated across all the Generators. Ideally they would call super and the base class would handle it. super(Generator, self).__init__(mode, input_filepath) > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:90 > + def __init__(self, model, input_filepath): > + self._model = model > + self._input_filepath = input_filepath Would be good for subclasses to use this! > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:132 > + def all_types_needing_shape_assertions(self): > + return self._types_needing_shape_assertions Nit: Unused, can be removed. Or, if you want to keep it, this should calculate_types_needing_shape_assertions like the single checker. > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:207 > + guard = _DOMAIN_TO_CPP_GUARD_MAP.get(domain.domain_name, None) Nit: None is the default missing value. > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:222 > + # Enum values could be integer literals, so don't try to convert those. > + if not isinstance(enum_value, (unicode, str)): > + return Is that true? I see no existing protocol enum's that are not "type":"string". Maybe we should make that a requirement, unless you have something in mind for non-string enums. > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:231 > + regex = '(\w*)(%s)(\w*)' % "|".join(_ALWAYS_UPPERCASED_ENUM_VALUE_SUBSTRINGS) > + > + def replaceCallback(match): > + return "".join([match.group(1), match.group(2).upper(), match.group(3)]) > + > + # Split on hyphen, introduce camelcase, and force uppercasing of acronyms. > + subwords = map(lambda x: x[0].upper() + x[1:], enum_value.split('-')) > + return re.sub(regex, replaceCallback, "".join(subwords), flags=re.IGNORECASE) The regex is too greedy and unnecessary complex. It will cause re.sub to only substitute the first occurrence of a found string. Also, you have a nice ucfirst function you can reuse. You should be able to simplify to: regex = '(%s)' % "|".join(_ALWAYS_UPPERCASED_ENUM_VALUE_SUBSTRINGS) def replaceCallback(match): return match.group(1).upper() # Split on hyphen, introduce camelcase, and force uppercasing of acronyms. subwords = map(ucfirst, name.split('-')) return re.sub(regex, replaceCallback, "".join(subwords), flags=re.IGNORECASE) Now this will convert an enum value like "html-to-xml" to "HTMLToXML" instead of "htmlToXML". > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:275 > + if isinstance(type, ObjectType) and len(type.members) == 0: > + return 'Inspector::InspectorObject' > + if isinstance(type, (ObjectType, AliasedType, EnumType)): > + return 'Inspector::TypeBuilder::%s::%s' % (type.type_domain().domain_name, type.raw_name()) > + elif isinstance(type, ArrayType): > + return 'Inspector::TypeBuilder::Array<%s>' % Generator.type_builder_string_for_type(type.element_type) > + elif isinstance(type, PrimitiveType): > + return Generator.cpp_name_for_primitive_type(type) I tried not to nit too much about if/else return style. But here you have both an if return and elif return chains. Personally, I prefer if return chains. > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:288 > + if isinstance(_type, AliasedType): > + _type = _type.aliased_type # Fall through to primitive. This comment should be "Fall through to enum or primitive." An aliased_type can be an enum if it is a $ref to a type that is a string enum. > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:295 > + if isinstance(_type, ObjectType) or _type.qualified_name() is 'object': > + return 'const RefPtr<Inspector::InspectorObject>' + sigil When would the "_type.qualified_name() is 'object'" be true when the first is not? > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:307 > + return "unknown_unchecked_formal_in_parameter_type" Maybe raise an exception here? > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:311 > + @staticmethod > + def type_string_for_checked_formal_in_parameter(parameter): > + return Generator.type_string_for_type_with_name(parameter.type, parameter.parameter_name, parameter.is_optional) I'm confused here. The name says "formal_in_parameter", but it looks like it is used by generation for Event parameters which are basically like out params. A better name would help. Also, how does this generate a $ref event parameter, like in Debugger.didSampleProbe? { "name": "didSampleProbe", "description": "Fires when a new probe sample is collected.", "parameters": [ { "name": "sample", "$ref": "ProbeSample", "description": "A collected probe sample." } ] }, We currently generate a FrontendDispatcer method like: void didSampleProbe(PassRefPtr<Inspector::TypeBuilder::Debugger::ProbeSample> sample); It looks to me like in type_string_for_type_with_name an ObjectType would do the right thing but an AliasedType does the wrong thing. How does this work? > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:330 > + elif isinstance(_type, AliasedType): > + builder_type = Generator.type_builder_string_for_type(_type) > + if is_optional: > + return 'const %s* const' % builder_type > + elif _type.aliased_type.qualified_name() in ['integer', 'number']: > + return Generator.cpp_name_for_primitive_type(_type.aliased_type) > + elif _type.aliased_type.qualified_name() in ['string']: > + return 'const %s&' % builder_type > + else: > + return builder_type This is the block I don't understand. > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:477 > + elif isinstance(type, AliasedType): > + return Generator.js_name_for_parameter_type(type.aliased_type) > + elif isinstance(type, EnumType): > + return Generator.js_name_for_parameter_type(type.primitive_type) This could do the fall through stuff if you wanted to continue to trend of avoiding unnecessary recursion. > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:486 > + # Decide whether certain helpers are necesary in a situation. Typo: "necesary" => "necessary" > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:497 > +class DummyGenerator(Generator): > + pass Not used anywhere. > Source/JavaScriptCore/inspector/scripts/codegen/models.py:59 > + 'RGBA': 'Rgba', The old generator said: "RGBA": "Rgba", # RGBA is reported to be conflicting with a define name in Windows CE. That might be good a good comment to keep or incorporate into your comment (the mention of Windows CE). I think we can easily rename to "RGBAColor" and then remove this whole thing! > Source/JavaScriptCore/inspector/scripts/codegen/models.py:123 > + if type_kind is not None and referenced_type_name is not None: > + raise ParseException("Type reference cannot have both 'type' and '$ref' keys.") > + > + if type_kind == "array" and array_items is None: > + raise ParseException("Type reference with type 'array' must have key 'items' to define array element type.") We can add a check that enums have at least 1 value. (See later comments regarding making enum_values None instead of empty array) if enum_values is not None and len(enum_values) == 0: raise ParseException("Type reference with enum values must have at least one enum value.") And perhaps this should have and is_enum or has_enum_values to make checks down below easier: def has_enum_values(self): return self._enum_values is not None > Source/JavaScriptCore/inspector/scripts/codegen/models.py:150 > + def raw_name(self): > + return _TYPES_NEEDING_RENAME_WORKAROUNDS.get(self._name, self._name) This is not overridden by subclasses, lets move it up above the comment about subclasses. > Source/JavaScriptCore/inspector/scripts/codegen/models.py:183 > + return 'AliasedType[%s -> %s]' % (self.qualified_name(), self.aliased_type.__repr__()) [Applies to multiple places] Apparently python string interpolation has %r to automatically do __repr__() instead of __str__. That would read better. > Source/JavaScriptCore/inspector/scripts/codegen/models.py:292 > + self._enum_value_count = 0 > + self._enum_values = {} Nit: Unused / moved! Can be removed. > Source/JavaScriptCore/inspector/scripts/codegen/models.py:315 > + raise ParseException("Malformed specification: types is not an array") Nit: "Malformed specification" => "Malformed domain specification" > Source/JavaScriptCore/inspector/scripts/codegen/models.py:338 > + raise ParseException("Malformed specification: properties is not an array") Nit: "Malformed specification" => "Malformed type specification" Etc for the other Malformed specification parse exceptions. > Source/JavaScriptCore/inspector/scripts/codegen/models.py:341 > + type_ref = TypeReference(json['type'], None, json.get('enum', []), json.get('items')) [Applies to multiple places] Can we default enum_values to None instead of an empty array, that seems cleaner. All the places that check "len(... .enum_values) > 0" can check if enum_values is not None or the earlier "is_enum" proposal. > Source/JavaScriptCore/inspector/scripts/codegen/models.py:383 > + def parse_parameter(self, json): This is used for parsing both "params" and "returns" of a command/event. How about naming this "parse_parameter_or_return_type_declaration". > Source/JavaScriptCore/inspector/scripts/codegen/models.py:395 > + for _primitive_type in ['boolean', 'integer', 'number']: > + self.types_by_name[_primitive_type] = PrimitiveType(_primitive_type) For my own education, why do you sometimes use _ prefixed variable names and other times regular ones. Is that a convention for loop variables? > Source/JavaScriptCore/inspector/scripts/codegen/models.py:424 > + elif kind == "array": > + type_instance = ArrayType(_declaration.type_ref.array_type_ref, _domain) > + elif kind == "object": > + type_instance = ObjectType(_declaration.type_name, _domain, _declaration.type_members) > + else: > + type_instance = AliasedType(_declaration.type_name, _domain, _declaration.type_ref) Object reads weird compared to the others that all explicitly have "type_ref". Maybe "type_members" should be named "member_type_refs". > Source/JavaScriptCore/inspector/scripts/codegen/models.py:506 > +class TypeDeclaration: > + def __init__(self, type_name, type_ref, description, type_members): Nit: I'm getting all excited about the warnings you are adding. Maybe we should add a warning here that the type name should start with a capital letter. This ensure someone doesn't add a new type that conflicts with a built-in type. > Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:88 > + try: > + read_file = open(self._filepath, "r") > + old_text = read_file.read() > + read_file.close() > + text_changed = old_text != self._output > + except: > + # Ignore, just overwrite by default > + pass We can avoid reading files entirely if self.force_output. > Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:107 > + protocol.parse_specification(parsed_json, isSupplemental=isSupplemental) Nit: "isSupplemental=isSupplemental" => "isSupplemental"? > Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:145 > + if concatenate_output: > + filename = os.path.join(os.path.basename(primary_specification_filepath) + '-result') > + output_file = IncrementalFileWriter(os.path.join(output_dirpath, filename), force_output) > + output_file.write('\n'.join(single_output_file_contents)) > + output_file.close() Style: Indented too far.

Comment on attachment 232043 [details] PFR v1 View in context: https://bugs.webkit.org/attachment.cgi?id=232043&action=review >> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:222 >> + return > > Is that true? I see no existing protocol enum's that are not "type":"string". Maybe we should make that a requirement, unless you have something in mind for non-string enums. I thought it was used somewhere. Will remove... >> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:231 >> + return re.sub(regex, replaceCallback, "".join(subwords), flags=re.IGNORECASE) > > The regex is too greedy and unnecessary complex. It will cause re.sub to only substitute the first occurrence of a found string. Also, you have a nice ucfirst function you can reuse. > > You should be able to simplify to: > > regex = '(%s)' % "|".join(_ALWAYS_UPPERCASED_ENUM_VALUE_SUBSTRINGS) > > def replaceCallback(match): > return match.group(1).upper() > > # Split on hyphen, introduce camelcase, and force uppercasing of acronyms. > subwords = map(ucfirst, name.split('-')) > return re.sub(regex, replaceCallback, "".join(subwords), flags=re.IGNORECASE) > > Now this will convert an enum value like "html-to-xml" to "HTMLToXML" instead of "htmlToXML". Good catch. >> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:295 >> + return 'const RefPtr<Inspector::InspectorObject>' + sigil > > When would the "_type.qualified_name() is 'object'" be true when the first is not? The 'any' type. I added a comment. >> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:311 >> + return Generator.type_string_for_type_with_name(parameter.type, parameter.parameter_name, parameter.is_optional) > > I'm confused here. The name says "formal_in_parameter", but it looks like it is used by generation for Event parameters which are basically like out params. A better name would help. > > Also, how does this generate a $ref event parameter, like in Debugger.didSampleProbe? > > { > "name": "didSampleProbe", > "description": "Fires when a new probe sample is collected.", > "parameters": [ > { "name": "sample", "$ref": "ProbeSample", "description": "A collected probe sample." } > ] > }, > > We currently generate a FrontendDispatcer method like: > > void didSampleProbe(PassRefPtr<Inspector::TypeBuilder::Debugger::ProbeSample> sample); > > It looks to me like in type_string_for_type_with_name an ObjectType would do the right thing but an AliasedType does the wrong thing. How does this work? It falls through to the last case for AliasedType, returning the type from type_builder_string_for_type, which accounts for aliased values. >> Source/JavaScriptCore/inspector/scripts/codegen/models.py:59 >> + 'RGBA': 'Rgba', > > The old generator said: > > "RGBA": "Rgba", # RGBA is reported to be conflicting with a define name in Windows CE. > > That might be good a good comment to keep or incorporate into your comment (the mention of Windows CE). I think we can easily rename to "RGBAColor" and then remove this whole thing! OK, we can refactor later. >> Source/JavaScriptCore/inspector/scripts/codegen/models.py:395 >> + self.types_by_name[_primitive_type] = PrimitiveType(_primitive_type) > > For my own education, why do you sometimes use _ prefixed variable names and other times regular ones. Is that a convention for loop variables? Usually it is to distinguish local variables. Sometimes it is to avoid builtin keywords (especially 'type').

Comment on attachment 232043 [details] PFR v1 View in context: https://bugs.webkit.org/attachment.cgi?id=232043&action=review >> Source/JavaScriptCore/inspector/scripts/codegen/models.py:123 >> + raise ParseException("Type reference with type 'array' must have key 'items' to define array element type.") > > We can add a check that enums have at least 1 value. (See later comments regarding making enum_values None instead of empty array) > > if enum_values is not None and len(enum_values) == 0: > raise ParseException("Type reference with enum values must have at least one enum value.") > > And perhaps this should have and is_enum or has_enum_values to make checks down below easier: > > def has_enum_values(self): > return self._enum_values is not None OK >> Source/JavaScriptCore/inspector/scripts/codegen/models.py:424 >> + type_instance = AliasedType(_declaration.type_name, _domain, _declaration.type_ref) > > Object reads weird compared to the others that all explicitly have "type_ref". Maybe "type_members" should be named "member_type_refs". type_members returns an array of TypeMembers. TypeReferences do not have an 'optional' flag, nor room for a description. That's why TypeMember exists.

Created attachment 236385 [details] PFR v2 The new patch addresses all comments to date, and removes stale test results.

Attachment 236385 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:61: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:90: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:98: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:134: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:174: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:44: [BackendDispatcherHeaderGenerator.output_filename] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:48: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:56: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'generate_license' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:61: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no '_input_filepath' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:64: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'domains_to_generate' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:77: [BackendDispatcherHeaderGenerator._generate_handler_declarations_for_domain] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:105: [BackendDispatcherHeaderGenerator._generate_anonymous_enum_for_parameter] Instance of 'BackendDispatcherHeaderGenerator' has no 'encoding_for_enum_value' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:162: [BackendDispatcherHeaderGenerator._generate_dispatcher_declarations_for_domain] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:45: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:75: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:86: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:215: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:57: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:115: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:134: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:151: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:63: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:61: at least two spaces before inline comment [pep8/E261] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:148: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:148: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:54: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:106: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:65: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:81: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:92: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:101: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:58: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:74: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:109: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:119: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:125: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:160: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:181: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:194: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:208: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:234: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:177: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:124: [generate_from_specification] Undefined variable 'FrontendDispatcherHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:126: [generate_from_specification] Undefined variable 'TypeBuilderHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:251: whitespace before ')' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:53: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:68: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:277: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:300: whitespace before '}' [pep8/E202] [5] ERROR: Source/WebCore/inspector/InspectorReplayAgent.h:107: The parameter name "segmentState" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:60: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:76: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:102: whitespace before '}' [pep8/E202] [5] Total errors found: 53 in 89 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 236385 [details] PFR v2 View in context: https://bugs.webkit.org/attachment.cgi?id=236385&action=review Made it through the generators today. A little hairier, but still great. I'll look at the tests next and that should mean I've looked at the entire patch. > Source/JavaScriptCore/ChangeLog:362 > +2014-08-11 Brian J. Burg <burg@cs.washington.edu> > + > + Web Inspector: use type builders to construct high fidelity type information payloads > + https://bugs.webkit.org/show_bug.cgi?id=135803 Oops. > Source/JavaScriptCore/CMakeLists.txt:942 > DEPENDS ${JavaScriptCore_INSPECTOR_SCRIPTS_DIR}/CodeGeneratorInspector.py > ${JavaScriptCore_INSPECTOR_SCRIPTS_DIR}/CodeGeneratorInspectorStrings.py You will need to update the DEPENDS for the new generators. > Source/JavaScriptCore/inspector/scripts/CodeGeneratorInspector.py:-1 > -#!/usr/bin/env python You should also delete the CodeGeneratorInspectorStrings.py, which surprisingly is not deleted! > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:33 > +from generator import Generator, ucfirst > +from string import Template > +from generator_templates import GeneratorTemplates as Templates > +from models import EnumType Should these be sorted somehow? The order differs between files. > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:47 > + def generate_output(self): > + domains_to_generate = [] Seems like this should override domains_to_generate from the superclass with the implementation below. > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:51 > + enum_types = filter(lambda declaration: isinstance(declaration.type, EnumType), _domain.type_declarations) > + if _domain.is_supplemental: > + continue Nit: enum_type line can go after this continue. > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:56 > + sections = [] > + sections.append(self.generate_license()) [Applies to multiple places] I think things would look nicer if these lines were put down with the rest of the sections modifications. Then you have all params initialized, and then all sections generated together in a tight block. > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:58 > + 'headerGuardString': re.sub('[-./]', '_', self.output_filename()), [Applies to multiple places] Maybe you will need \ too for Windows? I suppose this could be made very generic: re.sub('\W+', '_', self.output_filename()) > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:65 > + domains = self.domains_to_generate() > + domains = filter(lambda domain: len(domain.commands) > 0, domains) [Applies to multiple places] This could override domains_to_generate. Fine as is though. > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:95 > + def _generate_anonymous_enum_for_parameter(self, parameter, command): This is done in multiple places exactly identical. Maybe it could be shared somewhere. > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:36 > +# Use a global logger, which normally only logs errors. You can remove this comment. > Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:314 > + if type_member.member_name in ['value', 'array', 'string', 'number', 'integer']: > + lines.append(' using Inspector::InspectorObjectBase::set%s;' % ucfirst(type_member.member_name)) > + lines.append('') This does not sound right. We should not be looking at the name of the type_member, but the type of the type_member. It seems like this might be working by accident (as was the case in the old generator too). For example, given an existing type with optional fields: { "id": "KeyPath", "type": "object", "description": "Key path.", "properties": [ { "name": "type", "type": "string", "enum": ["null", "string", "array"], "description": "Key path type." }, { "name": "string", "type": "string", "optional": true, "description": "String value." }, { "name": "array", "type": "array", "optional": true, "items": { "type": "string" }, "description": "Array value." } ] } We generate this for the optional fields: /* * Synthetic constructor: * RefPtr<KeyPath> result = KeyPath::create() * .setType(...); */ static Builder<NoFieldsSet> create() { return Builder<NoFieldsSet>(Inspector::InspectorObject::create()); } typedef Inspector::TypeBuilder::StructItemTraits ItemTraits; void setString(const String& value) { this->setString(ASCIILiteral("string"), value); } using Inspector::InspectorObjectBase::setString; void setArray(PassRefPtr<Inspector::TypeBuilder::Array<String> > value) { this->setValue(ASCIILiteral("array"), value); } using Inspector::InspectorObjectBase::setArray; The "using" lines here makes sense by accident because the field was named "array". I have a feeling that all of these using lines can probably just be removed! The methods are all protected in the base class. If they are required, we should do it correctly (and match the implementation inside the setter). --- Having said that, the fact that setArray takes an Array and calls setValue also sounds wrong, but is fine because setArray/setValue/setObject are identical. We should still fix that. See comment later on. > Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:142 > + lines.append(""" { > + InspectorObject::iterator %(memberName)sPos; > + %(memberName)sPos = object->find("%(memberName)s"); > + ASSERT(%(memberName)sPos != object->end()); > + %(assertMethod)s(%(memberName)sPos->value.get()); > + }""" % args) [Applies to multiple places in the patch] I always wondered if part of what slows us down in debug builds is this find(...) for every single member. I wonder if there is a better way to check the properties. We can combine the type declaration and assignment, and we should use ASCIILiteral: InspectorObject::iterator %(memberName)sPos = object->find(ASCIILiteral("%(memberName)s")); > Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:177 > + lines.append(' WTF::String s;') I don't think the "WTF::" is needed here, we can drop it. > Source/JavaScriptCore/inspector/scripts/codegen/generator.py:251 > + if isinstance(type, ObjectType) or isinstance(type, ArrayType): > + return 'setValue' I think ObjectType can use setObject and ArrayType can use setArray. These 3 methods are all implemented identically right now in InspectorObjectBase. I don't think we ever have to actually use setValue ourselves. > Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:33 > + * Copyright (C) 2014 Apple Inc. All rights reserved. And: Copyright (c) 2014 University of Washington. All rights reserved. Also maybe some of the copyrights from the Strings generator file which we are replacing. > Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:58 > +// by the script: JavaScriptCore/replay/scripts/CodeGeneratorInspector.py""") Wrong script! Should be: Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py > Source/JavaScriptCore/inspector/scripts/codegen/models.py:540 > + self.is_optional = is_optional Judging by the bug you fixed, maybe we should type check things like "is_optional" (boolean) when we parse to catch errors even earlier! Great find.

Comment on attachment 236385 [details] PFR v2 View in context: https://bugs.webkit.org/attachment.cgi?id=236385&action=review r=me! I've now looked over everything. This is great! I wonder if we should and how we can have the run-input-generator-tests and run-inspector-gnerator-tests added to build bot that runs all tests. > Source/JavaScriptCore/inspector/scripts/tests/domains-with-varying-command-sizes.json:5 > + "domain": "Network1", > + > + "commands": [ Nit: Extra blank line can be removed. > Source/JavaScriptCore/inspector/scripts/tests/expected/domains-with-varying-command-sizes.json-result:374 > +#ifndef InspectorTestFrontendDispatchers_h > +#define InspectorTestFrontendDispatchers_h > + > +#if ENABLE(INSPECTOR) > + > +#include "InspectorTestTypeBuilders.h" > +#include <inspector/InspectorFrontendChannel.h> > +#include <inspector/InspectorValues.h> > +#include <wtf/PassRefPtr.h> > +#include <wtf/text/WTFString.h> > + > +namespace Inspector { > + > + > + > +} // namespace Inspector Heh. Maybe we should make empty files when generating frontend dispatchers for input without events. But seeing as this won't happen in practice it'll probably be a waste of time. > Source/JavaScriptCore/inspector/scripts/tests/expected/domains-with-varying-command-sizes.json-result:542 > +static const char* const enum_constant_values[] = { > +}; > + > +String getTestEnumConstantValue(int code) { > + return enum_constant_values[code]; > +} Likewise. > Source/WebCore/inspector/protocol/CSS.json:167 > + "type": "string", Nit: Broken indentation. > Tools/Scripts/webkitpy/inspector/main.py:124 > + input_directory = os.path.join('JavaScriptCore', 'inspector', 'scripts', 'tests') > + reference_directory = os.path.join('JavaScriptCore', 'inspector', 'scripts', 'tests', 'expected') Hmm, I wonder if we should put the tests outside of JavaScriptCore/inspector/scripts/tests. But you are matching a precedent WebCore/bindings/scripts/tests and JavaScriptCore/replay/scripts. Still, one big reason worth moving it out is that I don't want my code searches to be showing test results / expected results files when I do a search inside Source/JavaScriptCore. I do a lot of searches from Source/JavaScriptCore/inspector and I could see it picking up things in this directory.

Comment on attachment 236385 [details] PFR v2 View in context: https://bugs.webkit.org/attachment.cgi?id=236385&action=review >> Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:95 >> + def _generate_anonymous_enum_for_parameter(self, parameter, command): > > This is done in multiple places exactly identical. Maybe it could be shared somewhere. one is for commands, the other for events. They use different accessor names. >> Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:314 >> + lines.append('') > > This does not sound right. We should not be looking at the name of the type_member, but the type of the type_member. > > It seems like this might be working by accident (as was the case in the old generator too). > > For example, given an existing type with optional fields: > > { > "id": "KeyPath", > "type": "object", > "description": "Key path.", > "properties": [ > { "name": "type", "type": "string", "enum": ["null", "string", "array"], "description": "Key path type." }, > { "name": "string", "type": "string", "optional": true, "description": "String value." }, > { "name": "array", "type": "array", "optional": true, "items": { "type": "string" }, "description": "Array value." } > ] > } > > We generate this for the optional fields: > > /* > * Synthetic constructor: > * RefPtr<KeyPath> result = KeyPath::create() > * .setType(...); > */ > static Builder<NoFieldsSet> create() > { > return Builder<NoFieldsSet>(Inspector::InspectorObject::create()); > } > typedef Inspector::TypeBuilder::StructItemTraits ItemTraits; > > void setString(const String& value) > { > this->setString(ASCIILiteral("string"), value); > } > using Inspector::InspectorObjectBase::setString; > > > void setArray(PassRefPtr<Inspector::TypeBuilder::Array<String> > value) > { > this->setValue(ASCIILiteral("array"), value); > } > using Inspector::InspectorObjectBase::setArray; > > > The "using" lines here makes sense by accident because the field was named "array". > > I have a feeling that all of these using lines can probably just be removed! The methods are all protected in the base class. If they are required, we should do it correctly (and match the implementation inside the setter). > > --- > > Having said that, the fact that setArray takes an Array and calls setValue also sounds wrong, but is fine because setArray/setValue/setObject are identical. We should still fix that. See comment later on. It should just call InspectorObjectBase::%(keyedSet)s(...) to avoid this stupid 'using' stuff. Thanks for pointing it out, I probably didn't quite see the fix before. (Thanks for the test case too..) >> Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:142 >> + }""" % args) > > [Applies to multiple places in the patch] > > I always wondered if part of what slows us down in debug builds is this find(...) for every single member. I wonder if there is a better way to check the properties. > > We can combine the type declaration and assignment, and we should use ASCIILiteral: > > InspectorObject::iterator %(memberName)sPos = object->find(ASCIILiteral("%(memberName)s")); We should profile this and address it separately if it shows up. It would be nice to speed up debug builds. >> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:251 >> + return 'setValue' > > I think ObjectType can use setObject and ArrayType can use setArray. > > These 3 methods are all implemented identically right now in InspectorObjectBase. I don't think we ever have to actually use setValue ourselves. I changed InspectorObjectBase::set{Object,Array} to take the *Base types since that's what everything inherits from. But it seems to work now. Hooray. >> Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:58 >> +// by the script: JavaScriptCore/replay/scripts/CodeGeneratorInspector.py""") > > Wrong script! Should be: > > Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py I think this should reference the top-level script. A simple grep of the comment will trace back to the template file. >> Source/JavaScriptCore/inspector/scripts/codegen/models.py:540 >> + self.is_optional = is_optional > > Judging by the bug you fixed, maybe we should type check things like "is_optional" (boolean) when we parse to catch errors even earlier! Great find. Added. >> Source/JavaScriptCore/inspector/scripts/tests/expected/domains-with-varying-command-sizes.json-result:374 >> +} // namespace Inspector > > Heh. Maybe we should make empty files when generating frontend dispatchers for input without events. But seeing as this won't happen in practice it'll probably be a waste of time. Yeah, I didn't bother. >> Tools/Scripts/webkitpy/inspector/main.py:124 >> + reference_directory = os.path.join('JavaScriptCore', 'inspector', 'scripts', 'tests', 'expected') > > Hmm, I wonder if we should put the tests outside of JavaScriptCore/inspector/scripts/tests. But you are matching a precedent WebCore/bindings/scripts/tests and JavaScriptCore/replay/scripts. > > Still, one big reason worth moving it out is that I don't want my code searches to be showing test results / expected results files when I do a search inside Source/JavaScriptCore. I do a lot of searches from Source/JavaScriptCore/inspector and I could see it picking up things in this directory. The outputs use weird extensions exactly so you can filter by *cpp,*h,*py, whatever. I'm not sure where you would move these otherwise. webkitpy tests are in the Tools directory I think, but so are the scripts themselves.

Created attachment 236611 [details] Patch

Attachment 236611 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:63: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:91: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:99: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:135: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:175: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:45: [BackendDispatcherHeaderGenerator.output_filename] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:52: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:63: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no '_input_filepath' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:67: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'generate_license' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:78: [BackendDispatcherHeaderGenerator._generate_handler_declarations_for_domain] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:106: [BackendDispatcherHeaderGenerator._generate_anonymous_enum_for_parameter] Instance of 'BackendDispatcherHeaderGenerator' has no 'encoding_for_enum_value' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:163: [BackendDispatcherHeaderGenerator._generate_dispatcher_declarations_for_domain] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:47: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:77: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:88: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:217: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:55: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:115: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:134: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:150: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:60: at least two spaces before inline comment [pep8/E261] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:147: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:147: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:55: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:106: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:76: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:87: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:95: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:58: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:73: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:108: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:118: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:124: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:159: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:179: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:192: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:206: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:232: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:177: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:124: [generate_from_specification] Undefined variable 'FrontendDispatcherHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:126: [generate_from_specification] Undefined variable 'TypeBuilderHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:254: whitespace before ')' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:54: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:67: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:277: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:300: whitespace before '}' [pep8/E202] [5] ERROR: Source/WebCore/inspector/InspectorReplayAgent.h:107: The parameter name "segmentState" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:77: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:103: whitespace before '}' [pep8/E202] [5] Total errors found: 52 in 90 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #20) > (From update of attachment 236385 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236385&action=review > > >> Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:58 > >> +// by the script: JavaScriptCore/replay/scripts/CodeGeneratorInspector.py""") > > > > Wrong script! Should be: > > > > Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py > > I think this should reference the top-level script. A simple grep of the comment will trace back to the template file. > Oops, I copied the wrong script name, lol. What I mean is we should not have *replay* in the script name. This should be: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py

Comment on attachment 236611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236611&action=review > Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:61 > +// by the script: Source/JavaScriptCore/inspector/scripts/codegen/generate-inspector-protocol-bindings.py""") Almost fixed! The "/codegen" part is not true.

Created attachment 236631 [details] Fix for GTK build

Attachment 236631 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:63: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:91: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:99: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:135: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:175: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:45: [BackendDispatcherHeaderGenerator.output_filename] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:52: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:63: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no '_input_filepath' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:67: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'generate_license' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:78: [BackendDispatcherHeaderGenerator._generate_handler_declarations_for_domain] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:106: [BackendDispatcherHeaderGenerator._generate_anonymous_enum_for_parameter] Instance of 'BackendDispatcherHeaderGenerator' has no 'encoding_for_enum_value' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:163: [BackendDispatcherHeaderGenerator._generate_dispatcher_declarations_for_domain] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:47: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:77: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:88: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:217: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:55: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:115: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:134: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:150: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:60: at least two spaces before inline comment [pep8/E261] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:147: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:147: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:55: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:106: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:76: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:87: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:95: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:58: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:73: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:108: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:118: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:124: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:159: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:179: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:192: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:206: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:232: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:177: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:124: [generate_from_specification] Undefined variable 'FrontendDispatcherHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:126: [generate_from_specification] Undefined variable 'TypeBuilderHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:254: whitespace before ')' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:54: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:67: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:277: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:300: whitespace before '}' [pep8/E202] [5] ERROR: Source/WebCore/inspector/InspectorReplayAgent.h:107: The parameter name "segmentState" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:77: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:103: whitespace before '}' [pep8/E202] [5] Total errors found: 52 in 90 files If any of these errors are false positives, please file a bug against check-webkit-style.

bfulgham, I'm having trouble diagnosing the windows EWS failure. The latest patch won't apply, but it builds fine in my Win7 VM. Is this some SVN-ism that got messed up because I use git-svn? I would appreciate any pointers.

Created attachment 236640 [details] Fix for GTK build (take 2) Latest patch adds a workaround for re.sub taking different arguments in Python 2.6, which seems to be used by some GTK bots.

Attachment 236640 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:63: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:91: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:99: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:135: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:175: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:45: [BackendDispatcherHeaderGenerator.output_filename] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:52: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:63: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no '_input_filepath' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:67: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'generate_license' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:78: [BackendDispatcherHeaderGenerator._generate_handler_declarations_for_domain] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:106: [BackendDispatcherHeaderGenerator._generate_anonymous_enum_for_parameter] Instance of 'BackendDispatcherHeaderGenerator' has no 'encoding_for_enum_value' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:163: [BackendDispatcherHeaderGenerator._generate_dispatcher_declarations_for_domain] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:47: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:77: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:88: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:217: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:55: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:115: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:134: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:150: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:60: at least two spaces before inline comment [pep8/E261] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:147: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:147: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:55: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:106: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:76: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:87: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:95: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:58: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:73: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:108: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:118: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:124: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:159: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:179: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:192: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:206: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:232: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:177: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:124: [generate_from_specification] Undefined variable 'FrontendDispatcherHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:126: [generate_from_specification] Undefined variable 'TypeBuilderHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:254: whitespace before ')' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:54: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:67: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:277: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:300: whitespace before '}' [pep8/E202] [5] ERROR: Source/WebCore/inspector/InspectorReplayAgent.h:107: The parameter name "segmentState" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:77: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:103: whitespace before '}' [pep8/E202] [5] Total errors found: 52 in 90 files If any of these errors are false positives, please file a bug against check-webkit-style.

It seems this patch killed the EFL and GTK EWS bots, they aren't able to do builds since 6-7 hours. Carlos, Gyuyoung, could you trigger clean build on the EWS machines?

(In reply to comment #30) > It seems this patch killed the EFL and GTK EWS bots, > they aren't able to do builds since 6-7 hours. > > Carlos, Gyuyoung, could you trigger clean build on the EWS machines? The problem is that the newly generated WebKitBuild/Release/DerivedSources/JavaScriptCore/InspectorJSTypeBuilders.h remained in the build and the forwarding header still points to is instead of the original WebKitBuild/Release/DerivedSources/JavaScriptCore/inspector/InspectorJSTypeBuilders.h. and maybe there are many other leaking headers in the WebKitBuild ... So changing the generator and output path without changing the filenames can cause strange incremental build issues. :(

(In reply to comment #31) > (In reply to comment #30) > > It seems this patch killed the EFL and GTK EWS bots, > > they aren't able to do builds since 6-7 hours. > > > > Carlos, Gyuyoung, could you trigger clean build on the EWS machines? > > The problem is that the newly generated WebKitBuild/Release/DerivedSources/JavaScriptCore/InspectorJSTypeBuilders.h remained in the build and the > forwarding header still points to is instead of the original WebKitBuild/Release/DerivedSources/JavaScriptCore/inspector/InspectorJSTypeBuilders.h. > > and maybe there are many other leaking headers in the WebKitBuild ... > > So changing the generator and output path without changing the > filenames can cause strange incremental build issues. :( This was a bug in the CMakeLists.txt that was fixed. It was copying to the old placement, since this patch was originally written a few months ago. It should be fixed in the latest patch so that it overwrites the same file locations. I have gotten the EFL and GTK builds to pass locally, almost done with Windows.

(In reply to comment #27) > bfulgham, I'm having trouble diagnosing the windows EWS failure. The latest patch won't apply, but it builds fine in my Win7 VM. Is this some SVN-ism that got messed up because I use git-svn? I would appreciate any pointers. My guess would be line endings differences. Sometimes patches fail to apply because they are CRLF-ended (e.g., Visual Studio project files), but when edited on Mac they are LF-only. The patch system often chokes on this. I'll take a look.

This doesn't build for me: 18>c:\projects\webkit\opensource\webkitbuild\production\include\private\javascriptcore\InspectorJSTypeBuilders.h(130): error C2491: 'Inspector::TypeBuilder::getJSEnumConstantValue' : definition of dllimport function not allowed (..\Modules\websockets\WebSocket.cpp) This same error is repeated dozens of times in other files that end up including this header.

(In reply to comment #34) > This doesn't build for me: > > 18>c:\projects\webkit\opensource\webkitbuild\production\include\private\javascriptcore\InspectorJSTypeBuilders.h(130): error C2491: 'Inspector::TypeBuilder::getJSEnumConstantValue' : definition of dllimport function not allowed (..\Modules\websockets\WebSocket.cpp) > > This same error is repeated dozens of times in other files that end up including this header. The problem may be your new template declaration. I don't think you need the JS_EXPORT_PRIVATE on the template, since it exists entirely in the header.

Created attachment 236668 [details] Fix Windows build issues

Comment on attachment 236668 [details] Fix Windows build issues oof, looks like my changes to VS filters reformatted the entire solution, will put up a smaller diff.

Attachment 236668 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:63: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:91: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:99: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:135: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:175: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:45: [BackendDispatcherHeaderGenerator.output_filename] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:52: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:63: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no '_input_filepath' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:67: [BackendDispatcherHeaderGenerator.generate_output] Instance of 'BackendDispatcherHeaderGenerator' has no 'generate_license' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:78: [BackendDispatcherHeaderGenerator._generate_handler_declarations_for_domain] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:106: [BackendDispatcherHeaderGenerator._generate_anonymous_enum_for_parameter] Instance of 'BackendDispatcherHeaderGenerator' has no 'encoding_for_enum_value' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_header.py:163: [BackendDispatcherHeaderGenerator._generate_dispatcher_declarations_for_domain] Instance of 'BackendDispatcherHeaderGenerator' has no 'model' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:47: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:77: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:88: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator.py:217: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:55: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:115: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:134: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_implementation.py:150: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:60: at least two spaces before inline comment [pep8/E261] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:147: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/models.py:147: [Type.raw_name] Instance of 'Type' has no '_name' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:55: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_implementation.py:106: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/ChangeLog:31: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:76: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:87: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_commands.py:95: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:58: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:73: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:108: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:118: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:124: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:159: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:179: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:192: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:206: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:232: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:177: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:124: [generate_from_specification] Undefined variable 'FrontendDispatcherHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:126: [generate_from_specification] Undefined variable 'TypeBuilderHeaderGenerator' [pylint/E0602] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generator_templates.py:254: whitespace before ')' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:54: whitespace before ']' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:67: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:278: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_type_builder_header.py:301: whitespace before '}' [pep8/E202] [5] ERROR: Source/WebCore/inspector/InspectorReplayAgent.h:107: The parameter name "segmentState" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:62: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:77: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_frontend_dispatcher_header.py:103: whitespace before '}' [pep8/E202] [5] Total errors found: 53 in 92 files If any of these errors are false positives, please file a bug against check-webkit-style.

Cannot reproduce win EWS's problem. I have applied and built the patch on a Win7 VM with both git and svn checkouts. Is it safe to try landing this later today or tomorrow?

(In reply to comment #39) > Cannot reproduce win EWS's problem. I have applied and built the patch on a Win7 VM with both git and svn checkouts. Is it safe to try landing this later today or tomorrow? Sure -- if you can build locally let's go ahead and land it. Just please watch the bots and be ready to react if something goes wrong :-) I'm watching bots today, if you want to land it. Is JoePeck happy with it now? I'm fine on the Windows side.

(In reply to comment #40) > (In reply to comment #39) > > Cannot reproduce win EWS's problem. I have applied and built the patch on a Win7 VM with both git and svn checkouts. Is it safe to try landing this later today or tomorrow? > > Is JoePeck happy with it now? I'm fine on the Windows side. Heh, this is perhaps an understatement. I am very happy with it =). I thought of one other possible review comment, we (or rather I) will need to update: Source/WebInspectorUI/Scripts/update-LegacyInspectorBackendCommands.rb So you might want to do one more search for CodeGeneratorInspector.py and see if there are other Tools or Scripts that we might want to adjust. In any case, update-LegacyInspectorBackendCommands.rb can be done in a follow-up.

Committed r172655: <http://trac.webkit.org/changeset/172655>

(In reply to comment #32) > (In reply to comment #31) > > (In reply to comment #30) > > > It seems this patch killed the EFL and GTK EWS bots, > > > they aren't able to do builds since 6-7 hours. > > > > > > Carlos, Gyuyoung, could you trigger clean build on the EWS machines? > > > > The problem is that the newly generated WebKitBuild/Release/DerivedSources/JavaScriptCore/InspectorJSTypeBuilders.h remained in the build and the > > forwarding header still points to is instead of the original WebKitBuild/Release/DerivedSources/JavaScriptCore/inspector/InspectorJSTypeBuilders.h. > > > > and maybe there are many other leaking headers in the WebKitBuild ... > > > > So changing the generator and output path without changing the > > filenames can cause strange incremental build issues. :( > > This was a bug in the CMakeLists.txt that was fixed. It was copying to the old placement, since this patch was originally written a few months ago. It should be fixed in the latest patch so that it overwrites the same file locations. > > I have gotten the EFL and GTK builds to pass locally, almost done with Windows. I have gotten complaints from others that the GTK EWS bots are failing with warnings about this: <https://webkit-queues.appspot.com/results/6303728441229312> Do we need to clear the DerivedSources directory on the GTK bots so that they can get past this issue?

(In reply to comment #43) > (In reply to comment #32) > > (In reply to comment #31) > > > (In reply to comment #30) > > > > It seems this patch killed the EFL and GTK EWS bots, > > > > they aren't able to do builds since 6-7 hours. > > > > > > > > Carlos, Gyuyoung, could you trigger clean build on the EWS machines? > > > > > > The problem is that the newly generated WebKitBuild/Release/DerivedSources/JavaScriptCore/InspectorJSTypeBuilders.h remained in the build and the > > > forwarding header still points to is instead of the original WebKitBuild/Release/DerivedSources/JavaScriptCore/inspector/InspectorJSTypeBuilders.h. > > > > > > and maybe there are many other leaking headers in the WebKitBuild ... > > > > > > So changing the generator and output path without changing the > > > filenames can cause strange incremental build issues. :( > > > > This was a bug in the CMakeLists.txt that was fixed. It was copying to the old placement, since this patch was originally written a few months ago. It should be fixed in the latest patch so that it overwrites the same file locations. > > > > I have gotten the EFL and GTK builds to pass locally, almost done with Windows. > > I have gotten complaints from others that the GTK EWS bots are failing with warnings about this: > <https://webkit-queues.appspot.com/results/6303728441229312> > > Do we need to clear the DerivedSources directory on the GTK bots so that they can get past this issue? Some bots use an old version of python (2.6). I seemed to have missed one use of flags= in re.sub. I'll fix it.

Committed <http://trac.webkit.org/changeset/172686> to address some lingering issues with old bots. Committed <http://trac.webkit.org/changeset/172687> to rebaseline after fixing a typo in a template. Let me know here or on IRC if there are any more issues.

Comment on attachment 236668 [details] Fix Windows build issues Cleared review? from attachment 236668 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).