Bug 130111

Summary: Web Inspector: generated backend commands should reflect build system ENABLE settings
Product: WebKit Reporter: BJ Burg <bburg>
Component: Web InspectorAssignee: BJ Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bunhere, commit-queue, graouts, gyuyoung.kim, joepeck, mrobinson, rakuco, sergio, timothy, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 130110    
Bug Blocks:    
Attachments:
Description Flags
the patch
none
patch round 2
none
patch round 3 timothy: review+

Description BJ Burg 2014-03-11 19:27:31 PDT
Currently the commands that we generate (InspectorFooBackendCommands.js) include all domains, without checking whether their feature flags are actually enabled in the build system. This is fine for C++ code where we can just wrap the relevant agents and type builders in ENABLE(FOO_FEATURE).

This blocks frontend feature detection for ENABLE(WEB_REPLAY), which is not enabled by default. We want to support it being either on or off, but right now the frontend will always think it's available, whether or not the ReplayAgent backend has been compiled or not.
Comment 1 Timothy Hatcher 2014-03-12 11:56:06 PDT
Using the C preprocessor to filter things out of the JSON files and prevent generation sounds fine to me.
Comment 2 BJ Burg 2014-03-13 11:47:49 PDT
Joe suggested an even easier solution: conditionally add protocol spec files inside DerivedSources.make. I am going to wait to submit this until 130110 because it has no effect presently (the ruby script to update doesn't take into account ENABLE flags).
Comment 3 BJ Burg 2014-03-14 13:27:59 PDT
Created attachment 226759 [details]
the patch
Comment 4 Radar WebKit Bug Importer 2014-03-14 16:36:17 PDT
<rdar://problem/16332612>
Comment 5 Joseph Pecoraro 2014-03-14 17:31:09 PDT
Comment on attachment 226759 [details]
the patch

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

> Source/WebCore/CMakeLists.txt:765
>      inspector/protocol/IndexedDB.json

Same for IndexedDB as Databases?

> Source/WebCore/GNUmakefile.am:374
> +EnabledInspectorDomains: force
> +	echo '$(WebCore_INSPECTOR_DOMAINS)' | cmp -s - $@ || echo '$(WebCore_INSPECTOR_DOMAINS)' > $@
> +
>  DerivedSources/WebCore/InspectorWeb.json: $(INSPECTOR_SCRIPTS_DIR)/generate-combined-inspector-json.py $(WebCore_INSPECTOR_DOMAINS)

Doesn't InspectorWeb.json need to have "EnabledInspectorDomains" as a dependency?
Comment 6 BJ Burg 2014-03-14 19:12:47 PDT
Created attachment 226792 [details]
patch round 2
Comment 7 Philippe Normand 2014-03-16 04:45:47 PDT
Comment on attachment 226792 [details]
patch round 2

This breaks the generation of InspectorWeb.json on GTK at least. And it screws up the following builds on the GTK EWS, at least..
Comment 8 Philippe Normand 2014-03-16 04:59:21 PDT
Comment on attachment 226792 [details]
patch round 2

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

> Source/WebCore/CMakeLists.txt:2604
> +        inspector/protocol/Database.json

This line needs to be ${WEBCORE_DIR}/inspector/protocol/Database.json
Comment 9 BJ Burg 2014-03-16 11:21:47 PDT
(In reply to comment #5)
> (From update of attachment 226759 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226759&action=review
> 
> > Source/WebCore/CMakeLists.txt:765
> >      inspector/protocol/IndexedDB.json
> 
> Same for IndexedDB as Databases?

nothing else is conditional on ENABLE_INDEXED_DATABASE so I didn't bother.

> 
> > Source/WebCore/GNUmakefile.am:374
> > +EnabledInspectorDomains: force
> > +	echo '$(WebCore_INSPECTOR_DOMAINS)' | cmp -s - $@ || echo '$(WebCore_INSPECTOR_DOMAINS)' > $@
> > +
> >  DerivedSources/WebCore/InspectorWeb.json: $(INSPECTOR_SCRIPTS_DIR)/generate-combined-inspector-json.py $(WebCore_INSPECTOR_DOMAINS)
> 
> Doesn't InspectorWeb.json need to have "EnabledInspectorDomains" as a dependency?

Yes.
Comment 10 BJ Burg 2014-03-16 11:22:21 PDT
(In reply to comment #8)
> (From update of attachment 226792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226792&action=review
> 
> > Source/WebCore/CMakeLists.txt:2604
> > +        inspector/protocol/Database.json
> 
> This line needs to be ${WEBCORE_DIR}/inspector/protocol/Database.json

Oops, I should have changed that one too. Thanks.
Comment 11 BJ Burg 2014-03-16 11:24:09 PDT
Created attachment 226846 [details]
patch round 3
Comment 12 BJ Burg 2014-03-16 12:12:55 PDT
Ooops, forgot that we can't guard generating WEB_REPLAY on mac builds because Xcode doesn't like that. Will remove that before landing.
Comment 13 BJ Burg 2014-03-16 16:09:56 PDT
Committed r165714: <http://trac.webkit.org/changeset/165714>