[webkit-reviews] review denied: [Bug 193262] Web Inspector: expose Audit and Recording versions to the frontend : [Attachment 359254] [Patch] Protocol Version

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 193262: Web Inspector: expose Audit and Recording versions to the frontend
https://bugs.webkit.org/show_bug.cgi?id=193262

Attachment 359254: [Patch] Protocol Version

https://bugs.webkit.org/attachment.cgi?id=359254&action=review




--- Comment #13 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 359254
  --> https://bugs.webkit.org/attachment.cgi?id=359254
[Patch] Protocol Version

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

r- because I'd like to see another version of this patch.

I'd suggest:

    • Limiting to just `int`
    • In the frontend making it `Agent.version`.
    • Potential codegen error if a command is named `version` given it would
conflict with `Agent.version`, but this is not important as I don't expect this
to ever happen.

> Source/WebInspectorUI/ChangeLog:12
> +	   Add "Interface" version values.

Why are there quotes around Interface?

>
Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_hea
der.py:107
> +	       if isinstance(version, (int, long, float)):
> +		   domain_lines.append('static const auto VERSION = %s;' %
version)
> +	       elif isinstance(version, (str, unicode)):
> +		   domain_lines.append('static const auto VERSION = "%s";' %
version)

Lets use explicit types. `int` or `unsigned` for the version number and `const
char*`.

>
Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_hea
der.py:114
> +	   else:

Style: Drop the else

>
Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py
:91
> +	       if isinstance(version, (int, long, float)):
> +		   lines.append('InspectorBackend.registerVersion("%(domain)s",
%(version)s);' % version_args)
> +	       elif isinstance(version, (str, unicode)):
> +		   lines.append('InspectorBackend.registerVersion("%(domain)s",
"%(version)s");' % version_args)

This doesn't appear in the code-get test result of InspectorBackendCommands.js:
Source/JavaScriptCore/inspector/scripts/tests/generic/expected/version.json-res
ult

Looks like you will need to update `should_generate_domain` to say YES if there
is a version.

> Source/JavaScriptCore/inspector/scripts/codegen/models.py:378
> +	       if not isinstance(json['version'], (int, float, str, unicode)):
> +		   raise ParseException("Malformed domain specification:
version is not a number or string")

You allowed `long` in other places but only `int` here. Why not just
standardize on int?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:145
> +	   var agent = this._agentForDomain(domainName);

Style: `let`.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:146
> +	   agent.VERSION = version;

Why not `agent.version`? The capital letters do not add anything.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:249
> +	   this._version = undefined;

This is unused.