Keep incognito notification preferences separate from regular ones, and don't...

Keep incognito notification preferences separate from regular ones, and don't persist them permanently.

BUG=none
TEST=use notifications in incognito


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

[Peter Kasting, 2009-11-30 21:24:32]

http://codereview.chromium.org/455004/diff/1/3
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/455004/diff/1/3#newcode276
chrome/browser/notifications/desktop_notification_service.cc:276: if
(!profile_->IsOffTheRecord()) {
It really sucks to duplicate most of this block between the above function and
this one.  Probably the cleanest solution is to simply combine the two functions
into one.  Another idea is to cache the mutable list pointers, and have them be
NULL when the profile is in incognito mode.

[John Gregg, 2009-11-30 21:45:30]

new code posted.

http://codereview.chromium.org/455004/diff/1/3
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/455004/diff/1/3#newcode276
chrome/browser/notifications/desktop_notification_service.cc:276: if
(!profile_->IsOffTheRecord()) {
okay, fair point; i've collapsed the persistence portion of the two functions
into a single function.

On 2009/11/30 21:24:32, Peter Kasting wrote:
> It really sucks to duplicate most of this block between the above function and
> this one.  Probably the cleanest solution is to simply combine the two
functions
> into one.  Another idea is to cache the mutable list pointers, and have them
be
> NULL when the profile is in incognito mode.

[Peter Kasting, 2009-11-30 21:56:40]

http://codereview.chromium.org/455004/diff/4001/5002
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/455004/diff/4001/5002#newcode252
chrome/browser/notifications/desktop_notification_service.cc:252: if
(!profile_->IsOffTheRecord()) {
Nit: Other code in this file (and the majority of Chrome code) doesn't use {}
here

http://codereview.chromium.org/455004/diff/4001/5002#newcode266
chrome/browser/notifications/desktop_notification_service.cc:266: if
(!profile_->IsOffTheRecord()) {
Nit: See above

[John Gregg, 2009-11-30 21:59:46]

yep, fixed.

http://codereview.chromium.org/455004/diff/4001/5002
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/455004/diff/4001/5002#newcode252
chrome/browser/notifications/desktop_notification_service.cc:252: if
(!profile_->IsOffTheRecord()) {
On 2009/11/30 21:56:41, Peter Kasting wrote:
> Nit: Other code in this file (and the majority of Chrome code) doesn't use {}
> here

Done.

http://codereview.chromium.org/455004/diff/4001/5002#newcode266
chrome/browser/notifications/desktop_notification_service.cc:266: if
(!profile_->IsOffTheRecord()) {
On 2009/11/30 21:56:41, Peter Kasting wrote:
> Nit: See above

Done.

[michaeln, 2009-12-01 00:00:07]

lgtm

http://codereview.chromium.org/455004/diff/3003/3005
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/455004/diff/3003/3005#newcode277
chrome/browser/notifications/desktop_notification_service.cc:277: const GURL&
origin, bool is_allowed) {
maybe DCHECK(!profile_->IsOffTheRecord()), or push this test into the
PersistenPermissionChange method itself to share that last remaing line of code
that can be shared... ala...

if (IsOTR())
  return;