Add ResourceRequestInfo.

Add ResourceRequestInfo.

ResourceRequestInfo exposes the members of ResourceDispatcherHostRequestInfo required by src/chrome.

ResourceDispatcherHostRequestInfo remains for use by src/content (maybe it should have a different name), and ResourceDispatcherHostRequestInfo subclasses ResourceRequestInfo.

ResourceDispatcherHost::RenderViewForRequest is removed in favor of ResourceRequestInfo::GetAssociatedRenderView().  src/chrome uses ResourceRequestInfo::ForRequest(URLRequest*) instead of ResourceDispatcherHost::InfoForRequest.

Deletes DummyResourceHandler in favor of just having ~ResourceDispatcherHostRequestInfo null-test its ResourceHandler.

R=jam@chromium.org
TBR=mirandac@chromium.org,ben@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=125457

[jam, 2012-03-05 07:16:17]

http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resou...
File content/public/browser/resource_request_info.h (right):

http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resou...
content/public/browser/resource_request_info.h:21: class ResourceRequestInfo :
public base::SupportsUserData::Data {
nit: please add a little comment about what this class is used for, to help
people new to content when they navigate the public directory

http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resou...
content/public/browser/resource_request_info.h:29: ResourceContext* context()
const { return context_; }
for the Content API, we have tried to be strict that content/public holds
interfaces and implementation is in content/browser. for interfaces that
delegates derive from, we have stored pointers to parent objects etc to avoid
duplication of rote code.

in this case though, there's only one implementation and that's in content. so
it would be more consistent to have this be a pure abstract interface with all
these getters being GetFoo, and move the member variables to
ResourceDispatcherHostRequestInfo. then ResourceDispatcherHostRequestInfo can
also be renamed to ResourceResourceInfoImpl to match what we call other classes
in content that derive from an interface.

wdyt? i realize this may be a lot of renaming of code. if you agree, you can
commit as is and i can take care of the renaming while i wait for stuff.

[darin (slow to review), 2012-03-05 07:50:50]

On Sun, Mar 4, 2012 at 11:16 PM, <jam@chromium.org> wrote:

>
> http://codereview.chromium.**org/9580002/diff/4052/content/**
>
public/browser/resource_**request_info.h<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h>
> File content/public/browser/**resource_request_info.h (right):
>
> http://codereview.chromium.**org/9580002/diff/4052/content/**
>
public/browser/resource_**request_info.h#newcode21<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h#newcode21>
> content/public/browser/**resource_request_info.h:21: class
> ResourceRequestInfo : public base::SupportsUserData::Data {
> nit: please add a little comment about what this class is used for, to
> help people new to content when they navigate the public directory
>
> http://codereview.chromium.**org/9580002/diff/4052/content/**
>
public/browser/resource_**request_info.h#newcode29<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h#newcode29>
> content/public/browser/**resource_request_info.h:29: ResourceContext*
> context() const { return context_; }
> for the Content API, we have tried to be strict that content/public
> holds interfaces and implementation is in content/browser. for
> interfaces that delegates derive from, we have stored pointers to parent
> objects etc to avoid duplication of rote code.
>
> in this case though, there's only one implementation and that's in
> content. so it would be more consistent to have this be a pure abstract
> interface with all these getters being GetFoo, and move the member
> variables to ResourceDispatcherHostRequestI**nfo. then
> ResourceDispatcherHostRequestI**nfo can also be renamed to
> ResourceResourceInfoImpl to match what we call other classes in content
> that derive from an interface.
>
> wdyt? i realize this may be a lot of renaming of code. if you agree, you
> can commit as is and i can take care of the renaming while i wait for
> stuff.
>

Makes good sense.  One question:  Do you generally preserve the inline
getters for use inside content?  There are quite a lot of consumers of
ResourceDispatcherHostRequestInfo within content (not really so much
outside of content as you can see in this patch).

My inclination is to preserve the inline getters on ResourceRequestInfoImpl,
and just do a global rename of ResourceDispatcherHostRequestInfo to
ResourceRequestInfoImpl.

-Darin



>
>
http://codereview.chromium.**org/9580002/<http://codereview.chromium.org/9580...
>

[jam, 2012-03-05 07:58:50]

On Sun, Mar 4, 2012 at 11:50 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Sun, Mar 4, 2012 at 11:16 PM, <jam@chromium.org> wrote:
>
>>
>> http://codereview.chromium.**org/9580002/diff/4052/content/**
>>
public/browser/resource_**request_info.h<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h>
>> File content/public/browser/**resource_request_info.h (right):
>>
>> http://codereview.chromium.**org/9580002/diff/4052/content/**
>>
public/browser/resource_**request_info.h#newcode21<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h#newcode21>
>> content/public/browser/**resource_request_info.h:21: class
>> ResourceRequestInfo : public base::SupportsUserData::Data {
>> nit: please add a little comment about what this class is used for, to
>> help people new to content when they navigate the public directory
>>
>> http://codereview.chromium.**org/9580002/diff/4052/content/**
>>
public/browser/resource_**request_info.h#newcode29<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h#newcode29>
>> content/public/browser/**resource_request_info.h:29: ResourceContext*
>> context() const { return context_; }
>> for the Content API, we have tried to be strict that content/public
>> holds interfaces and implementation is in content/browser. for
>> interfaces that delegates derive from, we have stored pointers to parent
>> objects etc to avoid duplication of rote code.
>>
>> in this case though, there's only one implementation and that's in
>> content. so it would be more consistent to have this be a pure abstract
>> interface with all these getters being GetFoo, and move the member
>> variables to ResourceDispatcherHostRequestI**nfo. then
>> ResourceDispatcherHostRequestI**nfo can also be renamed to
>> ResourceResourceInfoImpl to match what we call other classes in content
>> that derive from an interface.
>>
>> wdyt? i realize this may be a lot of renaming of code. if you agree, you
>> can commit as is and i can take care of the renaming while i wait for
>> stuff.
>>
>
> Makes good sense.  One question:  Do you generally preserve the inline
> getters for use inside content?  There are quite a lot of consumers of
> ResourceDispatcherHostRequestInfo within content (not really so much
> outside of content as you can see in this patch).
>

I may not be understanding your question. For getters of objects that are
on the interface, these wouldn't be inline since they'd have to be pure
virtual methods and clang would complain if we inlined the implementation.

For getters on the Impl that are only visible in content, these can be
unix_hacker and inlined.


> My inclination is to preserve the inline getters on
> ResourceRequestInfoImpl,
> and just do a global rename of ResourceDispatcherHostRequestInfo to
> ResourceRequestInfoImpl.
>
> -Darin
>
>
>
>>
>>
http://codereview.chromium.**org/9580002/<http://codereview.chromium.org/9580...
>>
>
>

[jam, 2012-03-05 08:04:39]

On Sun, Mar 4, 2012 at 11:58 PM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Sun, Mar 4, 2012 at 11:50 PM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>> On Sun, Mar 4, 2012 at 11:16 PM, <jam@chromium.org> wrote:
>>
>>>
>>> http://codereview.chromium.**org/9580002/diff/4052/content/**
>>>
public/browser/resource_**request_info.h<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h>
>>> File content/public/browser/**resource_request_info.h (right):
>>>
>>> http://codereview.chromium.**org/9580002/diff/4052/content/**
>>>
public/browser/resource_**request_info.h#newcode21<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h#newcode21>
>>> content/public/browser/**resource_request_info.h:21: class
>>> ResourceRequestInfo : public base::SupportsUserData::Data {
>>> nit: please add a little comment about what this class is used for, to
>>> help people new to content when they navigate the public directory
>>>
>>> http://codereview.chromium.**org/9580002/diff/4052/content/**
>>>
public/browser/resource_**request_info.h#newcode29<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h#newcode29>
>>> content/public/browser/**resource_request_info.h:29: ResourceContext*
>>> context() const { return context_; }
>>> for the Content API, we have tried to be strict that content/public
>>> holds interfaces and implementation is in content/browser. for
>>> interfaces that delegates derive from, we have stored pointers to parent
>>> objects etc to avoid duplication of rote code.
>>>
>>> in this case though, there's only one implementation and that's in
>>> content. so it would be more consistent to have this be a pure abstract
>>> interface with all these getters being GetFoo, and move the member
>>> variables to ResourceDispatcherHostRequestI**nfo. then
>>> ResourceDispatcherHostRequestI**nfo can also be renamed to
>>> ResourceResourceInfoImpl to match what we call other classes in content
>>> that derive from an interface.
>>>
>>> wdyt? i realize this may be a lot of renaming of code. if you agree, you
>>> can commit as is and i can take care of the renaming while i wait for
>>> stuff.
>>>
>>
>> Makes good sense.  One question:  Do you generally preserve the inline
>> getters for use inside content?  There are quite a lot of consumers of
>> ResourceDispatcherHostRequestInfo within content (not really so much
>> outside of content as you can see in this patch).
>>
>
> I may not be understanding your question. For getters of objects that are
> on the interface, these wouldn't be inline since they'd have to be pure
> virtual methods and clang would complain if we inlined the implementation.
>

ok now I think I may be understanding your question: do you mean have two
getters for the same member variable in the impl, i..e one that's virtual
and CamelCase for the embedder, and one that's inline and unix_hacker for
content? if so, in these cases we just eliminate the inline one and only
keep the virtual Getter. It seems a bit like a premature optimization to
ensure it's inlined, and imo it seems redundant to have two getters for
each member variable.


> For getters on the Impl that are only visible in content, these can be
> unix_hacker and inlined.
>

>
>> My inclination is to preserve the inline getters on
>> ResourceRequestInfoImpl,
>> and just do a global rename of ResourceDispatcherHostRequestInfo to
>> ResourceRequestInfoImpl.
>>
>> -Darin
>>
>>
>>
>>>
>>>
http://codereview.chromium.**org/9580002/<http://codereview.chromium.org/9580...
>>>
>>
>>
>

[darin (slow to review), 2012-03-05 16:40:41]

On Mon, Mar 5, 2012 at 12:04 AM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Sun, Mar 4, 2012 at 11:58 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>>
>>
>> On Sun, Mar 4, 2012 at 11:50 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>>>
>>>
>>> On Sun, Mar 4, 2012 at 11:16 PM, <jam@chromium.org> wrote:
>>>
>>>>
>>>> http://codereview.chromium.**org/9580002/diff/4052/content/**
>>>>
public/browser/resource_**request_info.h<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h>
>>>> File content/public/browser/**resource_request_info.h (right):
>>>>
>>>> http://codereview.chromium.**org/9580002/diff/4052/content/**
>>>>
public/browser/resource_**request_info.h#newcode21<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h#newcode21>
>>>> content/public/browser/**resource_request_info.h:21: class
>>>> ResourceRequestInfo : public base::SupportsUserData::Data {
>>>> nit: please add a little comment about what this class is used for, to
>>>> help people new to content when they navigate the public directory
>>>>
>>>> http://codereview.chromium.**org/9580002/diff/4052/content/**
>>>>
public/browser/resource_**request_info.h#newcode29<http://codereview.chromium.org/9580002/diff/4052/content/public/browser/resource_request_info.h#newcode29>
>>>> content/public/browser/**resource_request_info.h:29: ResourceContext*
>>>> context() const { return context_; }
>>>> for the Content API, we have tried to be strict that content/public
>>>> holds interfaces and implementation is in content/browser. for
>>>> interfaces that delegates derive from, we have stored pointers to parent
>>>> objects etc to avoid duplication of rote code.
>>>>
>>>> in this case though, there's only one implementation and that's in
>>>> content. so it would be more consistent to have this be a pure abstract
>>>> interface with all these getters being GetFoo, and move the member
>>>> variables to ResourceDispatcherHostRequestI**nfo. then
>>>> ResourceDispatcherHostRequestI**nfo can also be renamed to
>>>> ResourceResourceInfoImpl to match what we call other classes in content
>>>> that derive from an interface.
>>>>
>>>> wdyt? i realize this may be a lot of renaming of code. if you agree, you
>>>> can commit as is and i can take care of the renaming while i wait for
>>>> stuff.
>>>>
>>>
>>> Makes good sense.  One question:  Do you generally preserve the inline
>>> getters for use inside content?  There are quite a lot of consumers of
>>> ResourceDispatcherHostRequestInfo within content (not really so much
>>> outside of content as you can see in this patch).
>>>
>>
>> I may not be understanding your question. For getters of objects that are
>> on the interface, these wouldn't be inline since they'd have to be pure
>> virtual methods and clang would complain if we inlined the implementation.
>>
>
> ok now I think I may be understanding your question: do you mean have two
> getters for the same member variable in the impl, i..e one that's virtual
> and CamelCase for the embedder, and one that's inline and unix_hacker for
> content? if so, in these cases we just eliminate the inline one and only
> keep the virtual Getter. It seems a bit like a premature optimization to
> ensure it's inlined, and imo it seems redundant to have two getters for
> each member variable.
>

Yeah, that was my question.  Works for me.  I just want to do whatever is
conventional :-)

-Darin




>
>
>> For getters on the Impl that are only visible in content, these can be
>> unix_hacker and inlined.
>>
>
>>
>>> My inclination is to preserve the inline getters on
>>> ResourceRequestInfoImpl,
>>> and just do a global rename of ResourceDispatcherHostRequestInfo to
>>> ResourceRequestInfoImpl.
>>>
>>> -Darin
>>>
>>>
>>>
>>>>
>>>>
http://codereview.chromium.**org/9580002/<http://codereview.chromium.org/9580...
>>>>
>>>
>>>
>>
>

[darin (slow to review), 2012-03-06 18:05:37]

PTAL.  There's more to cleanup after this change.  For example, I want to remove
ResourceDispatcherHost::RenderViewForRequest, and I think
ResourceDispatcherHost::InfoForRequest should probably be changed to
ResourceRequestInfoImpl::ForRequest.

[darin (slow to review), 2012-03-06 18:08:23]

http://codereview.chromium.org/9580002/diff/18022/content/public/browser/reso...
File content/public/browser/resource_request_info.h (right):

http://codereview.chromium.org/9580002/diff/18022/content/public/browser/reso...
content/public/browser/resource_request_info.h:32: CONTENT_EXPORT static void
DecorateForTesting(
Maybe "Decorate" is an abnormal term here.  Previously, I just had this method
named CreateForTesting and it returned a ResourceRequestInfo instance.  However,
ResourceRequestInfo has a protected destructor.  This meant that the only thing
callers of CreateForTesting could do was to pass the result to
SupportsUserData::SetUserData since the SupportsUserData::Data class has a
public destructor.

[jam, 2012-03-06 18:48:57]

lgtm

http://codereview.chromium.org/9580002/diff/18022/content/public/browser/reso...
File content/public/browser/resource_request_info.h (right):

http://codereview.chromium.org/9580002/diff/18022/content/public/browser/reso...
content/public/browser/resource_request_info.h:32: CONTENT_EXPORT static void
DecorateForTesting(
On 2012/03/06 18:08:23, darin wrote:
> Maybe "Decorate" is an abnormal term here.  Previously, I just had this method
> named CreateForTesting and it returned a ResourceRequestInfo instance. 
However,
> ResourceRequestInfo has a protected destructor.  This meant that the only
thing
> callers of CreateForTesting could do was to pass the result to
> SupportsUserData::SetUserData since the SupportsUserData::Data class has a
> public destructor.

I preferred the CreateForTesting since it's easier to understand as you say. it
seems like the destructor below can be made public with not much difference,
since if it's protected it can be casted to the parent class and deleted
directly if someone wants to be malicious :)

[darin (slow to review), 2012-03-06 22:44:54]

On Tue, Mar 6, 2012 at 10:48 AM, <jam@chromium.org> wrote:

> lgtm
>
>
>
> http://codereview.chromium.**org/9580002/diff/18022/**
>
content/public/browser/**resource_request_info.h<http://codereview.chromium.org/9580002/diff/18022/content/public/browser/resource_request_info.h>
> File content/public/browser/**resource_request_info.h (right):
>
> http://codereview.chromium.**org/9580002/diff/18022/**
>
content/public/browser/**resource_request_info.h#**newcode32<http://codereview.chromium.org/9580002/diff/18022/content/public/browser/resource_request_info.h#newcode32>
> content/public/browser/**resource_request_info.h:32: CONTENT_EXPORT static
> void DecorateForTesting(
> On 2012/03/06 18:08:23, darin wrote:
>
>> Maybe "Decorate" is an abnormal term here.  Previously, I just had
>>
> this method
>
>> named CreateForTesting and it returned a ResourceRequestInfo instance.
>>
>  However,
>
>> ResourceRequestInfo has a protected destructor.  This meant that the
>>
> only thing
>
>> callers of CreateForTesting could do was to pass the result to
>> SupportsUserData::SetUserData since the SupportsUserData::Data class
>>
> has a
>
>> public destructor.
>>
>
> I preferred the CreateForTesting since it's easier to understand as you
> say. it seems like the destructor below can be made public with not much
> difference, since if it's protected it can be casted to the parent class
> and deleted directly if someone wants to be malicious :)
>

Good point.  However, I just realized that ResourceRequestInfo really
doesn't need
to inherit from base::SupportsUserData::Data.  I can hide that within the
implementation.

What I want to avoid is making it easy for consumers to delete the
ResourceRequestInfo
while it is still assigned as the user data of an URLRequest.

In reality, consumers never need to call SetUserData or GetUserData.  They
just need to
call ResourceRequestInfo::ForRequest.

-Darin



>
>
http://codereview.chromium.**org/9580002/<http://codereview.chromium.org/9580...
>

[commit-bot: I haz the power, 2012-03-07 01:10:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/9580002/19057

[commit-bot: I haz the power, 2012-03-07 01:10:22]

Can't apply patch for file
content/browser/renderer_host/resource_request_info_impl.cc.
While running patch -p0 --forward --force;
patching file content/browser/renderer_host/resource_request_info_impl.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 22.
Hunk #3 FAILED at 59.
3 out of 3 hunks FAILED -- saving rejects to file
content/browser/renderer_host/resource_request_info_impl.cc.rej

[commit-bot: I haz the power, 2012-03-07 18:27:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/9580002/17061

[commit-bot: I haz the power, 2012-03-07 18:27:44]

Can't apply patch for file
chrome/browser/renderer_host/transfer_navigation_resource_throttle.cc.
While running patch -p0 --forward --force;
patching file
chrome/browser/renderer_host/transfer_navigation_resource_throttle.cc
Hunk #2 FAILED at 26.
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/renderer_host/transfer_navigation_resource_throttle.cc.rej

[commit-bot: I haz the power, 2012-03-07 18:43:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/9580002/23002

[commit-bot: I haz the power, 2012-03-07 18:44:02]

Can't apply patch for file
content/browser/renderer_host/resource_request_info_impl.cc.
While running patch -p0 --forward --force;
patching file content/browser/renderer_host/resource_request_info_impl.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 22.
Hunk #3 FAILED at 59.
3 out of 3 hunks FAILED -- saving rejects to file
content/browser/renderer_host/resource_request_info_impl.cc.rej