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

[bugzilla-daemon at webkit.org]

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

Attachment 379267: [PATCH] Proposed Fix

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




--- Comment #13 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 379267
  --> https://bugs.webkit.org/attachment.cgi?id=379267
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:428
>>		return this._url;
> 
> NIT: I'd invert this and have the early-return be the
`URL.createObjectURL(blobContent)`, so then the variable declaration is closer
to us usage.

I don't buy the "variable declaration is closer" since this is effectively a
bail. But I'll make the change and move the comment.

>> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:32
>> +	    this._originalRevision = new WI.SourceCodeRevision(this, null,
false, null);
> 
> Can we make these into `const` variables, so it's clear at the callsite what
they are?

I can just remove them, they were optional.

>> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:62
>> +	updateRevisionContent(content, base64Encoded, mimeType, blobContent)
> 
> It seems like everything other than `content` is optional, so can we move
them into an optional POD parameter?
> ```
>     updateRevisionContent(content, {base64Encoded, mimeType, blobContent} =
{})
> ```

Done.

Aside: What do you mean by an "optional POD parameter"? Do you mean an
"optional object parameter"?

>> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:81
>> +	    console.assert(this._targetElement);
> 
> How does the `this._targetElement` relate to `this.element`?	Should it be a
parent/child/sibling?
> 
> It seems like this class really should be more of a controller, not a view,
as it really just listens for events on a target and adds/removes styles
depending on them.

Indeed, DropZone is kind of both, hence the delegate. The DropZone view is
displayed based on external conditions, but once appearing it has its own event
handlers, over the content.

>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:43
>> +	    if (WI.NetworkManager.supportsLocalResourceOverrides()) {
> 
> At this point, should we move this to `WI.ResourceContentView`?  It looks
like the only other subclasses are `WI.FontResourceContentView` and
`WI.GeneralResourceContentView` (which I think never actually gets
created/used).

Yeah, I'll test for Fonts and if they work I'll try moving it up. This was in
my future plans but I can consider it now.

>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:52
>> +		    this._dropZoneView = new WI.DropZoneView(this);
> 
> Should we also allow drag/drop for text resource content?

My initial reaction was no. But that is not a strong opinion. Most text editors
inline text content or a file path when a file is dropped, not replace
contents. So I thought it was a bit weird for text content.

>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:189
>> +		this._imageElement.src = dataURL;
> 
> NIT: I'm not actually sure if this matters, but since image decoding is
async, perhaps we should move this above the other stuff.

Do you mean to encourage it to happen faster?

>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:126
>> +	    this.removeAllSubviews();
> 
> I think this is probably fine, given that this is always called only
immediately after we get content (regardless of error).  Should we have any
assertion about what the subviews are at this point, like that the only subview
(if any) is the drop zone?

I did this just in case. If there were any subviews they wouldn't have been
getting removed. I think I ran into that in earlier revisions, but kept this
because it just makes sense.