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
<rdar://problem/18668000>
Created attachment 239883 [details] [PATCH] Proposed Fix I expect some back and forth with bots.
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.
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;
Created attachment 239895 [details] [PATCH] Try Bots I don't know why the bots failed to apply the previous patch. Lets try this...
> > 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.
Well. I'm at a loss for why the bots can't apply these patches...
Created attachment 239918 [details] [PATCH] Try Bots
Created attachment 239958 [details] Patch
Trying a webkit-patch upload to see if that helps with the EWS bots!
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.
Created attachment 239959 [details] [PATCH] Please Work on Bots
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.
Created attachment 239978 [details] [PATCH] For Bots 3
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.
Patch looks good on the bots! GTK couldn't update, and windows can never apply my patches.
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.
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?
http://trac.webkit.org/changeset/174892
Follow up Windows build fix: http://trac.webkit.org/changeset/174894
Follow up Windows build fix #2: http://trac.webkit.org/changeset/174896
Brent heroically fixed Windows this with two follow-ups: https://trac.webkit.org/changeset/174900 https://trac.webkit.org/changeset/174904