[webkit-reviews] review granted: [Bug 174484] Web Inspector: create Recording tab for displaying recordings : [Attachment 316702] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 31 19:26:04 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 174484: Web Inspector: create Recording tab for displaying recordings
https://bugs.webkit.org/show_bug.cgi?id=174484

Attachment 316702: Patch

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




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

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

Looks good to me. Because of the list of comments I'd like to see another
version / answers to some of my questions but I still think this is good enough
to land.

> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:26
> +WebInspector.loadDataFromFile = function(callback)

I expected you'd put save in here as well.

> Source/WebInspectorUI/UserInterface/Images/Recording.svg:1
> +<?xml version="1.0" encoding="utf-8"?>

File a bug on Images/gtk for an image here.

> Source/WebInspectorUI/UserInterface/Main.html:167
> +    <link rel="stylesheet" href="Views/ScrubberNavigationItem.css">

Style: alphabetical

> Source/WebInspectorUI/UserInterface/Main.html:684
> +    <script src="Views/ScrubberNavigationItem.js"></script>

Style: alphabetical

> Source/WebInspectorUI/UserInterface/Models/Recording.js:133
> +		       if (Array.isArray(data))

Should we throw an error if this type check fails?

> Source/WebInspectorUI/UserInterface/Models/Recording.js:138
> +			   let context =
document.createElement("canvas").getContext("2d");

Maybe we can keep 1 scratch canvas around instead of creating a new one every
time we swizzle a gradient / pattern? We could destroy it after swizzling. This
seems kinda wasteful, but maybe its not a problem.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:169
> +		       if (typeof data === "string")

Should we throw an error if this type check fails?

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:59
> +	   let forType = WebInspector.RecordingAction._functionNames[type];
> +	   if (!forType)

The `forType` name is poor. Lets go with `functionNames`.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:68
>      get name() { return this._resolvedName; }
>      get parameters() { return this._resolvedParameters; }

Style: These gutters are non-trivial since they return something not matching
their name. Make them non-inlined.

Alternatively you could flip `name` / `resolvedName` to `payloadName` and
`name` or something like that.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:88
> +	   let forType =
WebInspector.RecordingAction._parameterSwizzleTypeForTypeAtIndex[type];

Style: There must be better variable names in here.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:122
> +    functionForType(type)
> +    {
> +	   return WebInspector.RecordingAction.functionForType(type,
this._resolvedName);
> +    }
> +
> +    getterForType(type)
> +    {
> +	   return !this.functionForType(type) &&
!this._resolvedParameters.length;
> +    }
> +
> +    visualForType(type)
> +    {
> +	   let forType = WebInspector.RecordingAction._visualNames[type];
> +	   if (!forType)
> +	       return false;
> +
> +	   return forType.includes(this._resolvedName);
> +    }
> +
> +    initialStateModifiersForType(type)

These are very confusing. They are all returning booleans but their names
appear to get something.

Secondly these all take a `type` but presumably that doesn't make sense to
change.

---

How about each of these states gets initialized in swizzle (which I think of as
"process") and then they are just accessors:

    get isFunction
    get isGetter
    get visual
    get stateModifiers

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:146
> +// make sure that inputs are only considered valid if they conform to the
structure defined below.

It would be instructive to add a complete example:

    // For Example:
    // 
    // IDL:
    // 
    //	   void fill(optional CanvasWindingRule winding = "nonzero");
    //	   void fill(DOMPath path, optional CanvasWindingRule winding =
"nonzero");
    //	   void fill(float a, float b, float c, float d);
    // 
    // Swizzle Entries:
    // 
    //	   - For the 1 parameter version parameter at index 0 needs to be
swizzled as a string
    //	   - For the 2 parameter version the parameters need to be swizzled as
a Path and String
    //	   - For the fake 4 parameter version, numbers don't need to be
swizzled, so it is not included
    // 
    //	   "fill": {
    //	       1: {0: String}
    //	       2: {0: Path2D, 1: String}
    //	   }

Something like this goes a much longer way than the opaque paragraph you have.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:153
> +WebInspector.RecordingAction._parameterSwizzleTypeForTypeAtIndex = {
> +    [WebInspector.Recording.Type.Canvas2D]: {
> +	   "clip": {
> +	       1: {0: WebInspector.Recording.Swizzle.String},
> +	       2: {0: WebInspector.Recording.Swizzle.Path2D, 1:
WebInspector.Recording.Swizzle.String},
> +	   },

Since this is code that is being evaluated, you can simplify it a bit if you
think it would enhance readability:

    {
	let {Array, CanvasStyle, Element, Image, ImageData, Path2D, String,
Invalid} = WebInspector.Recording.Swizzle;
	WebInspector.RecordingAction._parameterSwizzleTypeForTypeAtIndex = {
	    "clip" {
	      1: {0: String},
	      2: {0, Path2D, 1: String},
	    }
	    ...
	};
    }

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:261
> +WebInspector.RecordingAction._functionNames = {

Style: Wrap or one-per-line.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:104
> +	       if (error)
> +		   return;
> +

If we don't ever expect an error we can just do:

    console.assert(!error);

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:44
> +		   classNames.push(typeof representedObject.parameters[0]);

This is a weird way to infer the type. It seems to work heuristically well here
but maybe not always.

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:83
> +		       if (representedObject.parameters[i] instanceof
CanvasGradient)
> +			   parameterElement.textContent =
WebInspector.unlocalizedString("CanvasGradient");
> +		       else if (representedObject.parameters[i] instanceof
CanvasPattern)
> +			   parameterElement.textContent =
WebInspector.unlocalizedString("CanvasPattern");

How about just Gradient / Pattern? I don't know if the explicit type adds much
value here.

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:90
> +		   if (!isFunction)
> +		       break;

Is there a non-function version that has more than 1 parameter?

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:102
> +	   super(classNames, titleFragment, subtitle, representedObject);

This must be the latest call to super I've ever seen. This leave a lot of room
for mistakes (any use of `this` above would result in an exception). It
sometimes helps to move code above into a static method so it is both clear it
is not using `this` and the work before `super()` is visually small.

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:141
> +	   contextMenu.appendItem(WebInspector.UIString("Copy Action"), () => {
> +	       InspectorFrontendHost.copyText("context." +
this._mainTitleElement.textContent + ";");
> +	   });

Seems fine to keep, but I'm not sure I would have expected this.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:100
> +

Style: Extra newline

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:110
> +	   let snapshotIndex = Math.floor(index /
WebInspector.RecordingContentView._snapshotInterval);

Typically we just capitalize constants like this:

    WebInspector.RecordingContentView.SnapshotInterval = ...;

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:125
> +		   if (!(name in snapshot.context))
> +		       continue;

What case does this help with, or is it just a precaution?

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:160
> +	       while (snapshot.index && actions[snapshot.index].name !==
"beginPath")
> +		   --snapshot.index;

In the future this will not be required once you can get and apply an entire
path?

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:169
> +	       let lastSnapshot = snapshotIndex;

Style: `lastSnapshot` => `lastSnapshotIndex`.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:234
> +	   for (let snapshot of this._snapshots) {
> +	       if (snapshot)
> +		   snapshot.element.remove();
> +	   }

Maybe just `this._previewContainer.removeChildren()`?

>
Source/WebInspectorUI/UserInterface/Views/RecordingNavigationSidebarPanel.css:9
3
> +.sidebar > .panel.navigation.recording > .content .action.attribute.getter >
.icon {
> +    content: url(../Images/Eye.svg);
> +}

I still think this UI will be weird but we can tweak later.

>
Source/WebInspectorUI/UserInterface/Views/RecordingNavigationSidebarPanel.js:58
> +	   this._recording = recording;

Style: Add a newline after this.

> Source/WebInspectorUI/UserInterface/Views/RecordingTabContentView.js:161
> +	      
WebInspector.Recording.synthesizeError(WebInspector.UIString("unsupported
version"));

What does this look like? I'd expect a sentence like "Unsupported version." or
better yet "Invalid file format."

> Source/WebInspectorUI/UserInterface/Views/ScrubberNavigationItem.js:48
> +    set disabled(flag)

you might as well include a getter for this.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:233
> +	   if (!window.CanvasAgent)
> +	       return;

This would prevent the experimental stylesheet editor down below for older
versions of iOS that don't support canvas. This should not be an early return.

> LayoutTests/inspector/unit-tests/number-utilities.html:125
> +		   InspectorTest.expectEqual(Number.countDigits(-1 * num),
digits, `${-1 * num} should have ${digits} digits`);

`-1 * num` => `-num`. You can use the unary - instead of multiplication!


More information about the webkit-reviews mailing list