[webkit-reviews] review denied: [Bug 177606] Web Inspector: Show recordings in CanvasTabContentView : [Attachment 324326] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 20 14:16:48 PDT 2017


Devin Rousso <webkit at devinrousso.com> has denied Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 177606: Web Inspector: Show recordings in CanvasTabContentView
https://bugs.webkit.org/show_bug.cgi?id=177606

Attachment 324326: Patch

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




--- Comment #3 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 324326
  --> https://bugs.webkit.org/attachment.cgi?id=324326
Patch

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

r-, because there are bugs with the scrubber.  Also, in the previous scrubber,
the user was still able to scrub to the Initial State or the final action that
was recorded.  I think we should still provide this functionality, especially
allowing the user to go to the Initial State, to help understand what happens
before the first visual action or after the last one.

Should we remove WI.RecordingTabContentView in its entirety?  With this patch,
you can still reach it via the New Tab tab.
Along those lines, should we remove Canvas from Resources and simplify any
related logic?

> Source/WebInspectorUI/UserInterface/Models/Recording.js:36
> +	   this._visualActionIndices = [];

I personally prefer "indexes" over "indices", but that's just me ¯\_(ツ)_/¯

> Source/WebInspectorUI/UserInterface/Models/Recording.js:178
> +    get visualActionIndices() { return this._visualActionIndices; }

NIT: you can move this right below `get data()`.  The only reason I added an
extra newline between `get data()` and `get actions()` is because `get
actions()` returns a promise, but I don't think that was warranted.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:48
> +	   this._popover = null;
> +
> +	   this._recordingTreeOutline = new WI.TreeOutline;
> +	   this._recordingTreeOutline.disclosureButtons = false;
> +	  
this._recordingTreeOutline.addEventListener(WI.TreeOutline.Event.SelectionDidCh
ange, this._recordingTreeOutlineSelectionDidChange, this);

Instead of using a popover, I feel like it would be simpler to just have a
<select> with the different recording names.  They all seem to use the same
icon anyways, so the only differentiating factor is the name.  Doing it with a
<select> would also allow the user to select a recording with a single mouse
click, instead of two.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:89
> +	       console.error("Canvas content not available.",
this.representedObject);

I'd switch this with the if statement and early bail in a negation.

    this._pendingContent = content;
    if (!this._pendingContent) {
	console.error("Canvas content not available.", this.representedObject);
	return;
    }

    this.needsLayout();

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:260
> +	   if (!recording)

Can you combine this with the previous if statement, or would you rather have
it separate for readability?  Either is fine by me.

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:138
> +    position: relative;
> +    width: 16px;
> +    top: 4px;
> +    display: inline-block;
> +    content: url(../Images/Recording.svg);
> +    -webkit-padding-end: 4px;

NIT: I'd reorder these as such.

    display: inline-block;
    position: relative;
    top: 4px;
    width: 16px;
    -webkit-padding-end: 4px;
    content: url(../Images/Recording.svg);

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:156
> +    border-radius: 3px;
> +    background-color: var(--selected-background-color);

NIT: I'd switch the order of these two properties.

    background-color: var(--selected-background-color);
    border-radius: 3px;

> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:247
> +	   let extensionStart = filename.lastIndexOf(".");
> +	   if (extensionStart !== -1)
> +	       filename = filename.substring(0, extensionStart);

We should create a utility for this.

> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:259
> +	   let selectedTreeElement =
this.navigationSidebarPanel.contentTreeOutline.selectedTreeElement;

Is this different from `event.data.selectedElement`?  If not, I think it's
easier to read to use that instead.

> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:276
> +	       console.assert(canvasTreeElement, "Missing tree element for
canvas.", canvas);

This variable doesn't exist.  I think you mean `recording.source` instead of
`canvas`.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css:36
> +    color: var(--text-color-gray-dark);
> +    margin-bottom: 10px;

NIT: I'd reorder these.

    margin-bottom: 10px;
    color: var(--text-color-gray-dark);

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css:50
> +    max-width: 400px;
> +    width: 100%;

NIT: I'd reorder these.

    width: 100%;
    max-width: 400px;

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css:55
> +    border: 1px solid var(--border-color);
> +    border-radius: 4px;
> +    background-color: white;

NIT: I'd reorder these.

    background-color: white;
    border: 1px solid var(--border-color);
    border-radius: 4px;

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css:67
> +    border-radius: 1px;
> +    background-color: var(--border-color);

NIT: I'd reorder these.

    background-color: var(--border-color);
    border-radius: 1px;

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:109
>	       if (index < 0 || index >= actions.length || index ===
this._index)

You can remove the last check here since you add it above (104).

    if (index < 0 || index >= actions.length)

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:168
> +	       titleElement.textContent = WI.UIString("Imported Recording");

Instead of "Imported Recording", do you want to just display the name of the
file that was imported?

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:178
> +	   this._sliderContainer.classList.add("hidden");

Merge this with line (171).

    this._sliderContainer.className = "slider-container hidden";

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:191
> +	       this._sliderElement.max =
this.representedObject.visualActionIndices.length;

If you only want to let the user scrub visual actions, you either need to
subtract one from the length or set the min to 1.

    this._sliderElement.max = this.representedObject.visualActionIndices.length
- 1;

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:192
> +	       this._updateSliderValue(0);

This function takes no arguments.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:521
> +	   let visualActionIndex = 0;

This should be the index of the first visual action.

    let visualActionIndex = visualActionIndices[0];

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:525
> +		   visualActionIndex = 0;

Instead of setting this value back to 0, I would expect the "end" value to be
X/X (e.g. 18/18), not 0/X (e.g 0/18).

    visualActionIndex = visualActionIndices.lastValue;

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:529
> +	   this._sliderElement.value = visualActionIndex;
> +	   this._sliderValueElement.textContent = WI.UIString("%d of
%d").format(visualActionIndex + 1, visualActionIndices.length + 1);

You should find the recording index of the action in the list of visual
indexes.

    this._sliderElement.value = visualActionIndices.indexOf(visualActionIndex);
    this._sliderValueElement.textContent = WI.UIString("%d of
%d").format(visualActionIndices.indexOf(visualActionIndex) + 1,
visualActionIndices.length - 1);

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:549
> +	   console.assert(!isNaN(newIndex));

I think this check is unnecessary, since we are using a range input.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:550
> +	   this.updateActionIndex(newIndex);

This value is a visual action index, not a recording action index.

    let indexForVisualAction =
this.representedObject.visualActionIndices[newIndex];
    this.updateActionIndex();


More information about the webkit-reviews mailing list