Refactor the forPreload flag to mean speculative preload.

Refactor the forPreload to mean speculative preload.

Currently the forPreload() flag means both speculative preload as well
as link preload. as the linkPreload flag indicates link preload, the
forPreload flag is confusing.
This CL renames the flag to speculativePreload and makes sure that code
that currently uses it uses the appropriate signal: link, speculative or
both.

BUG=675883
Review-Url: https://codereview.chromium.org/2676163002
Cr-Commit-Position: refs/heads/master@{#448595}
Committed: https://chromium.googlesource.com/chromium/src/+/58831855e1f0366cf7a9eae129c6d10b711d3195

[Yoav Weiss, 2017-02-05 08:02:01]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2017-02-05 08:02:22]

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

[Yoav Weiss, 2017-02-05 08:27:07]

Hey Charlie,

Can you take a look to see this makes sense? I want to split the semantics of
forPreload and linkPreload which are currently mixed.

There's no intended functionality change, but there may be some side effects
from changing the flag semantics. Let me know if you catch any.

[Yoav Weiss, 2017-02-05 08:27:31]

yoav@yoav.ws changed reviewers:
+ csharrison@chromium.org

[Yoav Weiss, 2017-02-05 08:27:54]

Description was changed from

==========
Refactor the forPreload to mean speculative preload.

Currently the forPreload() flag means both speculative preload as well
as link preload. as the linkPreload flag indicates link preload, the
forPreload flag is confusing.
This CL renames the flag to speculativePreload and makes sure that code
that currently uses it uses the appropriate signal: link, speculative or
both.

BUG=
==========

to

==========
Refactor the forPreload to mean speculative preload.

Currently the forPreload() flag means both speculative preload as well
as link preload. as the linkPreload flag indicates link preload, the
forPreload flag is confusing.
This CL renames the flag to speculativePreload and makes sure that code
that currently uses it uses the appropriate signal: link, speculative or
both.

BUG=675883
==========

[commit-bot: I haz the power, 2017-02-05 09:36:20]

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

[commit-bot: I haz the power, 2017-02-05 09:36:21]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-02-05 18:39:12]

Generally LG. Feel free to push back on comment nits :P

I'll take a stab at looking at some of your TODOs in another pass to see if we
can clarify behavior.

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/LinkLoader.cpp (left):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/LinkLoader.cpp:342:
linkRequest.setForPreload(true, monotonicallyIncreasingTime());
I think this will affect metrics, but I think it is correct and those metrics
should only be for speculative preloads.

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:158:
request.options(), request.speculativePreload(),
nit: Can you inline /* suppressReport */ here? It is not immediately clear that
that's what this bool is doing.

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/FetchRequest.h (right):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/FetchRequest.h:62: // type. See
https://crbug.com/675883.
Move this to previous line.

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:282: //
preload.
I think this should still be for all preloads, not just link preloads. AFAIK we
don't have a way currently to speculatively preload fonts, but in principle we
should be able to if we had a robust enough speculation.

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:429:
resource->isLinkPreload() && !request.isLinkPreload()) {
nit: can we move the isLinkPreload and !isLinkPreload before the contains() call
for efficiency?

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:430:
m_nonBlockingLoaders.remove(resource->loader());
I am slightly confused what this method does, do you mind adding some
documentation?

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp:264:
response.url(), resource->options(), unusedPreload,
nit: inline comment that this bool is used to suppress reports

[Yoav Weiss, 2017-02-06 09:58:35]

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/LinkLoader.cpp (left):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/LinkLoader.cpp:342:
linkRequest.setForPreload(true, monotonicallyIncreasingTime());
On 2017/02/05 18:39:11, Charlie Harrison-slow til Feb3 wrote:
> I think this will affect metrics, but I think it is correct and those metrics
> should only be for speculative preloads.

Yeah, I agree

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:158:
request.options(), request.speculativePreload(),
On 2017/02/05 18:39:11, Charlie Harrison-slow til Feb3 wrote:
> nit: Can you inline /* suppressReport */ here? It is not immediately clear
that
> that's what this bool is doing.

I agree that currently the call-site is not very clear, but maybe an enum would
be a better choice to make the semantics conversion (i.e. preload/unusedPreload
means suppressed logs/reports) explicit. They also enforce an explicit decision
from all call sites, and called out as preferred in the style guide.

Let me know what you think.

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/FetchRequest.h (right):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/FetchRequest.h:62: // type. See
https://crbug.com/675883.
On 2017/02/05 18:39:11, Charlie Harrison-slow til Feb3 wrote:
> Move this to previous line.

ok

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:282: //
preload.
On 2017/02/05 18:39:12, Charlie Harrison-slow til Feb3 wrote:
> I think this should still be for all preloads, not just link preloads. AFAIK
we
> don't have a way currently to speculatively preload fonts, but in principle we
> should be able to if we had a robust enough speculation.

I don't know that it's worth-while to pay for an extra condition here just in
case we would start preloading fonts in the future. Wouldn't it be better to add
that condition once it's relevant?
Can we perhaps add a comment somewhere in the preloadScanner code to point folks
that may attempt it here? (Not sure where would be the ideal place for such a
comment)

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:429:
resource->isLinkPreload() && !request.isLinkPreload()) {
On 2017/02/05 18:39:12, Charlie Harrison-slow til Feb3 wrote:
> nit: can we move the isLinkPreload and !isLinkPreload before the contains()
call
> for efficiency?

Good call

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:430:
m_nonBlockingLoaders.remove(resource->loader());
On 2017/02/05 18:39:12, Charlie Harrison-slow til Feb3 wrote:
> I am slightly confused what this method does, do you mind adding some
> documentation?

renamed the method to a hopefully clearer name, and improved comment on the call
site. Let me know if you think further documentation is needed :)

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp:264:
response.url(), resource->options(), unusedPreload,
On 2017/02/05 18:39:12, Charlie Harrison-slow til Feb3 wrote:
> nit: inline comment that this bool is used to suppress reports 

enummed

[Yoav Weiss, 2017-02-06 09:59:20]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 09:59:42]

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-02-06 11:52:28]

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

[commit-bot: I haz the power, 2017-02-06 11:52:29]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-02-06 19:54:16]

LGTM

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:158:
request.options(), request.speculativePreload(),
On 2017/02/06 09:58:35, Yoav Weiss wrote:
> On 2017/02/05 18:39:11, Charlie Harrison-slow til Feb3 wrote:
> > nit: Can you inline /* suppressReport */ here? It is not immediately clear
> that
> > that's what this bool is doing.
> 
> I agree that currently the call-site is not very clear, but maybe an enum
would
> be a better choice to make the semantics conversion (i.e.
preload/unusedPreload
> means suppressed logs/reports) explicit. They also enforce an explicit
decision
> from all call sites, and called out as preferred in the style guide.
> 
> Let me know what you think.

Yes an enum sounds good.

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2676163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:282: //
preload.
On 2017/02/06 09:58:35, Yoav Weiss wrote:
> On 2017/02/05 18:39:12, Charlie Harrison-slow til Feb3 wrote:
> > I think this should still be for all preloads, not just link preloads. AFAIK
> we
> > don't have a way currently to speculatively preload fonts, but in principle
we
> > should be able to if we had a robust enough speculation.
> 
> I don't know that it's worth-while to pay for an extra condition here just in
> case we would start preloading fonts in the future. Wouldn't it be better to
add
> that condition once it's relevant?
> Can we perhaps add a comment somewhere in the preloadScanner code to point
folks
> that may attempt it here? (Not sure where would be the ideal place for such a
> comment)

Your call, it isn't a huge deal. Probably the best place for an extra comment
would be in the css preload scanner maybe?

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

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:127: enum class
LoggingPolicy { SuppressLogging, Log };
This is much better, but I wonder if we can clarify "Logging" in some way.

It doesn't mean UMA logging, TRACE_EVENT logging, or report logging. If you
don't think it's too specific (will anything else fall in this category?), I'd
just change it to V8ActivityLoggingPolicy or something.

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:186: //
TODO(yoav): It is not clear why this applies only to preloaded scripts.
I did some blaming to trace this logic. It stems from pmeenans priority work. I
think the problem was that the preload scanner would scan too far ahead in some
cases, treating script way ahead in the document as more important than it
should be. Arguably we could apply that logic to *all* scripts late in the
document, but the preload bit is an OK signal that we can probably lower
priority.

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:443: if
(request.speculativePreload() || request.isLinkPreload()) {
nit: seeing these together brings out the inconsistency in naming. Can we rename
to isSpeculativePreload? That better adheres to blink style.

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:581: //
TODO(yoav): What's the scenario where this can actually happen?
I can't think/see of any and this code is decently old (~June 2015). I would
support removing this block and adding CHECK_EQ(resource->getType(),
factory.type()). Possibly CQ will complain if this actually gets hit.

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:591: //
TODO(yoav): It is not clear why preloads are exempt from this check. Can we
This logic is very old [1], and priority calculations based on preloads should
not live here. Let's try changing it in a follow-up?

[1]
https://chromium.googlesource.com/chromium/src/+/79ff7ef8ea33fa66ff2792a7c432...

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

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h:196: void
makePreloadedResourceBlockOnloadIfNeeded(Resource*, const FetchRequest&);
I love it :D

[Yoav Weiss, 2017-02-06 23:15:49]

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

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:127: enum class
LoggingPolicy { SuppressLogging, Log };
On 2017/02/06 19:54:16, Charlie Harrison wrote:
> This is much better, but I wonder if we can clarify "Logging" in some way.
> 
> It doesn't mean UMA logging, TRACE_EVENT logging, or report logging. If you
> don't think it's too specific (will anything else fall in this category?), I'd
> just change it to V8ActivityLoggingPolicy or something.

done

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:186: //
TODO(yoav): It is not clear why this applies only to preloaded scripts.
On 2017/02/06 19:54:16, Charlie Harrison wrote:
> I did some blaming to trace this logic. It stems from pmeenans priority work.
I
> think the problem was that the preload scanner would scan too far ahead in
some
> cases, treating script way ahead in the document as more important than it
> should be. Arguably we could apply that logic to *all* scripts late in the
> document, but the preload bit is an OK signal that we can probably lower
> priority.

Changed to a comment stating the above

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:443: if
(request.speculativePreload() || request.isLinkPreload()) {
On 2017/02/06 19:54:16, Charlie Harrison wrote:
> nit: seeing these together brings out the inconsistency in naming. Can we
rename
> to isSpeculativePreload? That better adheres to blink style.

changed

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:581: //
TODO(yoav): What's the scenario where this can actually happen?
On 2017/02/06 19:54:16, Charlie Harrison wrote:
> I can't think/see of any and this code is decently old (~June 2015). I would
> support removing this block and adding CHECK_EQ(resource->getType(),
> factory.type()). Possibly CQ will complain if this actually gets hit.

I'll do that in a followup

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:591: //
TODO(yoav): It is not clear why preloads are exempt from this check. Can we
On 2017/02/06 19:54:16, Charlie Harrison wrote:
> This logic is very old [1], and priority calculations based on preloads should
> not live here. Let's try changing it in a follow-up?
> 
> [1]
>
https://chromium.googlesource.com/chromium/src/+/79ff7ef8ea33fa66ff2792a7c432...

sure

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

https://codereview.chromium.org/2676163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h:196: void
makePreloadedResourceBlockOnloadIfNeeded(Resource*, const FetchRequest&);
On 2017/02/06 19:54:16, Charlie Harrison wrote:
> I love it :D

cool :)

[Yoav Weiss, 2017-02-06 23:17:50]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[Yoav Weiss, 2017-02-06 23:18:14]

The CQ bit was unchecked by yoav@yoav.ws

[Yoav Weiss, 2017-02-06 23:18:24]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2017-02-06 23:18:25]

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

[commit-bot: I haz the power, 2017-02-06 23:18:42]

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-02-06 23:28:38]

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

[commit-bot: I haz the power, 2017-02-06 23:28:39]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Yoav Weiss, 2017-02-07 04:54:02]

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

[Yoav Weiss, 2017-02-07 04:54:03]

Hey Mike,

Can you take a look? (need an owner, since the fetch/ code moved out of core/
and into platform/)

[Takashi Toyoshima, 2017-02-07 07:23:17]

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

[Takashi Toyoshima, 2017-02-07 07:24:49]

Thank you for working on this.
I added yhirano@ to the reviewer list since he was the original reviewer who
asked to file the bug 675883, and can review platform/loader.

[kinuko, 2017-02-07 08:20:25]

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

[kinuko, 2017-02-07 08:20:26]

lgtm % nits

Nice cleanup!

https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:751: // items.
nit: update the comment?

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

https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:137: enum class
ReportingPolicy { SuppressReporting, Report };
The enum name 'ReportingPolicy' sounds very generic / is not very helpful for
readers to guess what this is about, could we have a comment and/or give it a
bit more specific name?

[yhirano, 2017-02-07 08:34:07]

lgtm

https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:750: // Don't send
CSP messages for preloads, we might never actually display those
This comment should be moved to caller now.

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

https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:137: enum class
ReportingPolicy { SuppressReporting, Report };
On 2017/02/07 08:20:26, kinuko wrote:
> The enum name 'ReportingPolicy' sounds very generic / is not very helpful for
> readers to guess what this is about, could we have a comment and/or give it a
> bit more specific name?

Having a generic ReportingPolicy would be good. We have one here, one in CSP,
one in MixedContent. Can we merge them (not in this CL)?

[kinuko, 2017-02-07 08:41:13]

On 2017/02/07 08:34:07, yhirano wrote:
> lgtm
> 
>
https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):
> 
>
https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:750: // Don't send
> CSP messages for preloads, we might never actually display those
> This comment should be moved to caller now.
> 
>
https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/loader/fetch/FetchContext.h (right):
> 
>
https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:137: enum class
> ReportingPolicy { SuppressReporting, Report };
> On 2017/02/07 08:20:26, kinuko wrote:
> > The enum name 'ReportingPolicy' sounds very generic / is not very helpful
for
> > readers to guess what this is about, could we have a comment and/or give it
a
> > bit more specific name?
> 
> Having a generic ReportingPolicy would be good. We have one here, one in CSP,
> one in MixedContent. Can we merge them (not in this CL)?

Yeah +1 for doing this (in a separate CL)

[Yoav Weiss, 2017-02-07 09:10:27]

https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:750: // Don't send
CSP messages for preloads, we might never actually display those
On 2017/02/07 08:34:06, yhirano wrote:
> This comment should be moved to caller now.

agreed

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

https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:137: enum class
ReportingPolicy { SuppressReporting, Report };
On 2017/02/07 08:34:06, yhirano wrote:
> On 2017/02/07 08:20:26, kinuko wrote:
> > The enum name 'ReportingPolicy' sounds very generic / is not very helpful
for
> > readers to guess what this is about, could we have a comment and/or give it
a
> > bit more specific name?
> 
> Having a generic ReportingPolicy would be good. We have one here, one in CSP,
> one in MixedContent. Can we merge them (not in this CL)?

I'd support that. It'd simplify the code, avoiding conversions from one enum to
the next.

[Yoav Weiss, 2017-02-07 09:13:08]

On 2017/02/07 09:10:27, Yoav Weiss wrote:
>
https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):
> 
>
https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:750: // Don't send
> CSP messages for preloads, we might never actually display those
> On 2017/02/07 08:34:06, yhirano wrote:
> > This comment should be moved to caller now.
> 
> agreed
> 
>
https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/loader/fetch/FetchContext.h (right):
> 
>
https://codereview.chromium.org/2676163002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:137: enum class
> ReportingPolicy { SuppressReporting, Report };
> On 2017/02/07 08:34:06, yhirano wrote:
> > On 2017/02/07 08:20:26, kinuko wrote:
> > > The enum name 'ReportingPolicy' sounds very generic / is not very helpful
> for
> > > readers to guess what this is about, could we have a comment and/or give
it
> a
> > > bit more specific name?
> > 
> > Having a generic ReportingPolicy would be good. We have one here, one in
CSP,
> > one in MixedContent. Can we merge them (not in this CL)?
> 
> I'd support that. It'd simplify the code, avoiding conversions from one enum
to
> the next.

With that said, I can also rename the current one for that CL. Would
SecurityViolationReportingPolicy work?

[Yoav Weiss, 2017-02-07 09:29:02]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 09:29:27]

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-02-07 10:57:11]

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

[commit-bot: I haz the power, 2017-02-07 10:57:12]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2017-02-07 11:00:50]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2017-02-07 11:00:50]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
yhirano@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2676163002/#ps60001
(title: "Renamed ReportingPolicy and moved comments")

[commit-bot: I haz the power, 2017-02-07 11:01:09]

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-02-07 11:05:39]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486465250077780,
"parent_rev": "061102c727668c85c166f7b2e736ed5e88196fee", "commit_rev":
"58831855e1f0366cf7a9eae129c6d10b711d3195"}

[commit-bot: I haz the power, 2017-02-07 11:06:17]

Description was changed from

==========
Refactor the forPreload to mean speculative preload.

Currently the forPreload() flag means both speculative preload as well
as link preload. as the linkPreload flag indicates link preload, the
forPreload flag is confusing.
This CL renames the flag to speculativePreload and makes sure that code
that currently uses it uses the appropriate signal: link, speculative or
both.

BUG=675883
==========

to

==========
Refactor the forPreload to mean speculative preload.

Currently the forPreload() flag means both speculative preload as well
as link preload. as the linkPreload flag indicates link preload, the
forPreload flag is confusing.
This CL renames the flag to speculativePreload and makes sure that code
that currently uses it uses the appropriate signal: link, speculative or
both.

BUG=675883

Review-Url: https://codereview.chromium.org/2676163002
Cr-Commit-Position: refs/heads/master@{#448595}
Committed:
https://chromium.googlesource.com/chromium/src/+/58831855e1f0366cf7a9eae129c6...
==========

[commit-bot: I haz the power, 2017-02-07 11:06:18]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/58831855e1f0366cf7a9eae129c6...