Bug 136312 - Web Replay: code generator should take supplemental specifications and allow cross-framework references
Summary: Web Replay: code generator should take supplemental specifications and allow ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Burg
URL:
Keywords:
: 136311 (view as bug list)
Depends on:
Blocks: 136294 136297
  Show dependency treegraph
 
Reported: 2014-08-27 14:44 PDT by Brian Burg
Modified: 2015-01-20 08:55 PST (History)
3 users (show)

See Also:


Attachments
Patch (115.00 KB, patch)
2015-01-18 23:22 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (w/ Windows fix) (116.22 KB, patch)
2015-01-19 13:23 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (for EWS) (116.84 KB, patch)
2015-01-19 14:01 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (for EWS) (116.68 KB, patch)
2015-01-19 14:22 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (always Windows...) (116.83 KB, patch)
2015-01-19 14:47 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (for EWS, 5) (116.84 KB, patch)
2015-01-19 16:24 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (EWS) (111.18 KB, patch)
2015-01-19 22:32 PST, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-08-27 14:44:44 PDT
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.
Comment 1 Brian Burg 2015-01-18 23:00:28 PST
*** Bug 136311 has been marked as a duplicate of this bug. ***
Comment 2 Brian Burg 2015-01-18 23:22:46 PST
Created attachment 244879 [details]
Patch
Comment 3 WebKit Commit Bot 2015-01-18 23:23:40 PST
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`)
Comment 4 WebKit Commit Bot 2015-01-18 23:23:48 PST
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.
Comment 5 Brian Burg 2015-01-19 13:23:03 PST
Created attachment 244922 [details]
Patch (w/ Windows fix)
Comment 6 Joseph Pecoraro 2015-01-19 13:43:53 PST
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.
Comment 7 Brian Burg 2015-01-19 13:53:39 PST
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.
Comment 8 Brian Burg 2015-01-19 14:01:54 PST
Created attachment 244924 [details]
Patch (for EWS)
Comment 9 Brian Burg 2015-01-19 14:22:53 PST
Created attachment 244925 [details]
Patch (for EWS)
Comment 10 WebKit Commit Bot 2015-01-19 14:25:27 PST
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.
Comment 11 Brian Burg 2015-01-19 14:47:38 PST
Created attachment 244927 [details]
Patch (always Windows...)
Comment 12 Brian Burg 2015-01-19 16:24:22 PST
Created attachment 244935 [details]
Patch (for EWS, 5)
Comment 13 Brian Burg 2015-01-19 22:32:57 PST
Created attachment 244969 [details]
Patch (EWS)
Comment 14 Brian Burg 2015-01-20 08:52:16 PST
Committed r178714: <http://trac.webkit.org/changeset/178714>
Comment 15 Brian Burg 2015-01-20 08:55:57 PST
(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.