Backend changes for notifications content settings.

Backend changes for notifications content settings.

NotificationExceptionsTableModel is now functional, and DesktopNotificationService::GetContentSetting contains the new permission behavior. Both aren't used anywhere yet, so still no functionality change (but this is the last CL without functionality changes).

Also add a ton of tests.

BUG=45547
TEST=unit tests

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

[bulach, 2010-07-06 10:05:45]

LGTM

nice! a few comments and suggestions below:

http://codereview.chromium.org/2868042/diff/15/12001
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/2868042/diff/15/12001#newcode443
chrome/browser/notifications/desktop_notification_service.cc:443: DCHECK_NE(-1,
allowed_sites->Remove(value)) << origin << " was not allowed";
I think DCHECK is not compiled in release mode (and in general I think we try to
avoid doing real work inside macros..)

perhaps:
int removed = allowed_sites->Remove(value);
DCHECK_NE(-1, removed) << origin << " was not allowed";

http://codereview.chromium.org/2868042/diff/15/12001#newcode444
chrome/browser/notifications/desktop_notification_service.cc:444:
ScopedPrefUpdate updateAllowed(
s/updateAllowed/update_allowed/

http://codereview.chromium.org/2868042/diff/15/12001#newcode461
chrome/browser/notifications/desktop_notification_service.cc:461: DCHECK_NE(-1,
denied_sites->Remove(value)) << origin << " was not blocked";
as above

http://codereview.chromium.org/2868042/diff/15/12001#newcode462
chrome/browser/notifications/desktop_notification_service.cc:462:
ScopedPrefUpdate updateAllowed(
:)
s/eAllowed/e_denied/

http://codereview.chromium.org/2868042/diff/15/12003
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

http://codereview.chromium.org/2868042/diff/15/12003#newcode36
chrome/browser/notifications/desktop_notification_service_unittest.cc:36: //
Calling PauseIOThread() here isn't save, because the runnable method
s/save/safe/

http://codereview.chromium.org/2868042/diff/15/12003#newcode48
chrome/browser/notifications/desktop_notification_service_unittest.cc:48:
NewRunnableMethod(this, &ThreadProxy::PauseIOThreadIO));
perhaps just call PauseIOThread() ? more below..

http://codereview.chromium.org/2868042/diff/15/12003#newcode54
chrome/browser/notifications/desktop_notification_service_unittest.cc:54:
ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
seems a bit confusing PauseIOThread and PauseIOThreadIO.

perhaps:
void PauseIOThread() {
  if (!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
    ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
        &ThreadProxy::PauseIOThread);
    return;
  }
  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
  io_event_.Wait();
}

...if you want to keep the methods separated, then perhaps RequestPauseIOThread
(for the one that calls PostTask) and PauseIOThread (for the one that calls
Wait) ?

http://codereview.chromium.org/2868042/diff/15/12003#newcode70
chrome/browser/notifications/desktop_notification_service_unittest.cc:70: void
PauseIOThreadIO() {
see above

http://codereview.chromium.org/2868042/diff/15/12004
File chrome/browser/notifications/notification_exceptions_table_model.cc
(right):

http://codereview.chromium.org/2868042/diff/15/12004#newcode19
chrome/browser/notifications/notification_exceptions_table_model.cc:19:
std::vector<GURL> allowed = service_->GetAllowedOrigins();
perhaps pre-allocate entries_?
 entries_.reserve(allowed.size() + blocked.sized());

http://codereview.chromium.org/2868042/diff/15/12004#newcode66
chrome/browser/notifications/notification_exceptions_table_model.cc:66: return
UTF8ToWide(GeolocationContentSettingsMap::OriginToString(
// TODO(bulach): factor out in a common function so that notification won't
depend on Geolocation.

[Nico, 2010-07-06 16:33:37]

http://codereview.chromium.org/2868042/diff/15/12001
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/2868042/diff/15/12001#newcode443
chrome/browser/notifications/desktop_notification_service.cc:443: DCHECK_NE(-1,
allowed_sites->Remove(value)) << origin << " was not allowed";
On 2010/07/06 10:05:45, bulach wrote:
> I think DCHECK is not compiled in release mode (and in general I think we try
to
> avoid doing real work inside macros..)
> 
> perhaps:
> int removed = allowed_sites->Remove(value);
> DCHECK_NE(-1, removed) << origin << " was not allowed";

Oh, right. I usually know this :-P

Done.

http://codereview.chromium.org/2868042/diff/15/12001#newcode444
chrome/browser/notifications/desktop_notification_service.cc:444:
ScopedPrefUpdate updateAllowed(
On 2010/07/06 10:05:45, bulach wrote:
> s/updateAllowed/update_allowed/

Done.

http://codereview.chromium.org/2868042/diff/15/12001#newcode461
chrome/browser/notifications/desktop_notification_service.cc:461: DCHECK_NE(-1,
denied_sites->Remove(value)) << origin << " was not blocked";
On 2010/07/06 10:05:45, bulach wrote:
> as above

Done.

http://codereview.chromium.org/2868042/diff/15/12001#newcode462
chrome/browser/notifications/desktop_notification_service.cc:462:
ScopedPrefUpdate updateAllowed(
On 2010/07/06 10:05:45, bulach wrote:
> :)
> s/eAllowed/e_denied/
> 

Done.

http://codereview.chromium.org/2868042/diff/15/12003
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

http://codereview.chromium.org/2868042/diff/15/12003#newcode36
chrome/browser/notifications/desktop_notification_service_unittest.cc:36: //
Calling PauseIOThread() here isn't save, because the runnable method
On 2010/07/06 10:05:45, bulach wrote:
> s/save/safe/

Done.

http://codereview.chromium.org/2868042/diff/15/12003#newcode54
chrome/browser/notifications/desktop_notification_service_unittest.cc:54:
ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
On 2010/07/06 10:05:45, bulach wrote:
> seems a bit confusing PauseIOThread and PauseIOThreadIO.
> 
> perhaps:
> void PauseIOThread() {
>   if (!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
>     ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
>         &ThreadProxy::PauseIOThread);
>     return;
>   }
>   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
>   io_event_.Wait();
> }
> 
> ...if you want to keep the methods separated, then perhaps
RequestPauseIOThread
> (for the one that calls PostTask) and PauseIOThread (for the one that calls
> Wait) ?

Having just one function is too complicated for me :-)

RequestPauseIOThread() would change the name of the public function, which I
don't want to do.

PauseIOThreadOnIOThread() and CacheHasPermissionOnIOThread() are a mouthful, so
I think I'll keep it as is for now. ThreadProxy is a fairly small class, so
hopefully it's ok.

http://codereview.chromium.org/2868042/diff/15/12004
File chrome/browser/notifications/notification_exceptions_table_model.cc
(right):

http://codereview.chromium.org/2868042/diff/15/12004#newcode19
chrome/browser/notifications/notification_exceptions_table_model.cc:19:
std::vector<GURL> allowed = service_->GetAllowedOrigins();
On 2010/07/06 10:05:45, bulach wrote:
> perhaps pre-allocate entries_?
>  entries_.reserve(allowed.size() + blocked.sized());

Done.

http://codereview.chromium.org/2868042/diff/15/12004#newcode66
chrome/browser/notifications/notification_exceptions_table_model.cc:66: return
UTF8ToWide(GeolocationContentSettingsMap::OriginToString(
On 2010/07/06 10:05:45, bulach wrote:
> // TODO(bulach): factor out in a common function so that notification won't
> depend on Geolocation.

:-) Done

[bulach, 2010-07-06 16:47:39]

clarifying one comment below:

http://codereview.chromium.org/2868042/diff/15/12003
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

http://codereview.chromium.org/2868042/diff/15/12003#newcode54
chrome/browser/notifications/desktop_notification_service_unittest.cc:54:
ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
On 2010/07/06 16:33:37, Nico wrote:
> On 2010/07/06 10:05:45, bulach wrote:
> > seems a bit confusing PauseIOThread and PauseIOThreadIO.
> > 
> > perhaps:
> > void PauseIOThread() {
> >   if (!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
> >     ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
> >         &ThreadProxy::PauseIOThread);
> >     return;
> >   }
> >   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
> >   io_event_.Wait();
> > }
> > 
> > ...if you want to keep the methods separated, then perhaps
> RequestPauseIOThread
> > (for the one that calls PostTask) and PauseIOThread (for the one that calls
> > Wait) ?
> 
> Having just one function is too complicated for me :-)
> 
> RequestPauseIOThread() would change the name of the public function, which I
> don't want to do.
> 
> PauseIOThreadOnIOThread() and CacheHasPermissionOnIOThread() are a mouthful,
so
> I think I'll keep it as is for now. ThreadProxy is a fairly small class, so
> hopefully it's ok.

my concern is that PauseIOThread and its evil twin PauseIOThreadIO are doing
completely different things on different threads, and yet their names are very
close.. :) perhaps then just change the private method name?
something like PauseIOThreadUntilSignaled?

anyways, not a big deal, it's just a naming suggestion..

[Nico, 2010-07-06 16:50:58]

http://codereview.chromium.org/2868042/diff/15/12003
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

http://codereview.chromium.org/2868042/diff/15/12003#newcode54
chrome/browser/notifications/desktop_notification_service_unittest.cc:54:
ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
On 2010/07/06 16:47:39, bulach wrote:
> On 2010/07/06 16:33:37, Nico wrote:
> > On 2010/07/06 10:05:45, bulach wrote:
> > > seems a bit confusing PauseIOThread and PauseIOThreadIO.
> > > 
> > > perhaps:
> > > void PauseIOThread() {
> > >   if (!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
> > >     ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
> > >         &ThreadProxy::PauseIOThread);
> > >     return;
> > >   }
> > >   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
> > >   io_event_.Wait();
> > > }
> > > 
> > > ...if you want to keep the methods separated, then perhaps
> > RequestPauseIOThread
> > > (for the one that calls PostTask) and PauseIOThread (for the one that
calls
> > > Wait) ?
> > 
> > Having just one function is too complicated for me :-)
> > 
> > RequestPauseIOThread() would change the name of the public function, which I
> > don't want to do.
> > 
> > PauseIOThreadOnIOThread() and CacheHasPermissionOnIOThread() are a mouthful,
> so
> > I think I'll keep it as is for now. ThreadProxy is a fairly small class, so
> > hopefully it's ok.
> 
> my concern is that PauseIOThread and its evil twin PauseIOThreadIO are doing
> completely different things on different threads, and yet their names are very
> close.. :) perhaps then just change the private method name?
> something like PauseIOThreadUntilSignaled?
> 
> anyways, not a big deal, it's just a naming suggestion..

PauseIOThread() does the default thread's part of work necessary to pause the io
thread, while PauseIOThreadIO() does the io thread's part of the work :-)