Add notifications to allow desktop notification permissions to be synced.

Add notifications to allow desktop notification permissions to be synced.
Fix a bug where sites can be added to allow and deny lists multiple times.

BUG=none
TEST=Open two browsers with separate profiles.  Request permission for notifications on a site on one.  Check that the permissions are the same on the other browser.

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

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

[John Gregg, 2010-04-09 00:14:48]

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

http://codereview.chromium.org/1578023/diff/2001/3001#newcode305
chrome/browser/notifications/desktop_notification_service.cc:305: }
This makes things very complicated; lots of ways to set the preferences.  Let's
simplify it by making a helper function that converts a ListValue* until a
std::vector<GURL>?

Then InitPrefs, instead of passing the ListValue*s in the constructor, can use
the helper methods and your new SetCache[Allowed|Denied] methods to set up the
prefs cache.

http://codereview.chromium.org/1578023/diff/2001/3001#newcode382
chrome/browser/notifications/desktop_notification_service.cc:382:
StartObserving();
Why is it insufficient to put StopObserving()/StartObserving() around the code
that's already there?  

Okay, I see you're trying to remove duplicates?  If you used a bool
InsertIfNotPresent(ListValue*, StringValue*) method this would be easier to
read.  It should really exist in a utility somewhere, kind of surprised it
doesn't.

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

http://codereview.chromium.org/1578023/diff/2001/3004#newcode37
chrome/browser/notifications/notifications_prefs_cache.h:37: void
SetCacheAllowedOrigins(const std::vector<GURL>& allowed);
Add a comment please that these reset the lists.

[Zachary Kuznia, 2010-04-09 18:27:56]

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

http://codereview.chromium.org/1578023/diff/2001/3001#newcode305
chrome/browser/notifications/desktop_notification_service.cc:305: }
On 2010/04/09 00:14:48, John Gregg wrote:
> This makes things very complicated; lots of ways to set the preferences. 
Let's
> simplify it by making a helper function that converts a ListValue* until a
> std::vector<GURL>?
> 
> Then InitPrefs, instead of passing the ListValue*s in the constructor, can use
> the helper methods and your new SetCache[Allowed|Denied] methods to set up the
> prefs cache.

Done.

http://codereview.chromium.org/1578023/diff/2001/3001#newcode382
chrome/browser/notifications/desktop_notification_service.cc:382:
StartObserving();
On 2010/04/09 00:14:48, John Gregg wrote:
> Why is it insufficient to put StopObserving()/StartObserving() around the code
> that's already there?  

The scoping was there for the scoped notifiers.  Since I moved them, it's not
needed anymore.  Removed.

> 
> Okay, I see you're trying to remove duplicates?  If you used a bool
> InsertIfNotPresent(ListValue*, StringValue*) method this would be easier to
> read.  It should really exist in a utility somewhere, kind of surprised it
> doesn't.

Done and done.

[John Gregg, 2010-04-09 18:36:00]

chrome style is not to put { } around one-liners, but besides that, LGTM!

http://codereview.chromium.org/1578023/diff/7001/8001
File base/values.cc (right):

http://codereview.chromium.org/1578023/diff/7001/8001#newcode838
base/values.cc:838: if ((*i)->Equals(in_value)) {
lose the { } on single-line if statement

http://codereview.chromium.org/1578023/diff/7001/8003
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/1578023/diff/7001/8003#newcode331
chrome/browser/notifications/desktop_notification_service.cc:331: }
nit: no { } on all these

[tony, 2010-04-12 08:46:03]

drive by review

http://codereview.chromium.org/1578023/diff/15001/16003
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/1578023/diff/15001/16003#newcode335
chrome/browser/notifications/desktop_notification_service.cc:335: if
(allowed_sites->AppendIfNotPresent(value))
It looks like you leak |value| if it's not appended.