[webkit-reviews] review denied: [Bug 63712] Web Inspector: Preflight OPTIONS requests are not shown on network panel for asynchronous XHRs. : [Attachment 99308] Suggested patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 63712: Web Inspector: Preflight OPTIONS requests are not shown on network
panel for asynchronous XHRs.
https://bugs.webkit.org/show_bug.cgi?id=63712

Attachment 99308: Suggested patch
https://bugs.webkit.org/attachment.cgi?id=99308&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.