[webkit-reviews] review denied: [Bug 37394] inspector/timeline-network-resource.html fails when run twice : [Attachment 77609] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 37394: inspector/timeline-network-resource.html fails when run twice
https://bugs.webkit.org/show_bug.cgi?id=37394

Attachment 77609: Patch
https://bugs.webkit.org/attachment.cgi?id=77609&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77609&action=review

> WebCore/inspector/InspectorController.cpp:803
> +    request.setReportLoadTiming(true);

This call should be made in case of no front-end as well. Otherwise WebTiming
breaks. Condition should be: enable load timing in case of (isMainResource ||
m_frontend).

> WebCore/inspector/InspectorController.cpp:807
> +    request.setReportRawHeaders(true);

raw headers should be collected only for the case of m_frontend.

> WebCore/inspector/InspectorController.h:280
> +    void willSendRequest(unsigned long identifier, ResourceRequest&, const
ResourceResponse& redirectResponse);

Please make sure that order in cpp file matches the order in header.

> WebCore/inspector/InspectorInstrumentation.cpp:380
> +    ic->ensureSettingsLoaded();

Could you add a comment on why this call is important to make here?

> WebCore/inspector/InspectorInstrumentation.cpp:427
> +InspectorInstrumentationCookie
InspectorInstrumentation::willReceiveResourceResponseImpl(InspectorController*
inspectorController, unsigned long identifier, DocumentLoader* loader, const
ResourceResponse& response)

Should willReceiveResponse call didReceiveResponse in inspector?

> WebCore/inspector/InspectorInstrumentation.cpp:437
> +	   resourceAgent->didReceiveResponse(identifier, loader, response);
> +    inspectorController->didReceiveResponse(identifier, response);

I feel that we should inline addConsoleMessage here instead. No need to make
inspector controller involved.

> WebCore/inspector/InspectorInstrumentation.cpp:463
> +    ic->didFailLoading(identifier, error);

ditto

> WebCore/inspector/InspectorInstrumentation.cpp:472
> +    ic->resourceRetrievedByXMLHttpRequest(url, sendURL, sendLineNumber);

Almost ditto, but depends on inspector state, I know...

> WebCore/inspector/InspectorInstrumentation.h:458
> +    if (!frame)

Are you not using inspectorControllerForFrame in order to get called in case of
no front-ends? You should then be explicit and document it here.

> WebCore/inspector/InspectorInstrumentation.h:475
> +#if ENABLE(INSPECTOR)

We only inline methods and delegate to Impl in case we do a fast
InspectorController fetch. I don't see how this inline helps.

> WebCore/inspector/InspectorInstrumentation.h:483
> +    didLoadResourceFromMemoryCacheImpl(page->inspectorController(), loader,
resource);

ditto

> WebCore/inspector/front-end/NetworkManager.js:92
> +	       this.mainResourceIdentifier = identifier;

Now mainResourceIdentifier might conflict mainResource defined in
didCommitLoadForFrame. You should instead mark resource as main here and assign
WebInspector.mainResource to it (and nuke didCommitLoadForFrame).

> WebCore/loader/ResourceLoadNotifier.cpp:69
> +    InspectorInstrumentationCookie cookie =
InspectorInstrumentation::willReceiveResourceResponse(loader->frameLoader()->fr
ame(), loader->identifier(), loader->documentLoader(), r);

We are now going to receive dupes / miss notifications. Please do not modify
original inspector controller resource reporting in this change.

> WebCore/loader/ResourceLoadNotifier.cpp:-139
> -	   page->inspectorController()->didReceiveResponse(identifier, loader,
r);

This clearly is a regression - you should call InspectorInstrumentation here.

> WebCore/loader/ResourceLoadNotifier.cpp:158
> +	   InspectorInstrumentationCookie cookie =
InspectorInstrumentation::willReceiveResourceResponse(loader->frameLoader()->fr
ame(), identifier, loader, response);

You should not make calls to inspector controller in the new places.