Remove LOAD_SUB_FRAME load flag.

Remove LOAD_SUB_FRAME load flag.

It's not used in net, and net/ knowing about frames is a layering
violation.  External consumers that depended on it now use the
corresponding ResourceType instead.

BUG=426442
Committed: https://crrev.com/f1aa907921ed7fd95c8a9fb35e41aaca36a31067
Cr-Commit-Position: refs/heads/master@{#330370}

[mmenke, 2015-05-11 15:43:18]

mmenke@chromium.org changed reviewers:
+ davidben@chromium.org

[mmenke, 2015-05-11 15:43:19]

David:  Just a simple Friday afternoon "I don't feel like starting anything
major" CL.  Sending this your way because I consider this an RDH CL.

A bit disturbing that I didn't have to modify any tests for either the RDH or
the ConnectInterceptor for this CL to pass tests.

[davidben, 2015-05-12 00:49:48]

https://codereview.chromium.org/1134733004/diff/40001/chrome/browser/net/conn...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/1134733004/diff/40001/chrome/browser/net/conn...
chrome/browser/net/connect_interceptor.cc:34: // TODO(mmenke):  Should the
predictor really be red requests without a
red -> fed?

https://codereview.chromium.org/1134733004/diff/40001/chrome/browser/net/conn...
chrome/browser/net/connect_interceptor.cc:37: content::ResourceType resoure_type
= info->GetResourceType();
resource_type

https://codereview.chromium.org/1134733004/diff/40001/components/policy/core/...
File components/policy/core/browser/url_blacklist_manager.h (right):

https://codereview.chromium.org/1134733004/diff/40001/components/policy/core/...
components/policy/core/browser/url_blacklist_manager.h:163: // Only requests fo
frames may be blocked; other sub resources or background
fo -> for

https://codereview.chromium.org/1134733004/diff/40001/components/policy/core/...
components/policy/core/browser/url_blacklist_manager.h:163: // Only requests fo
frames may be blocked; other sub resources or background
Why bother calling this function when is_request_for_frame is false anyway? You
already know it's not going to do anything.

[mmenke, 2015-05-12 15:41:48]

Patchset #4 (id:60001) has been deleted

[mmenke, 2015-05-12 16:11:40]

https://codereview.chromium.org/1134733004/diff/40001/chrome/browser/net/conn...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/1134733004/diff/40001/chrome/browser/net/conn...
chrome/browser/net/connect_interceptor.cc:34: // TODO(mmenke):  Should the
predictor really be red requests without a
On 2015/05/12 00:49:47, David Benjamin wrote:
> red -> fed?

Done...  I should take the time to skim over CLs before review, like I used to.

https://codereview.chromium.org/1134733004/diff/40001/chrome/browser/net/conn...
chrome/browser/net/connect_interceptor.cc:37: content::ResourceType resoure_type
= info->GetResourceType();
On 2015/05/12 00:49:47, David Benjamin wrote:
> resource_type

Done.

https://codereview.chromium.org/1134733004/diff/40001/components/policy/core/...
File components/policy/core/browser/url_blacklist_manager.h (right):

https://codereview.chromium.org/1134733004/diff/40001/components/policy/core/...
components/policy/core/browser/url_blacklist_manager.h:163: // Only requests fo
frames may be blocked; other sub resources or background
On 2015/05/12 00:49:47, David Benjamin wrote:
> fo -> for

Done.

https://codereview.chromium.org/1134733004/diff/40001/components/policy/core/...
components/policy/core/browser/url_blacklist_manager.h:163: // Only requests fo
frames may be blocked; other sub resources or background
On 2015/05/12 00:49:47, David Benjamin wrote:
> Why bother calling this function when is_request_for_frame is false anyway?
You
> already know it's not going to do anything.

Was just thinking from the perspective of keeping logic out of
ChromeNetworkDelegate, given all the random cruft it has, and the fact that it's
not unit tests.  Does seem a bit silly, though.  I've gone ahead and removed the
argument.

[mmenke, 2015-05-12 16:30:44]

mmenke@chromium.org changed reviewers:
+ atwilson@chromium.org

[mmenke, 2015-05-12 16:30:45]

[+atwilson]:  Please review chrome/browser/policy/ and
components/policy/core/browser/.

[davidben, 2015-05-12 16:43:02]

lgtm

[Andrew T Wilson (Slow), 2015-05-12 16:44:30]

https://codereview.chromium.org/1134733004/diff/100001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1134733004/diff/100001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:383: if (info &&
content::IsResourceTypeFrame(info->GetResourceType()) &&
Under what situations might info be null? I want to make sure there isn't some
way for frame loads to skip the blacklist check below...

https://codereview.chromium.org/1134733004/diff/100001/chrome/browser/policy/...
File chrome/browser/policy/url_blacklist_manager_unittest.cc (right):

https://codereview.chromium.org/1134733004/diff/100001/chrome/browser/policy/...
chrome/browser/policy/url_blacklist_manager_unittest.cc:644:
GURL("http://google.com"), &reason));
This is no longer checking the reason for the request being blocked - should
this also have:

EXPECT_EQ(net::ERR_BLOCKED_BY_ADMINISTRATOR, reason);

?

Also, we are assuming that ShouldBlockRequestForFrame() won't be called for
subresource requests - is there any test that ensure this is true?

https://codereview.chromium.org/1134733004/diff/100001/components/policy/core...
File components/policy/core/browser/url_blacklist_manager.h (right):

https://codereview.chromium.org/1134733004/diff/100001/components/policy/core...
components/policy/core/browser/url_blacklist_manager.h:164: // etc) should not
be not filtered. The sync signin page will also also not be
nit: also also

[mmenke, 2015-05-12 16:59:13]

Thanks for the feedback!

https://codereview.chromium.org/1134733004/diff/100001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1134733004/diff/100001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:383: if (info &&
content::IsResourceTypeFrame(info->GetResourceType()) &&
On 2015/05/12 16:44:30, Andrew T Wilson wrote:
> Under what situations might info be null? I want to make sure there isn't some
> way for frame loads to skip the blacklist check below...

If a request doesn't go through the ResourceDispatcherHost (In which case, it
wouldn't be a main frame page load, either).  So most anything issued from the
browser process.

https://codereview.chromium.org/1134733004/diff/100001/chrome/browser/policy/...
File chrome/browser/policy/url_blacklist_manager_unittest.cc (right):

https://codereview.chromium.org/1134733004/diff/100001/chrome/browser/policy/...
chrome/browser/policy/url_blacklist_manager_unittest.cc:644:
GURL("http://google.com"), &reason));
On 2015/05/12 16:44:30, Andrew T Wilson wrote:
> This is no longer checking the reason for the request being blocked - should
> this also have:
> 
> EXPECT_EQ(net::ERR_BLOCKED_BY_ADMINISTRATOR, reason);
> 
> ?

Yea, it should still check that (Added back).

> Also, we are assuming that ShouldBlockRequestForFrame() won't be called for
> subresource requests - is there any test that ensure this is true?

I'm not sure it looks like PolicyTest.URLBlacklist and
PolicyTest.FileURLBlacklist may be the only browser tests, and they don't seem
to check it.

The test coverage situation isn't actually much different than it was before, as
there are no tests that make sure we actually set LOAD_SUB_FRAME in the first
place, as this CL demonstrates.

https://codereview.chromium.org/1134733004/diff/100001/components/policy/core...
File components/policy/core/browser/url_blacklist_manager.h (right):

https://codereview.chromium.org/1134733004/diff/100001/components/policy/core...
components/policy/core/browser/url_blacklist_manager.h:164: // etc) should not
be not filtered. The sync signin page will also also not be
On 2015/05/12 16:44:30, Andrew T Wilson wrote:
> nit: also also

Done.

[mmenke, 2015-05-12 17:04:07]

https://codereview.chromium.org/1134733004/diff/100001/chrome/browser/policy/...
File chrome/browser/policy/url_blacklist_manager_unittest.cc (right):

https://codereview.chromium.org/1134733004/diff/100001/chrome/browser/policy/...
chrome/browser/policy/url_blacklist_manager_unittest.cc:644:
GURL("http://google.com"), &reason));
On 2015/05/12 16:59:13, mmenke wrote:
> On 2015/05/12 16:44:30, Andrew T Wilson wrote:
> > This is no longer checking the reason for the request being blocked - should
> > this also have:
> > 
> > EXPECT_EQ(net::ERR_BLOCKED_BY_ADMINISTRATOR, reason);
> > 
> > ?
> 
> Yea, it should still check that (Added back).
> 
> > Also, we are assuming that ShouldBlockRequestForFrame() won't be called for
> > subresource requests - is there any test that ensure this is true?
> 
> I'm not sure it looks like PolicyTest.URLBlacklist and
> PolicyTest.FileURLBlacklist may be the only browser tests, and they don't seem
> to check it.
> 
> The test coverage situation isn't actually much different than it was before,
as
> there are no tests that make sure we actually set LOAD_SUB_FRAME in the first
> place, as this CL demonstrates.

I'm happy to add an integration test in a followup CL, should be simple enough,
but seems a bit beyond the scope of this one.

[mmenke, 2015-05-14 17:57:10]

atwilson:  Ping!

[Andrew T Wilson (Slow), 2015-05-18 11:58:35]

LGTM. If it's easy to add test coverage for the subresource load case in a
followup, that'd be awesome.

[mmenke, 2015-05-18 14:50:10]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2015-05-18 14:50:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1134733004/#ps120001
(title: "Response to atwilson's comments")

[commit-bot: I haz the power, 2015-05-18 14:50:50]

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

[commit-bot: I haz the power, 2015-05-18 16:21:12]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-05-18 16:22:05]

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