Show media and VTT text tracks in the devtools.

Show media and VTT text tracks in the devtools.

This also introduces a Resource::Media type to the fetcher, and makes text
track fetches use the existing (but apparently unused) Resource::TextTrack
type.

This change does the initial work of wiring Media and TextTrack
resources into the devtools and leaves a lot of room for future
improvements, such as media previews in the Network tab.

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

[pwnall-personal, 2013-12-19 09:16:26]

Jochen, can you please take a look at this?

I'm not sure what's the right way to test this change, but I manually verified
it in the devtools, according to the bug steps.

[jochen (gone - plz use gerrit), 2013-12-19 09:21:59]

somebody from devtools should review this

[apavlov, 2013-12-19 09:31:52]

+"protocol.json" owners

https://codereview.chromium.org/114123005/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/114123005/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:389: 
extra blank line

https://codereview.chromium.org/114123005/diff/20001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/114123005/diff/20001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.cpp:47: #include
"platform/Logging.h"
a debugging leftover include?

[pwnall-personal, 2013-12-19 09:32:45]

On 2013/12/19 09:21:59, jochen wrote:
> somebody from devtools should review this

Can you please look at the fetch/ and loader/ code? There's a small subtlety in
how the Media requests are detected. I'm relying on the fact that the media
players use WebURLRequest::TargetIsMedia to set the right resource type. Is this
reasonable, or should I add an API that media players are supposed to use to
fetch resources?

I'll add someone from devtools for the inspector/ and devtools/ part.

[pwnall-personal, 2013-12-19 09:39:33]

Thank you, Alexander!

https://codereview.chromium.org/114123005/diff/20001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/114123005/diff/20001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.cpp:47: #include
"platform/Logging.h"
On 2013/12/19 09:31:52, apavlov wrote:
> a debugging leftover include?

Done. Fixed.

Rebasing leftover. Sorry :(

[pwnall-personal, 2013-12-20 08:54:35]

I figured out how to test this change -- I added a test that works at the remote
debug protocol level and checks resource types. I think this provides coverage
for all the C++ code changes.

I don't have coverage for the changes to the inspector JS. Should I look into
that?

[vsevik, 2013-12-20 15:28:26]

Added japhet@ to review adding new resource type.

I am not sure this is the way we want to move forward with.
ThreadableLoader is also used by file reader, AssociatedURLLoader and workers.
We need a single way to handle all this cases and I am not sure how well the
suggested approach scales.

As far as I understand, ideally DocumentThreadableLoader should know nothing
about semantics of the resources it loads.

[pwnall-personal, 2013-12-20 21:37:52]

On 2013/12/20 15:28:26, vsevik wrote:
> Added japhet@ to review adding new resource type.
> 
> I am not sure this is the way we want to move forward with.
> ThreadableLoader is also used by file reader, AssociatedURLLoader and workers.
> We need a single way to handle all this cases and I am not sure how well the
> suggested approach scales.

For clarity, are you referring to the DocumentThreadableLoader.cpp change?
https://chromiumcodereview.appspot.com/114123005/patch/130001/140015

> As far as I understand, ideally DocumentThreadableLoader should know nothing
> about semantics of the resources it loads.

Agreed, I mentioned in a previous comment I'm not too sure about this.

I think that <audio> and <video> are snowflakes because their resources are
fetched by media player implementations on the Chromium side.

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

These eventually call createAssociatedURLLoader, which ends up creating an
AssociatedURLLoader, which in turn creates a DocumentThreadableLoader.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

As far as I understand things, some resource types call methods on the parent
document's ResourceFetcher directly, for example:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

... and some other resource types create their own loader types, for example:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...


It seems to me that we can either add an entirely new API for the media player,
or lose generality somewhere on the path. Would it be better if
AssociatedURLLoader creates a different type of loader for media player
requests?

[pwnall-personal, 2013-12-24 17:52:17]

I addressed a piece of feedback that I missed earlier (sorry!) and fixed the
broken tests.

https://codereview.chromium.org/114123005/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/114123005/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:389: 
On 2013/12/19 09:31:52, apavlov wrote:
> extra blank line

Done.

[eustas, 2014-01-13 16:00:33]

The code looks good to me.

https://codereview.chromium.org/114123005/diff/170001/LayoutTests/inspector-p...
File LayoutTests/inspector-protocol/network/resource-type.html (right):

https://codereview.chromium.org/114123005/diff/170001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/network/resource-type.html:25: if ("error" in
messageObject) {
We usually don't use "in" keyword.

"if (messageObject.error)" looks better.

https://codereview.chromium.org/114123005/diff/170001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/network/resource-type.html:52: if ("gotType" in
resource) {
ditto

https://codereview.chromium.org/114123005/diff/170001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/network/resource-type.html:59:
InspectorTest.log("Received expected resource. " + resourcesLeft + " left.");
This additional output doesn't make much sense.

https://codereview.chromium.org/114123005/diff/170001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/network/resource-type.html:69: function
receivedAllResources()
If you turn protocol test common test (http/tests/inspector) you could benefit
by reusing code.

You can use InspectorTest.runAfterResourcesAreFinished and then check resource
types.
Also, this way resources could be baked into test page.

[pwnall-personal, 2014-01-13 20:33:29]

Thank you for your feedback, Eugene!

Can you please look at my question and advise me on how to proceed?

Thank you very much, and sorry for getting so easily confused!

https://codereview.chromium.org/114123005/diff/170001/LayoutTests/inspector-p...
File LayoutTests/inspector-protocol/network/resource-type.html (right):

https://codereview.chromium.org/114123005/diff/170001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/network/resource-type.html:25: if ("error" in
messageObject) {
On 2014/01/13 16:00:33, eustas wrote:
> We usually don't use "in" keyword.
> 
> "if (messageObject.error)" looks better.

Done.

Thank you!

https://codereview.chromium.org/114123005/diff/170001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/network/resource-type.html:52: if ("gotType" in
resource) {
On 2014/01/13 16:00:33, eustas wrote:
> ditto

Done.

https://codereview.chromium.org/114123005/diff/170001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/network/resource-type.html:59:
InspectorTest.log("Received expected resource. " + resourcesLeft + " left.");
On 2014/01/13 16:00:33, eustas wrote:
> This additional output doesn't make much sense.

Done.

Removed. Thank you!

https://codereview.chromium.org/114123005/diff/170001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/network/resource-type.html:69: function
receivedAllResources()
On 2014/01/13 16:00:33, eustas wrote:
> If you turn protocol test common test (http/tests/inspector) you could benefit
> by reusing code.
> 
> You can use InspectorTest.runAfterResourcesAreFinished and then check resource
> types.
> Also, this way resources could be baked into test page.

I was afraid to write test at that level because my test already depends on two
layers (core/fetch and core/inspector) and I was worried that adding one more
layer (the inspector JS) would make failures even harder to debug. At the same
time, all layout tests do rely on a lot of stuff to work correctly, so maybe I'm
worrying over nothing here.

Do you think it's worth moving and shrinking, at the expense of making failures
a bit more difficult to debug? If so, I'll move it.

[eustas, 2014-01-14 09:02:40]

lgtm

You are right, testing at lower level here makes sense.

Thank you for the patch.

PS: you still need owners lgtm.

[philipj_slow, 2014-04-01 07:03:37]

I don't understand what the new fetchMedia is for. Currently Chromium does the
fetching of media resources using (I think) a special-purpose byte range cache
on most platforms, and a completely separate network stack on Android. Fetching
it separately from that seems unlikely to end up in the same cache...

[eseidel, 2014-04-10 04:29:31]

Should this be CQ'd?

[pfeldman, 2014-04-10 05:00:22]

lgtm

I guess it was missing protocol.json owner.

https://codereview.chromium.org/114123005/diff/760001/LayoutTests/inspector-p...
File LayoutTests/inspector-protocol/network/resource-type.html (right):

https://codereview.chromium.org/114123005/diff/760001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/network/resource-type.html:19:
InspectorTest.log("Test started");
nit: wrong indentation.

[pwnall-personal, 2014-04-10 05:11:17]

On 2014/04/10 05:00:22, pfeldman wrote:
> lgtm
> 
> I guess it was missing protocol.json owner.
> 
>
https://codereview.chromium.org/114123005/diff/760001/LayoutTests/inspector-p...
> File LayoutTests/inspector-protocol/network/resource-type.html (right):
> 
>
https://codereview.chromium.org/114123005/diff/760001/LayoutTests/inspector-p...
> LayoutTests/inspector-protocol/network/resource-type.html:19:
> InspectorTest.log("Test started");
> nit: wrong indentation.

Thank you! I'll address this and rebase.

[pwnall-personal, 2014-04-10 12:55:54]

https://codereview.chromium.org/114123005/diff/760001/LayoutTests/inspector-p...
File LayoutTests/inspector-protocol/network/resource-type.html (right):

https://codereview.chromium.org/114123005/diff/760001/LayoutTests/inspector-p...
LayoutTests/inspector-protocol/network/resource-type.html:19:
InspectorTest.log("Test started");
On 2014/04/10 05:00:23, pfeldman wrote:
> nit: wrong indentation.

Done.

Thank you!

[pfeldman, 2014-04-10 13:04:43]

alright. feel free to CQ+ it.

[pwnall-personal, 2014-04-10 13:32:25]

The CQ bit was checked by costan@gmail.com

[commit-bot: I haz the power, 2014-04-10 13:32:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/costan@gmail.com/114123005/800001

[commit-bot: I haz the power, 2014-04-10 14:44:37]

Change committed as 171279