Bug 63712 - Web Inspector: Preflight OPTIONS requests are not shown on network panel for asynchronous XHRs.
Summary: Web Inspector: Preflight OPTIONS requests are not shown on network panel for ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-30 06:34 PDT by Vsevolod Vlasov
Modified: 2011-07-03 08:58 PDT (History)
13 users (show)

See Also:


Attachments
Suggested patch (26.05 KB, patch)
2011-06-30 08:32 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch with different approach (11.79 KB, patch)
2011-07-01 05:28 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patchi with fixes (11.64 KB, patch)
2011-07-03 04:47 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-06-30 06:34:57 PDT
Preflight OPTIONS requests are not shown on network panel for asynchronous XHRs.
Comment 1 Vsevolod Vlasov 2011-06-30 07:24:13 PDT
Chromium bug: http://crbug.com/71361
Comment 2 Vsevolod Vlasov 2011-06-30 08:32:07 PDT
Created attachment 99308 [details]
Suggested patch
Comment 3 Alexey Proskuryakov 2011-06-30 14:09:12 PDT
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.
Comment 4 Vsevolod Vlasov 2011-07-01 05:28:47 PDT
Created attachment 99462 [details]
Patch with different approach
Comment 5 Vsevolod Vlasov 2011-07-01 05:31:04 PDT
After discussion on IRC I rewrote this patch.
I just added InspectorInstrumentation calls to DocumentThreadableLoader.
Comment 6 Alexey Proskuryakov 2011-07-01 08:36:04 PDT
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.
Comment 7 Alexey Proskuryakov 2011-07-01 10:17:38 PDT
Comment on attachment 99462 [details]
Patch with different approach

Still no useful information in ChangeLog. You should add it.
Comment 8 Vsevolod Vlasov 2011-07-03 04:45:59 PDT
> 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.
Comment 9 Vsevolod Vlasov 2011-07-03 04:47:43 PDT
Created attachment 99572 [details]
Patchi with fixes
Comment 10 WebKit Review Bot 2011-07-03 08:58:51 PDT
Comment on attachment 99572 [details]
Patchi with fixes

Clearing flags on attachment: 99572

Committed r90340: <http://trac.webkit.org/changeset/90340>
Comment 11 WebKit Review Bot 2011-07-03 08:58:56 PDT
All reviewed patches have been landed.  Closing bug.