Bug 63712 – Web Inspector: Preflight OPTIONS requests are not shown on network panel for asynchronous XHRs.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

Preflight OPTIONS requests are not shown on network panel for asynchronous XHRs.

[Vsevolod Vlasov]

Chromium bug: http://crbug.com/71361

[Vsevolod Vlasov]

Created attachment 99308 [details]
Suggested patch

[Alexey Proskuryakov]

Comment on attachment 99308 [details]
Suggested patch

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

r- for boolean arguments, and for lack of ChangeLog explanation. I may have further comments once there is a better ChangeLog, and I actually understand the fix.

> Source/WebCore/ChangeLog:7
> +        Web Inspector: Preflight OPTIONS requests are not shown on network panel for asynchronous XHRs.
> +        https://bugs.webkit.org/show_bug.cgi?id=63712
> +

ChangeLog needs an explanation of what the fix is. I can't easily understand that from the patch.

> Source/WebCore/loader/MainResourceLoader.cpp:94
> +        frameLoader()->notifier()->didFailToLoad(true, this, error);

This is the reason why we prefer named enum values for such arguments - "true" and "false" at call sites don't tell you what they mean.

Also, why is this the first argument? Is it somehow the most important one?

> Source/WebCore/loader/ResourceLoadNotifier.cpp:127
> +        if (!request.isNull() && oldRequestURL != request.url().string().impl())

This is nonsense code (I see that you are not adding it, but nonetheless). One should never compare StringImpls by pointer, that doesn't mean anything.

[Vsevolod Vlasov]

Created attachment 99462 [details]
Patch with different approach

[Vsevolod Vlasov]

After discussion on IRC I rewrote this patch.
I just added InspectorInstrumentation calls to DocumentThreadableLoader.

[Alexey Proskuryakov]

Comment on attachment 99462 [details]
Patch with different approach

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

Makes sense to me. Would you be willing to file a bug and possibly make a patch for incorrect StringImpl usage?

> LayoutTests/http/tests/inspector/network-preflight-options.html:46
> +        //if (resource.requestMethod === "POST" && !/\?deny=yes/.test(resource.url))
> +            //InspectorTest.completeTest();

Commented out code?

> Source/WebCore/loader/DocumentThreadableLoader.cpp:266
> +    // m_actualRequest might have been reset to null in preflightFailure();
> +    if (m_preflightRequestIdentifier)
> +        InspectorInstrumentation::didFinishLoading(m_document->frame(), m_document->frame()->loader()->documentLoader(), m_preflightRequestIdentifier, finishTime);

How does this comment relate to code?

> Source/WebCore/loader/DocumentThreadableLoader.cpp:362
> +            // Because willSendRequest only gets called during redirects, we initialize
> +            // the identifier and the first willSendRequest here.

No need to wrap this line - there are longer lines just below it.

[Alexey Proskuryakov]

Comment on attachment 99462 [details]
Patch with different approach

Still no useful information in ChangeLog. You should add it.

[Vsevolod Vlasov]

> Makes sense to me. Would you be willing to file a bug and possibly make a patch for incorrect StringImpl usage?
I filed https://bugs.webkit.org/show_bug.cgi?id=63873 and will upload patch soon.

[Vsevolod Vlasov]

Created attachment 99572 [details]
Patchi with fixes

[WebKit Review Bot]

Comment on attachment 99572 [details]
Patchi with fixes

Clearing flags on attachment: 99572

Committed r90340: <http://trac.webkit.org/changeset/90340>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.