CORS: Update the redirection status in Inspector Network tab for CORS requests.

CORS: Update the redirection status in Inspector Network tab for CORS requests.

In the Inspector Network tab, 'Status' column shows '(cancelled)' and
'Type' shows 'pending' when CORS request is redirected. The status
is not updated when redirection is successful or fails due to access
control checks.

Added changes to update the response in Inspector when the CORS request
is redirected for both success and failure cases. Made changes to
log appropriate message to console when redirection fails
due to access control checks.

BUG=279203
R=abarth@chromium.org, pfeldman@chromium.org, bbudge@google.com

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156915

[ancilgeorge, 2013-08-27 11:59:00]

Requesting review.

[pfeldman, 2013-08-27 14:21:32]

https://codereview.chromium.org/23582002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorResourceAgent.cpp (right):

https://codereview.chromium.org/23582002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorResourceAgent.cpp:403: double finishTime = 0.0;
Is this copy-paste from above? Why not to call the one above?

https://codereview.chromium.org/23582002/diff/1/Source/core/loader/DocumentTh...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/23582002/diff/1/Source/core/loader/DocumentTh...
Source/core/loader/DocumentThreadableLoader.cpp:211:
InspectorInstrumentationCookie cookie =
InspectorInstrumentation::willReceiveResourceResponse(m_document->frame(),
resource->identifier(), redirectResponse);
You should not need 2 instrumenting signals here, especially that you introduce
the one below to cover this very use case. didReceiveCORSRedirectResponse would
suffice.

[ancilgeorge, 2013-08-28 08:14:35]

Did the changes as per the inputs. Please let me if any further changes are
required.

https://codereview.chromium.org/23582002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorResourceAgent.cpp (right):

https://codereview.chromium.org/23582002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorResourceAgent.cpp:403: double finishTime = 0.0;
On 2013/08/27 14:21:33, pfeldman wrote:
> Is this copy-paste from above? Why not to call the one above?
Yes. Changed it.

https://codereview.chromium.org/23582002/diff/1/Source/core/loader/DocumentTh...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/23582002/diff/1/Source/core/loader/DocumentTh...
Source/core/loader/DocumentThreadableLoader.cpp:211:
InspectorInstrumentationCookie cookie =
InspectorInstrumentation::willReceiveResourceResponse(m_document->frame(),
resource->identifier(), redirectResponse);
On 2013/08/27 14:21:33, pfeldman wrote:
> You should not need 2 instrumenting signals here, especially that you
introduce
> the one below to cover this very use case. didReceiveCORSRedirectResponse
would
> suffice.

Done.

[eustas, 2013-08-28 09:43:24]

https://codereview.chromium.org/23582002/diff/6001/Source/core/loader/Documen...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/23582002/diff/6001/Source/core/loader/Documen...
Source/core/loader/DocumentThreadableLoader.cpp:210:
InspectorInstrumentation::didReceiveCORSRedirectResponse(m_document->frame(),
resource->identifier(), m_document->frame()->loader()->documentLoader(),
redirectResponse, 0, resource->loadFinishTime());
I'm not sure that it is legal to use resource->loadFinishTime() here.

As I understand, execution path is 
WebURLLoaderImpl::Context::OnReceivedRedirect -> ResourceLoader::willSendRequest
-> RawResource::willSendRequest -> DocumentThreadableLoader::redirectReceived

And Resource::finish is not called along this path.

As a consequence, we should explicitly pass either 0.0, or current monotonic
time.

[ancilgeorge, 2013-08-28 14:05:10]

https://codereview.chromium.org/23582002/diff/6001/Source/core/loader/Documen...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/23582002/diff/6001/Source/core/loader/Documen...
Source/core/loader/DocumentThreadableLoader.cpp:210:
InspectorInstrumentation::didReceiveCORSRedirectResponse(m_document->frame(),
resource->identifier(), m_document->frame()->loader()->documentLoader(),
redirectResponse, 0, resource->loadFinishTime());
On 2013/08/28 09:43:25, eustas.ru wrote:
> I'm not sure that it is legal to use resource->loadFinishTime() here.
> 
> As I understand, execution path is 
> WebURLLoaderImpl::Context::OnReceivedRedirect ->
ResourceLoader::willSendRequest
> -> RawResource::willSendRequest -> DocumentThreadableLoader::redirectReceived
> 
> And Resource::finish is not called along this path.
> 
> As a consequence, we should explicitly pass either 0.0, or current monotonic
> time.
I agree that finish time will not be updated in the current path. However
m_loadFinishTime is initialized to 0. I am fine with change you have
recommended. Or will it better to remove the monotonicFinishTime parameter and
use 0.0 in InspectorResourceAgent::didReceiveCORSRedirectResponse while calling
InspectorResourceAgent::didFinishLoading?

[ancilgeorge, 2013-08-28 14:45:23]

Removed the monotonicFinishTime parameter. Please let me know if any further
changes are required. Thanks.

[pfeldman, 2013-08-28 15:22:47]

inspector lgtm. +mkwst for CORS error messages.

[Mike West, 2013-08-28 15:33:56]

https://codereview.chromium.org/23582002/diff/8001/Source/core/loader/Documen...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/23582002/diff/8001/Source/core/loader/Documen...
Source/core/loader/DocumentThreadableLoader.cpp:220:
accessControlErrorDescription = "Cross Origin Requests with preflight cannot be
redirected";
A bit more detail would be nice. How about something like: "The request was
redirected to XXX, which is disallowed for cross-origin requests that require
preflight."?

[ancilgeorge, 2013-08-29 07:09:37]

Made changes as per the comments. Also updated the error description for scheme
and userinfo check.

https://codereview.chromium.org/23582002/diff/8001/Source/core/loader/Documen...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/23582002/diff/8001/Source/core/loader/Documen...
Source/core/loader/DocumentThreadableLoader.cpp:220:
accessControlErrorDescription = "Cross Origin Requests with preflight cannot be
redirected";
On 2013/08/28 15:33:56, Mike West wrote:
> A bit more detail would be nice. How about something like: "The request was
> redirected to XXX, which is disallowed for cross-origin requests that require
> preflight."?

Done.

[Mike West, 2013-08-29 07:29:50]

On 2013/08/29 07:09:37, ancilgeorge wrote:
> Made changes as per the comments. Also updated the error description for
scheme
> and userinfo check.
> 
>
https://codereview.chromium.org/23582002/diff/8001/Source/core/loader/Documen...
> File Source/core/loader/DocumentThreadableLoader.cpp (right):
> 
>
https://codereview.chromium.org/23582002/diff/8001/Source/core/loader/Documen...
> Source/core/loader/DocumentThreadableLoader.cpp:220:
> accessControlErrorDescription = "Cross Origin Requests with preflight cannot
be
> redirected";
> On 2013/08/28 15:33:56, Mike West wrote:
> > A bit more detail would be nice. How about something like: "The request was
> > redirected to XXX, which is disallowed for cross-origin requests that
require
> > preflight."?
> 
> Done.

Error message LGTM, thanks!

[commit-bot: I haz the power, 2013-08-29 08:00:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ancilgeorge@samsung.com/23582002/13001

[ancilgeorge, 2013-08-29 08:09:58]

Can I go ahead and do a commit?

https://codereview.chromium.org/23582002/diff/34001/Source/core/loader/Docume...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/23582002/diff/34001/Source/core/loader/Docume...
Source/core/loader/DocumentThreadableLoader.cpp:517: } // namespace WebCore
nit. Removed the extra enter.

[commit-bot: I haz the power, 2013-08-29 09:20:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ancilgeorge@samsung.com/23582002/34001

[commit-bot: I haz the power, 2013-08-29 10:38:48]

Change committed as 156915