MixedContentChecker should not log blocked preload requests

MixedContentChecker should not log blocked preload requests

This CL adds a SuppressLog argument to MixedContentChecker::shouldBlockFetch to suppress logging for preload requests.
This makes it consistent with content security policy checks which also do not warn on preload requests.

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

[kouhei (in TOK), 2014-11-14 03:24:51]

kouhei@chromium.org changed reviewers:
+ japhet@chromium.org, mkwst@chromium.org, tyoshino@chromium.org

[kouhei (in TOK), 2014-11-14 03:25:08]

Would you take a look? This is to address comment on
https://codereview.chromium.org/724653002/

[Mike West, 2014-11-14 08:08:17]

Thanks for this. I'd like to see a test to verify the behavior; it surprises me
a little that we don't have any tests accidentally dumping multiple messages.

https://codereview.chromium.org/726563004/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/726563004/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:1259: else
Why this change?

https://codereview.chromium.org/726563004/diff/1/Source/core/loader/MixedCont...
File Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/726563004/diff/1/Source/core/loader/MixedCont...
Source/core/loader/MixedContentChecker.cpp:283: if (suppressLog != SuppressLog)
Nit: I find `== LogIfBlocked` clearer than the double-negative.

https://codereview.chromium.org/726563004/diff/1/Source/core/loader/MixedCont...
File Source/core/loader/MixedContentChecker.h (right):

https://codereview.chromium.org/726563004/diff/1/Source/core/loader/MixedCont...
Source/core/loader/MixedContentChecker.h:51: enum SuppressLogType {
LogIfBlocked, SuppressLog };
Please use the same language as we use in CSP:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

It might be worthwhile to move that enum somewhere else, actually, so we can
just use it whenever we're dealing with this kind of problem.

[kouhei (in TOK), 2014-11-14 09:19:31]

On 2014/11/14 08:08:17, Mike West wrote:
> Thanks for this. I'd like to see a test to verify the behavior; it surprises
me
> a little that we don't have any tests accidentally dumping multiple messages.

This code path will be tested on LayoutTests to be added in
https://codereview.chromium.org/724653002/

https://codereview.chromium.org/726563004/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/726563004/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:1259: else
On 2014/11/14 08:08:17, Mike West wrote:
> Why this change?
When fetchImage fails from CSP/mixedContent check, it returns null.
Second request which also fails was attempted before this CL.

https://codereview.chromium.org/726563004/diff/1/Source/core/loader/MixedCont...
File Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/726563004/diff/1/Source/core/loader/MixedCont...
Source/core/loader/MixedContentChecker.cpp:283: if (suppressLog != SuppressLog)
On 2014/11/14 08:08:17, Mike West wrote:
> Nit: I find `== LogIfBlocked` clearer than the double-negative.

Done.

https://codereview.chromium.org/726563004/diff/1/Source/core/loader/MixedCont...
File Source/core/loader/MixedContentChecker.h (right):

https://codereview.chromium.org/726563004/diff/1/Source/core/loader/MixedCont...
Source/core/loader/MixedContentChecker.h:51: enum SuppressLogType {
LogIfBlocked, SuppressLog };
On 2014/11/14 08:08:17, Mike West wrote:
> Please use the same language as we use in CSP:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Done.
 
> It might be worthwhile to move that enum somewhere else, actually, so we can
> just use it whenever we're dealing with this kind of problem.

I'm not sure where the best place for the enum would be. Do you think we can
include this in ResourceRequest?

[Mike West, 2014-11-14 10:09:41]

I'd suggest just merging the layout tests from the other CL into this one.
There's no reason to land this change separately from the tests which will
verify it. If this CL is somehow broken and we have to revert it, we'd then also
have to revert the other CL (or skip the tests).

LGTM otherwise. I'll look for a common spot to put the enum when I have some
free time. Nothing you need to worry about in this CL.

[kouhei (in TOK), 2014-11-17 03:43:14]

Thanks! merged CLs.

[kouhei (in TOK), 2014-11-17 03:43:17]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-11-17 03:43:39]

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

[tyoshino (SeeGerritForStatus), 2014-11-17 05:14:23]

lgtm

[commit-bot: I haz the power, 2014-11-17 05:58:19]

Committed patchset #3 (id:40001) as 185420

[kouhei (in TOK), 2014-11-19 02:07:30]

Patchset #4 (id:60001) has been deleted