[webkit-reviews] review granted: [Bug 128215] Web Replay: upstream replay input code generator and EncodedValue class : [Attachment 223361] new patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 128215: Web Replay: upstream replay input code generator and EncodedValue
class
https://bugs.webkit.org/show_bug.cgi?id=128215

Attachment 223361: new patch
https://bugs.webkit.org/attachment.cgi?id=223361&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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.j
son-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-help
ers-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-help
ers.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-help
ers.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 =).