Summary: | Web Inspector: put feature flags for Inspector domains in the protocol specification | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Burg <burg> | ||||||
Component: | Web Inspector | Assignee: | Brian Burg <burg> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | graouts, joepeck, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Brian Burg
2014-08-17 11:47:37 PDT
Created attachment 236735 [details]
Patch
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 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. (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. Created attachment 237054 [details]
Patch
(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 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 Committed r172980: <http://trac.webkit.org/changeset/172980> |