Bug 128215 – Web Replay: upstream replay input code generator and EncodedValue class

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Blaze Burg]

It has tests/regression detectors, so you need not look at all the real replay inputs to see what it does.

[Blaze Burg]

CC: keepers of template knowledge for EncodedValue.

[Blaze Burg]

Created attachment 223185 [details]
Current branch input specification for WebCore

For reference

[Blaze Burg]

Created attachment 223186 [details]
patch

[WebKit Commit Bot]

Attachment 223186 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers-with-guarded-values.json-TestReplayInputs.h:67:  The parameter name "button" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:95:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:101:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:141:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:538:  whitespace before ']'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:687:  whitespace before ']'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:823:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:833:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:841:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:865:  whitespace before ']'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:886:  whitespace before ']'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:973:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers.json-TestReplayInputs.h:80:  The parameter name "button" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 13 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 223187 [details]
WebCore generated implementation file

for reference

[Blaze Burg]

Created attachment 223188 [details]
WebCore generated header file

[Blaze Burg]

I'm looking at changing EncodedValue to be copyable, since it may make auto-serialization of vectors possible without hacks. So, maybe defer on reviewing that part until I post another comment.

[Blaze Burg]

Created attachment 223361 [details]
new patch

[Blaze Burg]

OK, everything is ready for review now.

[WebKit Commit Bot]

Attachment 223361 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers.json-TestReplayInputs.cpp:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers-with-guarded-values.json-TestReplayInputs.h:65:  The parameter name "button" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:70:  whitespace before ']'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:103:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:534:  whitespace before ']'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:686:  whitespace before ']'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:822:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:832:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:840:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:864:  whitespace before ']'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:885:  whitespace before ']'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:972:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers.json-TestReplayInputs.h:76:  The parameter name "button" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 13 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 223361 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223361&action=review

r=me. Most of my comments are style. This looks real good to me, nice and clean.

> Source/JavaScriptCore/inspector/InspectorValues.cpp:638
> +    *output = static_cast<long>(m_doubleValue);

This case should be <long long>.

> Source/JavaScriptCore/replay/EncodedValue.cpp:2
> + * Copyright (C) 2013 University of Washington. All rights reserved.

2014.

> Source/JavaScriptCore/replay/EncodedValue.h:70
> +    virtual ~EncodedValue() { }

Any reason this is needed? Is there going to be an EncodedValue subclass?

> Source/JavaScriptCore/replay/EncodedValue.h:97
> +template<> JS_EXPORT_PRIVATE unsigned long EncodedValue::convertTo<unsigned long>();

unsigned long doesn't match with uint32_t or uint64_t?

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:350
> +def check_properties(props, obj, what):

How about naming this check_for_required_properties?

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:415
> +        for ty_framework_name, ty_list in json['types'].iteritems():

Style: We don't normally abbreviate variable names. I'd prefer "type_framework_name" and "type_list"

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:431
> +        _type = Type(json['name'], TypeMode.fromString(json['mode']), framework, json.get('header', None), json.get('enclosing_class', None), json.get('values'), json.get('guarded_values', {}), json.get('storage'), json.get('flags', []))

This would reach much better split up across multiple lines:

    name = json['name']
    mode = TypeMode.fromString(json['mode'])
    header = json.get('header')
    ...
    _type = Type(name, mode, framework, header, ...)

Nit: json.get('header', None) the None is unnecessary here <http://docs.python.org/2/library/stdtypes.html#dict.get>.

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:438
> +            if _type.is_enum() and "storage" not in json:
> +                raise ParseException("C-style enums must also specify their storage type so they can be forward declared.")

You should include the name of the enum that this is failing for.

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:459
> +    def typecheck(self):
> +        for _type in self.types:
> +            self.typecheck_type(_type)
> +
> +        for _input in self.inputs:
> +            self.typecheck_input(_input)

"typecheck" being the action name sounds like a pure method. However it does go through and initialize self.types_by_name / self.inputs_by_name. Maybe this should be called "resolve_types".

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:491
> +            raise TypecheckException("Unknown type %s referenced in member %s"
> +                            % (input_member.typeName, input_member.memberName))

Style: unnecessary wrap

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:524
> +def lcfirst(s):
> +    return s[0].lower() + s[1:]

This is unused.

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:564
> +        head_file = IncrementalFileWriter(os.path.join(_dir, self.output_filename('h')), force)
> +        impl_file = IncrementalFileWriter(os.path.join(_dir, self.output_filename('cpp')), force)

Style: We don't normally abbreviate variable names. I'd prefer "header_file" and maybe "implementation_file" (though impl is commonplace)

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:675
> +

Style: Remove empty line

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:802
> +                'qualifiedEnumValue': "%s%s" % (enum_prefix, _value),

Should this be "%s::%s"? Likewise for the rest in this method?

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputsTemplates.py:102
> +    InputTraitsDeclaration = """template<> ${structOrClass} InputTraits<${qualifiedInputName}> {
> +    static InputQueue queue() { return InputQueue::${queueType}; }
> +    static const AtomicString& type();
> +
> +    static void encode(JSC::EncodedValue&, const ${qualifiedInputName}&);
> +    static bool decode(JSC::EncodedValue&, std::unique_ptr<${qualifiedInputName}>&);
> +};"""

I find this a little difficult to read. I prefer the format of the CodeGeneratorInspector:

    VariableName = (
    """...
    ...""")

This way there is no jarring indentation of the first line and the variable name is on its own line and easy to recognize.

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputsTemplates.py:182
> +    EnumClassDecodeCase = """    if (enumString == String(ASCIILiteral("${enumStringValue}"))) {
> +        enumValue = ${qualifiedEnumValue};

WTFString should have an == operator for String == char*:

    inline bool operator==(const String& a, const char* b)

So I don't think you will need to String(ASCIILiteral(...)) boxing. Just:

    if (enumString == "${enumStringValue}")

should do!

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputsTemplates.py:212
> +    EnumDecodeCase = """        if (enumString == String(ASCIILiteral("${enumStringValue}")))
> +            enumValue = static_cast<${qualifiedEnumName}>(enumValue | ${qualifiedEnumValue});"""

Ditto RE String(ASCIILiteral(...))

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputsTemplates.py:226
> +${inputName}::~${inputName}()
> +{
> +}"""

If the destructors are always empty, can we inline them in the header and avoid including them in the implementation?

> Source/JavaScriptCore/replay/scripts/tests/expected/fail-on-missing-input-name.json-error:1
> +ERROR: When parsing input, required property missing: name

Would be nice to say, "When parsing input 'Foo', ..."

> Source/JavaScriptCore/replay/scripts/tests/expected/fail-on-missing-type-mode.json-error:1
> +ERROR: When parsing type, required property missing: mode

Would be nice to say, "When parsing type 'Foo', ..."

> Source/JavaScriptCore/replay/scripts/tests/expected/fail-on-unknown-member-type.json-error:1
> +ERROR: Unknown type double referenced in member randomSeed

Most of the other errors end with the bad type, making it clear what was bad input. Might be nice to mark the user inputs here, e.g. something like 'double' and 'randomSeed' or rephrase the error.

> Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers-with-guarded-values.json-TestReplayInputs.cpp:77
> +EncodedValue EncodingTraits<WebCore::MouseButton>::encodeValue(const WebCore::MouseButton& enumValue)
> +{
> +    EncodedValue encodedValue = EncodedValue::createArray();
> +    if (enumValue & WebCore::NoButton)
> +        encodedValue.append<String>(ASCIILiteral("NoButton"));
> +    if (enumValue == WebCore::NoButton)
> +        return encodedValue;

So you're doing the double check so that you can support exclusive enums and bitwise enums? Seems fine.

Can you nest the '==' check within the '&' check? Or is it possible that something can be == but not &?

    if (enumValue & WebCore::NoButton) {
        encodedValue.append<String>(ASCIILiteral("NoButton"));
        if (enumValue == WebCore::NoButton)
            return encodedValue;
    }

> Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers.json-TestReplayInputs.cpp:72
> +}
> +EncodedValue EncodingTraits<InputQueue>::encodeValue(const InputQueue& enumValue)

Nit: Should be a newline here, if it is easy.

> Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers.json-error:1
> +ERROR: When parsing type, required property missing: storage

Does this still generate files despite the error?

> Source/JavaScriptCore/replay/scripts/tests/fail-on-unknown-input-queue.json:19
> +            "queue": "SCRIPT_MEOIZED",
> +            "members": [
> +                {
> +                    "name": "currentTime",
> +                    "type": "double"
> +                }
> +            ]
> +        }

A bunch of these tests have multiple errors. You're depending on the order of error checking for the current output. Would probably be good to just include one error per test (as that is what it sounds like the test is trying to do anyways). Here, the type "double" would throw an error.

> Source/JavaScriptCore/replay/scripts/tests/generate-inputs-with-flags.json:21
> +            "flags": ["HIDDEN"],

Should the generator warn about unknown flags (possible typos)? HIDDEN can just be a passive accepted flag.

> LayoutTests/inspector-protocol/model/probe-manager-add-remove-actions.html:1
> +<!doctype html>

Hehe, this probes related test is unrelated to this patch =).

[Blaze Burg]

Comment on attachment 223361 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223361&action=review

>> Source/JavaScriptCore/replay/EncodedValue.cpp:2
>> + * Copyright (C) 2013 University of Washington. All rights reserved.
> 
> 2014.

This file is based on EncoderContext/JSONEncoderContext, which haven't been edited in 2014 from UW.

>> Source/JavaScriptCore/replay/EncodedValue.h:70
>> +    virtual ~EncodedValue() { }
> 
> Any reason this is needed? Is there going to be an EncodedValue subclass?

Oops, cruft. Will remove.

>> Source/JavaScriptCore/replay/EncodedValue.h:97
>> +template<> JS_EXPORT_PRIVATE unsigned long EncodedValue::convertTo<unsigned long>();
> 
> unsigned long doesn't match with uint32_t or uint64_t?

We need this even though it's almost always a uint32_t, because the data type is defined in terms of unsigned long rather than a fixed-size type.

>> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:459
>> +            self.typecheck_input(_input)
> 
> "typecheck" being the action name sounds like a pure method. However it does go through and initialize self.types_by_name / self.inputs_by_name. Maybe this should be called "resolve_types".

OK

>> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:524
>> +    return s[0].lower() + s[1:]
> 
> This is unused.

Oops, used to be everywhere

>> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:802
>> +                'qualifiedEnumValue': "%s%s" % (enum_prefix, _value),
> 
> Should this be "%s::%s"? Likewise for the rest in this method?

Sometimes you don't need the prefix (c-style enum in same namespace with no enclosing class), so enum_prefix becomes nothing by the current implementation trick of adding a dummy element to prefix_components and joining with ::

>> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputsTemplates.py:102
>> +};"""
> 
> I find this a little difficult to read. I prefer the format of the CodeGeneratorInspector:
> 
>     VariableName = (
>     """...
>     ...""")
> 
> This way there is no jarring indentation of the first line and the variable name is on its own line and easy to recognize.

I forgot how to do that. Thanks!

>> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputsTemplates.py:226
>> +}"""
> 
> If the destructors are always empty, can we inline them in the header and avoid including them in the implementation?

They are only in the implementation file so that forward declarations of PassRefPtr'd things still work. I can fix this in a followup patch once some of those inputs are actually landed.

>> Source/JavaScriptCore/replay/scripts/tests/expected/fail-on-unknown-member-type.json-error:1
>> +ERROR: Unknown type double referenced in member randomSeed
> 
> Most of the other errors end with the bad type, making it clear what was bad input. Might be nice to mark the user inputs here, e.g. something like 'double' and 'randomSeed' or rephrase the error.

OK

>> Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers-with-guarded-values.json-TestReplayInputs.cpp:77
>> +        return encodedValue;
> 
> So you're doing the double check so that you can support exclusive enums and bitwise enums? Seems fine.
> 
> Can you nest the '==' check within the '&' check? Or is it possible that something can be == but not &?
> 
>     if (enumValue & WebCore::NoButton) {
>         encodedValue.append<String>(ASCIILiteral("NoButton"));
>         if (enumValue == WebCore::NoButton)
>             return encodedValue;
>     }

OK

>> Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers.json-error:1
>> +ERROR: When parsing type, required property missing: storage
> 
> Does this still generate files despite the error?

Oops, this file shouldn't exist.

>> Source/JavaScriptCore/replay/scripts/tests/generate-inputs-with-flags.json:21
>> +            "flags": ["HIDDEN"],
> 
> Should the generator warn about unknown flags (possible typos)? HIDDEN can just be a passive accepted flag.

I forgot to fix and rebaseline this one apparently.

>> LayoutTests/inspector-protocol/model/probe-manager-add-remove-actions.html:1
>> +<!doctype html>
> 
> Hehe, this probes related test is unrelated to this patch =).

Git is hard ._.

[Blaze Burg]

Committed r163918: <http://trac.webkit.org/changeset/163918>