[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