DevTools: expose linkPreload bit on the network request.

DevTools: expose linkPreload bit on the network request.

BUG=701597

[pfeldman, 2017-03-15 02:30:30]

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

[pfeldman, 2017-03-15 02:30:36]

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

[commit-bot: I haz the power, 2017-03-15 02:31:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pfeldman, 2017-03-15 02:32:19]

Description was changed from

==========
DevTools: exlore linkPreload bit on the network request.

BUG=701597
==========

to

==========
DevTools: expose linkPreload bit on the network request.

BUG=701597
==========

[pfeldman, 2017-03-15 02:37:59]

pfeldman@chromium.org changed reviewers:
+ kouhei@chromium.org

[dgozman, 2017-03-15 02:40:00]

https://codereview.chromium.org/2751043002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2751043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1018: { "name":
"isLinkPreload", "type": "boolean", "optional": true, "description": "Whether is
loaded via link preload." }
Experimental: true? So far we have only introduced non-experimental entities
when bumping protocol version. Let's keep that until we figure out a better way.

[pfeldman, 2017-03-15 02:47:51]

> So far we have only introduced non-experimental entities
> when bumping protocol version.

We've never had such a policy. If one is adding something that is safe to
expose, it can be public right away. It is safe to expose optional parameters as
well, so I think it is fine.

[kouhei (in TOK), 2017-03-15 03:12:18]

kouhei@chromium.org changed reviewers:
+ toyoshim@chromium.org, tyoshino@chromium.org, yhirano@chromium.org

[kouhei (in TOK), 2017-03-15 03:12:19]

+toyoshim, tyoshino, yhirano for FetchRequest, ResourceRequest expertise

[Takashi Toyoshima, 2017-03-15 03:59:47]

If we count crbug.com/675883 in, probably it's better to move speculative
preload flag together into ResourceRequest even though we have no plan to expose
it to DevTools?

[pfeldman, 2017-03-15 04:07:19]

I can move this second bit as is into ResourceRequest, so that you could follow
up with the enum, does that sound good?

[commit-bot: I haz the power, 2017-03-15 04:17:28]

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

[commit-bot: I haz the power, 2017-03-15 04:17:29]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-03-15 04:48:12]

PTAL

[pfeldman, 2017-03-15 04:48:52]

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

[commit-bot: I haz the power, 2017-03-15 04:49:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 06:40:23]

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

[commit-bot: I haz the power, 2017-03-15 06:40:24]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2017-03-15 08:20:03]

lgtm

https://codereview.chromium.org/2751043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/FetchRequest.h (right):

https://codereview.chromium.org/2751043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchRequest.h:105: void
setLinkPreload(bool isLinkPreload) {
optional: Can you leave "TODO(toyoshim): Will remove setLinkPreload() soon. Do
not use this interface any more." here?

Probably, we won't have an additional interface to modify ResourceRequest in an
arbitrary timing, and as I checked quickly, current callers can be easily
modified to preset the flag before passing ResourceRequest instance to the ctor.

[kouhei (in TOK), 2017-03-15 08:55:09]

lgtm

[yhirano, 2017-03-15 10:56:33]

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

[yhirano, 2017-03-15 10:56:34]

+japhet@

I think the reason why FetchRequest has m_linkPreload and other members is that
they are not passed to ResourceLoader. I'm not sure we need to keep doing that,
though. Nate, what do you think?

I'm also not sure the current code handles redirect correctly (the case
dispatchWillSendRequest called in ResourceLoader::willFollowRedirect).

If we want to keep the current design, you can add a parameter to
FetchContext::dispatchWillSendRequest and pass values via FetchRequest or
Resource.

Thanks!

[pfeldman, 2017-03-15 11:48:32]

Thanks for the review. @yhirano: should i wait for Nate to respond or is it find
to land? it is a bit unclear whether I am doing something wrong in this change
or existing code has something already that you want to discuss / question.

[pfeldman, 2017-03-15 11:51:57]

s/find/fine/

[yhirano, 2017-03-15 11:57:05]

On 2017/03/15 11:48:32, pfeldman_slow wrote:
> Thanks for the review. @yhirano: should i wait for Nate to respond or is it
find
> to land? it is a bit unclear whether I am doing something wrong in this change
> or existing code has something already that you want to discuss / question.

Sorry for being unclear. 

> I think the reason why FetchRequest has m_linkPreload and other members is
that
> they are not passed to ResourceLoader. I'm not sure we need to keep doing
that,
> though. Nate, what do you think?
I would like to hear Nate's opinion on this point before landing this CL.

> I'm also not sure the current code handles redirect correctly (the case
> dispatchWillSendRequest called in ResourceLoader::willFollowRedirect).
This is about this change. Do you want to mark all preloading redirects as
preloading in devtools?

[pfeldman, 2017-03-15 12:21:34]

> I would like to hear Nate's opinion on this point before landing this CL.

sounds good.

> This is about this change. Do you want to mark all preloading redirects as
> preloading in devtools?

We can do either way, it is not essential. DevTools will use this bit of
information to differentiate between render blocking and non-blocking requests.
Since redirect chain is connected, we can propagate this bit based on the first
request in the chain. So we should make this bit make sense to the rest of the
system and DevTools will adapt to it.

[dgozman, 2017-03-15 17:16:45]

protocol lgtm

[Nate Chapin, 2017-03-16 17:15:00]

https://codereview.chromium.org/2751043002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/2751043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ResourceRequest.h:321: bool
isLinkPreload() const { return m_linkPreload; }
The standard I typically use for deciding whether something should go on
ResourceRequest versus FetchRequest is: does it need to be handled outside of
blink?

I'm not aware of any plans to expose these bits to content/, so I'd be inclined
to leave them on FetchRequest and do the extra plumbing. If there are such plans
and I'm oblivious, then this is fine :)

[pfeldman, 2017-03-16 20:47:30]

> The standard I typically use for deciding whether something should go on
> ResourceRequest versus FetchRequest is: does it need to be handled outside of
> blink?

Given that blink/ and content/ are merging, using this kind of criteria could be
problematic in the future. In fact, most of the capabilities ResourceRequest
exposes are currently only used within blink/ source base. So I don't see how
you can enforce that without introducing proper layering here. I would hate to
add more into FetchInitiatorInfo and add into the existing ambiguity between the
FetchRequest, ResourceRequest and FetchInitiatorInfo. Can we do anything to make
it more straightforward?

[pfeldman, 2017-03-16 20:48:35]

(with the recent migration of ResourceRequest into fetch/, it is even less clear
why something FetchRequest wraps is not supposed to have that bit - the classes
are now sitting next to one another and are hardly distinguishable)

[yhirano, 2017-03-17 03:59:28]

On 2017/03/16 20:47:30, pfeldman_slow wrote:
> > The standard I typically use for deciding whether something should go on
> > ResourceRequest versus FetchRequest is: does it need to be handled outside
of
> > blink?
> 
> Given that blink/ and content/ are merging, using this kind of criteria could
be
> problematic in the future. In fact, most of the capabilities ResourceRequest
> exposes are currently only used within blink/ source base. So I don't see how
> you can enforce that without introducing proper layering here. I would hate to
> add more into FetchInitiatorInfo and add into the existing ambiguity between
the
> FetchRequest, ResourceRequest and FetchInitiatorInfo. Can we do anything to
make
> it more straightforward?

We are merging blink/platform and content/child, but the correspondence between
blink::ResourceRequest and content::ResourceRequest (through mojom::URLRequest)
will be kept. So I'm a bit uncomfortable with introducing renderer-only
members...

[pfeldman, 2017-03-17 04:05:59]

> We are merging blink/platform and content/child, but the correspondence
between
> blink::ResourceRequest and content::ResourceRequest (through
mojom::URLRequest)
> will be kept. So I'm a bit uncomfortable with introducing renderer-only
> members...

Could you elaborate on this? I'm moving the bit from FetchRequest to
ResourceRequest within blink/platform. How does this map to the refactoring you
mention above?

[yhirano, 2017-03-17 04:17:17]

On 2017/03/17 04:05:59, pfeldman_slow wrote:
> > We are merging blink/platform and content/child, but the correspondence
> between
> > blink::ResourceRequest and content::ResourceRequest (through
> mojom::URLRequest)
> > will be kept. So I'm a bit uncomfortable with introducing renderer-only
> > members...
> 
> Could you elaborate on this? I'm moving the bit from FetchRequest to
> ResourceRequest within blink/platform. How does this map to the refactoring
you
> mention above?

My mental model is blink::ResourceRequest is a representation of
mojom::URLRequest. Onion-soup will not change that.

[pfeldman, 2017-03-17 04:35:52]

> My mental model is blink::ResourceRequest is a representation of
> mojom::URLRequest. Onion-soup will not change that.

But if you look at what blink::ResourceRequest, it is far from being so. Is
there a design doc or anything I can look at that describes your plan?
For example, dispatch callbacks in FetchContext operate blink::ResourceRequest
and if you limit ResourceRequest to only be URLRequest, embedder will be missing
data it typially gets off ResourceRequest.

[kouhei (in TOK), 2017-03-17 05:07:25]

On 2017/03/17 04:35:52, pfeldman_slow wrote:
> > My mental model is blink::ResourceRequest is a representation of
> > mojom::URLRequest. Onion-soup will not change that.
> 
> But if you look at what blink::ResourceRequest, it is far from being so. Is
> there a design doc or anything I can look at that describes your plan?
> For example, dispatch callbacks in FetchContext operate blink::ResourceRequest
> and if you limit ResourceRequest to only be URLRequest, embedder will be
missing
> data it typially gets off ResourceRequest.

I think everyone agree “exposing linkPreload bit on the network request” makes
sense, so I think we can land the code which allows that, and then split
{Fetch,Resource}Request refactoring to another CL (by yhirano).
pfeldman: Can we achieve “expose linkPreload bit on the network request” on this
CL via adding a boolean param to FetchContext::dispatchWillSendRequest as
yhirano@ suggested earlier?
You can file TODO(yhirano) to clean up FetchContext interface afterwards.

[pfeldman, 2017-03-17 05:45:25]

> pfeldman: Can we achieve “expose linkPreload bit on the network request” on
this
> CL via adding a boolean param to FetchContext::dispatchWillSendRequest as
> yhirano@ suggested earlier?

I can do that, but we all know it is not pretty. We already have an ugly fetch
initiator info struct floating there and we are adding a stray boolean to the
bunch. Let me see if I can make it a part of the fetchinitiatorinfo. After all
rel=preload correlates with the "initiator" nature. Worst case I end up plumbing
it as a boolean in dispatchWillSendRequest. How does that sound?

[kouhei (in TOK), 2017-03-17 06:24:08]

On 2017/03/17 05:45:25, pfeldman_slow wrote:
> > pfeldman: Can we achieve “expose linkPreload bit on the network request” on
> this
> > CL via adding a boolean param to FetchContext::dispatchWillSendRequest as
> > yhirano@ suggested earlier?
> 
> I can do that, but we all know it is not pretty. We already have an ugly fetch
> initiator info struct floating there and we are adding a stray boolean to the
> bunch. Let me see if I can make it a part of the fetchinitiatorinfo. After all
> rel=preload correlates with the "initiator" nature. Worst case I end up
plumbing
> it as a boolean in dispatchWillSendRequest. How does that sound?

sgtm. Thanks!

[yhirano, 2017-03-17 06:30:18]

On 2017/03/17 06:24:08, kouhei wrote:
> On 2017/03/17 05:45:25, pfeldman_slow wrote:
> > > pfeldman: Can we achieve “expose linkPreload bit on the network request”
on
> > this
> > > CL via adding a boolean param to FetchContext::dispatchWillSendRequest as
> > > yhirano@ suggested earlier?
> > 
> > I can do that, but we all know it is not pretty. We already have an ugly
fetch
> > initiator info struct floating there and we are adding a stray boolean to
the
> > bunch. Let me see if I can make it a part of the fetchinitiatorinfo. After
all
> > rel=preload correlates with the "initiator" nature. Worst case I end up
> plumbing
> > it as a boolean in dispatchWillSendRequest. How does that sound?
> 
> sgtm. Thanks!

Sounds good.

[esprehn, 2017-03-17 06:45:52]

I'd prefer we didn't add booleans to the callbacks, those aren't very good for
long term maintainability. Looking at the code the FetchRequest and the
ResourceRequest both seem very complex, and the ResourceRequest is being passed
around by value even though it's quite large.

Note that the public API doesn't need to mirror the API inside blink (ex.
WebURLRequest can be it's own thing), and ideally the public API usage is
decreasing and surface area is also shrinking. :)

Having studied the code I think the best thing might be to merge FetchRequest
and ResourceRequest into a single object and just pass them both into all of
these delegate callbacks through the layers inside blink, it was pretty
confusing to me why FetchRequest goes in, but all the callbacks are
ResourceRequest omitting all those extra state fields. Then we can merge and
de-dup some of the information, and finally embed a mojo::URLRequest inside
there and replace all the nested duplicate fields that are that with the
instance.

pfeldman@'s patch here feels reasonable, it's moving things between these
already ambiguous looking classes, and it definitely feels like a better
intermediate state than plumbing bools while you all figure out the right
layering and refactoring around mojo and onion soup.

(Also I'm pretty sure content::ResourceRequest should go away long term, blink
will speak mojo to the network service, which will use some representation
internally of requests, there's no reason to have the content::Foo types, just
the stuff inside services/network.)

[yhirano, 2017-03-17 07:05:11]

On 2017/03/17 06:45:52, esprehn wrote:
> I'd prefer we didn't add booleans to the callbacks, those aren't very good for
> long term maintainability. Looking at the code the FetchRequest and the
> ResourceRequest both seem very complex, and the ResourceRequest is being
passed
> around by value even though it's quite large.
> 
Yeah, pfeldman@'s next CL won't change the callback's interface, and I'm happy
with that.

> Note that the public API doesn't need to mirror the API inside blink (ex.
> WebURLRequest can be it's own thing), and ideally the public API usage is
> decreasing and surface area is also shrinking. :)
> 
We are making efforts to delete WebURLRequest.

> Having studied the code I think the best thing might be to merge FetchRequest
> and ResourceRequest into a single object and just pass them both into all of
> these delegate callbacks through the layers inside blink, it was pretty
> confusing to me why FetchRequest goes in, but all the callbacks are
> ResourceRequest omitting all those extra state fields. Then we can merge and
> de-dup some of the information, and finally embed a mojo::URLRequest inside
> there and replace all the nested duplicate fields that are that with the
> instance.
> 

Merging FetchRequest and ResourceRequest is a big design change and I think we
should discuss the topic at loading-dev@, not here.

> pfeldman@'s patch here feels reasonable, it's moving things between these
> already ambiguous looking classes, and it definitely feels like a better
> intermediate state than plumbing bools while you all figure out the right
> layering and refactoring around mojo and onion soup.
> 

> (Also I'm pretty sure content::ResourceRequest should go away long term, blink
> will speak mojo to the network service, which will use some representation
> internally of requests, there's no reason to have the content::Foo types, just
> the stuff inside services/network.)

I think this doesn't affect my point that blink::ResourceRequest is, and will be
a representation of mojom::URLRequest, right?

[kouhei (in TOK), 2017-03-17 07:29:00]

On 2017/03/17 07:05:11, yhirano wrote:
> On 2017/03/17 06:45:52, esprehn wrote:
> > I'd prefer we didn't add booleans to the callbacks, those aren't very good
for
> > long term maintainability. Looking at the code the FetchRequest and the
> > ResourceRequest both seem very complex, and the ResourceRequest is being
> passed
> > around by value even though it's quite large.
> > 
> Yeah, pfeldman@'s next CL won't change the callback's interface, and I'm happy
> with that.
> 
> > Note that the public API doesn't need to mirror the API inside blink (ex.
> > WebURLRequest can be it's own thing), and ideally the public API usage is
> > decreasing and surface area is also shrinking. :)
> > 
> We are making efforts to delete WebURLRequest.
> 
> > Having studied the code I think the best thing might be to merge
FetchRequest
> > and ResourceRequest into a single object and just pass them both into all of
> > these delegate callbacks through the layers inside blink, it was pretty
> > confusing to me why FetchRequest goes in, but all the callbacks are
> > ResourceRequest omitting all those extra state fields. Then we can merge and
> > de-dup some of the information, and finally embed a mojo::URLRequest inside
> > there and replace all the nested duplicate fields that are that with the
> > instance.
> > 
> 
> Merging FetchRequest and ResourceRequest is a big design change and I think we
> should discuss the topic at loading-dev@, not here.
+1 to moving this discussion to loading-dev@.
I think this design discussion needs wider audience and consensus.

> > pfeldman@'s patch here feels reasonable, it's moving things between these
> > already ambiguous looking classes, and it definitely feels like a better
> > intermediate state than plumbing bools while you all figure out the right
> > layering and refactoring around mojo and onion soup.
> > 
> 
> > (Also I'm pretty sure content::ResourceRequest should go away long term,
blink
> > will speak mojo to the network service, which will use some representation
> > internally of requests, there's no reason to have the content::Foo types,
just
> > the stuff inside services/network.)
> 
> I think this doesn't affect my point that blink::ResourceRequest is, and will
be
> a representation of mojom::URLRequest, right?

I was proposing the boolean as an intermediate state, which is "ugly but can
live w/ it for a month" thing to unblock this work.
Looks like the best option here is the design pfeldman@ suggested in #37?

[pfeldman, 2017-03-17 07:35:21]

(I could not merge it into FetchInitiator since that was making us dupe that
boolean in both request and options, so the question stands)

I think what you are suggesting and Elliott is saying is the same thing in the
end, you just need to agree on the order of things. There will be a single class
representing request in blink and that class would aggregate mojom::URLRequest.
It seems like your line of though is 

1) ResourceRequest gets replaced with mojom::URLRequest
2) FetchRequest becomes the class blink uses

while Elliott is suggesting to

1) Merge FetchRequest with ResourceRequest
2) Replace some of the members of the new entity with mojom::URLRequest in order
to dedupe.

The result is exactly the same. I'll let you decide which order that would be,
but in terms of this change moving the bit from one to another is definitely
better than plumbing it via the callbacks. So I would ask you to approve my
patch set #3. Also, not that since historically blink/ was operating on
ResourceRequest, so it might be easier / more incremental to merge FetchRequest
into the ResourceRequest, in which case my change is simply the first step in
that direction.

[pfeldman, 2017-03-17 07:36:30]

s/Also not/Also note/ :)

[yhirano, 2017-03-17 08:04:53]

Does https://codereview.chromium.org/2751023005/ work for you?

[pfeldman, 2017-03-17 08:24:15]

Technically it solves my issue, but it is backwards to both versions of the
plan. We need to cetralize the bits that make sense to request instead of
burying it in a poorly defined fetch initiator. In fact, I would start with
removing that class altogether.

FetchInitiator should not carry any load the core functionality depends upon and
this change breaks this assumption. That's why I was saying I could not do it
without the dupe and gave up.

[yhirano, 2017-03-17 08:58:51]

On 2017/03/17 08:24:15, pfeldman_slow wrote:
> Technically it solves my issue, but it is backwards to both versions of the
> plan. We need to cetralize the bits that make sense to request instead of
> burying it in a poorly defined fetch initiator. In fact, I would start with
> removing that class altogether.
> 
> FetchInitiator should not carry any load the core functionality depends upon
and
> this change breaks this assumption. That's why I was saying I could not do it
> without the dupe and gave up.

Hmm.

> I think what you are suggesting and Elliott is saying is the same thing in the
> end, you just need to agree on the order of things. There will be a single
class
> representing request in blink and that class would aggregate
mojom::URLRequest.
> It seems like your line of though is 

> 1) ResourceRequest gets replaced with mojom::URLRequest
> 2) FetchRequest becomes the class blink uses

For the record, 'A FetchRequest is a "parameter object" for
ResourceFetcher::requestResource to avoid the method having too many arguments.'
according to the comment in FetchRequest.h and I don't have a plan to change the
role.

I'll start a thread in loading-dev@. Let the discussion block this CL.

Thanks,

[Takashi Toyoshima, 2017-03-17 09:36:42]

I feel merging FetchRequest and ResourceRequest isn't a good idea. Having all
data in one place and just exposing it to all delegate methods would easily
increase mutual code dependencies unnecessarily, and make it hard to change the
design in the future. Also it sounds conflicting with the original intention to
have the FetchRequest.

We discuss what the FetchRequest is, a few days ago in Tokyo loading meeting.
ResourceRequest is alive while corresponding Resource is alive, but FetchRequest
is originally designed to pass so many arguments to *Resource:fetch() or
ResourceFetcher::requestResource() within single object. So I think
ResourceRequest should contain only information that is needed while
corresponding Resource instance is alive, and FetchRequest could contain other
temporary information that is only needed for fetch().

From this viewpoint, preload information would be better to live in
ResourceRequest.

Probably the current design that FetchRequest contains ResourceRequest gives the
impression that there are two duplicated similar and confusing things? How about
removing ResourceRequest reference from FetchRequest, and renaming FetchRequest
to FetchReuestOptions or something, then passing both together to fetch() and so
on? Also we may want to make FetchRequest a struct rather than a class?

Anyway, it would be better to discuss Fetchrequest vs ResourceRequest in another
place? This CL is not needed to be blocked by this discussion.

[yhirano, 2017-03-17 11:54:54]

On 2017/03/17 09:36:42, Takashi Toyoshima wrote:
> I feel merging FetchRequest and ResourceRequest isn't a good idea. Having all
> data in one place and just exposing it to all delegate methods would easily
> increase mutual code dependencies unnecessarily, and make it hard to change
the
> design in the future. Also it sounds conflicting with the original intention
to
> have the FetchRequest.
> 
> We discuss what the FetchRequest is, a few days ago in Tokyo loading meeting.
> ResourceRequest is alive while corresponding Resource is alive, but
FetchRequest
> is originally designed to pass so many arguments to *Resource:fetch() or
> ResourceFetcher::requestResource() within single object. So I think
> ResourceRequest should contain only information that is needed while
> corresponding Resource instance is alive, and FetchRequest could contain other
> temporary information that is only needed for fetch().
> 
> From this viewpoint, preload information would be better to live in
> ResourceRequest.
> 
> Probably the current design that FetchRequest contains ResourceRequest gives
the
> impression that there are two duplicated similar and confusing things? How
about
> removing ResourceRequest reference from FetchRequest, and renaming
FetchRequest
> to FetchReuestOptions or something, then passing both together to fetch() and
so
> on? Also we may want to make FetchRequest a struct rather than a class?
> 
> Anyway, it would be better to discuss Fetchrequest vs ResourceRequest in
another
> place? This CL is not needed to be blocked by this discussion.

Started a thread:
https://groups.google.com/a/chromium.org/forum/#!topic/loading-dev/xMQ4WGn870E .
Let's discuss there.
If we land a change that starts merging FetchRequest and ResourceRequest, I
would like to get consensus there before that.

[pfeldman, 2017-03-24 17:57:51]

Does this patch make more sense or less sense after the discussion?

[tyoshino (SeeGerritForStatus), 2017-03-27 05:41:08]

https://codereview.chromium.org/2751043002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2751043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:641:
requestInfo->setIsLinkPreload(true);
does it make sense to include these in buildObjectForResourceRequest()? maybe
also setReferrerPolicy at L639?

https://codereview.chromium.org/2751043002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/FetchRequest.h (right):

https://codereview.chromium.org/2751043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchRequest.h:106: //
TODO(toyoshim): do not use this method, to be removed.
let's put the warning outside of the TODO as it's a message for everyone.

Do not use this method, to be removed.
// TODO(toyoshim): Remove this

[yhirano, 2017-03-27 06:47:57]

The discussion's conclusions are:

 1. Keep the distinction between FetchRequest and ResourceRequest.
 2. Stop FetchRequest from holding ResourceRequest, rename it to
FetchParameters, and provide them separately to functions that need them.

So I think https://codereview.chromium.org/2751023005/ is better.

[pfeldman, 2017-03-27 21:07:24]

Closing in favor of https://codereview.chromium.org/2751023005/