Bug 131596 – Web Inspector: rewrite CodeGeneratorInspector to be modular and testable

• Component: Web Inspector
• Status: RESOLVED FIXED

[Brian Burg]

Right now it's pretty scary to touch the code generator. It's a big ball of mud.

I'm working on a prototype that splits parsing/typechecking from the separate code generators.
The approach has worked well for the replay inputs generator, both from a testing and maintainability perspective.
Once we have some tests, it will be economically feasible to make minor changes, cleanup, optimization to the protocol specification format or code generators without excessive nail biting.

[Radar WebKit Bug Importer]

<rdar://problem/16603443>

[Brian Burg]

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

[Brian Burg]

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

[Timothy Hatcher]

Can you post example output files?

[Brian Burg]

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

[Brian Burg]

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.

[Brian Burg]

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

[Brian Burg]

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

[Brian Burg]

Created attachment 232019 [details]
patch, with windows fixes

[Brian Burg]

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

[Brian Burg]

Created attachment 232043 [details]
PFR v1

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

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.

[Brian Burg]

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').

[Brian Burg]

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.

[Brian Burg]

Created attachment 236385 [details]
PFR v2

The new patch addresses all comments to date, and removes stale test results.

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

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
> [phone]  Brian J. Burg  <[e-mail]>
> +
> +        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.

[Joseph Pecoraro]

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.

[Brian Burg]

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.

[Brian Burg]

Created attachment 236611 [details]
Patch

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

(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

[Joseph Pecoraro]

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.

[Brian Burg]

Created attachment 236631 [details]
Fix for GTK build

[WebKit Commit Bot]

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.

[Brian Burg]

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.

[Brian Burg]

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.

[WebKit Commit Bot]

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.

[Csaba Osztrogonác]

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?

[Csaba Osztrogonác]

(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. :(

[Brian Burg]

(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.

[Brent Fulgham]

(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.

[Brent Fulgham]

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.

[Brent Fulgham]

(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.

[Brian Burg]

Created attachment 236668 [details]
Fix Windows build issues

[Brian Burg]

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.

[WebKit Commit Bot]

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.

[Brian Burg]

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?

[Brent Fulgham]

(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.

[Joseph Pecoraro]

(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.

[Brian Burg]

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

[Joseph Pecoraro]

(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?

[Brian Burg]

(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.

[Brian Burg]

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.

[Csaba Osztrogonác]

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).