Remove resource type-specific fetching logic from ResourceFetcher

Remove resource type-specific fetching logic from ResourceFetcher

Instead of having a whole bunch of ResourceFetcher::fetchFoo() functions, start a fetch by calling FooResource::fetch(). This allows ResourceFetcher to just have generic loading logic, and puts all of the specific logic for a given type in one place.

This requires adding a ResourceFactory class hierarchy, which allows ResourceFetcher to create the proper Resource subclass simply by calling ResourceFactory::create.

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

[Nate Chapin, 2015-06-05 17:21:29]

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

[Nate Chapin, 2015-06-05 17:21:30]

I think this change is worthwhile on its own. However, it also gives us the
option of further isolating core/fetch/ from the rest of core/ by moving
Resource subclasses closer to the code that uses them.

ScriptResource, e.g., knows about TextResourceDecoder, which in turn knows about
html parsing. If fetch/ should be entirely isolated, then ScriptResource needs a
new home from which it can talk to TextResourceDecoder (core/dom/ perhaps?).
It's not clear to me whether dispersing the Resource subclasses throughout core/
is intrinsically a good thing or a bad thing (and if it is a bad thing, whether
it's a reasonable price to pay for a modularized fetch/ directory). I drafted
that followup at https://codereview.chromium.org/1167583003/ to try to get my
head around it.

[Mike West, 2015-06-07 15:37:13]

mkwst@chromium.org changed reviewers:
+ sofbjornf@opera.com

[Mike West, 2015-06-07 15:37:14]

I like this change. I want to skim it a bit more to make sure I understand it,
but this looks like a good direction to be going in. I'd also suggest that
moving resource types down into core/ doesn't seem like a bad thing at all.
Putting a thing next to the things it requires/implies seems like a reasonable
change. I'll comment in the other CL.

+sof for the ResourcePtr change, as I'm still not comfortable with Oilpanisms.

[Yoav Weiss, 2015-06-08 06:44:16]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2015-06-08 06:44:18]

https://codereview.chromium.org/1170503003/diff/1/Source/core/html/parser/HTM...
File Source/core/html/parser/HTMLResourcePreloader.cpp (right):

https://codereview.chromium.org/1170503003/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLResourcePreloader.cpp:78: 
Can't say that I like adding all that resource type specific logic here... Is it
possible to abstract is away somewhere in fetch? (e.g. maybe move it out of
ResourceFetcher into a "ResourceTypeHandler")

https://codereview.chromium.org/1170503003/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLResourcePreloader.cpp:80: if
(preload->resourceType() == Resource::MainResource)
Is there a scenario where we preload a main resource?

[sof, 2015-06-08 11:46:59]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com
- sofbjornf@opera.com

[sof, 2015-06-08 13:08:46]

ResourcePtr<> (and ResourceOwner<>) aren't first-class Oilpan abstractions, as
they do keep around references that aren't traced per normal. But as long as
we've keep ResourcePtr<>s references on non-heap objects, we have to make
compromises.

This CL doesn't alter the situation substantially, so looks fine from an Oilpan
POV.

+Cc: oilpan-reviews (and dropped the use "sofbjornf" on the reviewer line;
please don't use that invalid entry..while I work out how to get it rm'ed from
rietveld's db :) )

[Mike West, 2015-06-08 18:44:31]

LGTM % Yoav's comments. I agree that there's some tension between teaching all
of core/ all about all the resource types, and your sanity with regard to the
number of layers we add to fetch/. I don't think this CL is tooooo far over the
line, but it doesn't seem like the parser should need to know about resource
types.

Folding that into some sort of fetch/-side "Handler" sounds like a reasonable
thing to try out.

Otherwise, the copy/pasting around all seems pretty reasonable. Thanks for
refactoring this out!

[Nate Chapin, 2015-06-08 19:01:22]

https://codereview.chromium.org/1170503003/diff/1/Source/core/html/parser/HTM...
File Source/core/html/parser/HTMLResourcePreloader.cpp (right):

https://codereview.chromium.org/1170503003/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLResourcePreloader.cpp:78: 
On 2015/06/08 06:44:18, Yoav Weiss wrote:
> Can't say that I like adding all that resource type specific logic here... Is
it
> possible to abstract is away somewhere in fetch? (e.g. maybe move it out of
> ResourceFetcher into a "ResourceTypeHandler")

For this patch by itself, yes. However, for the proposed followup of moving
Resource subclasses out of fetch/, ResourceTypeHandler would need to not be in
fetch in order to call ImageResource::fetch().

I think I'm missing a bit of context. HTMLPreloadScanner is well aware of
Resource types, is there a particular reason HTMLResourcePreloader shouldn't be?

https://codereview.chromium.org/1170503003/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLResourcePreloader.cpp:80: if
(preload->resourceType() == Resource::MainResource)
On 2015/06/08 06:44:17, Yoav Weiss wrote:
> Is there a scenario where we preload a main resource?

I think there was hope a year or two ago that we would be able to preload a main
resource, but I don't think it ever got fully hooked up (and we never really got
MemoryCache to support main resource). Will remove.

[Yoav Weiss, 2015-06-09 06:04:30]

On 2015/06/08 19:01:22, Nate Chapin wrote:
>
https://codereview.chromium.org/1170503003/diff/1/Source/core/html/parser/HTM...
> File Source/core/html/parser/HTMLResourcePreloader.cpp (right):
> 
>
https://codereview.chromium.org/1170503003/diff/1/Source/core/html/parser/HTM...
> Source/core/html/parser/HTMLResourcePreloader.cpp:78: 
> On 2015/06/08 06:44:18, Yoav Weiss wrote:
> > Can't say that I like adding all that resource type specific logic here...
Is
> it
> > possible to abstract is away somewhere in fetch? (e.g. maybe move it out of
> > ResourceFetcher into a "ResourceTypeHandler")
> 
> For this patch by itself, yes. However, for the proposed followup of moving
> Resource subclasses out of fetch/, ResourceTypeHandler would need to not be in
> fetch in order to call ImageResource::fetch().

Maybe in loader, then?

As an aside, is there a doc explaining these design changes and their ultimate
goal?

> 
> I think I'm missing a bit of context. HTMLPreloadScanner is well aware of
> Resource types, is there a particular reason HTMLResourcePreloader shouldn't
be?

HTMLPreloadScanner has a dependency on Resource types, but it doesn't contain
any of their logic (it simply has a map of tags to Resource types).
My fear is that adding that logic to HTMLResourcePreloader would result in
future, since that logic here would fall out of sync with future changes to
these image resources.

> 
>
https://codereview.chromium.org/1170503003/diff/1/Source/core/html/parser/HTM...
> Source/core/html/parser/HTMLResourcePreloader.cpp:80: if
> (preload->resourceType() == Resource::MainResource)
> On 2015/06/08 06:44:17, Yoav Weiss wrote:
> > Is there a scenario where we preload a main resource?
> 
> I think there was hope a year or two ago that we would be able to preload a
main
> resource, but I don't think it ever got fully hooked up (and we never really
got
> MemoryCache to support main resource). Will remove.

[Nate Chapin, 2015-06-09 18:39:56]

Ok, I moved the Resource creation portion of preloading to DocumentLoader. It's
not a perfect location (since DocumentLoader mostly deals with the main
resource) and I still personally favor HTMLResourcePreloader for keeping preload
logic in one place, but this is ok, too.

In terms of context, there's the bug in the patch description, as well as the
announcement on blink-dev:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/jupwgkQCdUo/JLECcC6h....

[Nate Chapin, 2015-06-15 18:09:35]

On 2015/06/09 18:39:56, Nate Chapin wrote:
> Ok, I moved the Resource creation portion of preloading to DocumentLoader.
It's
> not a perfect location (since DocumentLoader mostly deals with the main
> resource) and I still personally favor HTMLResourcePreloader for keeping
preload
> logic in one place, but this is ok, too.
> 
> In terms of context, there's the bug in the patch description, as well as the
> announcement on blink-dev:
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/jupwgkQCdUo/JLECcC6h....

I'm going to go ahead and land this, I'm happy to update it if you have further
suggestions.

[Nate Chapin, 2015-06-15 18:09:42]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-06-15 18:09:43]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1170503003/#ps20001
(title: "Address yoav's comments")

[Nate Chapin, 2015-06-15 18:09:44]

The CQ bit was unchecked by japhet@chromium.org

[Nate Chapin, 2015-06-15 18:09:59]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-06-15 18:10:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1170503003/20001

[commit-bot: I haz the power, 2015-06-15 20:08:22]

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

[commit-bot: I haz the power, 2015-06-15 20:08:24]

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

[Nate Chapin, 2015-06-15 22:37:00]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-06-15 22:37:01]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1170503003/#ps40001
(title: "Null-check Document::loader() before calling startPreload()")

[commit-bot: I haz the power, 2015-06-15 22:37:30]

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

[commit-bot: I haz the power, 2015-06-15 23:39:29]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197145