[webkit-reviews] review granted: [Bug 131596] Web Inspector: rewrite CodeGeneratorInspector to be modular and testable : [Attachment 236385] PFR v2

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg
<burg at cs.washington.edu>'s request for review:
Bug 131596: Web Inspector: rewrite CodeGeneratorInspector to be modular and
testable
https://bugs.webkit.org/show_bug.cgi?id=131596

Attachment 236385: PFR v2
https://bugs.webkit.org/attachment.cgi?id=236385&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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-size
s.json:5
> +    "domain": "Network1",
> +
> +    "commands": [

Nit: Extra blank line can be removed.

>
Source/JavaScriptCore/inspector/scripts/tests/expected/domains-with-varying-com
mand-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-com
mand-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.