Bug 136027 - Web Inspector: put feature flags for Inspector domains in the protocol specification
Summary: Web Inspector: put feature flags for Inspector domains in the protocol specif...
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: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-08-17 11:47 PDT by Brian Burg
Modified: 2014-08-26 15:38 PDT (History)
4 users (show)

See Also:


Attachments
Patch (39.44 KB, patch)
2014-08-17 13:45 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (39.58 KB, patch)
2014-08-24 18:02 PDT, Brian Burg
timothy: review+
burg: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-08-17 11:47:37 PDT
They are currently hardcoded into the generator. :|
Comment 1 Radar WebKit Bug Importer 2014-08-17 11:47:46 PDT
<rdar://problem/18044046>
Comment 2 Brian Burg 2014-08-17 13:45:11 PDT
Created attachment 236735 [details]
Patch
Comment 3 Joseph Pecoraro 2014-08-18 11:03:18 PDT
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.
Comment 4 Timothy Hatcher 2014-08-18 23:44:56 PDT
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.
Comment 5 Brian Burg 2014-08-24 17:46:12 PDT
(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.
Comment 6 Brian Burg 2014-08-24 18:02:33 PDT
Created attachment 237054 [details]
Patch
Comment 7 Joseph Pecoraro 2014-08-25 11:58:11 PDT
(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.
Comment 8 Timothy Hatcher 2014-08-26 14:11:08 PDT
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
Comment 9 Brian Burg 2014-08-26 15:38:46 PDT
Committed r172980: <http://trac.webkit.org/changeset/172980>