[webkit-reviews] review granted: [Bug 188385] Web Inspector: XHR content sometimes shows as error even though load succeeded : [Attachment 346744] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 188385: Web Inspector: XHR content sometimes shows as error even though
load succeeded
https://bugs.webkit.org/show_bug.cgi?id=188385

Attachment 346744: [PATCH] Proposed Fix

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




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

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

r=me

Just out of curiosity, does this do anything for that longstanding bug about
base64 content from XHRs not being previewable?

> LayoutTests/http/tests/inspector/network/xhr-response-body.html:7
> +function xhrGet(url, asyncFlag=true) {

Style: do we prefer to add spaces around the `=` for optionals?

> Source/WebCore/inspector/NetworkResourcesData.h:127
> +    ResourceData const* maybeAddResourceData(const String& requestId, const
char* data, size_t dataLength);

Style: Is this a new style "rule"?  IIRC, this is the same as `const
ResourceData*` since `const` applies to the left 🤔

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:493
> +	   // For non-sync data would be transferred from a cached resource,
but sync XHRs may not have those.

This reads awkwardly.  "Sync XHRs might not have a cached resource, while
non-sync XHRs usually transfer the data over on completion." or something like
that.