Store blocked cookies in the tab contents.

Store blocked and accessed cookies in the tab contents.

This will enable chrome to display all cookies used/blocked on the current site to the user instead of just the information that something was blocked. The cookies are collected in data structures suitable for displaying them using the cookies tree model.

BUG=45230
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51545

[jochen (gone - plz use gerrit), 2010-05-28 11:21:34]

please review.

This patch collects information about all blocked cookies in the tab contents
object. This will allow for displaying the blocked cookies in a cookiestreemodel
later.

the biggest part of this change is touching appcache, so I choose you as
reviewer

[michaeln, 2010-05-28 17:28:45]

I haven't looked thru this fully yet, but I do think I see how we can shrink the
number of files that need to be changed for the appcache bits.

I'll provide more complete comments this afternoon (for me).

http://codereview.chromium.org/2370001/diff/1/25
File webkit/appcache/appcache_group.h (right):

http://codereview.chromium.org/2370001/diff/1/25#newcode35
webkit/appcache/appcache_group.h:35: const GURL& manifest_url) = 0;
I think you can cut down on a number of changes by removing the manifest_url
argument from this virtual method. The manifest_url is a property of the
'group'... see group->manifest_url()

http://codereview.chromium.org/2370001/diff/1/25#newcode57
webkit/appcache/appcache_group.h:57: const GURL& manifest_url() const { return
manifest_url_; }
here

http://codereview.chromium.org/2370001/diff/1/25#newcode112
webkit/appcache/appcache_group.h:112: void NotifyContentBlocked(const GURL&
manifest_url);
can also be removed i think

[michaeln, 2010-05-28 20:14:02]

Hi again, still haven't looked thru entirely, but I found a couple of appcache
related things that need some massaging. Once those are addressed I'll take a
closer look at the rest of things.

http://codereview.chromium.org/2370001/diff/1/27
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/2370001/diff/1/27#newcode50
webkit/appcache/appcache_host.cc:50: frontend_->OnContentBlocked(host_id_,
manifest_url);
The value of manifest_url is not guaranteed to be what you want it to be, it may
be empty and it may even have a value that is not empty but different than the
value of the 'blocked' app cache.

See comments in appcache_request_handler about how to get a valid value for this
param.

http://codereview.chromium.org/2370001/diff/1/31
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/2370001/diff/1/31#newcode198
webkit/appcache/appcache_request_handler.cc:198: if (was_blocked_by_policy)
When was_blocked_by_policy is set, manifest_url will always be an empty url
given how AppCacheStorageImpl is currently coded. See
AppCacheStorageImpl::CheckPolicyAndCallOnMainResponseFound. Maybe change that
method to pass the value to this method.

In the deferred notification case (for IsFrame()), looks like we need to store
the value (in AppCacheHost) until the notification is actually sent.

[jochen (gone - plz use gerrit), 2010-05-31 09:36:10]

http://codereview.chromium.org/2370001/diff/1/25
File webkit/appcache/appcache_group.h (right):

http://codereview.chromium.org/2370001/diff/1/25#newcode35
webkit/appcache/appcache_group.h:35: const GURL& manifest_url) = 0;
On 2010/05/28 17:28:45, michaeln wrote:
> I think you can cut down on a number of changes by removing the manifest_url
> argument from this virtual method. The manifest_url is a property of the
> 'group'... see group->manifest_url()

Done.

http://codereview.chromium.org/2370001/diff/1/25#newcode112
webkit/appcache/appcache_group.h:112: void NotifyContentBlocked(const GURL&
manifest_url);
On 2010/05/28 17:28:45, michaeln wrote:
> can also be removed i think

Done.

http://codereview.chromium.org/2370001/diff/1/31
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/2370001/diff/1/31#newcode198
webkit/appcache/appcache_request_handler.cc:198: if (was_blocked_by_policy)
On 2010/05/28 20:14:03, michaeln wrote:
> When was_blocked_by_policy is set, manifest_url will always be an empty url
> given how AppCacheStorageImpl is currently coded. See
> AppCacheStorageImpl::CheckPolicyAndCallOnMainResponseFound. Maybe change that
> method to pass the value to this method.
> 
> In the deferred notification case (for IsFrame()), looks like we need to store
> the value (in AppCacheHost) until the notification is actually sent.

Done.

[michaeln, 2010-05-31 16:54:45]

I did see some minor inconsistencies in param names and what values are actually
delivered. The largest change I'm asking for is to eliminate the unneeded
parameter in url_request.h.

These are pretty minor things, so LGTM assuming you take care of those minor
things in some way.

http://codereview.chromium.org/2370001/diff/13001/9004
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/2370001/diff/13001/9004#newcode271
chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc:271: url);
Actually looks like you're providing greater specificty than 'origin' at this
call site, so the param name should be changed.

http://codereview.chromium.org/2370001/diff/13001/9007
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/2370001/diff/13001/9007#newcode312
chrome/browser/renderer_host/render_view_host_delegate.h:312: virtual void
OnCookieBlocked(const GURL& origin,
Do we want 'url' here instead of 'origin' for greater specificity?

http://codereview.chromium.org/2370001/diff/13001/9007#newcode318
chrome/browser/renderer_host/render_view_host_delegate.h:318: virtual void
OnLocalStorageBlocked(const GURL& origin) = 0;
ditto

http://codereview.chromium.org/2370001/diff/13001/9007#newcode328
chrome/browser/renderer_host/render_view_host_delegate.h:328: virtual void
OnWebDatabaseBlocked(const std::string& origin,
Please use GURL here for the 'origin'

http://codereview.chromium.org/2370001/diff/13001/9011
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2370001/diff/13001/9011#newcode2178
chrome/browser/tab_contents/tab_contents.cc:2178:
blocked_web_databases_[GURL(origin)].insert(UTF16ToUTF8(name));
An empty string is a valid value for 'name'. Just fyi for when you get to using
this blocked_web_databases_ collection. I don't recall how dbs with an empty
'name' are represented in the cookie tree view, but you'll probably want to
harmonize with that.

http://codereview.chromium.org/2370001/diff/13001/9016
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2370001/diff/13001/9016#newcode3006
chrome/renderer/render_view.cc:3006: origin.toString().utf8(),
Further downstream, i see that you're relying on origin.toString() being a well
formed URL. Please ensure that the origin.toString() method returns data in that
format, and if so consider using GURL for the IPC param type and intervening
method args from here to TabContents.

http://codereview.chromium.org/2370001/diff/13001/9019
File net/url_request/url_request.h (right):

http://codereview.chromium.org/2370001/diff/13001/9019#newcode196
net/url_request/url_request.h:196: const GURL& origin,
The origin param is redundant, it can be determined based on request->url.  I'd
remove so there's no chance to make the programming error where the 'origin'
doesn't match what request.url says.

http://codereview.chromium.org/2370001/diff/13001/9020
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2370001/diff/13001/9020#newcode472
net/url_request/url_request_http_job.cc:472: request_, request_->url(),
oh... url != origin... so removing the 'origin' param actually eliminates an
instance of that programming error in this CL :)

http://codereview.chromium.org/2370001/diff/13001/9026
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/2370001/diff/13001/9026#newcode91
webkit/appcache/appcache_host.h:91: // work properly, this method needs to by
invokde prior to cache selection.
ooops a preexisting 'invokde' typo, would be nice to fix since you've got the
file cracked open

[jochen (gone - plz use gerrit), 2010-06-15 14:42:27]

fixed all your comments, but also did the following changes:

- i stripped out the app cache stuff for now, that will follow in a separate CL
- i also record accessed (but not blocked) cookies
- stuff is collected using the canned browsing data helpers

please review :)

[michaeln, 2010-06-16 00:13:29]

> please review :)

oh... just noticed this... i'll take a look

[darin (slow to review), 2010-06-16 15:59:12]

It's not clear from this CL how the data added to TabContents will be used.  Why
do we need to store all of the allowed cookies?  Please make sure the CL
description explains 'big picture' details like this.

http://codereview.chromium.org/2370001/diff/27023/30007
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/2370001/diff/27023/30007#newcode271
chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc:271: url, result
== WebKit::WebStorageArea::ResultBlockedByPolicy);
no need for the WebKit:: prefix.  see the using declaration at the top of the
file.

http://codereview.chromium.org/2370001/diff/27023/30016
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2370001/diff/27023/30016#newcode1168
chrome/browser/tab_contents/tab_contents.h:1168: class LocalSharedObjects {
this is a fair bit of complexity to add to TabContents.  please break this out
into separate .h and .cc files, and try to push as much of the new logic out of
TabContents as possible.

http://codereview.chromium.org/2370001/diff/27023/30016#newcode1184
chrome/browser/tab_contents/tab_contents.h:1184:
scoped_refptr<net::CookieMonster> cookies_;
public member variables should not have a "_" suffix.  it would probably be
better to make public accessor functions though.

http://codereview.chromium.org/2370001/diff/27023/30016#newcode1192
chrome/browser/tab_contents/tab_contents.h:1192: LocalSharedObjects
accessed_lsos_;
please avoid uncommon abbreviations.  "lsos" is going to be mysterious and
confusing everywhere it is used.  accessed_local_shared_objects_ would be far
better, but i'm also not sure that "local shared objects" best describes this
class.  that sounds like it has to do with .so files! :-)

also, in the various messages you use the term "accessed" to mean things that
may have been allowed or blocked, but here you use it only for things that were
allowed.  maybe allowed_* and blocked_* would be better?

http://codereview.chromium.org/2370001/diff/27023/30018
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2370001/diff/27023/30018#newcode3112
chrome/renderer/render_view.cc:3112: Send(new
ViewHostMsg_WebDatabaseAccessed(routing_id_,
note: in this case the database has not been accessed yet.  in the cookie case,
the notification comes after the cookie database has been modified.  i'm not
sure if that difference will matter.

http://codereview.chromium.org/2370001/diff/27023/30019
File net/url_request/url_request.h (right):

http://codereview.chromium.org/2370001/diff/27023/30019#newcode195
net/url_request/url_request.h:195: virtual void OnCookieAccessed(URLRequest*
request,
this name does not match the comment.  it is also no longer consistent with the
method name OnGetCookiesBlocked.

maybe this method should be called OnSetCookie and the other should be called
OnGetCookies?  then both can have a 'bool blocked_by_policy' param?

http://codereview.chromium.org/2370001/diff/27023/30020
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2370001/diff/27023/30020#newcode485
net/url_request/url_request_http_job.cc:485:
request_->delegate()->OnCookieAccessed(
why do you need to know about cookies that are not blocked?  bug 45230 seems to
only be about blocked cookies.

http://codereview.chromium.org/2370001/diff/27023/30021
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/2370001/diff/27023/30021#newcode1658
net/url_request/url_request_unittest.cc:1658: 
it seems like you should add a test for the non-blocked SetCookie case if that
is important to you.

[michaeln, 2010-06-17 00:13:46]

About naming... LSO... i generally think of that as "local storage object"
rather than "shared". I'll take a closer look after you've responded to darin's
comments.

[jochen (gone - plz use gerrit), 2010-06-18 09:28:00]

http://codereview.chromium.org/2370001/diff/27023/30007
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/2370001/diff/27023/30007#newcode271
chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc:271: url, result
== WebKit::WebStorageArea::ResultBlockedByPolicy);
On 2010/06/16 15:59:13, darin wrote:
> no need for the WebKit:: prefix.  see the using declaration at the top of the
> file.

Done.

http://codereview.chromium.org/2370001/diff/27023/30016
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2370001/diff/27023/30016#newcode1168
chrome/browser/tab_contents/tab_contents.h:1168: class LocalSharedObjects {
On 2010/06/16 15:59:13, darin wrote:
> this is a fair bit of complexity to add to TabContents.  please break this out
> into separate .h and .cc files, and try to push as much of the new logic out
of
> TabContents as possible.

Done.

http://codereview.chromium.org/2370001/diff/27023/30016#newcode1184
chrome/browser/tab_contents/tab_contents.h:1184:
scoped_refptr<net::CookieMonster> cookies_;
On 2010/06/16 15:59:13, darin wrote:
> public member variables should not have a "_" suffix.  it would probably be
> better to make public accessor functions though.

Done.

http://codereview.chromium.org/2370001/diff/27023/30016#newcode1192
chrome/browser/tab_contents/tab_contents.h:1192: LocalSharedObjects
accessed_lsos_;
On 2010/06/16 15:59:13, darin wrote:
> please avoid uncommon abbreviations.  "lsos" is going to be mysterious and
> confusing everywhere it is used.  accessed_local_shared_objects_ would be far
> better, but i'm also not sure that "local shared objects" best describes this
> class.  that sounds like it has to do with .so files! :-)
> 
> also, in the various messages you use the term "accessed" to mean things that
> may have been allowed or blocked, but here you use it only for things that
were
> allowed.  maybe allowed_* and blocked_* would be better?

I expanded it to local_shared_objects and used the term allowed_. However, local
shared objects is the right term for our cookies imho, so I sticked with it :)

http://codereview.chromium.org/2370001/diff/27023/30018
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2370001/diff/27023/30018#newcode3112
chrome/renderer/render_view.cc:3112: Send(new
ViewHostMsg_WebDatabaseAccessed(routing_id_,
On 2010/06/16 15:59:13, darin wrote:
> note: in this case the database has not been accessed yet.  in the cookie
case,
> the notification comes after the cookie database has been modified.  i'm not
> sure if that difference will matter.

Since a web page can at any point in time decide to access a cookie (-like
object), I don't think that really matters for the purpose of displaying a
snapshot of what is used currently.

http://codereview.chromium.org/2370001/diff/27023/30019
File net/url_request/url_request.h (right):

http://codereview.chromium.org/2370001/diff/27023/30019#newcode195
net/url_request/url_request.h:195: virtual void OnCookieAccessed(URLRequest*
request,
On 2010/06/16 15:59:13, darin wrote:
> this name does not match the comment.  it is also no longer consistent with
the
> method name OnGetCookiesBlocked.
> 
> maybe this method should be called OnSetCookie and the other should be called
> OnGetCookies?  then both can have a 'bool blocked_by_policy' param?

Done.

http://codereview.chromium.org/2370001/diff/27023/30020
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2370001/diff/27023/30020#newcode485
net/url_request/url_request_http_job.cc:485:
request_->delegate()->OnCookieAccessed(
On 2010/06/16 15:59:13, darin wrote:
> why do you need to know about cookies that are not blocked?  bug 45230 seems
to
> only be about blocked cookies.

While playing with this it occured to me that you also want to know which
cookies were not blocked. I'll update the bug.

http://codereview.chromium.org/2370001/diff/27023/30021
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/2370001/diff/27023/30021#newcode1658
net/url_request/url_request_unittest.cc:1658: 
On 2010/06/16 15:59:13, darin wrote:
> it seems like you should add a test for the non-blocked SetCookie case if that
> is important to you.

Done.

[darin (slow to review), 2010-06-18 16:28:32]

http://codereview.chromium.org/2370001/diff/42001/41026
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2370001/diff/42001/41026#newcode2151
chrome/browser/tab_contents/tab_contents.cc:2151: void
TabContents::OnCookieAccessed(const GURL& url,
I was also hoping you could move more of this logic into the
LocalStoredObjectsContainer class.  However that class contains either the set
of blocked or allowed objects, but not both, and you have the calls to
OnContentBlocked here.

It feels like we should be able to factor out all of the allowed/blocked objects
state into a separate class so that the TabContents can just own that class and
feed events to it.

Then TabContents wouldn't need to be the controller of allowed/blocked objects
state.

It is bad times if TabContents has to know about everything!

[jochen (gone - plz use gerrit), 2010-06-23 12:32:40]

I implemented the content settings delegate in a separate class and made
tab_contents inherit from it...

please review.

[brettw, 2010-06-23 16:12:06]

http://codereview.chromium.org/2370001/diff/46001/47017
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2370001/diff/46001/47017#newcode2140
chrome/browser/tab_contents/tab_contents.cc:2140:
TabContents::GetContentSettingsDelegate() {
The preferred way to do this is with composition rather than inheritance.
Fortunately, this is already set up for you to do easily. Instead of T.C.
inheriting from the ContentSettingsDelegate, make the C.S.Delegate a member, and
just return the address of that member from this Getter. This way, the T.C. API
is smaller overall and we don't add to the excessive multiple inheritance T.C.
already has.

[jochen (gone - plz use gerrit), 2010-06-24 14:19:40]

On 2010/06/23 16:12:06, brettw wrote:
> http://codereview.chromium.org/2370001/diff/46001/47017
> File chrome/browser/tab_contents/tab_contents.cc (right):
> 
> http://codereview.chromium.org/2370001/diff/46001/47017#newcode2140
> chrome/browser/tab_contents/tab_contents.cc:2140:
> TabContents::GetContentSettingsDelegate() {
> The preferred way to do this is with composition rather than inheritance.
> Fortunately, this is already set up for you to do easily. Instead of T.C.
> inheriting from the ContentSettingsDelegate, make the C.S.Delegate a member,
and
> just return the address of that member from this Getter. This way, the T.C.
API
> is smaller overall and we don't add to the excessive multiple inheritance T.C.
> already has.

done.

[jochen (gone - plz use gerrit), 2010-06-28 18:45:27]

ping?

On 2010/06/24 14:19:40, jochen wrote:
> On 2010/06/23 16:12:06, brettw wrote:
> > http://codereview.chromium.org/2370001/diff/46001/47017
> > File chrome/browser/tab_contents/tab_contents.cc (right):
> > 
> > http://codereview.chromium.org/2370001/diff/46001/47017#newcode2140
> > chrome/browser/tab_contents/tab_contents.cc:2140:
> > TabContents::GetContentSettingsDelegate() {
> > The preferred way to do this is with composition rather than inheritance.
> > Fortunately, this is already set up for you to do easily. Instead of T.C.
> > inheriting from the ContentSettingsDelegate, make the C.S.Delegate a member,
> and
> > just return the address of that member from this Getter. This way, the T.C.
> API
> > is smaller overall and we don't add to the excessive multiple inheritance
T.C.
> > already has.
> 
> done.

[darin (slow to review), 2010-06-29 20:02:33]

http://codereview.chromium.org/2370001/diff/52001/53009
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/2370001/diff/52001/53009#newcode200
chrome/browser/content_setting_bubble_model.cc:200:
tab_contents()->GetContentSettingsDelegate());
it seems odd that this static_cast should be necessary.  why does a getter named
GetFoo not return a pointer to type Foo?

http://codereview.chromium.org/2370001/diff/52001/53010
File chrome/browser/content_setting_bubble_model_unittest.cc (right):

http://codereview.chromium.org/2370001/diff/52001/53010#newcode82
chrome/browser/content_setting_bubble_model_unittest.cc:82:
RenderViewHostDelegate::ContentSettings* render_view_host_delegate =
nit: this variable name is not so hot.  maybe it should be named
content_settings to match the type name?  same goes for the tests above.

http://codereview.chromium.org/2370001/diff/52001/53011
File chrome/browser/content_setting_image_model.cc (right):

http://codereview.chromium.org/2370001/diff/52001/53011#newcode57
chrome/browser/content_setting_image_model.cc:57:
tab_contents->GetContentSettingsDelegate()) : NULL;
if you do go with naming the implementation type TabSpecificContentSettings,
then i think it would be reasonable for this GetContentSettingsDelegate method
to be replaced by a GetTabSpecificContentSettings() method that just returns a
pointer to TabSpecificContentSettings.  i don't know that there is too much
value in trying to hide the implementation from consumers if so many consumers
are just going to have to static_cast anyways.

http://codereview.chromium.org/2370001/diff/52001/53028
File chrome/browser/tab_contents/content_settings_delegate.h (right):

http://codereview.chromium.org/2370001/diff/52001/53028#newcode23
chrome/browser/tab_contents/content_settings_delegate.h:23: class Delegate {
the naming is unfortunate here.... the ContentSettingsDelegate's Delegate? :(

it seems like ContentSettingsDelegate could be better named.  this
implementation of the ContentSettings delegate provides what?  it provides
management of content settings for a particular tab, right?

maybe it should be named TabSpecificContentSettings?  <-- just a strawman of a
name... i'm not sure that i like it that much.

at any rate, with a name like TabSpecificContentSettings, it now makes sense for
it to have its own delegate.

http://codereview.chromium.org/2370001/diff/52001/53028#newcode57
chrome/browser/tab_contents/content_settings_delegate.h:57: friend class
TabContents;
i'm not sure that making TabContents be a friend is the best idea.  that couples
this to TabContents and makes it harder to unit test.  how about making the
methods public that TabContents would call, and then write some unit tests?

http://codereview.chromium.org/2370001/diff/52001/53030
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2370001/diff/52001/53030#newcode700
chrome/browser/tab_contents/tab_contents.h:700: // The ContentSettingsDelegate
is used to query to blocked content state by
this comment mentions the type ContentSettingsDelegate but the actual type
returned is something else.

http://codereview.chromium.org/2370001/diff/52001/53039
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2370001/diff/52001/53039#newcode443
net/url_request/url_request_http_job.cc:443: if (policy ==
net::ERR_ACCESS_DENIED) {
note: it looks like we will not call OnGetCookies when LOAD_DO_NOT_SEND_COOKIES
is specified.  it would be good to mention this in the comments in
url_request.h.  same goes for the OnSetCookie case and LOAD_DO_NOT_SAVE_COOKIES.

[jochen (gone - plz use gerrit), 2010-06-29 20:08:41]

http://codereview.chromium.org/2370001/diff/52001/53009
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/2370001/diff/52001/53009#newcode200
chrome/browser/content_setting_bubble_model.cc:200:
tab_contents()->GetContentSettingsDelegate());
On 2010/06/29 20:02:33, darin wrote:
> it seems odd that this static_cast should be necessary.  why does a getter
named
> GetFoo not return a pointer to type Foo?

The reason for this is that GetContentSettingsDelegate() is part of the render
view host delegate interface implemented by tab contents...

[darin (slow to review), 2010-06-30 00:05:41]

On Tue, Jun 29, 2010 at 1:08 PM, <jochen@chromium.org> wrote:

>
> http://codereview.chromium.org/2370001/diff/52001/53009
> File chrome/browser/content_setting_bubble_model.cc (right):
>
> http://codereview.chromium.org/2370001/diff/52001/53009#newcode200
> chrome/browser/content_setting_bubble_model.cc:200:
> tab_contents()->GetContentSettingsDelegate());
> On 2010/06/29 20:02:33, darin wrote:
>
>> it seems odd that this static_cast should be necessary.  why does a
>>
> getter named
>
>> GetFoo not return a pointer to type Foo?
>>
>
> The reason for this is that GetContentSettingsDelegate() is part of the
> render view host delegate interface implemented by tab contents...
>
>
understood.... what about my suggestion of naming ContentSettingsDelegate
something like TabSpecificContentSettings?

-darin



>
> http://codereview.chromium.org/2370001/show
>

[brettw, 2010-06-30 21:19:27]

I'll assume Darin has this covered so don't wait for an LGTM from me.

[jochen (gone - plz use gerrit), 2010-07-02 12:15:20]

http://codereview.chromium.org/2370001/diff/52001/53009
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/2370001/diff/52001/53009#newcode200
chrome/browser/content_setting_bubble_model.cc:200:
tab_contents()->GetContentSettingsDelegate());
On 2010/06/29 20:08:42, jochen wrote:
> On 2010/06/29 20:02:33, darin wrote:
> > it seems odd that this static_cast should be necessary.  why does a getter
> named
> > GetFoo not return a pointer to type Foo?
> 
> The reason for this is that GetContentSettingsDelegate() is part of the render
> view host delegate interface implemented by tab contents...
> 
> 

Done.

http://codereview.chromium.org/2370001/diff/52001/53010
File chrome/browser/content_setting_bubble_model_unittest.cc (right):

http://codereview.chromium.org/2370001/diff/52001/53010#newcode82
chrome/browser/content_setting_bubble_model_unittest.cc:82:
RenderViewHostDelegate::ContentSettings* render_view_host_delegate =
On 2010/06/29 20:02:33, darin wrote:
> nit: this variable name is not so hot.  maybe it should be named
> content_settings to match the type name?  same goes for the tests above.

Done.

http://codereview.chromium.org/2370001/diff/52001/53011
File chrome/browser/content_setting_image_model.cc (right):

http://codereview.chromium.org/2370001/diff/52001/53011#newcode57
chrome/browser/content_setting_image_model.cc:57:
tab_contents->GetContentSettingsDelegate()) : NULL;
On 2010/06/29 20:02:33, darin wrote:
> if you do go with naming the implementation type TabSpecificContentSettings,
> then i think it would be reasonable for this GetContentSettingsDelegate method
> to be replaced by a GetTabSpecificContentSettings() method that just returns a
> pointer to TabSpecificContentSettings.  i don't know that there is too much
> value in trying to hide the implementation from consumers if so many consumers
> are just going to have to static_cast anyways.

Done.

http://codereview.chromium.org/2370001/diff/52001/53028
File chrome/browser/tab_contents/content_settings_delegate.h (right):

http://codereview.chromium.org/2370001/diff/52001/53028#newcode23
chrome/browser/tab_contents/content_settings_delegate.h:23: class Delegate {
On 2010/06/29 20:02:33, darin wrote:
> the naming is unfortunate here.... the ContentSettingsDelegate's Delegate? :(
> 
> it seems like ContentSettingsDelegate could be better named.  this
> implementation of the ContentSettings delegate provides what?  it provides
> management of content settings for a particular tab, right?
> 
> maybe it should be named TabSpecificContentSettings?  <-- just a strawman of a
> name... i'm not sure that i like it that much.
> 
> at any rate, with a name like TabSpecificContentSettings, it now makes sense
for
> it to have its own delegate.

Done.

http://codereview.chromium.org/2370001/diff/52001/53030
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2370001/diff/52001/53030#newcode700
chrome/browser/tab_contents/tab_contents.h:700: // The ContentSettingsDelegate
is used to query to blocked content state by
On 2010/06/29 20:02:33, darin wrote:
> this comment mentions the type ContentSettingsDelegate but the actual type
> returned is something else.

Done.

http://codereview.chromium.org/2370001/diff/52001/53039
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2370001/diff/52001/53039#newcode443
net/url_request/url_request_http_job.cc:443: if (policy ==
net::ERR_ACCESS_DENIED) {
On 2010/06/29 20:02:33, darin wrote:
> note: it looks like we will not call OnGetCookies when
LOAD_DO_NOT_SEND_COOKIES
> is specified.  it would be good to mention this in the comments in
> url_request.h.  same goes for the OnSetCookie case and
LOAD_DO_NOT_SAVE_COOKIES.

Done.

[darin (slow to review), 2010-07-02 16:09:49]

LGTM

http://codereview.chromium.org/2370001/diff/64001/45024
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2370001/diff/64001/45024#newcode698
chrome/browser/tab_contents/tab_contents.h:698: // The
TabSpecificContentSettings object is used to query to blocked content
nit: "use to query to" -> "use to query the"