[webkit-reviews] review granted: [Bug 193262] Web Inspector: expose Audit and Recording versions to the frontend : [Attachment 359000] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 193262: Web Inspector: expose Audit and Recording versions to the frontend
https://bugs.webkit.org/show_bug.cgi?id=193262

Attachment 359000: Patch

https://bugs.webkit.org/attachment.cgi?id=359000&action=review




--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 359000
  --> https://bugs.webkit.org/attachment.cgi?id=359000
Patch

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