Bug 130110 - Web Inspector: vended backend commands file should be generated as part of the build
Summary: Web Inspector: vended backend commands file should be generated as part of th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 130111 130846
  Show dependency treegraph
 
Reported: 2014-03-11 19:22 PDT by BJ Burg
Modified: 2014-03-27 10:28 PDT (History)
14 users (show)

See Also:


Attachments
wip (missing CMake, some GTK bundling magic) (66.13 KB, patch)
2014-03-13 19:43 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
the patch - missing Win/CMake bits for WebInspectorUI (72.54 KB, patch)
2014-03-14 13:27 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
patch round 2 (75.34 KB, patch)
2014-03-14 19:12 PDT, BJ 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 BJ Burg 2014-03-11 19:22:03 PDT
Files like InspectorWebBackendCommands.js are currently hand-generated but the update script just calls the code generator that already runs during the build. AFAIK, there's no reason to not tie it back into the Derived Sources build phase.

The WebInspectorUI project will then depend on WebCore so it can copy over the JS file to its framework.
Comment 1 Timothy Hatcher 2014-03-12 11:55:11 PDT
Making it depend on WebCore is fine. WebCore will just need to copy the commands into its bundle resources for the WebInspectorUI project to find and copy out.
Comment 2 Joseph Pecoraro 2014-03-13 13:22:26 PDT
(In reply to comment #1)
> Making it depend on WebCore is fine. WebCore will just need to copy the commands into its bundle resources for the WebInspectorUI project to find and copy out.

Probably not "bundle resources" but "private headers". If it went to the Resources directory, we might accidentally include the scripts in WebCore.framework/Resources. PrivateHeaders we already use for this kind of thing with code generator scripts, and they are not included in what we ship.
Comment 3 Timothy Hatcher 2014-03-13 13:56:24 PDT
Ah, yes that is what I meant.
Comment 4 BJ Burg 2014-03-13 19:43:59 PDT
Created attachment 226641 [details]
wip (missing CMake, some GTK bundling magic)
Comment 5 BJ Burg 2014-03-14 13:27:17 PDT
Created attachment 226758 [details]
the patch - missing Win/CMake bits for WebInspectorUI
Comment 6 Radar WebKit Bug Importer 2014-03-14 16:33:36 PDT
<rdar://problem/16332581>
Comment 7 BJ Burg 2014-03-14 19:12:08 PDT
Created attachment 226791 [details]
patch round 2
Comment 8 WebKit Commit Bot 2014-03-16 11:44:22 PDT
Comment on attachment 226791 [details]
patch round 2

Clearing flags on attachment: 226791

Committed r165704: <http://trac.webkit.org/changeset/165704>
Comment 9 WebKit Commit Bot 2014-03-16 11:44:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Joseph Pecoraro 2014-03-17 10:56:52 PDT
Comment on attachment 226791 [details]
patch round 2

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

Late comments. Good stuff!

> Source/WebCore/CMakeLists.txt:3023
> +    OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorWebBackendDispatchers.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorWebBackendDispatchers.h ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorWebFrontendDispatchers.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorWebFrontendDispatchers.h ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorWebTypeBuilders.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorWebTypeBuilders.h ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorWebBackendCommands.js

Oops! Good catch on these.

> Source/WebInspectorUI/GNUmakefile.am:37
>      $(WebInspectorUI)/Localizations/en.lproj/localizedStrings.js \
> +    $(shell ls $(GENSOURCES_JAVASCRIPTCORE)/InspectorJSBackendCommands.js) \
> +    $(shell ls $(GENSOURCES_WEBCORE)/InspectorWebBackendCommands.js) \

I don't think the "shell ls" is needed for individual files. This could be just like the line above it.
Comment 11 Brent Fulgham 2014-03-27 09:59:03 PDT
This change broke the Windows WebInspector. I'm putting together a fix.