[webkit-reviews] review canceled: [Bug 177641] Web Inspector: Include Beacon and Ping requests in Network tab : [Attachment 322245] [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 177641: Web Inspector: Include Beacon and Ping requests in Network tab
https://bugs.webkit.org/show_bug.cgi?id=177641

Attachment 322245: [PATCH] Proposed Fix

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




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

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

I'll try to figure out what happened on the bot.

>>> Source/WebCore/loader/PingLoader.cpp:208
>>> +	 platformStrategies()->loaderStrategy()->startPingLoad(frame, request,
WTFMove(originalRequestHeaders), options, [framePtr = WTFMove(framePtr),
identifier = identifier] (const ResourceError& error, const ResourceResponse&
response) {
>> 
>> Is there no way to simplify the `identifier = identifier`?
> 
> identifier would be enough.
> 
> also [protectedFrame = makeRef(frame)]

Excellent! I saw code that looked like and forgot to go back to using it.

>>> Source/WebCore/loader/cache/CachedResource.cpp:280
>>> +		 platformStrategies()->loaderStrategy()->startPingLoad(frame,
request, *m_originalRequestHeaders, m_options, [this, protectedThis =
WTFMove(protectedThis), framePtr = framePtr, identifier = identifier] (const
ResourceError& error, const ResourceResponse& response) {
>> 
>> You `WTFMove(framePtr)` in PingLoader.cpp.  Is there a reason you don't
here?
>> 
>> Also, you don't use `protectedThis`.  Is it necessary?
> 
> Same comments as above.

protectedThis (not added by my patch) is necessary to keep `this` alive since
it is used in the lambda called at some later time (`this->error`).

>> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:505
>> +		if (type === "Document" && frame.mainResource.url === url &&
frame.loaderIdentifier === loaderIdentifier)
> 
> Can you compare to `PageAgent.ResourceType.Document` instead?

Sure. I think ultimately we want this code to go away and instead use
Page.frameNavigated / frameScheduledLoad and all those.