WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
193262
Web Inspector: expose Audit and Recording versions to the frontend
https://bugs.webkit.org/show_bug.cgi?id=193262
Summary
Web Inspector: expose Audit and Recording versions to the frontend
Devin Rousso
Reported
2019-01-08 15:43:26 PST
In order to have versioning support for Audits, we need a place where we can show version information (e.g. "Recording System Version 1"). These version values are split into two "types": - Interface: the WebInspector frontend's ability to display the data (e.g. whether audits can show metadata information) - System: the inspected target's ability to generate/process/execute the data (e.g. whether audits are able to use the injected `AUDIT` object)
Attachments
Patch
(25.82 KB, patch)
2019-01-08 16:21 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
[Image] After Patch is applied
(636.32 KB, image/png)
2019-01-08 16:22 PST
,
Devin Rousso
no flags
Details
[Patch] Command Version
(18.82 KB, patch)
2019-01-12 20:10 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
[Patch] Protocol Version
(72.71 KB, patch)
2019-01-15 20:35 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
[Patch] Protocol Version
(71.96 KB, patch)
2019-01-15 21:00 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
[Patch] Protocol Version
(72.16 KB, patch)
2019-01-22 15:40 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(70.90 KB, patch)
2019-01-22 19:38 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-01-08 15:44:27 PST
<
rdar://problem/47130684
>
Devin Rousso
Comment 2
2019-01-08 16:21:47 PST
Created
attachment 358648
[details]
Patch Not sure if "Interface" and "System" are really the right `UIString`s for this, but I figured I'd put the code up and then worry about it instead of the other way around 😅
Devin Rousso
Comment 3
2019-01-08 16:22:00 PST
Created
attachment 358649
[details]
[Image] After Patch is applied
Devin Rousso
Comment 4
2019-01-12 20:10:39 PST
Created
attachment 359000
[details]
[Patch] Command Version Removed the UI parts of this patch to focus more on the functionality. Added a requirement that the system version be kept in sync with the frontend version (enforced by a test) so that when doing a "compatibility" check we only need to do compare the listed version against the minimum of the system and frontend versions: - if system < version < frontend (newer frontend connected to older backend), then we lack required functionality - if frontend < version < system (older frontend connected to newer backend), then we are able to run the audit, but may be unable to display certain result values - if version < system <=> frontend, the test is fully supported - if system <=> frontend < version, the test is not supported
Joseph Pecoraro
Comment 5
2019-01-14 19:34:39 PST
Comment on
attachment 359000
[details]
[Patch] Command Version View in context:
https://bugs.webkit.org/attachment.cgi?id=359000&action=review
r=me
> Source/JavaScriptCore/inspector/protocol/Audit.json:9 > + "types": [ > + { > + "id": "Version", > + "type": "number" > + } > + ],
This should give this a description. For example in this case the importance behind the Version number is that the tests that are run have access to a global `WebInspectorAudit` API object. Were that to change this Version would need to change to indicate a new type of API surface. Side note: It might even be nice to eventually turn this into a constant right in the protocol file. { id: "Version", type: "number", value: 1, description: "..." }. The tricky bit would be $ref: "Version" would need to know to fill the value at generation time.
> Source/JavaScriptCore/inspector/protocol/Recording.json:9 > + { > + "id": "Version", > + "type": "number" > + },
This should also use a description. The importance of this version number is that the backend generates the JSON recording format, and if the JSON needs to change in any material way the version number would need to change.
> Source/WebInspectorUI/UserInterface/Models/Recording.js:55 > - if (isNaN(payload.version) || payload.version <= 0) > + if (isNaN(payload.version) || payload.version <= 0 || payload.version > WI.Recording.Version) > return null;
Maybe this should include a warning reason dumped to the console. I think there is already a warning dumped to the console when an import fails, but knowing it is because the version number might be useful. Nobody will run into this if the version doesn't change, but when it does...
Devin Rousso
Comment 6
2019-01-15 20:31:49 PST
Comment on
attachment 359000
[details]
[Patch] Command Version View in context:
https://bugs.webkit.org/attachment.cgi?id=359000&action=review
>> Source/JavaScriptCore/inspector/protocol/Audit.json:9 >> + ], > > This should give this a description. For example in this case the importance behind the Version number is that the tests that are run have access to a global `WebInspectorAudit` API object. Were that to change this Version would need to change to indicate a new type of API surface. > > Side note: It might even be nice to eventually turn this into a constant right in the protocol file. { id: "Version", type: "number", value: 1, description: "..." }. The tricky bit would be $ref: "Version" would need to know to fill the value at generation time.
I was actually initially playing around with adding a constant to the protocol, but I was more focused on building a UI at that point.
>> Source/WebInspectorUI/UserInterface/Models/Recording.js:55 >> return null; > > Maybe this should include a warning reason dumped to the console. I think there is already a warning dumped to the console when an import fails, but knowing it is because the version number might be useful. Nobody will run into this if the version doesn't change, but when it does...
The default error is "unsupported version", so it should cover this. We should make the error messages a bit better (more comprehensive). The same is true for the various audit model objects (default error is "invalid JSON").
Devin Rousso
Comment 7
2019-01-15 20:32:23 PST
Comment on
attachment 359000
[details]
[Patch] Command Version View in context:
https://bugs.webkit.org/attachment.cgi?id=359000&action=review
>>> Source/WebInspectorUI/UserInterface/Models/Recording.js:55 >>> return null; >> >> Maybe this should include a warning reason dumped to the console. I think there is already a warning dumped to the console when an import fails, but knowing it is because the version number might be useful. Nobody will run into this if the version doesn't change, but when it does... > > The default error is "unsupported version", so it should cover this. We should make the error messages a bit better (more comprehensive). > > The same is true for the various audit model objects (default error is "invalid JSON").
<
https://webkit.org/b/193476
>
Devin Rousso
Comment 8
2019-01-15 20:35:45 PST
Created
attachment 359252
[details]
[Patch] Protocol Version
EWS Watchlist
Comment 9
2019-01-15 20:36:42 PST
Comment hidden (obsolete)
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`) This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
EWS Watchlist
Comment 10
2019-01-15 20:36:55 PST
Comment hidden (obsolete)
Attachment 359252
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:86: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:74: [JSBackendCommandsGenerator.generate_domain] Instance of 'JSBackendCommandsGenerator' has no 'version_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:97: [CppProtocolTypesHeaderGenerator._generate_versions] Instance of 'CppProtocolTypesHeaderGenerator' has no 'version_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:110: [CppProtocolTypesHeaderGenerator._generate_versions] Instance of 'CppProtocolTypesHeaderGenerator' has no 'wrap_with_guard_for_domain' member [pylint/E1101] [5] Total errors found: 4 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Devin Rousso
Comment 11
2019-01-15 21:00:04 PST
Created
attachment 359254
[details]
[Patch] Protocol Version
EWS Watchlist
Comment 12
2019-01-15 21:03:31 PST
Comment hidden (obsolete)
Attachment 359254
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:86: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:74: [JSBackendCommandsGenerator.generate_domain] Instance of 'JSBackendCommandsGenerator' has no 'version_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:97: [CppProtocolTypesHeaderGenerator._generate_versions] Instance of 'CppProtocolTypesHeaderGenerator' has no 'version_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:110: [CppProtocolTypesHeaderGenerator._generate_versions] Instance of 'CppProtocolTypesHeaderGenerator' has no 'wrap_with_guard_for_domain' member [pylint/E1101] [5] Total errors found: 4 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 13
2019-01-22 14:52:52 PST
Comment on
attachment 359254
[details]
[Patch] Protocol Version View in context:
https://bugs.webkit.org/attachment.cgi?id=359254&action=review
r- because I'd like to see another version of this patch. I'd suggest: • Limiting to just `int` • In the frontend making it `Agent.version`. • Potential codegen error if a command is named `version` given it would conflict with `Agent.version`, but this is not important as I don't expect this to ever happen.
> Source/WebInspectorUI/ChangeLog:12 > + Add "Interface" version values.
Why are there quotes around Interface?
> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:107 > + if isinstance(version, (int, long, float)): > + domain_lines.append('static const auto VERSION = %s;' % version) > + elif isinstance(version, (str, unicode)): > + domain_lines.append('static const auto VERSION = "%s";' % version)
Lets use explicit types. `int` or `unsigned` for the version number and `const char*`.
> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:114 > + else:
Style: Drop the else
> Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:91 > + if isinstance(version, (int, long, float)): > + lines.append('InspectorBackend.registerVersion("%(domain)s", %(version)s);' % version_args) > + elif isinstance(version, (str, unicode)): > + lines.append('InspectorBackend.registerVersion("%(domain)s", "%(version)s");' % version_args)
This doesn't appear in the code-get test result of InspectorBackendCommands.js: Source/JavaScriptCore/inspector/scripts/tests/generic/expected/version.json-result Looks like you will need to update `should_generate_domain` to say YES if there is a version.
> Source/JavaScriptCore/inspector/scripts/codegen/models.py:378 > + if not isinstance(json['version'], (int, float, str, unicode)): > + raise ParseException("Malformed domain specification: version is not a number or string")
You allowed `long` in other places but only `int` here. Why not just standardize on int?
> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:145 > + var agent = this._agentForDomain(domainName);
Style: `let`.
> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:146 > + agent.VERSION = version;
Why not `agent.version`? The capital letters do not add anything.
> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:249 > + this._version = undefined;
This is unused.
Joseph Pecoraro
Comment 14
2019-01-22 14:54:10 PST
Comment on
attachment 359254
[details]
[Patch] Protocol Version View in context:
https://bugs.webkit.org/attachment.cgi?id=359254&action=review
>> Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:91 >> + lines.append('InspectorBackend.registerVersion("%(domain)s", "%(version)s");' % version_args) > > This doesn't appear in the code-get test result of InspectorBackendCommands.js: > Source/JavaScriptCore/inspector/scripts/tests/generic/expected/version.json-result > > Looks like you will need to update `should_generate_domain` to say YES if there is a version.
Err, I meant "in the codegen test result"
Devin Rousso
Comment 15
2019-01-22 15:40:26 PST
Created
attachment 359790
[details]
[Patch] Protocol Version
EWS Watchlist
Comment 16
2019-01-22 15:43:27 PST
Comment hidden (obsolete)
Attachment 359790
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:86: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:55: [JSBackendCommandsGenerator.should_generate_domain] Instance of 'JSBackendCommandsGenerator' has no 'version_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:55: [JSBackendCommandsGenerator.should_generate_domain] Instance of 'JSBackendCommandsGenerator' has no 'commands_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:55: [JSBackendCommandsGenerator.should_generate_domain] Instance of 'JSBackendCommandsGenerator' has no 'events_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:74: [JSBackendCommandsGenerator.generate_domain] Instance of 'JSBackendCommandsGenerator' has no 'version_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:97: [CppProtocolTypesHeaderGenerator._generate_versions] Instance of 'CppProtocolTypesHeaderGenerator' has no 'version_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:108: [CppProtocolTypesHeaderGenerator._generate_versions] Instance of 'CppProtocolTypesHeaderGenerator' has no 'wrap_with_guard_for_domain' member [pylint/E1101] [5] Total errors found: 7 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 17
2019-01-22 17:00:59 PST
Comment on
attachment 359790
[details]
[Patch] Protocol Version View in context:
https://bugs.webkit.org/attachment.cgi?id=359790&action=review
r=me
> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:306 > + if (agent.VERSION !== undefined) > + clone.VERSION = agent.VERSION;
Is this necessary? var foo = {version:1}; var bar = Object.create(foo); assert(bar.version === 1); // true
> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:331 > + if (agent.VERSION !== undefined) > + clone.VERSION = agent.VERSION;
Ditto.
Devin Rousso
Comment 18
2019-01-22 19:38:26 PST
Created
attachment 359837
[details]
Patch
EWS Watchlist
Comment 19
2019-01-22 19:40:10 PST
Comment hidden (obsolete)
Attachment 359837
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:86: whitespace before '}' [pep8/E202] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:55: [JSBackendCommandsGenerator.should_generate_domain] Instance of 'JSBackendCommandsGenerator' has no 'version_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:55: [JSBackendCommandsGenerator.should_generate_domain] Instance of 'JSBackendCommandsGenerator' has no 'commands_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:55: [JSBackendCommandsGenerator.should_generate_domain] Instance of 'JSBackendCommandsGenerator' has no 'events_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_js_backend_commands.py:74: [JSBackendCommandsGenerator.generate_domain] Instance of 'JSBackendCommandsGenerator' has no 'version_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:97: [CppProtocolTypesHeaderGenerator._generate_versions] Instance of 'CppProtocolTypesHeaderGenerator' has no 'version_for_domain' member [pylint/E1101] [5] ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:108: [CppProtocolTypesHeaderGenerator._generate_versions] Instance of 'CppProtocolTypesHeaderGenerator' has no 'wrap_with_guard_for_domain' member [pylint/E1101] [5] Total errors found: 7 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 20
2019-01-22 20:17:12 PST
Comment on
attachment 359837
[details]
Patch Clearing flags on attachment: 359837 Committed
r240318
: <
https://trac.webkit.org/changeset/240318
>
WebKit Commit Bot
Comment 21
2019-01-22 20:17:14 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug