[webkit-reviews] review granted: [Bug 202016] Web Inspector: Local Resource Overrides: UI for overriding image and font resource content : [Attachment 380679] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 202016: Web Inspector: Local Resource Overrides: UI for overriding image
and font resource content
https://bugs.webkit.org/show_bug.cgi?id=202016

Attachment 380679: [PATCH] Proposed Fix

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




--- Comment #18 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 380679
  --> https://bugs.webkit.org/attachment.cgi?id=380679
[PATCH] Proposed Fix

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

r=me, awesome work!

> LayoutTests/inspector/unit-tests/mimetype-utilities.html:13
> +	       InspectorTest.expectEqual(WI.fileExtensionForFilename(null),
null, `File extension for null filename should be null.`);

NIT: `InspectorTest.expectNull`

> Source/WebInspectorUI/UserInterface/Base/BlobUtilities.js:26
> +WI.BlobUtilities = class BlobUtilities {

NICE =D

> Source/WebInspectorUI/UserInterface/Base/BlobUtilities.js:39
> +	   var byteCharacters = atob(base64Data);

Optional: while we're moving this code, why not update the `var` too =D

> Source/WebInspectorUI/UserInterface/Base/BlobUtilities.js:50
> +		   bytes[i] = byteCharacters[offset].charCodeAt(0);

Optional: you could also use `offset - begin` instead of `i`

> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:174
> +    static async readDataURL(fileOrList, callback)

This doesn't appear to be used.

> Source/WebInspectorUI/UserInterface/Base/MIMETypeUtilities.js:39
> +}

Style: missing semicolon

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:-1702
> -function textToBlob(text, mimeType)

It looks like you missed:
 - http/tests/inspector/network/fetch-response-body.html
 - http/tests/inspector/network/xhr-response-body.html

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:-1707
> -function blobAsText(blob, callback)

Ditto

> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:61
> +    updateRevisionContent(content, {base64Encoded, mimeType, blobContent} =
{})

NIT: I'd probably call this `update` or `updateContent` (given that this class
already has "revision" in the name), but I understand if you'd rather keep it
this way for better code searching

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.css:40
> +    text-align: center;

NIT: here's the order I'd use so the styles order from content
inside/foreground to outside/background:
```
    font-size: 60px;
    text-align: center;
    color: white;
    text-shadow: 0 1px black;
    background-color: hsla(0, 0%, 50%, 0.5);
    border: 3px dashed hsla(0, 0%, 100%, 0.9);
    -webkit-backdrop-filter: blur(10px);
```

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.css:51
> +	   background-color: hsla(0, 0%, 30%, 0.5);

Ditto:
```
    color: hsl(0, 0%, 80%);
    text-shadow: 0 1px hsl(0, 0%, 20%);
    background-color: hsla(0, 0%, 30%, 0.5);
    border: 3px dashed hsla(0, 0%, 65%, 0.9);
```

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:109
> +	   if (this._activelyHandlingDrag)

We should assert that this view is attached to the DOM tree, so that this UI is
shown.
```
    console.assert(this.isAttached);
```

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.css:31
>  

might as well remove this too =D

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:37
> +	   if (this.showingLocalResourceOverride)
> +	       this._dropZoneView = new WI.DropZoneView(this, {text:
WI.UIString("Drop Font")});

NIT: should we do this inside an `initialLayout`?

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:78
> +    {

I know this was just moved ,but please add `super.shown();`

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:89
> +    }

I know this was just moved ,but please add `super.hidden();`

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:99
> +    }

I know this was just moved ,but please add `super.closed();`

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:130
> +	       let dataURL = fileReader.result;

Should we inline this?

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:181
> +	   let lines = WI.FontResourceContentView.PreviewLines;

Ditto (this could also be `const`)

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.css:31
> +

Can we remove these extra newlines?

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:44
> +	   if (this.showingLocalResourceOverride)
> +	       this._dropZoneView = new WI.DropZoneView(this, {text:
WI.UIString("Drop Image")});

Ditto (FontResourceContentView.js:36)

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:52
> +

I'd personally remove these newlines, but I'm fine either way

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:82
> +	   this._draggingInternalImageElement = false;

Should we define this in the constructor instead?

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:129
> +	   // Do not appear if the drag is the current image inside this view.
> +	   if (this._draggingInternalImageElement)

Good thinking!

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:154
> +	       let {base64, data, mimeType} = parseDataURL(dataURL);

Should we also attempt to derive the `mimeType` if `FileReader` doesn't give a
MIME type for some reason, or do you think that that's unlikely?

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:95
> +

Ditto (ImageResourceContentView.js:52)

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:120
> +    {

We should add a `// Implemented by subclasses if needed.`

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:121
> +	   return undefined;

Can we use `null` instead?

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:78
>  

Ditto (ImageResourceContentView.js:52)