Migrate Obsolete NotificationsSettings and remove content_settings::NotificationsProvider.

Migrate Obsolete NotificationsSettings and remove content_settings::NotificationsProvider.

BUG=63656
TEST=host_content_settings_map_unittest.cc,
     content_settings_pref_provider.cc,
     desktop_notifications_service_unittest.cc

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

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

[markusheintz_, 2011-08-22 08:45:52]

Dominic could you please review this CL for me?

Thanks.

[battre, 2011-08-22 15:00:57]

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:133:
pattern_pair, &settings);
nit: indentation

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:134:
DCHECK(found);
I thought this would cause a compiler error on the build bots because |found| is
never used if DCHECK is evaluated to nothing in release mode.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:389:
PrefService::SYNCABLE_PREF);
please update pref_names.* to indicate that these preferences are obsolete and
what they have been replaced with. I would even consider renaming the constant
names.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:940:
allowed_sites->Remove(*value, NULL);
here you leak |value|

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1277: //
contain the lattest settings.
nit: typo "latest"

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1328: prefs_,
prefs::kDesktopNotificationAllowedOrigins);
nit: indentation -2

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1352: int
setting_value;
nit: initialize

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1354:
kTypeNames[CONTENT_SETTINGS_TYPE_NOTIFICATIONS], &setting_value);
Return value is not checked. I propose to simplify this, by replacing the HasKey
call above with the following.

int setting_value = 0;
if (settings_dictionary->GetInteger(..., &setting_value)) {
  UpdateObsolete...
}

(no previous check whether the key exists)

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1363:
kTypeNames[CONTENT_SETTINGS_TYPE_GEOLOCATION])) {
same here

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:699:
ListValue* allowed_origin_list = new ListValue();
memory leak

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:705:
ListValue* denied_origin_list = new ListValue();
memory leak

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:748:
allowed_origin_list = new ListValue();
memory leak

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:791:
ContentSettingsPattern::Wildcard();
nit: single line?

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:798:
DictionaryValue* all_settings_dictionary = update.Get();
nit/opt: You can also do something like:
DictionaryPrefUpdate all_settings_dictionary(prefs, ...);
all_settings_dictionary->SetWithoutPathExpansion(...)

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

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:65: if (setting ==
tuple.c) {
Who allowed HostContentSettingsMap::PatternSettingSourceTuple to be implemented
as a Tuple4? brrrrrr....

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:350: if
(!settings_details->update_all_types() &&
should this be an || ?

[markusheintz_, 2011-08-24 00:47:12]

I still have one test that breaks.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:133:
pattern_pair, &settings);
On 2011/08/22 15:00:57, battre wrote:
> nit: indentation

Done.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:134:
DCHECK(found);
On 2011/08/22 15:00:57, battre wrote:
> I thought this would cause a compiler error on the build bots because |found|
is
> never used if DCHECK is evaluated to nothing in release mode.

You might be right. I removed the DCHECK since it is not really useful anyway.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:389:
PrefService::SYNCABLE_PREF);
On 2011/08/22 15:00:57, battre wrote:
> please update pref_names.* to indicate that these preferences are obsolete and
> what they have been replaced with. I would even consider renaming the constant
> names.

I marked the pref as obsolete in pref_names.h.

Why should I rename the constants?

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:940:
allowed_sites->Remove(*value, NULL);
On 2011/08/22 15:00:57, battre wrote:
> here you leak |value|

Changed value to a scoped_ptr<StringValue>

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1277: //
contain the lattest settings.
On 2011/08/22 15:00:57, battre wrote:
> nit: typo "latest"

Done.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1328: prefs_,
prefs::kDesktopNotificationAllowedOrigins);
On 2011/08/22 15:00:57, battre wrote:
> nit: indentation -2

Done.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1352: int
setting_value;
On 2011/08/22 15:00:57, battre wrote:
> nit: initialize

Done.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1354:
kTypeNames[CONTENT_SETTINGS_TYPE_NOTIFICATIONS], &setting_value);
On 2011/08/22 15:00:57, battre wrote:
> Return value is not checked. I propose to simplify this, by replacing the
HasKey
> call above with the following.
> 
> int setting_value = 0;
> if (settings_dictionary->GetInteger(..., &setting_value)) {
>   UpdateObsolete...
> }
> 
> (no previous check whether the key exists)

Done.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1363:
kTypeNames[CONTENT_SETTINGS_TYPE_GEOLOCATION])) {
On 2011/08/22 15:00:57, battre wrote:
> same here

Done.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:699:
ListValue* allowed_origin_list = new ListValue();
On 2011/08/22 15:00:57, battre wrote:
> memory leak

Done.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:705:
ListValue* denied_origin_list = new ListValue();
On 2011/08/22 15:00:57, battre wrote:
> memory leak

Done.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:748:
allowed_origin_list = new ListValue();
On 2011/08/22 15:00:57, battre wrote:
> memory leak

Done.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:791:
ContentSettingsPattern::Wildcard();
On 2011/08/22 15:00:57, battre wrote:
> nit: single line?

No 81 characters :(

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:798:
DictionaryValue* all_settings_dictionary = update.Get();
On 2011/08/22 15:00:57, battre wrote:
> nit/opt: You can also do something like:
> DictionaryPrefUpdate all_settings_dictionary(prefs, ...);
> all_settings_dictionary->SetWithoutPathExpansion(...)

Cool. Thanks for pointing this out to me :).

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

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:65: if (setting ==
tuple.c) {
On 2011/08/22 15:00:57, battre wrote:
> Who allowed HostContentSettingsMap::PatternSettingSourceTuple to be
implemented
> as a Tuple4? brrrrrr....

Yeah sorry I had to sell deposit my for that. But for sure I want my soul back
:-). I will fix that in the future.

http://codereview.chromium.org/7655019/diff/3001/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:350: if
(!settings_details->update_all_types() &&
On 2011/08/22 15:00:57, battre wrote:
> should this be an || ?
That's correct this way.

[markusheintz_, 2011-08-29 10:00:55]

I fixed all unittests now. So please continue reviewing my CL. Thanks

[battre, 2011-08-29 17:44:04]

ExtensionApiTest.Notifications still needs to be looked at, fails on Windows and
Linux (Mac is still in process).

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_extension_provider.cc
(right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_extension_provider.cc:38: 
nit: is this really better?

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:1286: const
ListValue* allow_sites =
nit: rename to allowed_origins (+ed, sites-->origins)

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:1303: const
ListValue* denied_sites =
dito

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:1331:
ListPrefUpdate update_allowed_sites(
dito

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:303: void
DesktopNotificationService::ResetAllowedOrigin(
This is identical to ResetBlockedOrigin. Should this be generlized to
"Reset(const ContentSettingsPattern& primary_pattern)"?

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:336: return
provided_setting;
nit: return directly without storing it first?

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:449: default:  //
Make gcc happy.
the correct approach would be:
switch (setting) {
  case CONTENT_SETTING_ALLOW:
    return WebKit::WebNotificationPresenter::PermissionAllowed;
  case CONTENT_SETTING_BLOCK:
    return WebKit::WebNotificationPresenter::PermissionDenied;
  case CONTENT_SETTING_DEFAULT:
  case CONTENT_SETTING_ASK:
  case CONTENT_SETTING_SESSION_ONLY:
  case CONTENT_SETTING_NUM_SETTINGS:
    return WebKit::WebNotificationPresenter::PermissionNotAllowed;
  }
}

This way the compiler is happy that all cases are covered and it will complain
if a new enum value is introduced.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service.h (right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.h:108: void
GetNotificationsSettings(
nit: Method description missing and unclear to me.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_unittest.cc:20: //
NotificationsPrefsCache wants to be called on the IO thread. This class
you have deleted the NotificationPrefsCache. Please update the comment. Is this
class still needed?

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_unittest.cc:40: int
ServiceHasPermission(DesktopNotificationService* service,
Nit: Function description.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_unittest.cc:41: const
GURL& url) {
nit: indentation

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_unittest.cc:88: int
permission_;
this seems like voodoo to me. What is 5 permission? ;-)

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:174:
pattern.ToString());
nit: single line

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:522:
GetNotificationExceptionForPage(tuple.a, tuple.c));
please add a TODO, to HostContentSettingsMap::PatternSettingsSourceTuple to get
rid of .a, .b, .c, etc. The removal can be done in a separate CL.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:653: if
(content_setting == CONTENT_SETTING_ALLOW) {
this can be simplified if ResetAllowedOrigin and ResetBlockedOrigin are unified
to.

DCHECK(content_setting == CONTENT_SETTING_ALLOW || content_setting ==
CONTENT_SETTING);
DesktopNotificationServiceFactory::GetForProfile(profile)->Reset(ContentSettingsPattern::FromString(origin));

[markusheintz_, 2011-08-30 15:14:02]

ExtensionApiTest.Notifications passes green on my local machine. Try bots are
running.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_extension_provider.cc
(right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_extension_provider.cc:38: 
On 2011/08/29 17:44:04, battre wrote:
> nit: is this really better?

Ahhhh no not at all. I just did this to LOG the setting for debbung purposes.
Then I happiely removed the LOG(ERROR) without changing this back. *grrrr* to
myself.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:1286: const
ListValue* allow_sites =
On 2011/08/29 17:44:04, battre wrote:
> nit: rename to allowed_origins (+ed, sites-->origins)

Done.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:1303: const
ListValue* denied_sites =
On 2011/08/29 17:44:04, battre wrote:
> dito

Done.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:1331:
ListPrefUpdate update_allowed_sites(
On 2011/08/29 17:44:04, battre wrote:
> dito

Done.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:303: void
DesktopNotificationService::ResetAllowedOrigin(
On 2011/08/29 17:44:04, battre wrote:
> This is identical to ResetBlockedOrigin. Should this be generlized to
> "Reset(const ContentSettingsPattern& primary_pattern)"?

Added a ClearSettings method to replace the Reset*Origin methods.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:336: return
provided_setting;
On 2011/08/29 17:44:04, battre wrote:
> nit: return directly without storing it first?

Done.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:449: default:  //
Make gcc happy.
On 2011/08/29 17:44:04, battre wrote:
> the correct approach would be:
> switch (setting) {
>   case CONTENT_SETTING_ALLOW:
>     return WebKit::WebNotificationPresenter::PermissionAllowed;
>   case CONTENT_SETTING_BLOCK:
>     return WebKit::WebNotificationPresenter::PermissionDenied;
>   case CONTENT_SETTING_DEFAULT:
>   case CONTENT_SETTING_ASK:
>   case CONTENT_SETTING_SESSION_ONLY:
>   case CONTENT_SETTING_NUM_SETTINGS:
>     return WebKit::WebNotificationPresenter::PermissionNotAllowed;
>   }
> }
> 
> This way the compiler is happy that all cases are covered and it will complain
> if a new enum value is introduced.

Ahhh ... no. Don't like because everytime a new content type is added we need to
touch this switch.

I guess I could change it to a if - else if - else since we have only three
cases anyway (incl. the default)

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service.h (right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.h:108: void
GetNotificationsSettings(
On 2011/08/29 17:44:04, battre wrote:
> nit: Method description missing and unclear to me.

Done.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_unittest.cc:20: //
NotificationsPrefsCache wants to be called on the IO thread. This class
On 2011/08/29 17:44:04, battre wrote:
> you have deleted the NotificationPrefsCache. Please update the comment. Is
this
> class still needed?

Done.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_unittest.cc:40: int
ServiceHasPermission(DesktopNotificationService* service,
On 2011/08/29 17:44:04, battre wrote:
> Nit: Function description.

Done.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_unittest.cc:41: const
GURL& url) {
On 2011/08/29 17:44:04, battre wrote:
> nit: indentation

Done.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_unittest.cc:88: int
permission_;
On 2011/08/29 17:44:04, battre wrote:
> this seems like voodoo to me. What is 5 permission? ;-)

I was tempted to pull the "legacy code" joker, but you are absolutely right.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:174:
pattern.ToString());
On 2011/08/29 17:44:04, battre wrote:
> nit: single line

Done.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:522:
GetNotificationExceptionForPage(tuple.a, tuple.c));
On 2011/08/29 17:44:04, battre wrote:
> please add a TODO, to HostContentSettingsMap::PatternSettingsSourceTuple to
get
> rid of .a, .b, .c, etc. The removal can be done in a separate CL.

Added TODO in host_content_settings_map.h where this tuple is declared.

http://codereview.chromium.org/7655019/diff/20001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:653: if
(content_setting == CONTENT_SETTING_ALLOW) {
On 2011/08/29 17:44:04, battre wrote:
> this can be simplified if ResetAllowedOrigin and ResetBlockedOrigin are
unified
> to.
> 
> DCHECK(content_setting == CONTENT_SETTING_ALLOW || content_setting ==
> CONTENT_SETTING);
>
DesktopNotificationServiceFactory::GetForProfile(profile)->Reset(ContentSettingsPattern::FromString(origin));

You are right. I was afraid that this CL might grow too much so I wanted to
defer this to the next CL. But I better change it now

[battre, 2011-08-30 15:57:21]

LGTM

http://codereview.chromium.org/7655019/diff/26001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7655019/diff/26001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:46: // I really want
my soul back, so I really will change this ASAP.
:-)

http://codereview.chromium.org/7655019/diff/26001/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

http://codereview.chromium.org/7655019/diff/26001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_unittest.cc:20: // the
HasPermission method of the DesktopNotificationService wants to be called
Nit. Capital T in "the"

[markusheintz_, 2011-08-30 17:04:16]

Thanks a lot for reviewing.

I fixed the last typo and will wait for green trybots before submitting.

http://codereview.chromium.org/7655019/diff/26001/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

http://codereview.chromium.org/7655019/diff/26001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service_unittest.cc:20: // the
HasPermission method of the DesktopNotificationService wants to be called
On 2011/08/30 15:57:21, battre wrote:
> Nit. Capital T in "the"

Done.

[markusheintz_, 2011-08-31 10:12:45]

@bauerb: Dominic is in a training today, maybe you could review patchset 13.
This broke one of the chromiumos builder s. Thanks a lot. 

@battre, @bauerb: It appears that there is a second unit_tests for desktop
notifications that I was not aware of in:
chrome/browser/chromeos/notifications/desktop_notifications_unittest.h

I had to remove any dependencies to the deleted NotificationsPrefCache in there.
I will have a closer look at that unit_test since it appears to have some unused
code. But I do not like to include these change in this CL.

[Bernhard Bauer, 2011-08-31 10:22:50]

Which patch set was landed originally?

[markusheintz_, 2011-08-31 10:30:46]

Originally patchset 12 was landed and reverted :(

[Bernhard Bauer, 2011-08-31 10:35:17]

desktop_notifications_unittest.h LGTM.

[markusheintz_, 2011-08-31 10:39:56]

On 2011/08/31 10:35:17, Bernhard Bauer wrote:
> desktop_notifications_unittest.h LGTM.

Thanks a lot for reviewing. I will try to land the CL after the trybots pass
green again.