Bug 137748 – Web Inspector: Generate all Inspector domains together in JavaScriptCore

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

This is the first step towards allowing alternative inspector domain dispatchers. Generate all domains at once, so that we always know about the list of all possible domains/agent.

This work entails:

    - Source/WebCore/inspector/protocol JSON files into Source/JavaScriptCore/inspector/protocol
    - Simplify generators to not require framework prefixes previously necessary for multiple generated file types (e.g. InspectorJSFoo -> InspectorFoo)
    - Now that the frontend always has one big list of all the agents, only activate particular ones (more work to come to clean this up)
    - Update scripts, and build phases, to work in this simplified world

[Radar WebKit Bug Importer]

<rdar://problem/18668000>

[Joseph Pecoraro]

Created attachment 239883 [details]
[PATCH] Proposed Fix

I expect some back and forth with bots.

[Brian Burg]

Comment on attachment 239883 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/CMakeLists.txt:974
>      ${JAVASCRIPTCORE_DIR}/inspector/protocol/InspectorDomain.json

IMO, it's not good to have InspectorDomain.json and Inspector.json (the combined protocol file). I would name this one Inspector.json and the other one CombinedProtocol.json...

> Source/JavaScriptCore/CMakeLists.txt:1033
> +    ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector/InspectorBackendDispatchers.cpp

I have a patch in development to remove the Inspector* prefixes from the generated file names. This makes even more sense now because the includes can look like <inspector/BackendDispatchers.h> now.

I don't think you need to include that in this patch, though.

> Source/JavaScriptCore/ChangeLog:23
> +        * inspector/InspectorProtocolTypesBase.h: Renamed from Source/JavaScriptCore/inspector/InspectorProtocolTypes.h.

What about calling the generated one InspectorProtocolObjects.h or InspectorDomainTypes.h? It seems weird to make this have a *Base suffix because it's included many places in order to use Inspector{Value,Object,Array} directly. It's only used for the base class types from the generated files.

> Source/JavaScriptCore/ChangeLog:45
> +        * inspector/scripts/tests/expected/type-requiring-runtime-casts.json-result:

So, did you just rebaseline or did the actual tests change?

> Source/JavaScriptCore/DerivedSources.make:174
> +Inspector.json : inspector/scripts/generate-combined-inspector-json.py $(INSPECTOR_DOMAINS) EnabledInspectorDomains

See comment above on what to name the combined protocol file.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_protocol_types_header.py:217
> +                export_macro = self.model().framework.setting('export_macro', None)

Please note this in the change log. Is this covered by existing tests?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:-59
> -#include <inspector/InspectorValues.h>

Is this the correct header change?

> Source/WebInspectorUI/ChangeLog:39
> +        * Configurations/Base.xcconfig:

I am assuming that protocol JSON files are stripped out somewhere for production builds. If so, has this been preserved?

> Source/WebInspectorUI/UserInterface/Base/Main.js:56
> +    // FIXME: InspectorBackendCommands.js should specify which agents should be activated by default.

Please add a webkit.org/b/NNNN link.

> Source/WebInspectorUI/UserInterface/Base/Main.js:57
> +    console.log("debuggableType", InspectorFrontendHost.debuggableType());

oops.

> Source/WebInspectorUI/UserInterface/Base/Main.js:58
> +    if (InspectorFrontendHost.debuggableType() === "javascript")

Should we make this a symbolic constant somewhere?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:102
> +    activateDomains: function(domains)

It would be good to add a layout test for this behavior (mocking out InspectorFrontendHost.debuggableType) once it actually works. You need to have a lot of stuff set up to test JSContext inspection.

[Joseph Pecoraro]

Comment on attachment 239883 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/CMakeLists.txt:974
>>      ${JAVASCRIPTCORE_DIR}/inspector/protocol/InspectorDomain.json
> 
> IMO, it's not good to have InspectorDomain.json and Inspector.json (the combined protocol file). I would name this one Inspector.json and the other one CombinedProtocol.json...

That sounds good. Want me to do that now or in a small follow-up?

>> Source/JavaScriptCore/CMakeLists.txt:1033
>> +    ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector/InspectorBackendDispatchers.cpp
> 
> I have a patch in development to remove the Inspector* prefixes from the generated file names. This makes even more sense now because the includes can look like <inspector/BackendDispatchers.h> now.
> 
> I don't think you need to include that in this patch, though.

Yeah, that sounds good!

>> Source/JavaScriptCore/ChangeLog:45
>> +        * inspector/scripts/tests/expected/type-requiring-runtime-casts.json-result:
> 
> So, did you just rebaseline or did the actual tests change?

Just rebaseline. Could have been an extra export macro, but Test domains don't have an export macro.

>> Source/JavaScriptCore/inspector/scripts/codegen/generate_protocol_types_header.py:217
>> +                export_macro = self.model().framework.setting('export_macro', None)
> 
> Please note this in the change log. Is this covered by existing tests?

Oops, I forgot about this. This was because WebCore uses these strings for strings previously in InspectorWebProtocolTypes. Now that they are in the JSC generated file they needed to be exported, as the definition is in the CPP not the header. This doesn't appear to be tested by any existing tests.

>> Source/WebCore/inspector/InspectorCSSAgent.cpp:-59
>> -#include <inspector/InspectorValues.h>
> 
> Is this the correct header change?

When re-sorting the headers in each of the affected files I would remove headers that are not needed. In this particular case InspectorValues.h was already included in CSSAgent.h, so I'm removing it from the .cpp. Other #include changes here look correct.

>> Source/WebInspectorUI/ChangeLog:39
>> +        * Configurations/Base.xcconfig:
> 
> I am assuming that protocol JSON files are stripped out somewhere for production builds. If so, has this been preserved?

Protocol JSON files only live in the Source Tree, so nothing needs to be done.

We generate Inspector.json (soon to be CombinedDomains.json) and InspectorBackendCommands.js to JavaScriptCore's DerivedSources/PrivateHeaders. The backend commands gets copied into WebInspectorUI, the combined JSON file doesn't go anywhere.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:56
>> +    // FIXME: InspectorBackendCommands.js should specify which agents should be activated by default.
> 
> Please add a webkit.org/b/NNNN link.

Chicken and egg problem. I'll file one!

>> Source/WebInspectorUI/UserInterface/Base/Main.js:58
>> +    if (InspectorFrontendHost.debuggableType() === "javascript")
> 
> Should we make this a symbolic constant somewhere?

I should just switch to using the debuggableType we define later on. I don't know why I didn't just use that one.

        this.debuggableType = InspectorFrontendHost.debuggableType() === "web" ? WebInspector.DebuggableType.Web : WebInspector.DebuggableType.JavaScript;

[Joseph Pecoraro]

Comment on attachment 239883 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/CMakeLists.txt:974
>>      ${JAVASCRIPTCORE_DIR}/inspector/protocol/InspectorDomain.json
> 
> IMO, it's not good to have InspectorDomain.json and Inspector.json (the combined protocol file). I would name this one Inspector.json and the other one CombinedProtocol.json...

That sounds good. Want me to do that now or in a small follow-up?

>> Source/JavaScriptCore/CMakeLists.txt:1033
>> +    ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector/InspectorBackendDispatchers.cpp
> 
> I have a patch in development to remove the Inspector* prefixes from the generated file names. This makes even more sense now because the includes can look like <inspector/BackendDispatchers.h> now.
> 
> I don't think you need to include that in this patch, though.

Yeah, that sounds good!

>> Source/JavaScriptCore/ChangeLog:45
>> +        * inspector/scripts/tests/expected/type-requiring-runtime-casts.json-result:
> 
> So, did you just rebaseline or did the actual tests change?

Just rebaseline. Could have been an extra export macro, but Test domains don't have an export macro.

>> Source/JavaScriptCore/inspector/scripts/codegen/generate_protocol_types_header.py:217
>> +                export_macro = self.model().framework.setting('export_macro', None)
> 
> Please note this in the change log. Is this covered by existing tests?

Oops, I forgot about this. This was because WebCore uses these strings for strings previously in InspectorWebProtocolTypes. Now that they are in the JSC generated file they needed to be exported, as the definition is in the CPP not the header. This doesn't appear to be tested by any existing tests.

>> Source/WebCore/inspector/InspectorCSSAgent.cpp:-59
>> -#include <inspector/InspectorValues.h>
> 
> Is this the correct header change?

When re-sorting the headers in each of the affected files I would remove headers that are not needed. In this particular case InspectorValues.h was already included in CSSAgent.h, so I'm removing it from the .cpp. Other #include changes here look correct.

>> Source/WebInspectorUI/ChangeLog:39
>> +        * Configurations/Base.xcconfig:
> 
> I am assuming that protocol JSON files are stripped out somewhere for production builds. If so, has this been preserved?

Protocol JSON files only live in the Source Tree, so nothing needs to be done.

We generate Inspector.json (soon to be CombinedDomains.json) and InspectorBackendCommands.js to JavaScriptCore's DerivedSources/PrivateHeaders. The backend commands gets copied into WebInspectorUI, the combined JSON file doesn't go anywhere.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:56
>> +    // FIXME: InspectorBackendCommands.js should specify which agents should be activated by default.
> 
> Please add a webkit.org/b/NNNN link.

Chicken and egg problem. I'll file one!

>> Source/WebInspectorUI/UserInterface/Base/Main.js:58
>> +    if (InspectorFrontendHost.debuggableType() === "javascript")
> 
> Should we make this a symbolic constant somewhere?

I should just switch to using the debuggableType we define later on. I don't know why I didn't just use that one.

        this.debuggableType = InspectorFrontendHost.debuggableType() === "web" ? WebInspector.DebuggableType.Web : WebInspector.DebuggableType.JavaScript;

[Joseph Pecoraro]

Created attachment 239895 [details]
[PATCH] Try Bots

I don't know why the bots failed to apply the previous patch. Lets try this...

[Joseph Pecoraro]

> > Source/JavaScriptCore/ChangeLog:23
> > +        * inspector/InspectorProtocolTypesBase.h: Renamed from Source/JavaScriptCore/inspector/InspectorProtocolTypes.h.
> 
> What about calling the generated one InspectorProtocolObjects.h or InspectorDomainTypes.h? It seems weird to make this have a *Base suffix because it's included many places in order to use Inspector{Value,Object,Array} directly. It's only used for the base class types from the generated files.

I like this idea. I'll go with:

    Keep "InspectorProtocolTypes.h" for the base types.
    Generate "InspectorProtocolObjects.h" for the generated domain types.

[Joseph Pecoraro]

Well. I'm at a loss for why the bots can't apply these patches...

[Joseph Pecoraro]

Created attachment 239918 [details]
[PATCH] Try Bots

[Joseph Pecoraro]

Created attachment 239958 [details]
Patch

[Joseph Pecoraro]

Trying a webkit-patch upload to see if that helps with the EWS bots!

[Joseph Pecoraro]

Looks like another issue with tweaks to a Windows file:

    Source/WebCore/WebCore.vcxproj/copyForwardingHeaders.cmd.orig
    Source/WebCore/WebCore.vcxproj/copyForwardingHeaders.cmd.rej

I'll just remove that from my diff and try again.

[Joseph Pecoraro]

Created attachment 239959 [details]
[PATCH] Please Work on Bots

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_protocol_types_header.py:217:  [ProtocolTypesHeaderGenerator._generate_class_for_object_declaration] Instance of 'ProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/WebInspectorUI/ChangeLog:21:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 109 files


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

[Joseph Pecoraro]

Created attachment 239978 [details]
[PATCH] For Bots 3

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_protocol_types_header.py:217:  [ProtocolTypesHeaderGenerator._generate_class_for_object_declaration] Instance of 'ProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
Total errors found: 1 in 109 files


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

[Joseph Pecoraro]

Patch looks good on the bots! GTK couldn't update, and windows can never apply my patches.

[Joseph Pecoraro]

Note, I forgot to re-rebaseline inspector script tests for the InspectorProtocolTypes -> InspectorProtocolObjects rename. I have done it locally. I won't do another patch churn until a review though.

[Brian Burg]

Comment on attachment 239978 [details]
[PATCH] For Bots 3

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

Aside from renaming the combined json file, looks ready. Please be around when landing this in case windows does not work.

> Source/JavaScriptCore/CMakeLists.txt:1002
> +    OUTPUT ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/Inspector.json

Can you change this to CombinedProtocol.json in this patch?

[Joseph Pecoraro]

http://trac.webkit.org/changeset/174892

[Joseph Pecoraro]

Follow up Windows build fix:
http://trac.webkit.org/changeset/174894

[Joseph Pecoraro]

Follow up Windows build fix #2:
http://trac.webkit.org/changeset/174896

[Joseph Pecoraro]

Brent heroically fixed Windows this with two follow-ups:
https://trac.webkit.org/changeset/174900
https://trac.webkit.org/changeset/174904