Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
9 years, 9 months ago by Bernhard Bauer Modified:
9 years, 7 months ago Reviewers:
jam, jochen (gone - plz use gerrit), willchan no longer on Chromium, Mattias Nissler (ping if slow) CC:
chromium-reviews, Erik does not do reviews, brettw-cc_chromium.org, Aaron Boodman, pam+watch_chromium.org, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr., darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd enableReferrers and enableHyperlinkAuditing to contentSettings.misc extension API.
BUG=71067
TEST=ExtensionApiTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81420
Patch Set 1 #Patch Set 2 : fix #
Total comments: 6
Patch Set 3 : update #Patch Set 4 : cleanup #Patch Set 5 : observe pref changes in tabcontents #Patch Set 6 : git try #
Total comments: 9
Patch Set 7 : review #
Total comments: 2
Patch Set 8 : fix grammar #
Total comments: 13
Patch Set 9 : remove comment #Patch Set 10 : observe referrersEnabled in system network delegate #Patch Set 11 : update comment #Patch Set 12 : check for torn-down prefmember #Patch Set 13 : undo #Patch Set 14 : fix #Patch Set 15 : add comment #
Total comments: 1
Patch Set 16 : add shutdown methods #Patch Set 17 : comments #Patch Set 18 : check for null network_delegate_ #Patch Set 19 : remove unnecessary include #
Total comments: 5
Patch Set 20 : next try #Patch Set 21 : use protected accessor #Patch Set 22 : clean up #Patch Set 23 : forward-declare booleanprefmember #
Total comments: 12
Patch Set 24 : review #Patch Set 25 : review #
Total comments: 4
Patch Set 26 : review #
Created: 9 years, 8 months ago
Messages
Total messages: 38 (0 generated)
Please review (jochen: extension stuff, john: content stuff).
http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host... File chrome/browser/renderer_host/browser_render_process_host.h (right): http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host... chrome/browser/renderer_host/browser_render_process_host.h:217: // Weak, owned by |channel_|. what happens if OnChannelError() is called? channel_ will be reset and this filter will be deleted. so if you want to hold a pointer to it, use a scoped_refptr http://codereview.chromium.org/6773006/diff/2001/content/browser/renderer_hos... File content/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/6773006/diff/2001/content/browser/renderer_hos... content/browser/renderer_host/resource_message_filter.cc:9: #include "chrome/common/pref_names.h" this isn't needed http://codereview.chromium.org/6773006/diff/2001/content/browser/renderer_hos... File content/browser/renderer_host/resource_message_filter.h (right): http://codereview.chromium.org/6773006/diff/2001/content/browser/renderer_hos... content/browser/renderer_host/resource_message_filter.h:63: bool referrers_enabled() { return enableReferrers_; } nit: const http://codereview.chromium.org/6773006/diff/2001/content/browser/renderer_hos... content/browser/renderer_host/resource_message_filter.h:83: bool enableReferrers_; camel caps for member variables, i.e. enable_referrers_. same for the parameter name for the setter
http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host... File chrome/browser/renderer_host/browser_render_process_host.h (right): http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host... chrome/browser/renderer_host/browser_render_process_host.h:217: // Weak, owned by |channel_|. On 2011/03/29 18:17:16, John Abd-El-Malek wrote: > what happens if OnChannelError() is called? channel_ will be reset and this > filter will be deleted. so if you want to hold a pointer to it, use a > scoped_refptr I guess I should set the pointer to NULL in OnChannelError() then? I don't want to hold on to the old ResourceMessageFilter after the channel is closed.
On 2011/03/29 19:26:57, Bernhard Bauer wrote: > http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host... > File chrome/browser/renderer_host/browser_render_process_host.h (right): > > http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host... > chrome/browser/renderer_host/browser_render_process_host.h:217: // Weak, owned > by |channel_|. > On 2011/03/29 18:17:16, John Abd-El-Malek wrote: > > what happens if OnChannelError() is called? channel_ will be reset and this > > filter will be deleted. so if you want to hold a pointer to it, use a > > scoped_refptr > > I guess I should set the pointer to NULL in OnChannelError() then? I don't want > to hold on to the old ResourceMessageFilter after the channel is closed. Thinking about it more, we really should avoid having to hold on to the ResourceMessageFilter in BrowserRenderProcessHost. RMF lives on the IO thread, and BRPH lives on the UI thread. Storing pointer from BRPH to RMF and using locks to guard it is unwieldy. Also watching the pref on the IO thread, and keeping the pointer, strikes me as something that should be avoided. Instead of storing this in the browser, and sending a referrer in the renderer that then gets thrown out, how about just storing this in the renderer? BRPH can send a message when this pref changes, and either RD or another object can store it.
will you add some extension api tests for this? will it be possible to e.g. disable referrers only for incognito profiles? http://codereview.chromium.org/6773006/diff/2001/chrome/common/extensions/api... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6773006/diff/2001/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:4785: "value": ["enablePing", {"type":"boolean"}], I think enabledHyperlinkAuditing would be a better name
On Wed, Mar 30, 2011 at 10:29, <jochen@chromium.org> wrote: > will you add some extension api tests for this? > > will it be possible to e.g. disable referrers only for incognito profiles? It uses the standard preference API for extensions, so assuming that the BrowserRenderProcessHost is hooked up to the OffTheRecordProfile, it should all work automatically. > > http://codereview.chromium.org/6773006/diff/2001/chrome/common/extensions/api... > File chrome/common/extensions/api/extension_api.json (right): > > http://codereview.chromium.org/6773006/diff/2001/chrome/common/extensions/api... > chrome/common/extensions/api/extension_api.json:4785: "value": > ["enablePing", {"type":"boolean"}], > I think enabledHyperlinkAuditing would be a better name > > http://codereview.chromium.org/6773006/ >
On Tue, Mar 29, 2011 at 23:04, <jam@chromium.org> wrote: > On 2011/03/29 19:26:57, Bernhard Bauer wrote: > > http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host... >> >> File chrome/browser/renderer_host/browser_render_process_host.h (right): > > > http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host... >> >> chrome/browser/renderer_host/browser_render_process_host.h:217: // Weak, >> owned >> by |channel_|. >> On 2011/03/29 18:17:16, John Abd-El-Malek wrote: >> > what happens if OnChannelError() is called? channel_ will be reset and >> > this >> > filter will be deleted. so if you want to hold a pointer to it, use a >> > scoped_refptr > >> I guess I should set the pointer to NULL in OnChannelError() then? I don't > > want >> >> to hold on to the old ResourceMessageFilter after the channel is closed. > > Thinking about it more, we really should avoid having to hold on to the > ResourceMessageFilter in BrowserRenderProcessHost. RMF lives on the IO > thread, > and BRPH lives on the UI thread. Storing pointer from BRPH to RMF and using > locks to guard it is unwieldy. Also watching the pref on the IO thread, and > keeping the pointer, strikes me as something that should be avoided. > > Instead of storing this in the browser, and sending a referrer in the > renderer > that then gets thrown out, how about just storing this in the renderer? > BRPH > can send a message when this pref changes, and either RD or another object > can > store it. Hm, we already have (in this CL) an additional enable_referrers flag in RendererPreferences (I figured referrers are not a chrome-specific feature), and some debugging seems to imply that RenderView::willSendRequest already gets to see the request (and strips referrers), so doing the same thing in ResourceDispatcher might not even be necessary. Of course, I don't know that much about the request loading mechanisms; I basically just tried to replace all occurrences of |CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoReferrers)| with something which reads from preferences. > http://codereview.chromium.org/6773006/ >
OK, I now strip referrers in the renderer in RenderView (for WebKit) and in the browser in ChromeNetworkDelegate (for requests which are not directly coming from the renderer, like downloads). John, Jochen, could you take another look?
lgtm but i'm not familiar with the browser/prefs code, so please have someone more familiar with the API design of it look those changes over. It seems that prefs and profile are peers, and they shouldn't have to know about each other. http://codereview.chromium.org/6773006/diff/12025/chrome/browser/extensions/e... File chrome/browser/extensions/extension_content_settings_apitest.cc (right): http://codereview.chromium.org/6773006/diff/12025/chrome/browser/extensions/e... chrome/browser/extensions/extension_content_settings_apitest.cc:27: EXPECT_TRUE(pref_service->GetBoolean(prefs::kEnableReferrers)); nit: what is this testing?
On 2011/04/04 23:36:32, John Abd-El-Malek wrote: > lgtm but i'm not familiar with the browser/prefs code, so please have someone > more familiar with the API design of it look those changes over. It seems that > prefs and profile are peers, and they shouldn't have to know about each other. Mattias, could you take a look? http://codereview.chromium.org/6773006/diff/12025/chrome/browser/extensions/e... File chrome/browser/extensions/extension_content_settings_apitest.cc (right): http://codereview.chromium.org/6773006/diff/12025/chrome/browser/extensions/e... chrome/browser/extensions/extension_content_settings_apitest.cc:27: EXPECT_TRUE(pref_service->GetBoolean(prefs::kEnableReferrers)); On 2011/04/04 23:36:32, John Abd-El-Malek wrote: > nit: what is this testing? The test extension reads the preference value and tests that it is false, then sets it to true, and we test it here.
extension bits LGTM for the network delegate, I'd propose to add willchan
LG, just some nits I found while looking at chrome/browser/prefs/* and friends. http://codereview.chromium.org/6773006/diff/12025/chrome/browser/prefs/pref_m... File chrome/browser/prefs/pref_member.cc (right): http://codereview.chromium.org/6773006/diff/12025/chrome/browser/prefs/pref_m... chrome/browser/prefs/pref_member.cc:77: } No need for curlies (or make it consistent with the NOTREACHED below?) http://codereview.chromium.org/6773006/diff/12025/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/6773006/diff/12025/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:117: scoped_ptr<BooleanPrefMember> enable_referrers; Any reason this can't be a regular data member, Destroy()ing it when done? http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h File chrome/common/pref_names.h (right): http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h#... chrome/common/pref_names.h:475: extern const char kEnableReferrers[]; Note that the lower part of this file is for local state prefs, while profile prefs go in the top part. I always tell people, but nobody seems to care :(
http://codereview.chromium.org/6773006/diff/12025/chrome/browser/prefs/pref_m... File chrome/browser/prefs/pref_member.cc (right): http://codereview.chromium.org/6773006/diff/12025/chrome/browser/prefs/pref_m... chrome/browser/prefs/pref_member.cc:77: } On 2011/04/05 08:48:27, Mattias Nissler wrote: > No need for curlies (or make it consistent with the NOTREACHED below?) Done. http://codereview.chromium.org/6773006/diff/12025/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/6773006/diff/12025/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:117: scoped_ptr<BooleanPrefMember> enable_referrers; On 2011/04/05 08:48:27, Mattias Nissler wrote: > Any reason this can't be a regular data member, Destroy()ing it when done? I initialize it on the UI thread, and then pass it to the ChromeNetworkDelegate, which accesses it from the IO thread, so I need to pass the pointer around. http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h File chrome/common/pref_names.h (right): http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h#... chrome/common/pref_names.h:475: extern const char kEnableReferrers[]; On 2011/04/05 08:48:27, Mattias Nissler wrote: > Note that the lower part of this file is for local state prefs, while profile > prefs go in the top part. I always tell people, but nobody seems to care :( :-( I didn't realize that. Added a comment to make it a bit clearer.
Pref stuff LGTM (with a nit). http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h File chrome/common/pref_names.h (right): http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h#... chrome/common/pref_names.h:475: extern const char kEnableReferrers[]; On 2011/04/05 10:48:34, Bernhard Bauer wrote: > On 2011/04/05 08:48:27, Mattias Nissler wrote: > > Note that the lower part of this file is for local state prefs, while profile > > prefs go in the top part. I always tell people, but nobody seems to care :( > > :-( I didn't realize that. Added a comment to make it a bit clearer. Thanks. I guess the state it's currently makes it very hard to figure out what's the right thing. I filed a bug to clean this up: http://code.google.com/p/chromium/issues/detail?id=78415 http://codereview.chromium.org/6773006/diff/14026/chrome/common/pref_names.h#... chrome/common/pref_names.h:15: // Profile prefs Grammar.
Pref stuff LGTM (with a nit). http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h File chrome/common/pref_names.h (right): http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h#... chrome/common/pref_names.h:475: extern const char kEnableReferrers[]; On 2011/04/05 10:48:34, Bernhard Bauer wrote: > On 2011/04/05 08:48:27, Mattias Nissler wrote: > > Note that the lower part of this file is for local state prefs, while profile > > prefs go in the top part. I always tell people, but nobody seems to care :( > > :-( I didn't realize that. Added a comment to make it a bit clearer. Thanks. I guess the state it's currently makes it very hard to figure out what's the right thing. I filed a bug to clean this up: http://code.google.com/p/chromium/issues/detail?id=78415 http://codereview.chromium.org/6773006/diff/14026/chrome/common/pref_names.h#... chrome/common/pref_names.h:15: // Profile prefs Grammar.
On 2011/04/05 07:47:30, jochen wrote: > extension bits LGTM > > for the network delegate, I'd propose to add willchan Will, could you take a look at the network delegate part? http://codereview.chromium.org/6773006/diff/14026/chrome/common/pref_names.h File chrome/common/pref_names.h (right): http://codereview.chromium.org/6773006/diff/14026/chrome/common/pref_names.h#... chrome/common/pref_names.h:15: // Profile prefs. Please Local State prefs below instead. On 2011/04/05 10:57:51, Mattias Nissler wrote: > Grammar. Whoops, forgot to save.
http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.h (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:26: // PrefService and moved to the IO thread. It can be NULL. This object will Why are you allowing it to be NULL? Does it crash otherwise in tests? If possible, it'd be nice if it were always non-NULL and you added a DCHECK to enforce this. It makes new readers of the code have to think less and avoids having to do NULL checks everywhere. OIC why, it looks like you are passing in NULL for the system ChromeNetworkDelegate. Is that really what you want to do? Should we be setting the referer for system requests if an extension disables them? http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:137: params->enable_referrers->ObserveProfileDestruction(profile); Why is this necessary? The associated ProfileIOData / ChromeNetworkDelegate should die soon after the Profile dies. http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:116: // Initialized on the UI thread. No need for comment. Everything in this struct has to be initialized on the UI thread. See the comment above.
Thanks for the review! http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.h (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:26: // PrefService and moved to the IO thread. It can be NULL. This object will On 2011/04/05 13:10:41, willchan wrote: > Why are you allowing it to be NULL? Does it crash otherwise in tests? If > possible, it'd be nice if it were always non-NULL and you added a DCHECK to > enforce this. It makes new readers of the code have to think less and avoids > having to do NULL checks everywhere. > > OIC why, it looks like you are passing in NULL for the system > ChromeNetworkDelegate. Is that really what you want to do? Should we be setting > the referer for system requests if an extension disables them? Yeah, I thought about adding support for the pref in the system network delegate as well, I just decided against doing it right now, as the preference is not registered in Local State, and there is no client which would set it either. If you think it's worthwhile though, I can re-add it. It's also somewhat unlikely that not stripping the referrer from system requests is going to leak information, as the referrers would probably have to be explicitly specified anyway. http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:137: params->enable_referrers->ObserveProfileDestruction(profile); On 2011/04/05 13:10:41, willchan wrote: > Why is this necessary? The associated ProfileIOData / ChromeNetworkDelegate > should die soon after the Profile dies. Sadly, not soon enough. When the PrefMember is destroyed, it has to be unregistered from observing pref changes, which has to happen a) on the UI thread, and b) before the PrefService is destroyed (which is not guaranteed to be after the ChromeNetworkDelegate is destroyed). http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:116: // Initialized on the UI thread. On 2011/04/05 13:10:41, willchan wrote: > No need for comment. Everything in this struct has to be initialized on the UI > thread. See the comment above. Easy enough :-) Done.
http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.h (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:26: // PrefService and moved to the IO thread. It can be NULL. This object will On 2011/04/05 14:49:02, Bernhard Bauer wrote: > On 2011/04/05 13:10:41, willchan wrote: > > Why are you allowing it to be NULL? Does it crash otherwise in tests? If > > possible, it'd be nice if it were always non-NULL and you added a DCHECK to > > enforce this. It makes new readers of the code have to think less and avoids > > having to do NULL checks everywhere. > > > > OIC why, it looks like you are passing in NULL for the system > > ChromeNetworkDelegate. Is that really what you want to do? Should we be > setting > > the referer for system requests if an extension disables them? > > Yeah, I thought about adding support for the pref in the system network delegate > as well, I just decided against doing it right now, as the preference is not > registered in Local State, and there is no client which would set it either. If > you think it's worthwhile though, I can re-add it. I think I'd prefer this, but I don't feel too strongly. I mostly like avoiding NULL checks where possible since people often forget when they are necessary. > > It's also somewhat unlikely that not stripping the referrer from system requests > is going to leak information, as the referrers would probably have to be > explicitly specified anyway. Heh, you never know :) What if we have a DomUI page for configuring system settings (profile-agnostic), and it uses XHR? That would probably be a non-profile associated URLRequestContext that might have a referrer. http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:137: params->enable_referrers->ObserveProfileDestruction(profile); On 2011/04/05 14:49:02, Bernhard Bauer wrote: > On 2011/04/05 13:10:41, willchan wrote: > > Why is this necessary? The associated ProfileIOData / ChromeNetworkDelegate > > should die soon after the Profile dies. > > Sadly, not soon enough. When the PrefMember is destroyed, it has to be > unregistered from observing pref changes, which has to happen a) on the UI > thread, and b) before the PrefService is destroyed (which is not guaranteed to > be after the ChromeNetworkDelegate is destroyed). Wait, but this BooleanPrefMember lives in the ChromeNetworkDelegate which lives on the IO thread. We'd accessing this without locks? Is this safe? It seems a bit dodgy.
http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.h (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:26: // PrefService and moved to the IO thread. It can be NULL. This object will On 2011/04/05 15:06:18, willchan wrote: > On 2011/04/05 14:49:02, Bernhard Bauer wrote: > > On 2011/04/05 13:10:41, willchan wrote: > > > Why are you allowing it to be NULL? Does it crash otherwise in tests? If > > > possible, it'd be nice if it were always non-NULL and you added a DCHECK to > > > enforce this. It makes new readers of the code have to think less and avoids > > > having to do NULL checks everywhere. > > > > > > OIC why, it looks like you are passing in NULL for the system > > > ChromeNetworkDelegate. Is that really what you want to do? Should we be > > setting > > > the referer for system requests if an extension disables them? > > > > Yeah, I thought about adding support for the pref in the system network > delegate > > as well, I just decided against doing it right now, as the preference is not > > registered in Local State, and there is no client which would set it either. > If > > you think it's worthwhile though, I can re-add it. > > I think I'd prefer this, but I don't feel too strongly. I mostly like avoiding > NULL checks where possible since people often forget when they are necessary. > > > > > It's also somewhat unlikely that not stripping the referrer from system > requests > > is going to leak information, as the referrers would probably have to be > > explicitly specified anyway. > > Heh, you never know :) What if we have a DomUI page for configuring system > settings (profile-agnostic), and it uses XHR? That would probably be a > non-profile associated URLRequestContext that might have a referrer. Fair enough. Done. http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:137: params->enable_referrers->ObserveProfileDestruction(profile); On 2011/04/05 15:06:18, willchan wrote: > On 2011/04/05 14:49:02, Bernhard Bauer wrote: > > On 2011/04/05 13:10:41, willchan wrote: > > > Why is this necessary? The associated ProfileIOData / ChromeNetworkDelegate > > > should die soon after the Profile dies. > > > > Sadly, not soon enough. When the PrefMember is destroyed, it has to be > > unregistered from observing pref changes, which has to happen a) on the UI > > thread, and b) before the PrefService is destroyed (which is not guaranteed to > > be after the ChromeNetworkDelegate is destroyed). > > Wait, but this BooleanPrefMember lives in the ChromeNetworkDelegate which lives > on the IO thread. We'd accessing this without locks? Is this safe? It seems a > bit dodgy. We specifically added support for reading prefs without locks on other threads, by observing pref changes on the UI thread and then updating the cached pref value on the IO thread.
http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:137: params->enable_referrers->ObserveProfileDestruction(profile); On 2011/04/05 16:12:59, Bernhard Bauer wrote: > On 2011/04/05 15:06:18, willchan wrote: > > On 2011/04/05 14:49:02, Bernhard Bauer wrote: > > > On 2011/04/05 13:10:41, willchan wrote: > > > > Why is this necessary? The associated ProfileIOData / > ChromeNetworkDelegate > > > > should die soon after the Profile dies. > > > > > > Sadly, not soon enough. When the PrefMember is destroyed, it has to be > > > unregistered from observing pref changes, which has to happen a) on the UI > > > thread, and b) before the PrefService is destroyed (which is not guaranteed > to > > > be after the ChromeNetworkDelegate is destroyed). Which PrefService? Sorry, there seem to be a whole bunch and the shutdown ordering is not obvious to me. > > > > Wait, but this BooleanPrefMember lives in the ChromeNetworkDelegate which > lives > > on the IO thread. We'd accessing this without locks? Is this safe? It seems a > > bit dodgy. > > We specifically added support for reading prefs without locks on other threads, > by observing pref changes on the UI thread and then updating the cached pref > value on the IO thread. I see, sounds great. I see that the PROFILE_DESTROYED notification is issued at the start of ~OffTheRecordProfile and ~ProfileImpl. What guarantee is there that this will hold true? Is this documented somewhere? What happens if a profile releases the last reference to ProfileIOData before this notification is issued? Currently the reference is held as a member of a Profile subclass, so it should be destroyed after the destructor body has run. We don't have any tests enforcing any ordering, nor any comments. If you're adding an ordering dependency, some sort of enforcement would be nice.
http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:137: params->enable_referrers->ObserveProfileDestruction(profile); On 2011/04/05 17:12:51, willchan wrote: > On 2011/04/05 16:12:59, Bernhard Bauer wrote: > > On 2011/04/05 15:06:18, willchan wrote: > > > On 2011/04/05 14:49:02, Bernhard Bauer wrote: > > > > On 2011/04/05 13:10:41, willchan wrote: > > > > > Why is this necessary? The associated ProfileIOData / > > ChromeNetworkDelegate > > > > > should die soon after the Profile dies. > > > > > > > > Sadly, not soon enough. When the PrefMember is destroyed, it has to be > > > > unregistered from observing pref changes, which has to happen a) on the UI > > > > thread, and b) before the PrefService is destroyed (which is not > guaranteed > > to > > > > be after the ChromeNetworkDelegate is destroyed). > > Which PrefService? Sorry, there seem to be a whole bunch and the shutdown > ordering is not obvious to me. The one returned by |GetPrefs()| (which is the incognito PrefService for an OffTheRecordProfile and the regular one for a regular ProfileImpl), and it is destroyed via the ProfileManager in the destructor of BrowserProcessImpl (before the IO thread is stopped in the same destructor). > > > > > > Wait, but this BooleanPrefMember lives in the ChromeNetworkDelegate which > > lives > > > on the IO thread. We'd accessing this without locks? Is this safe? It seems > a > > > bit dodgy. > > > > We specifically added support for reading prefs without locks on other > threads, > > by observing pref changes on the UI thread and then updating the cached pref > > value on the IO thread. > > I see, sounds great. > > I see that the PROFILE_DESTROYED notification is issued at the start of > ~OffTheRecordProfile and ~ProfileImpl. What guarantee is there that this will > hold true? I guess none apart from the fact that probably a lot of other stuff will start failing if we don't send the notification ;-) I discussed this with Mattias and we agreed that in this case it's probably easier to watch for a notification than to explicitly |Destroy()| the PrefMember, as it is pretty hidden in the ChromeNetworkDelegate. > Is this documented somewhere? What happens if a profile releases the > last reference to ProfileIOData before this notification is issued? Currently > the reference is held as a member of a Profile subclass, so it should be > destroyed after the destructor body has run. So we should be fine at the moment, right? > We don't have any tests enforcing > any ordering, nor any comments. If you're adding an ordering dependency, some > sort of enforcement would be nice. Would you be okay with me just adding a comment which states that the notification should be sent before the other stuff is destroyed? The use-after-free we'd possibly get when getting it wrong would probably be hard to reliably test for :-/
On Tue, Apr 5, 2011 at 20:24, <bauerb@chromium.org> wrote: > > http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... > File chrome/browser/profiles/profile_io_data.cc (right): > > http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_io_data.cc:137: > params->enable_referrers->ObserveProfileDestruction(profile); > On 2011/04/05 17:12:51, willchan wrote: >> >> On 2011/04/05 16:12:59, Bernhard Bauer wrote: >> > On 2011/04/05 15:06:18, willchan wrote: >> > > On 2011/04/05 14:49:02, Bernhard Bauer wrote: >> > > > On 2011/04/05 13:10:41, willchan wrote: >> > > > > Why is this necessary? The associated ProfileIOData / >> > ChromeNetworkDelegate >> > > > > should die soon after the Profile dies. >> > > > >> > > > Sadly, not soon enough. When the PrefMember is destroyed, it has > > to be >> >> > > > unregistered from observing pref changes, which has to happen a) > > on the UI >> >> > > > thread, and b) before the PrefService is destroyed (which is not >> guaranteed >> > to >> > > > be after the ChromeNetworkDelegate is destroyed). > >> Which PrefService? Sorry, there seem to be a whole bunch and the > > shutdown >> >> ordering is not obvious to me. > > The one returned by |GetPrefs()| (which is the incognito PrefService for > an OffTheRecordProfile and the regular one for a regular ProfileImpl), > and it is destroyed via the ProfileManager in the destructor of > BrowserProcessImpl (before the IO thread is stopped in the same > destructor). > >> > > >> > > Wait, but this BooleanPrefMember lives in the > > ChromeNetworkDelegate which >> >> > lives >> > > on the IO thread. We'd accessing this without locks? Is this safe? > > It seems >> >> a >> > > bit dodgy. >> > >> > We specifically added support for reading prefs without locks on > > other >> >> threads, >> > by observing pref changes on the UI thread and then updating the > > cached pref >> >> > value on the IO thread. > >> I see, sounds great. > >> I see that the PROFILE_DESTROYED notification is issued at the start > > of >> >> ~OffTheRecordProfile and ~ProfileImpl. What guarantee is there that > > this will >> >> hold true? > > I guess none apart from the fact that probably a lot of other stuff will > start failing if we don't send the notification ;-) > > I discussed this with Mattias and we agreed that in this case it's > probably easier to watch for a notification than to explicitly > |Destroy()| the PrefMember, as it is pretty hidden in the > ChromeNetworkDelegate. > >> Is this documented somewhere? What happens if a profile releases the >> last reference to ProfileIOData before this notification is issued? > > Currently >> >> the reference is held as a member of a Profile subclass, so it should > > be >> >> destroyed after the destructor body has run. > > So we should be fine at the moment, right? > >> We don't have any tests enforcing >> any ordering, nor any comments. If you're adding an ordering > > dependency, some >> >> sort of enforcement would be nice. > > Would you be okay with me just adding a comment which states that the > notification should be sent before the other stuff is destroyed? The > use-after-free we'd possibly get when getting it wrong would probably be > hard to reliably test for :-/ Ping? I added comments to the destructors of ProfileImpl and OffTheRecordProfileImpl right before they send the notification, explaining that this should happen first thing. I realize that the shutdown code is pretty fragile (to say the least; your recent mail to chromium-dev pretty much hit the nail on the head), but I'm not sure how I could otherwise improve the situation here. Bernhard. > http://codereview.chromium.org/6773006/ >
http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:137: params->enable_referrers->ObserveProfileDestruction(profile); On 2011/04/05 18:24:47, Bernhard Bauer wrote: > On 2011/04/05 17:12:51, willchan wrote: > > On 2011/04/05 16:12:59, Bernhard Bauer wrote: > > > On 2011/04/05 15:06:18, willchan wrote: > > > > On 2011/04/05 14:49:02, Bernhard Bauer wrote: > > > > > On 2011/04/05 13:10:41, willchan wrote: > > > > > > Why is this necessary? The associated ProfileIOData / > > > ChromeNetworkDelegate > > > > > > should die soon after the Profile dies. > > > > > > > > > > Sadly, not soon enough. When the PrefMember is destroyed, it has to be > > > > > unregistered from observing pref changes, which has to happen a) on the > UI > > > > > thread, and b) before the PrefService is destroyed (which is not > > guaranteed > > > to > > > > > be after the ChromeNetworkDelegate is destroyed). I don't think I like adding implicit notifications like this. First off, it's dependent on the Profile. It's possible that a good chunk of this code will move into content/ which doesn't know about Chrome profiles. I prefer explicit shutdown notifications. I'd rather you add a Shutdown member function to ProfileImplIOData::Handle(), which calls ProfileImplIOData::Shutdown(), which notifies objects as necessary. This way, if we need to forward such shutdown notifications to the content/ code, we won't have to hook the Profile destroyed notification. > > > > Which PrefService? Sorry, there seem to be a whole bunch and the shutdown > > ordering is not obvious to me. > > The one returned by |GetPrefs()| (which is the incognito PrefService for an > OffTheRecordProfile and the regular one for a regular ProfileImpl), and it is > destroyed via the ProfileManager in the destructor of BrowserProcessImpl (before > the IO thread is stopped in the same destructor). > > > > > > > > > Wait, but this BooleanPrefMember lives in the ChromeNetworkDelegate which > > > lives > > > > on the IO thread. We'd accessing this without locks? Is this safe? It > seems > > a > > > > bit dodgy. > > > > > > We specifically added support for reading prefs without locks on other > > threads, > > > by observing pref changes on the UI thread and then updating the cached pref > > > value on the IO thread. > > > > I see, sounds great. > > > > I see that the PROFILE_DESTROYED notification is issued at the start of > > ~OffTheRecordProfile and ~ProfileImpl. What guarantee is there that this will > > hold true? > > I guess none apart from the fact that probably a lot of other stuff will start > failing if we don't send the notification ;-) > > I discussed this with Mattias and we agreed that in this case it's probably > easier to watch for a notification than to explicitly |Destroy()| the > PrefMember, as it is pretty hidden in the ChromeNetworkDelegate. > > > Is this documented somewhere? What happens if a profile releases the > > last reference to ProfileIOData before this notification is issued? Currently > > the reference is held as a member of a Profile subclass, so it should be > > destroyed after the destructor body has run. > > So we should be fine at the moment, right? > > > We don't have any tests enforcing > > any ordering, nor any comments. If you're adding an ordering dependency, some > > sort of enforcement would be nice. > > Would you be okay with me just adding a comment which states that the > notification should be sent before the other stuff is destroyed? The > use-after-free we'd possibly get when getting it wrong would probably be hard to > reliably test for :-/ http://codereview.chromium.org/6773006/diff/24001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): http://codereview.chromium.org/6773006/diff/24001/chrome/browser/io_thread.h#... chrome/browser/io_thread.h:211: // Weak, owned by the ChromeNetworkDelegate (which is transitively owned Why don't you just put this in the ChromeNetworkDelegate destructor? Oh I see, it's because the destructor is shared between the system one and the profile specific ChromeNetworkDelegate. We should just refactor them to be different classes. Using this weak pointer like this is a bit risky.
On Fri, Apr 8, 2011 at 21:10, <willchan@chromium.org> wrote: > > http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... > File chrome/browser/profiles/profile_io_data.cc (right): > > http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_io_data.cc:137: > params->enable_referrers->ObserveProfileDestruction(profile); > On 2011/04/05 18:24:47, Bernhard Bauer wrote: >> >> On 2011/04/05 17:12:51, willchan wrote: >> > On 2011/04/05 16:12:59, Bernhard Bauer wrote: >> > > On 2011/04/05 15:06:18, willchan wrote: >> > > > On 2011/04/05 14:49:02, Bernhard Bauer wrote: >> > > > > On 2011/04/05 13:10:41, willchan wrote: >> > > > > > Why is this necessary? The associated ProfileIOData / >> > > ChromeNetworkDelegate >> > > > > > should die soon after the Profile dies. >> > > > > >> > > > > Sadly, not soon enough. When the PrefMember is destroyed, it > > has to be >> >> > > > > unregistered from observing pref changes, which has to happen > > a) on the >> >> UI >> > > > > thread, and b) before the PrefService is destroyed (which is > > not >> >> > guaranteed >> > > to >> > > > > be after the ChromeNetworkDelegate is destroyed). > > I don't think I like adding implicit notifications like this. First off, > it's dependent on the Profile. It's possible that a good chunk of this > code will move into content/ which doesn't know about Chrome profiles. I > prefer explicit shutdown notifications. I'd rather you add a Shutdown > member function to ProfileImplIOData::Handle(), which calls > ProfileImplIOData::Shutdown(), which notifies objects as necessary. This > way, if we need to forward such shutdown notifications to the content/ > code, we won't have to hook the Profile destroyed notification. Ok, I added Shutdown() methods to the ProfileIOData subclasses and a ShutdownOnUIThread() method to ChromeNetworkDelegate. PTAL. > >> > >> > Which PrefService? Sorry, there seem to be a whole bunch and the > > shutdown >> >> > ordering is not obvious to me. > >> The one returned by |GetPrefs()| (which is the incognito PrefService > > for an >> >> OffTheRecordProfile and the regular one for a regular ProfileImpl), > > and it is >> >> destroyed via the ProfileManager in the destructor of > > BrowserProcessImpl (before >> >> the IO thread is stopped in the same destructor). > >> > > > >> > > > Wait, but this BooleanPrefMember lives in the > > ChromeNetworkDelegate which >> >> > > lives >> > > > on the IO thread. We'd accessing this without locks? Is this > > safe? It >> >> seems >> > a >> > > > bit dodgy. >> > > >> > > We specifically added support for reading prefs without locks on > > other >> >> > threads, >> > > by observing pref changes on the UI thread and then updating the > > cached pref >> >> > > value on the IO thread. >> > >> > I see, sounds great. >> > >> > I see that the PROFILE_DESTROYED notification is issued at the start > > of >> >> > ~OffTheRecordProfile and ~ProfileImpl. What guarantee is there that > > this will >> >> > hold true? > >> I guess none apart from the fact that probably a lot of other stuff > > will start >> >> failing if we don't send the notification ;-) > >> I discussed this with Mattias and we agreed that in this case it's > > probably >> >> easier to watch for a notification than to explicitly |Destroy()| the >> PrefMember, as it is pretty hidden in the ChromeNetworkDelegate. > >> > Is this documented somewhere? What happens if a profile releases the >> > last reference to ProfileIOData before this notification is issued? > > Currently >> >> > the reference is held as a member of a Profile subclass, so it > > should be >> >> > destroyed after the destructor body has run. > >> So we should be fine at the moment, right? > >> > We don't have any tests enforcing >> > any ordering, nor any comments. If you're adding an ordering > > dependency, some >> >> > sort of enforcement would be nice. > >> Would you be okay with me just adding a comment which states that the >> notification should be sent before the other stuff is destroyed? The >> use-after-free we'd possibly get when getting it wrong would probably > > be hard to >> >> reliably test for :-/ > > http://codereview.chromium.org/6773006/diff/24001/chrome/browser/io_thread.h > File chrome/browser/io_thread.h (right): > > http://codereview.chromium.org/6773006/diff/24001/chrome/browser/io_thread.h#... > chrome/browser/io_thread.h:211: // Weak, owned by the > ChromeNetworkDelegate (which is transitively owned > Why don't you just put this in the ChromeNetworkDelegate destructor? Oh > I see, it's because the destructor is shared between the system one and > the profile specific ChromeNetworkDelegate. We should just refactor them > to be different classes. Using this weak pointer like this is a bit > risky. This is now taken care of by ChromeNetworkDelegate::ShutdownOnUIThread(). > http://codereview.chromium.org/6773006/ >
http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if (network_delegate_.get()) This is called on the UI thread, but |network_delegate_| may be being constructed. This is a race. Can we do something similar to what you had with the notification based approach, where we post a shutdown task to the IO thread?
http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if (network_delegate_.get()) On 2011/04/11 15:21:41, willchan wrote: > This is called on the UI thread, but |network_delegate_| may be being > constructed. This is a race. > > Can we do something similar to what you had with the notification based > approach, where we post a shutdown task to the IO thread? Ultimately, we need to unregister the PrefMember from its PrefService, which has to happen on the UI thread :-(
http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if (network_delegate_.get()) On 2011/04/11 15:56:02, Bernhard Bauer wrote: > On 2011/04/11 15:21:41, willchan wrote: > > This is called on the UI thread, but |network_delegate_| may be being > > constructed. This is a race. > > > > Can we do something similar to what you had with the notification based > > approach, where we post a shutdown task to the IO thread? > > Ultimately, we need to unregister the PrefMember from its PrefService, which has > to happen on the UI thread :-( > Ugh, I'm sorry! I had thought your previous CL did a roundtrip from UI=>IO=>UI. I misread that. Lemme think for a sec. Sorry also if I seem to be jerking you around over a small detail, I definitely was not planning on that.
http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if (network_delegate_.get()) On 2011/04/11 16:00:20, willchan wrote: > On 2011/04/11 15:56:02, Bernhard Bauer wrote: > > On 2011/04/11 15:21:41, willchan wrote: > > > This is called on the UI thread, but |network_delegate_| may be being > > > constructed. This is a race. > > > > > > Can we do something similar to what you had with the notification based > > > approach, where we post a shutdown task to the IO thread? > > > > Ultimately, we need to unregister the PrefMember from its PrefService, which > has > > to happen on the UI thread :-( > > > > Ugh, I'm sorry! I had thought your previous CL did a roundtrip from UI=>IO=>UI. > I misread that. Lemme think for a sec. Sorry also if I seem to be jerking you > around over a small detail, I definitely was not planning on that. Uh, no, I totally realize that we should do something about it. I think we'd also get a problem if we shut down after InitializeProfileParams has been called (so the PrefMember is initialized), but before the NetworkDelegate has been created, so it would still live in the LazyParams :-/
http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if (network_delegate_.get()) On 2011/04/11 16:00:20, willchan wrote: > On 2011/04/11 15:56:02, Bernhard Bauer wrote: > > On 2011/04/11 15:21:41, willchan wrote: > > > This is called on the UI thread, but |network_delegate_| may be being > > > constructed. This is a race. > > > > > > Can we do something similar to what you had with the notification based > > > approach, where we post a shutdown task to the IO thread? > > > > Ultimately, we need to unregister the PrefMember from its PrefService, which > has > > to happen on the UI thread :-( > > > > Ugh, I'm sorry! I had thought your previous CL did a roundtrip from UI=>IO=>UI. > I misread that. Lemme think for a sec. Sorry also if I seem to be jerking you > around over a small detail, I definitely was not planning on that. The only other solution I can come up with involves adding the PrefMember directly into the ProfileIOData, so we know it exists, and then have the ChromeNetworkDelegate keep a raw pointer to it. I'm not sure how much I like this solution either =/ It's not clear to me which is better. Choose whichever you prefer (your previous solution with the profile observing, or my other proposal where you add the PrefMember directly into the ProfileIOData).
On Mon, Apr 11, 2011 at 18:24, <willchan@chromium.org> wrote: > > http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... > File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): > > http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off... > chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if > (network_delegate_.get()) > On 2011/04/11 16:00:20, willchan wrote: >> >> On 2011/04/11 15:56:02, Bernhard Bauer wrote: >> > On 2011/04/11 15:21:41, willchan wrote: >> > > This is called on the UI thread, but |network_delegate_| may be > > being >> >> > > constructed. This is a race. >> > > >> > > Can we do something similar to what you had with the notification > > based >> >> > > approach, where we post a shutdown task to the IO thread? >> > >> > Ultimately, we need to unregister the PrefMember from its > > PrefService, which >> >> has >> > to happen on the UI thread :-( >> > > >> Ugh, I'm sorry! I had thought your previous CL did a roundtrip from > > UI=>IO=>UI. >> >> I misread that. Lemme think for a sec. Sorry also if I seem to be > > jerking you >> >> around over a small detail, I definitely was not planning on that. > > The only other solution I can come up with involves adding the > PrefMember directly into the ProfileIOData, so we know it exists, and > then have the ChromeNetworkDelegate keep a raw pointer to it. I'm not > sure how much I like this solution either =/ Done! I chose this solution, because the other one seemed like too much action at a distance to me, the more I thought about it. > It's not clear to me which is better. Choose whichever you prefer (your > previous solution with the profile observing, or my other proposal where > you add the PrefMember directly into the ProfileIOData). > > http://codereview.chromium.org/6773006/
http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:284: void ProfileIOData::Shutdown() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:122: explicit ProfileIOData(Profile* profile, bool is_incognito); Not explicit anymore. I have to say, I resisted adding Profile for a long time. The reason is because ProfileIOData will live on the IO thread. We really don't want to cache the |profile| pointer, because any use on the IO thread is a bug. I've been trying to pass the explicit dependency (the initialized BooleanPrefMember in this case), although I acknowledge that would introduce a bit of code redundancy. I'm also worried about the ProfileIOData subclasses caching Profile*. I'm not sure how draconian I should be. I guess I'd be fine with it if there were comments in the constructors indicating not to cache the Profile*. http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:138: protected: Look at line 73, there's already a protected section. Actually, everything below should be private. Other than the new member functions you've added. http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:167: void Shutdown(); ShutdownOnUI() http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:174: scoped_ptr<BooleanPrefMember> enable_referrers_; Why use a pointer? Can't we just directly embed the BooleanPrefMember and construct it in the initializer list?
(I'll work on the other comments tomorrow, when I'm back at the office) http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:122: explicit ProfileIOData(Profile* profile, bool is_incognito); On 2011/04/12 16:34:18, willchan wrote: > Not explicit anymore. > > I have to say, I resisted adding Profile for a long time. The reason is because > ProfileIOData will live on the IO thread. We really don't want to cache the > |profile| pointer, because any use on the IO thread is a bug. > > I've been trying to pass the explicit dependency (the initialized > BooleanPrefMember in this case), although I acknowledge that would introduce a > bit of code redundancy. I'm also worried about the ProfileIOData subclasses > caching Profile*. I'm not sure how draconian I should be. I guess I'd be fine > with it if there were comments in the constructors indicating not to cache the > Profile*. Hmm, I could initialize the BooleanPrefMember in the ::Handle constructor, right? There we already have the Profile, and I could even extract a static helper method similar to InitializeProfileParams() that does the intialization.
http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:122: explicit ProfileIOData(Profile* profile, bool is_incognito); On 2011/04/12 20:26:05, Bernhard Bauer wrote: > On 2011/04/12 16:34:18, willchan wrote: > > Not explicit anymore. > > > > I have to say, I resisted adding Profile for a long time. The reason is > because > > ProfileIOData will live on the IO thread. We really don't want to cache the > > |profile| pointer, because any use on the IO thread is a bug. > > > > I've been trying to pass the explicit dependency (the initialized > > BooleanPrefMember in this case), although I acknowledge that would introduce a > > bit of code redundancy. I'm also worried about the ProfileIOData subclasses > > caching Profile*. I'm not sure how draconian I should be. I guess I'd be fine > > with it if there were comments in the constructors indicating not to cache the > > Profile*. > > Hmm, I could initialize the BooleanPrefMember in the ::Handle constructor, > right? There we already have the Profile, and I could even extract a static > helper method similar to InitializeProfileParams() that does the intialization. Yes, I would be fine with that. That's what I referred to in causing code redundancy, since It'd have to be done for both ProfileImplIOData::Handle and OffTheRecordProfileIOData::Handle.
http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:284: void ProfileIOData::Shutdown() { On 2011/04/12 16:34:18, willchan wrote: > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Done. http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:122: explicit ProfileIOData(Profile* profile, bool is_incognito); On 2011/04/12 22:43:07, willchan wrote: > On 2011/04/12 20:26:05, Bernhard Bauer wrote: > > On 2011/04/12 16:34:18, willchan wrote: > > > Not explicit anymore. > > > > > > I have to say, I resisted adding Profile for a long time. The reason is > > because > > > ProfileIOData will live on the IO thread. We really don't want to cache the > > > |profile| pointer, because any use on the IO thread is a bug. > > > > > > I've been trying to pass the explicit dependency (the initialized > > > BooleanPrefMember in this case), although I acknowledge that would introduce > a > > > bit of code redundancy. I'm also worried about the ProfileIOData subclasses > > > caching Profile*. I'm not sure how draconian I should be. I guess I'd be > fine > > > with it if there were comments in the constructors indicating not to cache > the > > > Profile*. > > > > Hmm, I could initialize the BooleanPrefMember in the ::Handle constructor, > > right? There we already have the Profile, and I could even extract a static > > helper method similar to InitializeProfileParams() that does the > intialization. > > Yes, I would be fine with that. That's what I referred to in causing code > redundancy, since It'd have to be done for both ProfileImplIOData::Handle and > OffTheRecordProfileIOData::Handle. OK, done (in ChromeNetworkDelegate now, so it can be used by IOThread as well). http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:138: protected: On 2011/04/12 16:34:18, willchan wrote: > Look at line 73, there's already a protected section. Actually, everything below > should be private. Other than the new member functions you've added. Oops, done. http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:167: void Shutdown(); On 2011/04/12 16:34:18, willchan wrote: > ShutdownOnUI() Done. http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:174: scoped_ptr<BooleanPrefMember> enable_referrers_; On 2011/04/12 16:34:18, willchan wrote: > Why use a pointer? Can't we just directly embed the BooleanPrefMember and > construct it in the initializer list? Very nice! Done.
LGTM! http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h#... chrome/browser/io_thread.h:185: BooleanPrefMember enable_referrers_; Nit: maybe we should call this system_enable_referrers_, to indicate that this is Profile agnostic. Up to you! http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:57: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Please include browser_thread.h for this.
Committing now. Thanks for your patience with this CL! http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h#... chrome/browser/io_thread.h:185: BooleanPrefMember enable_referrers_; On 2011/04/13 14:59:00, willchan wrote: > Nit: maybe we should call this system_enable_referrers_, to indicate that this > is Profile agnostic. Up to you! Sounds good. http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:57: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2011/04/13 14:59:00, willchan wrote: > Please include browser_thread.h for this. Done.
Hah, thanks for your patience in putting up with my nittipickiness/strictness :) On Wed, Apr 13, 2011 at 5:14 PM, <bauerb@chromium.org> wrote: > Committing now. Thanks for your patience with this CL! > > > > > http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h > File chrome/browser/io_thread.h (right): > > > http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h#... > chrome/browser/io_thread.h:185: BooleanPrefMember enable_referrers_; > On 2011/04/13 14:59:00, willchan wrote: > >> Nit: maybe we should call this system_enable_referrers_, to indicate >> > that this > >> is Profile agnostic. Up to you! >> > > Sounds good. > > > > http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n... > File chrome/browser/net/chrome_network_delegate.cc (right): > > > http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n... > chrome/browser/net/chrome_network_delegate.cc:57: > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); > On 2011/04/13 14:59:00, willchan wrote: > >> Please include browser_thread.h for this. >> > > Done. > > > http://codereview.chromium.org/6773006/ > |
