Implement ManifestFetcher, helper to fetch Web Manifests.

Implement ManifestFetcher, helper to fetch Web Manifests.

This is implemented on top of ResourceFetcher.

BUG=410414
Committed: https://crrev.com/b16c57177635dcc6d88f6eae2eee41af1acf5f58
Cr-Commit-Position: refs/heads/master@{#294744}

[mlamouri (slow - plz ping), 2014-09-03 18:54:06]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org

[mlamouri (slow - plz ping), 2014-09-03 19:07:28]

This is used in a WIP CL:
https://code.google.com/p/chromium/issues/detail?id=409996

[mlamouri (slow - plz ping), 2014-09-03 19:07:56]

On 2014/09/03 19:07:28, Mounir Lamouri wrote:
> This is used in a WIP CL:
> https://code.google.com/p/chromium/issues/detail?id=409996

https://codereview.chromium.org/537053002/

[kenneth.christiansen, 2014-09-05 13:10:36]

Mike West, do you see a nice way to add CSP support to this? I am wondering
whether that will be straightforward or very complicated and I haven't had a
look yet.

Mounir, will unit testing be similar to the other patch?

[kenneth.christiansen, 2014-09-05 13:13:43]

kenneth.christiansen@gmail.com changed reviewers:
+ kenneth.christiansen@gmail.com

[kenneth.christiansen, 2014-09-05 13:13:44]

https://codereview.chromium.org/532773002/diff/60001/content/renderer/fetcher...
File content/renderer/fetchers/manifest_fetcher.cc (right):

https://codereview.chromium.org/532773002/diff/60001/content/renderer/fetcher...
content/renderer/fetchers/manifest_fetcher.cc:35:
request_.setRequestContext(blink::WebURLRequest::RequestContextManifest);
So it keeps part of your former patch?

[Mike West, 2014-09-05 13:21:37]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-09-05 13:21:38]

I don't see a simple way of adding CSP support with this approach. //content
doesn't currently know anything about CSP, so we'd have to either do the checks
before leaving Blink, or pipe checks back through Blink, as we do with
redirects.

I'm not sure that's a blocker for the functionality in general, but it's
something we should resolve one way or another.

[mlamouri (slow - plz ping), 2014-09-05 14:10:41]

https://codereview.chromium.org/532773002/diff/60001/content/renderer/fetcher...
File content/renderer/fetchers/manifest_fetcher.cc (right):

https://codereview.chromium.org/532773002/diff/60001/content/renderer/fetcher...
content/renderer/fetchers/manifest_fetcher.cc:35:
request_.setRequestContext(blink::WebURLRequest::RequestContextManifest);
On 2014/09/05 13:13:44, kenneth.christiansen wrote:
> So it keeps part of your former patch?

No, WebURLRequest already has that. It's part of the fetch spec.

[mlamouri (slow - plz ping), 2014-09-08 15:07:00]

mkwst@, is there any chance you could review this CL as non-owner?

[Mike West, 2014-09-10 07:45:30]

This CL looks fine. Mostly because it's almost an exact copy of
ResourceFetcherImpl. :) I'd like to see a way to reduce that copy/pasting.
Perhaps there's room for a shared superclass that implements the shared
WebURLLoaderClient bits, and holds the shared state? Otherwise, these two
classes are bound to diverge, which seems unfortunate.

https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
File content/renderer/fetchers/manifest_fetcher.cc (right):

https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
content/renderer/fetchers/manifest_fetcher.cc:37:
request_.setFrameType(blink::WebURLRequest::FrameTypeNone);
The only difference between ResourceFetcherImpl and this method is the
hard-coded contexts and types.

https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
content/renderer/fetchers/manifest_fetcher.cc:55: completed_ = true;
The only difference here is the timeout.

https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
content/renderer/fetchers/manifest_fetcher.cc:63: void
ManifestFetcher::didReceiveResponse(
The implementation of these four methods is exactly the same as
ResourceFetcherImpl.

[mlamouri (slow - plz ping), 2014-09-10 11:16:05]

Basically, my version of the fetcher is following the web semantics while the
resource fetcher ignores everything like cors, mixed content, etc.

https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
File content/renderer/fetchers/manifest_fetcher.cc (right):

https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
content/renderer/fetchers/manifest_fetcher.cc:37:
request_.setFrameType(blink::WebURLRequest::FrameTypeNone);
On 2014/09/10 07:45:30, Mike West wrote:
> The only difference between ResourceFetcherImpl and this method is the
> hard-coded contexts and types.

Not really. I'm using frame->createAssociatedURLLoader() while
ResourceFetcherImpl uses Platform::current()->createURLLoader().

[Mike West, 2014-09-10 11:49:56]

https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
File content/renderer/fetchers/manifest_fetcher.cc (right):

https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
content/renderer/fetchers/manifest_fetcher.cc:37:
request_.setFrameType(blink::WebURLRequest::FrameTypeNone);
On 2014/09/10 11:16:05, Mounir Lamouri wrote:
> On 2014/09/10 07:45:30, Mike West wrote:
> > The only difference between ResourceFetcherImpl and this method is the
> > hard-coded contexts and types.
> 
> Not really. I'm using frame->createAssociatedURLLoader() while
> ResourceFetcherImpl uses Platform::current()->createURLLoader().

You're right. I missed the difference in line #40.

Still.

[mlamouri (slow - plz ping), 2014-09-10 12:26:29]

On 2014/09/10 11:49:56, Mike West wrote:
>
https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
> File content/renderer/fetchers/manifest_fetcher.cc (right):
> 
>
https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
> content/renderer/fetchers/manifest_fetcher.cc:37:
> request_.setFrameType(blink::WebURLRequest::FrameTypeNone);
> On 2014/09/10 11:16:05, Mounir Lamouri wrote:
> > On 2014/09/10 07:45:30, Mike West wrote:
> > > The only difference between ResourceFetcherImpl and this method is the
> > > hard-coded contexts and types.
> > 
> > Not really. I'm using frame->createAssociatedURLLoader() while
> > ResourceFetcherImpl uses Platform::current()->createURLLoader().
> 
> You're right. I missed the difference in line #40.
> 
> Still.

Yes, I agree, still... I'm not a big fan of that copy-paste myself. I'm looking
into how to make that cleaner without making it too heary to use.

[mlamouri (slow - plz ping), 2014-09-10 13:24:59]

On 2014/09/10 12:26:29, Mounir Lamouri wrote:
> On 2014/09/10 11:49:56, Mike West wrote:
> >
>
https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
> > File content/renderer/fetchers/manifest_fetcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/532773002/diff/140001/content/renderer/fetche...
> > content/renderer/fetchers/manifest_fetcher.cc:37:
> > request_.setFrameType(blink::WebURLRequest::FrameTypeNone);
> > On 2014/09/10 11:16:05, Mounir Lamouri wrote:
> > > On 2014/09/10 07:45:30, Mike West wrote:
> > > > The only difference between ResourceFetcherImpl and this method is the
> > > > hard-coded contexts and types.
> > > 
> > > Not really. I'm using frame->createAssociatedURLLoader() while
> > > ResourceFetcherImpl uses Platform::current()->createURLLoader().
> > 
> > You're right. I missed the difference in line #40.
> > 
> > Still.
> 
> Yes, I agree, still... I'm not a big fan of that copy-paste myself. I'm
looking
> into how to make that cleaner without making it too heary to use.

I've improved the situation a bit by adding a WebURLLoaderClientImpl. I will
look into whether I can make the function creating the WebURLLoader a template
argument.

[Mike West, 2014-09-10 14:00:12]

LGTM with the one note about more potential copy/paste removal.

You'll still need a //content OWNER to sign off on it, but I think this is a
good step up from the initial pass. Thanks!

https://codereview.chromium.org/532773002/diff/160001/content/renderer/fetche...
File content/renderer/fetchers/manifest_fetcher.cc (right):

https://codereview.chromium.org/532773002/diff/160001/content/renderer/fetche...
content/renderer/fetchers/manifest_fetcher.cc:35:
request_.setRequestContext(blink::WebURLRequest::RequestContextManifest);
Perhaps this could be deduped by adding ::Start method to the superclass, and
passing through the call here along with the context, type, and loader?

[mlamouri (slow - plz ping), 2014-09-10 14:00:45]

There should be no more code copy-pasting.

Mike, PTAL

[Mike West, 2014-09-10 14:06:44]

Patchset 10 LGTM. Thanks for taking another pass on the piece I commented on
before I even commented on it. :)

[mlamouri (slow - plz ping), 2014-09-10 14:17:24]

Jochen, could you PTAL?

[jochen (gone - plz use gerrit), 2014-09-12 11:50:42]

lgtm

[mlamouri (slow - plz ping), 2014-09-12 11:55:24]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-09-12 11:55:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/532773002/200001

[commit-bot: I haz the power, 2014-09-12 12:51:29]

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

[commit-bot: I haz the power, 2014-09-12 12:51:31]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2014-09-12 12:53:06]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-09-12 12:53:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/532773002/200001

[commit-bot: I haz the power, 2014-09-12 13:57:22]

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

[commit-bot: I haz the power, 2014-09-12 13:57:24]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2014-09-13 11:45:06]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-09-13 11:45:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/532773002/220001

[commit-bot: I haz the power, 2014-09-13 12:49:51]

Committed patchset #12 (id:220001) as 49973ef98f592fb3e71b63515a7b96da03d0c6b7

[commit-bot: I haz the power, 2014-09-13 12:53:26]

Patchset 12 (id:??) landed as
https://crrev.com/b16c57177635dcc6d88f6eae2eee41af1acf5f58
Cr-Commit-Position: refs/heads/master@{#294744}