Add a default content setting to the notifications service.

Add a default content setting to the notifications service.

Not yet used anywhere, no behavior change.

BUG=45547
TEST=none (unit tests)

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

[Nico, 2010-07-03 01:40:20]

Writing tests for this is hairier than I thought – I hadn't written
multithreaded tests before.

Hence, I broke this backend change up in two parts. The final part, the changes
to notification_exceptions_table_model.cc will follow in a separate cl.

[dhollowa, 2010-07-03 02:12:01]

http://codereview.chromium.org/2842043/diff/2001/3001
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/2842043/diff/2001/3001#newcode40
chrome/browser/notifications/desktop_notification_service.cc:40: const
ContentSetting kDefaultSetting = CONTENT_SETTING_ASK;
Please wrap in anonymous namespace.

http://codereview.chromium.org/2842043/diff/2001/3001#newcode422
chrome/browser/notifications/desktop_notification_service.cc:422: setting ==
CONTENT_SETTING_DEFAULT ?  kDefaultSetting : setting);
There's an asymmetry here with respect to |GetDefaultContentSetting|.  You read
CONTENT_SETTING_DEFAULT but you never write CONTENT_SETTING_DEFAULT.  Is this
intended?

http://codereview.chromium.org/2842043/diff/2001/3008
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/2842043/diff/2001/3008#newcode268
chrome/test/testing_profile.h:268:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
nit: two space indent.

[Nico, 2010-07-03 02:18:29]

http://codereview.chromium.org/2842043/diff/2001/3001
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/2842043/diff/2001/3001#newcode40
chrome/browser/notifications/desktop_notification_service.cc:40: const
ContentSetting kDefaultSetting = CONTENT_SETTING_ASK;
On 2010/07/03 02:12:01, dhollowa wrote:
> Please wrap in anonymous namespace.

Not necessary, consts implicitly have internal linkage.

http://codereview.chromium.org/2842043/diff/2001/3001#newcode422
chrome/browser/notifications/desktop_notification_service.cc:422: setting ==
CONTENT_SETTING_DEFAULT ?  kDefaultSetting : setting);
On 2010/07/03 02:12:01, dhollowa wrote:
> There's an asymmetry here with respect to |GetDefaultContentSetting|.  You
read
> CONTENT_SETTING_DEFAULT but you never write CONTENT_SETTING_DEFAULT.  Is this
> intended?

Yes, CONTENT_SETTING_DEFAULT should never be stored on disk.

http://codereview.chromium.org/2842043/diff/2001/3008
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/2842043/diff/2001/3008#newcode268
chrome/test/testing_profile.h:268:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
On 2010/07/03 02:12:01, dhollowa wrote:
> nit: two space indent.

Done.

[dhollowa, 2010-07-03 02:22:22]

LGTM, thanks.

[Nico, 2010-07-03 02:33:18]

http://codereview.chromium.org/2842043/diff/2001/3001
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/2842043/diff/2001/3001#newcode422
chrome/browser/notifications/desktop_notification_service.cc:422: setting ==
CONTENT_SETTING_DEFAULT ?  kDefaultSetting : setting);
On 2010/07/03 02:18:29, Nico wrote:
> On 2010/07/03 02:12:01, dhollowa wrote:
> > There's an asymmetry here with respect to |GetDefaultContentSetting|.  You
> read
> > CONTENT_SETTING_DEFAULT but you never write CONTENT_SETTING_DEFAULT.  Is
this
> > intended?
> 
> Yes, CONTENT_SETTING_DEFAULT should never be stored on disk.

(This mirrors SetDefaultContentSetting() and ReadDefaultSettings() in
c/b/host_content_settings_map.cc)

[bulach, 2010-07-05 09:54:54]

LGTM

couple of meta-comments below, probably not worth a new change on their own but
you may want to take a look if you're modifying these files again:

http://codereview.chromium.org/2842043/diff/2001/3003
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

http://codereview.chromium.org/2842043/diff/2001/3003#newcode37
chrome/browser/notifications/desktop_notification_service_unittest.cc:37:
ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
shouldn't this post to IO? also, perhaps DCHECK on the function that it's
blocking the right thread? or perhaps rename LetIOThreadWait to something like
BlockUntilSignaled()?

http://codereview.chromium.org/2842043/diff/2001/3005
File chrome/browser/notifications/notifications_prefs_cache.h (right):

http://codereview.chromium.org/2842043/diff/2001/3005#newcode26
chrome/browser/notifications/notifications_prefs_cache.h:26: // Once
is_initialized_() is set, all accesses must happen on the IO thread.
s/is_initialized_()/is_initialized()/

[Nico, 2010-07-05 15:50:13]

Thanks! I'll try to get the spello fixed in my next CL in this area (hopefully
in the next few hours).

http://codereview.chromium.org/2842043/diff/2001/3003
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

http://codereview.chromium.org/2842043/diff/2001/3003#newcode37
chrome/browser/notifications/desktop_notification_service_unittest.cc:37:
ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
On 2010/07/05 09:54:54, bulach wrote:
> shouldn't this post to IO? also, perhaps DCHECK on the function that it's
> blocking the right thread? or perhaps rename LetIOThreadWait to something like
> BlockUntilSignaled()?

Yes: http://src.chromium.org/viewvc/chrome?view=rev&revision=51588 :-/

http://codereview.chromium.org/2842043/diff/2001/3005
File chrome/browser/notifications/notifications_prefs_cache.h (right):

http://codereview.chromium.org/2842043/diff/2001/3005#newcode26
chrome/browser/notifications/notifications_prefs_cache.h:26: // Once
is_initialized_() is set, all accesses must happen on the IO thread.
On 2010/07/05 09:54:54, bulach wrote:
> s/is_initialized_()/is_initialized()/

Done.