Bug 130111 – Web Inspector: generated backend commands should reflect build system ENABLE settings

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

[Blaze Burg]

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.

[Timothy Hatcher]

Using the C preprocessor to filter things out of the JSON files and prevent generation sounds fine to me.

[Blaze Burg]

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

[Blaze Burg]

Created attachment 226759 [details]
the patch

[Radar WebKit Bug Importer]

<rdar://problem/16332612>

[Joseph Pecoraro]

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?

[Blaze Burg]

Created attachment 226792 [details]
patch round 2

[Philippe Normand]

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

[Philippe Normand]

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

[Blaze Burg]

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

[Blaze Burg]

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

[Blaze Burg]

Created attachment 226846 [details]
patch round 3

[Blaze Burg]

Ooops, forgot that we can't guard generating WEB_REPLAY on mac builds because Xcode doesn't like that. Will remove that before landing.

[Blaze Burg]

Committed r165714: <http://trac.webkit.org/changeset/165714>