Bug 136027 – Web Inspector: put feature flags for Inspector domains in the protocol specification

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

[Brian Burg]

They are currently hardcoded into the generator. :|

[Radar WebKit Bug Importer]

<rdar://problem/18044046>

[Brian Burg]

Created attachment 236735 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 236735 [details]
Patch

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

> Source/JavaScriptCore/inspector/scripts/tests/expected/events-with-optional-parameters.json-result:363
>  }
>  
> -#if ENABLE(SQL_DATABASE)
>  namespace Database {
>  /* Database error. */

It looks like these tests are regressing.

> Source/WebCore/inspector/protocol/Database.json:3
> +    "feature_guard": "ENABLE(SQL_DATABASE)",

Maybe this should be "webkit_feature_guard", or an entire "webkit" object:

    "webkit": {
        "feature_guard": "ENABLE(SQL_DATABASE)"
    }

I like the fact that that json files are relatively pure right now.

[Timothy Hatcher]

Comment on attachment 236735 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Add 'feature_guard' settings to wrap domains with ENABLE macros.

featureGuard! Death to the underscore.

>> Source/WebCore/inspector/protocol/Database.json:3
>> +    "feature_guard": "ENABLE(SQL_DATABASE)",
> 
> Maybe this should be "webkit_feature_guard", or an entire "webkit" object:
> 
>     "webkit": {
>         "feature_guard": "ENABLE(SQL_DATABASE)"
>     }
> 
> I like the fact that that json files are relatively pure right now.

I think it is fine to have it be at the top level with no WebKit qualification. It is in the WebKit project.

[Brian Burg]

(In reply to comment #3)
> (From update of attachment 236735 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236735&action=review
> 
> > Source/JavaScriptCore/inspector/scripts/tests/expected/events-with-optional-parameters.json-result:363
> >  }
> >  
> > -#if ENABLE(SQL_DATABASE)
> >  namespace Database {
> >  /* Database error. */
> 
> It looks like these tests are regressing.

They are rebaselined, before the 'Database' domain would always include the guards, but now they don't because the feature guard isn't included in all of those tests. I don't think we need to test it in all those different test cases since the guards are always emitted in the generated files.

> > Source/WebCore/inspector/protocol/Database.json:3
> > +    "feature_guard": "ENABLE(SQL_DATABASE)",
> 
> Maybe this should be "webkit_feature_guard", or an entire "webkit" object:
> 
>     "webkit": {
>         "feature_guard": "ENABLE(SQL_DATABASE)"
>     }
> 
> I like the fact that that json files are relatively pure right now.

I am ambivalent.

[Brian Burg]

Created attachment 237054 [details]
Patch

[Joseph Pecoraro]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 236735 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=236735&action=review
> > 
> > > Source/JavaScriptCore/inspector/scripts/tests/expected/events-with-optional-parameters.json-result:363
> > >  }
> > >  
> > > -#if ENABLE(SQL_DATABASE)
> > >  namespace Database {
> > >  /* Database error. */
> > 
> > It looks like these tests are regressing.
> 
> They are rebaselined, before the 'Database' domain would always include the guards, but now they don't because the feature guard isn't included in all of those tests. I don't think we need to test it in all those different test cases since the guards are always emitted in the generated files.

Oh, hah. Got it.

[Timothy Hatcher]

Comment on attachment 237054 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Add 'feature_guard' settings to wrap domains with ENABLE macros.

featureGuard

[Brian Burg]

Committed r172980: <http://trac.webkit.org/changeset/172980>