Bug 136312 – Web Replay: code generator should take supplemental specifications and allow cross-framework references

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brian Burg]

Right now, the input JSON files duplicate any enums defined in one namespace and used in another one. Instead, the code generator should take specifications from other frameworks as supplemental files, like the Inspector generator does.

[Brian Burg]

*** Bug 136311 has been marked as a duplicate of this bug. ***

[Brian Burg]

Created attachment 244879 [details]
Patch

[WebKit Commit Bot]

This patch modifies the WEB_REPLAY inputs generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-input-generator-tests --reset-results`)

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-with-guard.json-TestReplayInputs.h:71:  The parameter name "phase" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:795:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers-with-guarded-values.json-TestReplayInputs.h:60:  The parameter name "button" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers.json-TestReplayInputs.h:81:  The parameter name "button" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 41 files


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

[Brian Burg]

Created attachment 244922 [details]
Patch (w/ Windows fix)

[Joseph Pecoraro]

Comment on attachment 244922 [details]
Patch (w/ Windows fix)

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

r=me

> Source/WebCore/ChangeLog:11
> +        * replay/WebInputs.json: Moved common types to JSINputs.json.

Typo: "JSINputs.json"

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:787
> +        if len(self.setting('exportMacro')) > 0:

Would just "if len(...):" do?

> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:981
> +    if len(input_filepaths) == 0:

This could also be "if !len(...):". Do what you think is clearer.

> Source/WebCore/DerivedSources.make:1164
>  INPUT_GENERATOR_SPECIFICATIONS = \
>  	$(WebCore)/replay/WebInputs.json \
> +	$(WebReplayScripts)/JSInputs.json \
>  #

I wonder if we should split this up into two variables so one day someone doesn't try to sort these in a different order and inadvertently break things since the order here is important.

Nothing to do here unless you want to.

[Brian Burg]

Comment on attachment 244922 [details]
Patch (w/ Windows fix)

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

Will rebase and EWS one more time before landing.

>> Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:787
>> +        if len(self.setting('exportMacro')) > 0:
> 
> Would just "if len(...):" do?

There are only a few places in WebKit python code that use such implicit conversions with len(), so I'm shy to add more.

>> Source/WebCore/DerivedSources.make:1164
>>  #
> 
> I wonder if we should split this up into two variables so one day someone doesn't try to sort these in a different order and inadvertently break things since the order here is important.
> 
> Nothing to do here unless you want to.

The order is *not* important. Specifications are not type-checked (i.e., try to lookup all type references between files) until all input files are parsed. Maybe I should put that into the changelog.

[Brian Burg]

Created attachment 244924 [details]
Patch (for EWS)

[Brian Burg]

Created attachment 244925 [details]
Patch (for EWS)

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-with-guard.json-TestReplayInputs.h:71:  The parameter name "phase" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/replay/scripts/CodeGeneratorReplayInputs.py:806:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers-with-guarded-values.json-TestReplayInputs.h:60:  The parameter name "button" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/replay/scripts/tests/expected/generate-enum-encoding-helpers.json-TestReplayInputs.h:81:  The parameter name "button" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 42 files


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

[Brian Burg]

Created attachment 244927 [details]
Patch (always Windows...)

[Brian Burg]

Created attachment 244935 [details]
Patch (for EWS, 5)

[Brian Burg]

Created attachment 244969 [details]
Patch (EWS)

[Brian Burg]

Committed r178714: <http://trac.webkit.org/changeset/178714>

[Brian Burg]

(In reply to comment #14)
> Committed r178714: <http://trac.webkit.org/changeset/178714>

Based on the reviewed patch with fix for windows (locally confirmed to fix known build issue). Landed manually since I was unable to convince EWS to apply changes to copy-files.cmd, which address the windows issue. I'll be watching in case Windows still has problems.