Bug 130110

Summary:

Web Inspector: vended backend commands file should be generated as part of the build

Product:

WebKit

Reporter:

Blaze Burg <bburg>

Component:

Web Inspector

Assignee:

Blaze Burg <bburg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

berto, bfulgham, bunhere, cdumez, commit-queue, graouts, gyuyoung.kim, joepeck, mrobinson, rakuco, sergio, timothy, webkit-bug-importer, zan

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

130111, 130846

Attachments:

Description	Flags
wip (missing CMake, some GTK bundling magic)	none
the patch - missing Win/CMake bits for WebInspectorUI	none
patch round 2	none

Blaze Burg

Reported 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.

Attachments
wip (missing CMake, some GTK bundling magic) (66.13 KB, patch) 2014-03-13 19:43 PDT, Blaze 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, Blaze Burg	no flags	Details Formatted Diff Diff
patch round 2 (75.34 KB, patch) 2014-03-14 19:12 PDT, Blaze Burg	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Timothy Hatcher

Comment 1 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.

Joseph Pecoraro

Comment 2 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.

Timothy Hatcher

Comment 3 2014-03-13 13:56:24 PDT

Ah, yes that is what I meant.

Blaze Burg

Comment 4 2014-03-13 19:43:59 PDT

Created attachment 226641 [details] wip (missing CMake, some GTK bundling magic)

Blaze Burg

Comment 5 2014-03-14 13:27:17 PDT

Created attachment 226758 [details] the patch - missing Win/CMake bits for WebInspectorUI

Radar WebKit Bug Importer

Comment 6 2014-03-14 16:33:36 PDT

<rdar://problem/16332581>

Blaze Burg

Comment 7 2014-03-14 19:12:08 PDT

Created attachment 226791 [details] patch round 2

WebKit Commit Bot

Comment 8 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>

WebKit Commit Bot

Comment 9 2014-03-16 11:44:28 PDT

All reviewed patches have been landed. Closing bug.

Joseph Pecoraro

Comment 10 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.

Brent Fulgham

Comment 11 2014-03-27 09:59:03 PDT

This change broke the Windows WebInspector. I'm putting together a fix.

Note You need to log in before you can comment on or make changes to this bug.