[webkit-reviews] review granted: [Bug 195311] Web Inspector: Canvas: export recording as HTML : [Attachment 363668] Patch

Wed Mar 6 22:16:44 PST 2019

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 195311: Web Inspector: Canvas: export recording as HTML
https://bugs.webkit.org/show_bug.cgi?id=195311

Attachment 363668: Patch

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

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

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

This is awesome!

r=me

> Source/WebInspectorUI/UserInterface/Models/Recording.js:498
> +	       return JSON.stringify(value);

Nice! This will protect against injection!

> Source/WebInspectorUI/UserInterface/Models/Recording.js:514
> +	   lines.push(`<!DOCTYPE html>`);
> +	   lines.push(`<body>`);
> +	   lines.push(`<style>`);
> +	   lines.push(`    body {`);
> +	   lines.push(`        margin: 0;`);
> +	   lines.push(`    }`);
> +	   lines.push(`    canvas {`);
> +	   lines.push(`        max-width: calc(100% - 40px);`);
> +	   lines.push(`        max-height: calc(100% - 40px);`);
> +	   lines.push(`        padding: 20px;`);
> +	   lines.push(`    }`);
> +	   lines.push(`</style>`);
> +	   lines.push(`<script>`);
> +	   lines.push(`"use strict";`);

Template strings allow newlines, so this could be cleaned up a bit, that said I
like when things line up nicely.

Lets also make an effort to make this more standard and include a nice title
for browser tabs. Maybe:

    <html>
    <head>
	<title>${this._displayName}</title>
    </head>
    <body>
    ...

Though you'd want to escape the displayName if it can be user defined to avoid
script injection.

For example if someone did `console.record(ctx, {name:
"</title><script>alert(1);</script>});`

> Source/WebInspectorUI/UserInterface/Models/Recording.js:546
> +		   if (name === "setPath" || name === "currentX" || name ===
"currentY")
> +		       continue;

"getPath" is also an InspectorAddition that may need to be skipped.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:566
> +	   lines.push(` 	   console.record(context, {name:
"${this._displayName}"});`);

If the displayName is user defined you should probably protect against script
injection. You could JSON.stringify it here:

    lines.push(` console.record(context, {name:
${JSON.stringify(this._displayName)});`);

For example if someone did `console.record(ctx, {name: "\"}); alert(1);
({foo:\");`.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:584
> +		   if (!action.valid)
> +		       contextString = `// ` + contextString;

Clever! This could move above the call string, next to the other contextString
setup.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:656
> +		   lines.push(` 	  
createImageBitmap(image).then(function (imageBitmap) {`);

Style: It is unusual to have the space after `function` here.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:702
> +		       lines.push(`rebuildDOMMatrix(${index},
${JSON.stringify(data)});`)

How about just passing this as an array:

    let data = [object.a, object.b, object.c, object.d, object.e, object.f];

Then rebuildDOMMatrix just takes the data:

    function rebuildDOMMatrix(key, data) {
	objects[key] = new DOMMatrix(data)
    }

Also interesting, DOMMatrix has a toString() which the constructor takes, so
you could alternatively do:

    let data = object.toString();

But I like the array version better.