DevTools: add support for logging fetch requests when XHR logging is enabled

DevTools: add support for logging fetch requests when XHR logging is enabled

- generalize support for XHR & EventSource in InspectorResourceAgent;
- add Fetch and EventSource resource types;
- instrument fetch manager and handle it in resource agent similar to XHR & co;
- add support for arguments in InspectorTest.invokePageFunctionAsync()
- promote page methods to top of inspector-test.js to make line numbers less
    volatile in test expectations;


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

[caseq, 2015-07-29 01:50:52]

caseq@chromium.org changed reviewers:
+ dgozman@chromium.org, pfeldman@chromium.org

[dgozman, 2015-07-29 15:26:07]

https://codereview.chromium.org/1259393002/diff/1/LayoutTests/http/tests/insp...
File LayoutTests/http/tests/inspector/network/network-fetch-expected.txt
(right):

https://codereview.chromium.org/1259393002/diff/1/LayoutTests/http/tests/insp...
LayoutTests/http/tests/inspector/network/network-fetch-expected.txt:1: Tests
fetch network resource type and content.
Can we also have a test for EventSource type and content?

https://codereview.chromium.org/1259393002/diff/1/LayoutTests/http/tests/insp...
LayoutTests/http/tests/inspector/network/network-fetch-expected.txt:4:
resource.type: other
I think test should dump "Fetch" type somewhere.

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

https://codereview.chromium.org/1259393002/diff/1/Source/core/inspector/Inspe...
Source/core/inspector/InspectorResourceAgent.cpp:535: m_pendingRequest =
nullptr;
I think didFinishXHR may come asynchronously after delay and |m_pendingRequest|
may be the other one (or even fetch request).

https://codereview.chromium.org/1259393002/diff/1/Source/core/inspector/Inspe...
Source/core/inspector/InspectorResourceAgent.cpp:582: void
InspectorResourceAgent::completeFetch(ThreadableLoaderClient* client)
Inline this?

https://codereview.chromium.org/1259393002/diff/1/Source/core/inspector/Inspe...
Source/core/inspector/InspectorResourceAgent.cpp:754: ASSERT(!m_pendingRequest);
Should we clear |m_knownRequestIdMap| here?

[caseq, 2015-07-30 21:50:15]

ptal

[caseq, 2015-07-30 21:51:39]

On 2015/07/29 15:26:07, dgozman wrote:
>
https://codereview.chromium.org/1259393002/diff/1/LayoutTests/http/tests/insp...
> File LayoutTests/http/tests/inspector/network/network-fetch-expected.txt
> (right):
> 
>
https://codereview.chromium.org/1259393002/diff/1/LayoutTests/http/tests/insp...
> LayoutTests/http/tests/inspector/network/network-fetch-expected.txt:1: Tests
> fetch network resource type and content.
> Can we also have a test for EventSource type and content?

Done.

>
https://codereview.chromium.org/1259393002/diff/1/LayoutTests/http/tests/insp...
> LayoutTests/http/tests/inspector/network/network-fetch-expected.txt:4:
> resource.type: other
> I think test should dump "Fetch" type somewhere.

Ouch, it should of course, thanks for spotting. Fixed.

> 
>
https://codereview.chromium.org/1259393002/diff/1/Source/core/inspector/Inspe...
> File Source/core/inspector/InspectorResourceAgent.cpp (right):
> 
>
https://codereview.chromium.org/1259393002/diff/1/Source/core/inspector/Inspe...
> Source/core/inspector/InspectorResourceAgent.cpp:535: m_pendingRequest =
> nullptr;
> I think didFinishXHR may come asynchronously after delay and
|m_pendingRequest|
> may be the other one (or even fetch request).

Nope, because we only have m_pendingRequest meaningful between
willLoadXHR/willStartFetch and documentThreadableLoaderStartedLoadingForClient,
which is supposed to happen synchronously, so didFinishXHR shouldn't intervene.
This code used to be here and is harmless and I'm not entirely sure what
implications removing it might have, so I'd rather keep it.

https://codereview.chromium.org/1259393002/diff/1/Source/core/inspector/Inspe...
> Source/core/inspector/InspectorResourceAgent.cpp:582: void
> InspectorResourceAgent::completeFetch(ThreadableLoaderClient* client)
> Inline this?

Done.

https://codereview.chromium.org/1259393002/diff/1/Source/core/inspector/Inspe...
> Source/core/inspector/InspectorResourceAgent.cpp:754:
ASSERT(!m_pendingRequest);
> Should we clear |m_knownRequestIdMap| here?

Done.

[dgozman, 2015-07-31 06:50:38]

lgtm with comments

https://codereview.chromium.org/1259393002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/inspector/inspector-test.js (right):

https://codereview.chromium.org/1259393002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/inspector/inspector-test.js:117:
InspectorTest.invokePageFunctionAsync = function(functionName, optionalArgs,
callback)
Should comment or annotate that optionalArgs is not an array but rather a
vararg.

https://codereview.chromium.org/1259393002/diff/20001/Source/core/inspector/I...
File Source/core/inspector/InspectorResourceAgent.cpp (right):

https://codereview.chromium.org/1259393002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.cpp:490: if (!client || client !=
m_pendingRequest)
Should we ASSERT(client == m_pendingRequest) if both of them not-null?

https://codereview.chromium.org/1259393002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InspectorResourceAgent.cpp:951: ,
m_pendingRequest(nullptr)
Initialize |m_pendingRequestType|.

https://codereview.chromium.org/1259393002/diff/20001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1259393002/diff/20001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:389: performNetworkError("Can't create
ThreadableLoader");
This method calls |failed| which in turn calls |didFailFetch|.
More generally, I think we should instrument exactly before every call to
|notifyFinished|.

[caseq, 2015-07-31 22:01:23]

On 2015/07/31 06:50:38, dgozman wrote:
> lgtm with comments
> 
>
https://codereview.chromium.org/1259393002/diff/20001/LayoutTests/http/tests/...
> File LayoutTests/http/tests/inspector/inspector-test.js (right):
> 
>
https://codereview.chromium.org/1259393002/diff/20001/LayoutTests/http/tests/...
> LayoutTests/http/tests/inspector/inspector-test.js:117:
> InspectorTest.invokePageFunctionAsync = function(functionName, optionalArgs,
> callback)
> Should comment or annotate that optionalArgs is not an array but rather a
> vararg.

Done.

> 
>
https://codereview.chromium.org/1259393002/diff/20001/Source/core/inspector/I...
> File Source/core/inspector/InspectorResourceAgent.cpp (right):
> 
>
https://codereview.chromium.org/1259393002/diff/20001/Source/core/inspector/I...
> Source/core/inspector/InspectorResourceAgent.cpp:490: if (!client || client !=
> m_pendingRequest)
> Should we ASSERT(client == m_pendingRequest) if both of them not-null?

Done.

> 
>
https://codereview.chromium.org/1259393002/diff/20001/Source/core/inspector/I...
> Source/core/inspector/InspectorResourceAgent.cpp:951: ,
> m_pendingRequest(nullptr)
> Initialize |m_pendingRequestType|.

Not done, as I don't see a good (null-like) value to initialize to and we guard
that this is initialized before usage via m_pendingRequest checks.

>
https://codereview.chromium.org/1259393002/diff/20001/Source/modules/fetch/Fe...
> File Source/modules/fetch/FetchManager.cpp (right):
> 
>
https://codereview.chromium.org/1259393002/diff/20001/Source/modules/fetch/Fe...
> Source/modules/fetch/FetchManager.cpp:389: performNetworkError("Can't create
> ThreadableLoader");
> This method calls |failed| which in turn calls |didFailFetch|.
> More generally, I think we should instrument exactly before every call to
> |notifyFinished|.

Thanks, removed the call.

[caseq, 2015-07-31 22:01:34]

The CQ bit was checked by caseq@chromium.org

[caseq, 2015-07-31 22:01:35]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1259393002/#ps40001
(title: "review comments addressed")

[commit-bot: I haz the power, 2015-07-31 22:01:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259393002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259393002/40001

[commit-bot: I haz the power, 2015-07-31 22:06:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-31 22:06:34]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[caseq, 2015-07-31 23:01:04]

The CQ bit was checked by caseq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-31 23:01:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259393002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259393002/60001

[commit-bot: I haz the power, 2015-08-01 00:22:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-01 00:22:10]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2015-08-03 16:34:27]

lgtm

[caseq, 2015-08-03 16:34:45]

The CQ bit was checked by caseq@chromium.org

[caseq, 2015-08-03 16:34:45]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1259393002/#ps60001
(title: "fixed the test")

[commit-bot: I haz the power, 2015-08-03 16:35:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259393002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259393002/60001

[commit-bot: I haz the power, 2015-08-03 18:01:00]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199904