[webkit-reviews] review granted: [Bug 141389] Web Inspector: content views for resources loaded through XHR do not reflect declared mime-type : [Attachment 328403] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Joseph Pecoraro <joepeck at webkit.org>'s
request for review:
Bug 141389: Web Inspector: content views for resources loaded through XHR do
not reflect declared mime-type
https://bugs.webkit.org/show_bug.cgi?id=141389

Attachment 328403: [PATCH] Proposed Fix

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




--- Comment #10 from Brian Burg <bburg at apple.com> ---
Comment on attachment 328403
  --> https://bugs.webkit.org/attachment.cgi?id=328403
[PATCH] Proposed Fix

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

r=me, I am so glad you finally dug into this. It's one of my biggest pet
peeves.

> LayoutTests/inspector/page/searchInResources.html:27
> +    InspectorTest.debug();

Please remove.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:872
> +	   || MIMETypeRegistry::isSupportedNonImageMIMEType(mimeType);

I'm curious what will happen with video content and video metadata. This is
something we can revisit later since we do a poor job anyway and it's not a
current focus.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:875
> +RefPtr<TextResourceDecoder> InspectorNetworkAgent::createTextDecoder(const
String& mimeType, const String& textEncodingName)

Nit: this should return a Ref.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:898
> +    if (InspectorNetworkAgent::cachedResourceContent(cachedResource,
&result, &base64Encoded)) {

Eww, we should clean up the parameter types. Can you make this by reference?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:906
> +bool InspectorNetworkAgent::cachedResourceContent(CachedResource& resource,
String* result, bool* base64Encoded)

Ditto. Looks like there are only two use sites.

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:492
> +	   if (auto* resource = cachedResource(frame, kurl)) {

kurl 👴🏻

> Source/WebInspectorUI/ChangeLog:26
> +	   Better handle a no content cases.

-a

> Source/WebInspectorUI/ChangeLog:32
> +	   This is done by looking at the ResourceType, and MIME Type.

-,