Add is_main_frame and parent_is_main_frame to ResourceRequestInfo::AllocateForTesting.

Add is_main_frame and parent_is_main_frame to ResourceRequestInfo::AllocateForTesting.

These are required in https://codereview.chromium.org/516663002/

BUG=412468
TBR=kmadhusu, mkosiba

Committed: https://crrev.com/f9d1558ef8f702e94548f2dfac858fd8fc342d51
Cr-Commit-Position: refs/heads/master@{#304034}

[gab, 2014-11-12 20:56:14]

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

[gab, 2014-11-12 20:56:14]

Hey Matt, please take a look (as owner of content/browser/loader).

Thanks!
Gab

[mmenke, 2014-11-12 21:17:49]

https://codereview.chromium.org/624073002/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_request_info_impl.cc (right):

https://codereview.chromium.org/624073002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_request_info_impl.cc:41: DCHECK(resource_type !=
RESOURCE_TYPE_MAIN_FRAME || is_main_frame);
Hrm...Is there any case where resource_type == RESOURCE_TYPE_MAIN_FRAME &&
!is_main_frame?

If not, this should be DCHECK_EQ(is_main_frame, resource_type ==
RESOURCE_TYPE_MAIN_FRAME);

Or could just get rid of is_main_frame, and keep parent_is_main_frame.

Actually, if that's the case, should probably get rid of
ResourceRequestImpl::is_main_frame_ completely, in favor of using the comparison
everywhere, but that's a bit beyond the scope of this CL.

[gab, 2014-11-12 21:19:14]

https://codereview.chromium.org/624073002/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_request_info_impl.cc (right):

https://codereview.chromium.org/624073002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_request_info_impl.cc:41: DCHECK(resource_type !=
RESOURCE_TYPE_MAIN_FRAME || is_main_frame);
On 2014/11/12 21:17:49, mmenke wrote:
> Hrm...Is there any case where resource_type == RESOURCE_TYPE_MAIN_FRAME &&
> !is_main_frame?
> 
> If not, this should be DCHECK_EQ(is_main_frame, resource_type ==
> RESOURCE_TYPE_MAIN_FRAME);
> 
> Or could just get rid of is_main_frame, and keep parent_is_main_frame.
> 
> Actually, if that's the case, should probably get rid of
> ResourceRequestImpl::is_main_frame_ completely, in favor of using the
comparison
> everywhere, but that's a bit beyond the scope of this CL.

Yes, every resource loaded as part of the main frame has |is_main_frame ==
true|.

[mmenke, 2014-11-12 21:21:45]

On 2014/11/12 21:19:14, gab wrote:
>
https://codereview.chromium.org/624073002/diff/20001/content/browser/loader/r...
> File content/browser/loader/resource_request_info_impl.cc (right):
> 
>
https://codereview.chromium.org/624073002/diff/20001/content/browser/loader/r...
> content/browser/loader/resource_request_info_impl.cc:41: DCHECK(resource_type
!=
> RESOURCE_TYPE_MAIN_FRAME || is_main_frame);
> On 2014/11/12 21:17:49, mmenke wrote:
> > Hrm...Is there any case where resource_type == RESOURCE_TYPE_MAIN_FRAME &&
> > !is_main_frame?
> > 
> > If not, this should be DCHECK_EQ(is_main_frame, resource_type ==
> > RESOURCE_TYPE_MAIN_FRAME);
> > 
> > Or could just get rid of is_main_frame, and keep parent_is_main_frame.
> > 
> > Actually, if that's the case, should probably get rid of
> > ResourceRequestImpl::is_main_frame_ completely, in favor of using the
> comparison
> > everywhere, but that's a bit beyond the scope of this CL.
> 
> Yes, every resource loaded as part of the main frame has |is_main_frame ==
> true|.

Thanks!  I'm a fairly new loader owner.  :)

LGTM

[gab, 2014-11-12 21:27:42]

Thanks @ avi for content/public/browser OWNERS (very minor test API tweak).

[gab, 2014-11-12 21:28:05]

gab@chromium.org changed reviewers:
+ avi@chromium.org

[gab, 2014-11-12 21:28:14]

On 2014/11/12 21:27:42, gab wrote:
> Thanks @ avi for content/public/browser OWNERS (very minor test API tweak).

+avi for real :-)!

[gab, 2014-11-12 21:33:28]

gab@chromium.org changed reviewers:
+ kalman@chromium.org, kmadhusu@chromium.org, mkosiba@chromium.org

[gab, 2014-11-12 21:33:33]

On 2014/11/12 21:28:14, gab wrote:
> On 2014/11/12 21:27:42, gab wrote:
> > Thanks @ avi for content/public/browser OWNERS (very minor test API tweak).
> 
> +avi for real :-)!

@kalman for c/b/extensions test side-effects
@kmadhusu  for c/b/search test side-effects
@mkosiba for components/navigation_interception test side-effects

Thanks!
Gab

[Avi (use Gerrit), 2014-11-12 21:44:46]

content/public/browser lgtm though at some point you're going to need some kind
of parameter structure for sanity's sake rather than the nine or so individual
parameters.

[not at google - send to devlin, 2014-11-12 22:55:10]

extensions rubberstamp lgtm

[gab, 2014-11-13 15:45:51]

On 2014/11/12 22:55:10, kalman wrote:
> extensions rubberstamp lgtm

@kalman, FYI: had to tweak two more files under your ownership in
extensions/browser/api/declarative_webrequest. The tweaks should be no-ops as
far as the triggered test logic is concerned.

TBR kmadhusu  for c/b/search test side-effect (which is a no-op as far as the
resulting logic is concerned)
TBR mkosiba for components/navigation_interception test side-effect (which is a
no-op as far as the resulting logic is concerned)

[gab, 2014-11-13 15:47:00]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-11-13 15:47:57]

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

[commit-bot: I haz the power, 2014-11-13 16:40:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-13 16:41:12]

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