Turn on and use the AppInfo data.

Turn on and use the AppInfo data.

Synced Notifications uses the SyncedNotificationAppInfo struct to name
the sending services and supply icons for them.  The first two checkins
in this larger change established the datatype and the sync change
processor for it.  This checkin wires up the data type to the rest of the
Synced Notifications code.

This checkin now uses the SNAI class to provide the bitmap and service name,
and removes the hardcoding to our first service name provider and bitmap.
This checkin also adds code to fetch the bitmaps provided by the SNAI
protobuf.

New unit tests have been added to cover the new functionality.

A lifetime dependency has been set up to ensure that the
SyncedNotificationAppInfoService is always present whenever we have a
ChromeNotifierService.

BUG=280266
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260433

[dewittj, 2014-03-11 01:34:07]

missing AppInfo factory files.

[Pete Williamson, 2014-03-11 16:51:47]

On 2014/03/11 01:34:07, dewittj wrote:
> missing AppInfo factory files.

The AppInfo factory was already checked in with my previous changelist, and did
not need to change with this changelist.

[dewittj, 2014-03-17 21:43:43]

Here are some preliminary comments.  Additionally, all classes need class-level
comments, and all public class methods need method comments.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(left):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:152: //
underlying problem causing bad data.
What was the underlying problem?

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:73: if
(NULL == synced_notification_app_info_service_) {
I think a DCHECK is appropriate here.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:362: //
TODO(mukai|petewil): Check the profile's eligibility before adding the
Does this TODO still apply?

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:385: iter
= find(enabled_sending_services_.begin(),
n^2 search in this array.  Probably do better with YASD (yet another set
difference)

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:500:
return std::string();
Is this ever expected to happen?  Please document the cases.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:770:
notifier::SyncedNotificationAppInfo* app_info =
Does the comment still need addressing?  Is there ever a case when it's possible
for app info not to exist for a notification?

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:824:
message_center::NotifierId notifier_id = message_center::NotifierId(
Probably should make a SyncedNotificationAppInfo::GetNotifierId() method.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:840:
base::UTF8ToUTF16(app_info->settings_display_name()),
Has the encoding of this string been set before hand?  Do we have any way to
internationalize this?

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.h:105: void
SetAddedAppIds(std::vector<std::string> added_app_ids);
nit: rename to OnAppIdsAdded

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.h:106: void
SetRemovedAppIds(std::vector<std::string> removed_app_ids);
nit: rename to OnAppIdsRemoved

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification.h (right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.h:102: void
ShowIfNewlyEnabled(NotificationUIManager* notification_manager,
Can you explain why you sometimes use the stored NotificationUIManager pointer
and sometimes use a function argument?  The inconsistency is really confusing,
especially since it's basically a singleton anyway.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.h:103:
ChromeNotifierService* notifier_service,
Same.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.h:108: void
HideIfNewlyRemoved(NotificationUIManager* notification_manager,
Wouldn't this be better named HideAllForAppID?

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.h:109:
ChromeNotifierService* notifier_service,
Argument unused, please remove.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.h:110: Profile*
profile,
Same.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:98:
void SyncedNotificationAppInfo::AddBitmapToFetchQueue(const GURL& url) {
This is improperly named since there is not a queue

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.h
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:50:
void SetAddedAppIds(std::vector<std::string> added_app_ids) {
I don't like this added_app_ids_ and removed_app_ids_.  It is completely unclear
what it's used for.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:66:
void GetAppIdList(std::vector<std::string>* app_id_list);
Can this just return the vector?  It's more readable and I think that STL
containers are ok to return wholesale.

It would also match the above functions.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:91:
GURL settings_low_dpi_icon_url_;
Let's make these into objects, something like:

scoped_ptr<SyncedNotificationResource> settings_icon_

and a class ResourceRep which takes a scale factor and a URL.  The constructor
for SyncedNotificationReosurce would take a vector of ResourceReps (a lot like
the ImageSkiaRep stuff).

Then each resource could have bitmap fetchers, a ready bit, and a gfx::image
that is constructed over time.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc:23:
bool SyncedNotificationAppInfoService::avoid_bitmap_fetching_for_test_ = false;
maybe a better way to do this is to have a bitmapfetcherfactory* passed into the
constructor, and then a fakebitmapfetcherfactory used in tests.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc:180:
// uses assignment to array elements, not push_back.
Consider using std::back_inserter here, like this:

set_difference(
    old_app_ids.begin(), old_app_ids.end(),
    new_app_ids.begin(), new_app_ids.end(),
    std::back_inserter(removed_app_ids));

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h:72:
std::vector<std::string> removed_app_ids);
This method signature is really confusing to me.  What do removed app IDs have
to do with the fetches being done?

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h:93:
void SetChromeNotifierService(ChromeNotifierService* notifier) {
nit: set_chrome_notifier_service

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h:139:
ChromeNotifierService* chrome_notifier_service_;
possibly a weak pointer is appropriate here?

[dewittj, 2014-03-20 18:00:28]

here are my comments for the SyncedNotificationAppInfo* classes:

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:367:
synced_notification_app_info_service_->GetAllSendingServiceNames();
We should just GetAllSendingServices, not the Names..  Currently you're doing an
n^2 loop over the SyncedNotificationAppInfos, when you can just do a single
loop.  This also has the benefit of making GetAllSendingServiceNames unnecessary
and FindSyncedNotificationAppInfoServiceByName a private method.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:375:
sending_service_names[ii]);
This should call SyncedNotificationAppInfo::GetNotifierId()

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:839:
app_info->GetNotifierId()));
this should probably just be |notifier_id|

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc
(right):

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:105:
if (url.is_valid()) {
hm, possibly push this check above the loop to avoid it in the worst case?

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:108:
}
Should there be some sort of handler for bad URL?  All the other checks look for
empty, but this looks for valid... so it seems like if there is an invalid but
non-empty URL the app info will never "finish".

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:129:
// Notification Center.
Let's actually do that here.  CreateFrom1xBitmap is a really simple function:

Image(ImageSkia(ImageSkiaRep(bitmap, 1.0f)));

so something like

ImageSkia settings_icon_image_skia;
settings_icon_image_skia.AddRep(ImageSkiaRep(bitmap, 1.0f));
settings_icon_image_skia.AddRep(ImageSkiaRep(bitmap, 2.0f));
settings_icon_ = gfx::Image(settings_icon_image_skia);

should do the trick.  Now that I think of it, probably the AppInfo class should
hold an ImageSkia, not an Image, so that it's easier to manipulate the
representations.  Then it can construct an image from the ImageSkia on demand.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.h
(right):

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:61: //
ids so we can later remove any affected notfications.
Shouldn't remove be instant?  We don't have to wait for any downloads or
anything... best to not hold this data for longer than we need, right?

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:103:
gfx::Image settings_low_dpi_icon_;
There is no need for settings_low_dpi_icon_ and settings_high_dpi_icon_. 
gfx::Image and gfx::ImageSkia can store either or both representations in its
underlying ImageSkia.

Same for monochrome_*_icon_ and welcome_*_icon_

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc
(right):

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc:146:
void SyncedNotificationAppInfoService::ProcessIncomingAppInfoProtobuf(
I think this code is resilient to the race conditions I've thought of so far but
it would be nice to comment how that is achieved here.  Something about how
duplicate URL fetchers are deduped, and that if multiple appinfoprotobufs come
in then no callbacks to the chromenotifierservice will happen until all the
unique urls have finished downloading.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc:180:
std::sort(old_app_ids.begin(), old_app_ids.end());
is it better to maintain a sorted order within SyncedNotificationAppInfo?

[Pete Williamson, 2014-03-21 01:22:30]

CR fixes as suggested by DewittJ from his first round.  (I haven't looked at the
second round yet).

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:73: if
(NULL == synced_notification_app_info_service_) {
On 2014/03/17 21:43:43, dewittj wrote:
> I think a DCHECK is appropriate here.

Done.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:362: //
TODO(mukai|petewil): Check the profile's eligibility before adding the
On 2014/03/17 21:43:43, dewittj wrote:
> Does this TODO still apply?

Yes.  We have a bug tracking this.  For instance, if the user is not signed up
for G+, we wouldn't show G+ as an option.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:385: iter
= find(enabled_sending_services_.begin(),
On 2014/03/17 21:43:43, dewittj wrote:
> n^2 search in this array.  Probably do better with YASD (yet another set
> difference)

Today the set is small, and it should be for the forseeable future.  I added a
TODO to change the code if the set of sending services becomes large or we
complete third party support.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:500:
return std::string();
On 2014/03/17 21:43:43, dewittj wrote:
> Is this ever expected to happen?  Please document the cases.

It should never be able to happen.  I replaced the check with a DCHECK.  It was
left over from an earlier version of the code and not removed.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:770:
notifier::SyncedNotificationAppInfo* app_info =
On 2014/03/17 21:43:43, dewittj wrote:
> Does the comment still need addressing?  Is there ever a case when it's
possible
> for app info not to exist for a notification?

Changed the comment to clarify that we have addressed the issue.  (although to
be fair, there is a bug here, and the bug fix is in my next changelist after
this one).

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:824:
message_center::NotifierId notifier_id = message_center::NotifierId(
On 2014/03/17 21:43:43, dewittj wrote:
> Probably should make a SyncedNotificationAppInfo::GetNotifierId() method.

Done.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:840:
base::UTF8ToUTF16(app_info->settings_display_name()),
On 2014/03/17 21:43:43, dewittj wrote:
> Has the encoding of this string been set before hand?  Do we have any way to
> internationalize this?

I'm not sure I understand the first question.  The string comes to us in UTF-8,
already localized for this user, so no further internationalization should be
needed.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification.h (right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.h:102: void
ShowIfNewlyEnabled(NotificationUIManager* notification_manager,
On 2014/03/17 21:43:43, dewittj wrote:
> Can you explain why you sometimes use the stored NotificationUIManager pointer
> and sometimes use a function argument?  The inconsistency is really confusing,
> especially since it's basically a singleton anyway.

It was to make the code easier to test by putting the dependency on the method
instead of the object.  I added a public setter as the normal way to set this.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.h:103:
ChromeNotifierService* notifier_service,
On 2014/03/17 21:43:43, dewittj wrote:
> Same.

Done.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.h:108: void
HideIfNewlyRemoved(NotificationUIManager* notification_manager,
On 2014/03/17 21:43:43, dewittj wrote:
> Wouldn't this be better named HideAllForAppID?

Done (also renamed ShowIfNewlyEnabled -> ShowAllForAppId.

Thanks! I was having a hard time finding a good name, the one you suggested is
clearly better.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.h:109:
ChromeNotifierService* notifier_service,
On 2014/03/17 21:43:43, dewittj wrote:
> Argument unused, please remove.

Done.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.h:110: Profile*
profile,
On 2014/03/17 21:43:43, dewittj wrote:
> Same.

Done.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:98:
void SyncedNotificationAppInfo::AddBitmapToFetchQueue(const GURL& url) {
On 2014/03/17 21:43:43, dewittj wrote:
> This is improperly named since there is not a queue

Renamed to CreateBitmapFetcher everywhere.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.h
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:50:
void SetAddedAppIds(std::vector<std::string> added_app_ids) {
On 2014/03/17 21:43:43, dewittj wrote:
> I don't like this added_app_ids_ and removed_app_ids_.  It is completely
unclear
> what it's used for.

Are you objecting to the name, the existence of the variable, or the lack of
documentation?  I'm guessing the latter so I added more comments.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:66:
void GetAppIdList(std::vector<std::string>* app_id_list);
On 2014/03/17 21:43:43, dewittj wrote:
> Can this just return the vector?  It's more readable and I think that STL
> containers are ok to return wholesale.
> 
> It would also match the above functions.

Done.  (I was trying to avoid copying the STL struct, perhaps my efficiency
worries were being penny-wise).

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:91:
GURL settings_low_dpi_icon_url_;
On 2014/03/17 21:43:43, dewittj wrote:
> Let's make these into objects, something like:
> 
> scoped_ptr<SyncedNotificationResource> settings_icon_
> 
> and a class ResourceRep which takes a scale factor and a URL.  The constructor
> for SyncedNotificationReosurce would take a vector of ResourceReps (a lot like
> the ImageSkiaRep stuff).
> 
> Then each resource could have bitmap fetchers, a ready bit, and a gfx::image
> that is constructed over time.

Done.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc:23:
bool SyncedNotificationAppInfoService::avoid_bitmap_fetching_for_test_ = false;
On 2014/03/17 21:43:43, dewittj wrote:
> maybe a better way to do this is to have a bitmapfetcherfactory* passed into
the
> constructor, and then a fakebitmapfetcherfactory used in tests.

In my opinion the simplicity of having a flag is better than the complexity
introduced by dependency injection in this case, but if you feel strongly I
could definitely change this.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc:180:
// uses assignment to array elements, not push_back.
On 2014/03/17 21:43:43, dewittj wrote:
> Consider using std::back_inserter here, like this:
> 
> set_difference(
>     old_app_ids.begin(), old_app_ids.end(),
>     new_app_ids.begin(), new_app_ids.end(),
>     std::back_inserter(removed_app_ids));

Done.  Good catch, I'd forgotten about back_inserter!

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h:72:
std::vector<std::string> removed_app_ids);
On 2014/03/17 21:43:43, dewittj wrote:
> This method signature is really confusing to me.  What do removed app IDs have
> to do with the fetches being done?

Nothing, other than we need to wait until the bitmap fetches are done before
telling chrome what new app ids have been added.  Do you have a preferred
approach?

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h:93:
void SetChromeNotifierService(ChromeNotifierService* notifier) {
On 2014/03/17 21:43:43, dewittj wrote:
> nit: set_chrome_notifier_service

Done.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h:139:
ChromeNotifierService* chrome_notifier_service_;
On 2014/03/17 21:43:43, dewittj wrote:
> possibly a weak pointer is appropriate here?

We could do that, but I'm not sure the extra complexity buys us much.  Do you
have a strong preference?  The chrome notifier service nulls this out at exit,
so it should always be correct.

[dewittj, 2014-03-24 17:35:50]

I like the imageholder thanks

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
(right):

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.h:37: //
TODO(petewil): Remove this once we figure out how to do UMA per name.
per name of what?

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
(right):

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:196:
// Set this icon GURL.
fix comment plural

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/image_holder.h (right):

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/image_holder.h:31: ImageHolder(const
GURL& low_dpi_url,
I would prefer something similar to ImageSkia where you specify GURL + float
DPI, rather than just high/low.  Probably a constructor like:

class URLWithScale {
 public:
  URLWithScale(GURL url, float scale);
}

ImageHolder(Profile* profile, ImageHolderDelegate* delegate,
std::vector<URLWithScale> urls)

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc
(right):

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc:247:
app_info->SetSettingsURLs(GURL(icon_url), GURL());
please comment why blank GURL is used here (protobuf being upgraded)

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service_unittest.cc
(right):

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service_unittest.cc:114:
test_item1->SetSettingsURLs(GURL(kTestIconUrl), GURL());
probably want to test with non-empty highres urls

[Pete Williamson, 2014-03-25 00:08:37]

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:367:
synced_notification_app_info_service_->GetAllSendingServiceNames();
On 2014/03/20 18:00:28, dewittj wrote:
> We should just GetAllSendingServices, not the Names..  Currently you're doing
an
> n^2 loop over the SyncedNotificationAppInfos, when you can just do a single
> loop.  This also has the benefit of making GetAllSendingServiceNames
unnecessary
> and FindSyncedNotificationAppInfoServiceByName a private method.

My intent here is to keep the services decoupled, so that we don't hold pointers
to app_info objects.

Instead of doing as you suggest, to keep the services decoupled, I got all the
data that we might want, and passed a copy of it into chrome_notifier_service
(the new SyncedNotificationSendingServiceSettingsData class is what we use for
this).

This has the happy effect of fixing the n^2 algorithm.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:375:
sending_service_names[ii]);
On 2014/03/20 18:00:28, dewittj wrote:
> This should call SyncedNotificationAppInfo::GetNotifierId()

Done.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:839:
app_info->GetNotifierId()));
On 2014/03/20 18:00:28, dewittj wrote:
> this should probably just be |notifier_id|

Done.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc
(right):

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:105:
if (url.is_valid()) {
On 2014/03/20 18:00:28, dewittj wrote:
> hm, possibly push this check above the loop to avoid it in the worst case?

Instead, I pushed up the check into the calling function.  (Note that the code
moved into image_holder.cc now)

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:108:
}
On 2014/03/20 18:00:28, dewittj wrote:
> Should there be some sort of handler for bad URL?  All the other checks look
for
> empty, but this looks for valid... so it seems like if there is an invalid but
> non-empty URL the app info will never "finish".

Fixed by pushing up the check into the caller.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:108:
}
On 2014/03/20 18:00:28, dewittj wrote:
> Should there be some sort of handler for bad URL?  All the other checks look
for
> empty, but this looks for valid... so it seems like if there is an invalid but
> non-empty URL the app info will never "finish".

handler added in ImageHandler::ImageHandler()

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:129:
// Notification Center.
On 2014/03/20 18:00:28, dewittj wrote:
> Let's actually do that here.  CreateFrom1xBitmap is a really simple function:
> 
> Image(ImageSkia(ImageSkiaRep(bitmap, 1.0f)));
> 
> so something like
> 
> ImageSkia settings_icon_image_skia;
> settings_icon_image_skia.AddRep(ImageSkiaRep(bitmap, 1.0f));
> settings_icon_image_skia.AddRep(ImageSkiaRep(bitmap, 2.0f));
> settings_icon_ = gfx::Image(settings_icon_image_skia);
> 
> should do the trick.  Now that I think of it, probably the AppInfo class
should
> hold an ImageSkia, not an Image, so that it's easier to manipulate the
> representations.  Then it can construct an image from the ImageSkia on demand.
> 

Done.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.h
(right):

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:61: //
ids so we can later remove any affected notfications.
On 2014/03/20 18:00:28, dewittj wrote:
> Shouldn't remove be instant?  We don't have to wait for any downloads or
> anything... best to not hold this data for longer than we need, right?

I chose to wait until the bitmap fetch is done for both the removal and adds, so
they both take effect at the same time.  Let me know if you prefer to do the
removal instantly.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:103:
gfx::Image settings_low_dpi_icon_;
On 2014/03/20 18:00:28, dewittj wrote:
> There is no need for settings_low_dpi_icon_ and settings_high_dpi_icon_. 
> gfx::Image and gfx::ImageSkia can store either or both representations in its
> underlying ImageSkia.
> 
> Same for monochrome_*_icon_ and welcome_*_icon_

already fixed by my most recent patch.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc
(right):

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc:146:
void SyncedNotificationAppInfoService::ProcessIncomingAppInfoProtobuf(
On 2014/03/20 18:00:28, dewittj wrote:
> I think this code is resilient to the race conditions I've thought of so far
but
> it would be nice to comment how that is achieved here.  Something about how
> duplicate URL fetchers are deduped, and that if multiple appinfoprotobufs come
> in then no callbacks to the chromenotifierservice will happen until all the
> unique urls have finished downloading.

Done.

https://codereview.chromium.org/193773003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc:180:
std::sort(old_app_ids.begin(), old_app_ids.end());
On 2014/03/20 18:00:28, dewittj wrote:
> is it better to maintain a sorted order within SyncedNotificationAppInfo?

Adding a new app info is incredibly rare, as is removing one.  I prefer the code
to be less complicated, and spend a bit more time in this rare case.

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
(right):

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.h:37: //
TODO(petewil): Remove this once we figure out how to do UMA per name.
On 2014/03/24 17:35:50, dewittj wrote:
> per name of what?

Comment improved.

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
(right):

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:196:
// Set this icon GURL.
On 2014/03/24 17:35:50, dewittj wrote:
> fix comment plural

Done.

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/image_holder.h (right):

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/image_holder.h:31: ImageHolder(const
GURL& low_dpi_url,
On 2014/03/24 17:35:50, dewittj wrote:
> I would prefer something similar to ImageSkia where you specify GURL + float
> DPI, rather than just high/low.  Probably a constructor like:
> 
> class URLWithScale {
>  public:
>   URLWithScale(GURL url, float scale);
> }
> 
> ImageHolder(Profile* profile, ImageHolderDelegate* delegate,
> std::vector<URLWithScale> urls)

I'd rather that the ImageHolder was able to hold both bitmaps.  See if you like
the current refactoring that I did to use an ImageSkia.

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc
(right):

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.cc:247:
app_info->SetSettingsURLs(GURL(icon_url), GURL());
On 2014/03/24 17:35:50, dewittj wrote:
> please comment why blank GURL is used here (protobuf being upgraded)

Done.

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service_unittest.cc
(right):

https://codereview.chromium.org/193773003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service_unittest.cc:114:
test_item1->SetSettingsURLs(GURL(kTestIconUrl), GURL());
On 2014/03/24 17:35:50, dewittj wrote:
> probably want to test with non-empty highres urls

We do test that in ImageHolderTest.IsFetchingDoneTest.

[dewittj, 2014-03-25 22:31:13]

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(left):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:152: //
underlying problem causing bad data.
On 2014/03/17 21:43:43, dewittj wrote:
> What was the underlying problem?

Do you have an answer for this comment?

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:840:
base::UTF8ToUTF16(app_info->settings_display_name()),
On 2014/03/21 01:22:31, Pete Williamson wrote:
> On 2014/03/17 21:43:43, dewittj wrote:
> > Has the encoding of this string been set before hand?  Do we have any way to
> > internationalize this?
> 
> I'm not sure I understand the first question.  The string comes to us in
UTF-8,
> already localized for this user, so no further internationalization should be
> needed.

I was just wondering if the expected encoding is explicitly defined in the
protobuf or other spec.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.h:105: void
SetAddedAppIds(std::vector<std::string> added_app_ids);
On 2014/03/17 21:43:43, dewittj wrote:
> nit: rename to OnAppIdsAdded

This comment was not addressed.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.h:106: void
SetRemovedAppIds(std::vector<std::string> removed_app_ids);
On 2014/03/17 21:43:43, dewittj wrote:
> nit: rename to OnAppIdsRemoved

This comment was not addressed.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:69:
profile_, Profile::EXPLICIT_ACCESS);
Based on the comments in Profile, I would expect you actually want
IMPLICIT_ACCESS.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:412:
(*notification_iter)->set_notifier_service(this);
The way you changed the call to ShowAllForAppId doesn't really address what I
was trying to get at, which could be rephrased as: Why change this service and
the notification manager at all during the lifetime of the object?  They both
outlive synced notifications and are required for correct functioning.  So
unless there is some other reason to set it here, I would greatly prefer adding
a ChromeNotifierService* and NotificationUIManager* to the constructor of
SyncedNotification, and reserving set_notifier_service and
set_notification_manager for testing purposes.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:563:
kSendingService1Name);
nit: no newline

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:572:
notifier.OnSyncedNotificationServiceEnabled(kSendingService1Name,
same

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:582:
notifier.OnSyncedNotificationServiceEnabled(kSendingService1Name,
same

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:733:
// have a valid app info for the notification.
Did you plan to write this test in the next CL?

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/image_holder.cc (right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.cc:10: // #include
"ui/gfx/image/image_skia_rep.h"
nit: fix this include.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.cc:47: return (
(low_dpi_url_.is_empty() || low_dpi_fetched_) &&
nit: no whitespace between ( ( and ) )

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.cc:76: const SkBitmap*
bitmap) {
nit:no newline

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.cc:77: // TODO(petewil):
Should I retry if a fetch fails?
Please open a bug for this, maybe even mark launch1

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/image_holder.h (right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.h:6: // and has code to
fetch the bitmap from the URL.
I think that this comment and the one on line 28 should be merged.  No comments
typically appear before the #include section.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.h:50: void
set_low_dpi_url(const GURL& low_dpi_url) {
nit: unused, remove.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.h:54: void
set_high_dpi_url(const GURL& high_dpi_url) {
nit: unused, remove.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.h:58: gfx::Image
high_dpi_image() { return gfx::Image(image_); }
nit: remove high_dpi_image() since it's not used, and rename low_dpi_image() to
image()

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:27:
void SyncedNotificationAppInfo::SetSettingsURLs(
nit: declaration order and definition order should be the same.  Please
double-check all other h/cc pairs for this property as well.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.h
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:45:
void SetSettingsURLs(const GURL& settings_low_dpi,
nit: need comment on all public methods.  Ideally it'd explain in what places in
the UI the different URLs will be used.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:59:
void SetAddedAppIds(std::vector<std::string> added_app_ids) {
nit: put all inline methods beneath all non-inline methods.
nit: set_added_app_ids if you want to keep this inlined.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:67:
void SetRemovedAppIds(std::vector<std::string> removed_app_ids) {
nit: set_removed_app_ids if you want to keep this inlined.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:75:
gfx::Image icon() {
This method is too complex to inline.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h:27:
struct SyncedNotificationSendingServiceSettingsData {
nit: class comment

[dewittj, 2014-03-26 17:04:11]

fyi, since you were wondering if there's a style guide rule for
declaration/definition ordering, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

"Method definitions in the corresponding .cc file should be the same as the
declaration order, as much as possible."

[Pete Williamson, 2014-03-26 18:12:54]

This should address all outstanding comments except the reordering of functions
to have the order in the .h file match the .cc file.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(left):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:152: //
underlying problem causing bad data.
On 2014/03/25 22:31:13, dewittj wrote:
> On 2014/03/17 21:43:43, dewittj wrote:
> > What was the underlying problem?
> 
> Do you have an answer for this comment?

Oops, my bad, sorry I missed it the first time.  It was on the sync side and had
long since been fixed, I forget the exact cause.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:840:
base::UTF8ToUTF16(app_info->settings_display_name()),
On 2014/03/25 22:31:13, dewittj wrote:
> On 2014/03/21 01:22:31, Pete Williamson wrote:
> > On 2014/03/17 21:43:43, dewittj wrote:
> > > Has the encoding of this string been set before hand?  Do we have any way
to
> > > internationalize this?
> > 
> > I'm not sure I understand the first question.  The string comes to us in
> UTF-8,
> > already localized for this user, so no further internationalization should
be
> > needed.
> 
> I was just wondering if the expected encoding is explicitly defined in the
> protobuf or other spec.

No, it is not defined in the protobuf, but IIRC, UTF-8 is the default for google
for all protobufs.

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
(right):

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.h:105: void
SetAddedAppIds(std::vector<std::string> added_app_ids);
On 2014/03/25 22:31:13, dewittj wrote:
> On 2014/03/17 21:43:43, dewittj wrote:
> > nit: rename to OnAppIdsAdded
> 
> This comment was not addressed.

Oops, missed that, done now.  I think that my method of moving from comment to
the next was flawed, I'll do something differently now. (not rely on "J" to have
things in the same order as the CR comments).

https://codereview.chromium.org/193773003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.h:106: void
SetRemovedAppIds(std::vector<std::string> removed_app_ids);
On 2014/03/25 22:31:13, dewittj wrote:
> On 2014/03/17 21:43:43, dewittj wrote:
> > nit: rename to OnAppIdsRemoved
> 
> This comment was not addressed.

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:69:
profile_, Profile::EXPLICIT_ACCESS);
On 2014/03/25 22:31:14, dewittj wrote:
> Based on the comments in Profile, I would expect you actually want
> IMPLICIT_ACCESS.

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:412:
(*notification_iter)->set_notifier_service(this);
On 2014/03/25 22:31:14, dewittj wrote:
> The way you changed the call to ShowAllForAppId doesn't really address what I
> was trying to get at, which could be rephrased as: Why change this service and
> the notification manager at all during the lifetime of the object?  They both
> outlive synced notifications and are required for correct functioning.  So
> unless there is some other reason to set it here, I would greatly prefer
adding
> a ChromeNotifierService* and NotificationUIManager* to the constructor of
> SyncedNotification, and reserving set_notifier_service and
> set_notification_manager for testing purposes.

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:563:
kSendingService1Name);
On 2014/03/25 22:31:14, dewittj wrote:
> nit: no newline

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:572:
notifier.OnSyncedNotificationServiceEnabled(kSendingService1Name,
On 2014/03/25 22:31:14, dewittj wrote:
> same

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:582:
notifier.OnSyncedNotificationServiceEnabled(kSendingService1Name,
On 2014/03/25 22:31:14, dewittj wrote:
> same

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:733:
// have a valid app info for the notification.
On 2014/03/25 22:31:14, dewittj wrote:
> Did you plan to write this test in the next CL?

No immediate plans to write this test.  I can move it up if you think we need it
sooner.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/image_holder.cc (right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.cc:10: // #include
"ui/gfx/image/image_skia_rep.h"
On 2014/03/25 22:31:14, dewittj wrote:
> nit: fix this include.

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.cc:47: return (
(low_dpi_url_.is_empty() || low_dpi_fetched_) &&
On 2014/03/25 22:31:14, dewittj wrote:
> nit: no whitespace between ( ( and ) )

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.cc:76: const SkBitmap*
bitmap) {
On 2014/03/25 22:31:14, dewittj wrote:
> nit:no newline

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.cc:77: // TODO(petewil):
Should I retry if a fetch fails?
On 2014/03/25 22:31:14, dewittj wrote:
> Please open a bug for this, maybe even mark launch1

crbug.com/256466

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/image_holder.h (right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.h:6: // and has code to
fetch the bitmap from the URL.
On 2014/03/25 22:31:14, dewittj wrote:
> I think that this comment and the one on line 28 should be merged.  No
comments
> typically appear before the #include section.

I just removed the comment.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.h:50: void
set_low_dpi_url(const GURL& low_dpi_url) {
On 2014/03/25 22:31:14, dewittj wrote:
> nit: unused, remove.

I'd really like to leave these in.  They form an integral part of the class
interface, and without them it is hard to understand what this class is supposed
to do.  As accessors, I don't think it is important to remove them if they
aren't called yet.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.cc:27:
void SyncedNotificationAppInfo::SetSettingsURLs(
On 2014/03/25 22:31:14, dewittj wrote:
> nit: declaration order and definition order should be the same.  Please
> double-check all other h/cc pairs for this property as well.

I'll do that in the next changelist so you can more easily see the fixes.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/synced_notification_app_info.h
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:45:
void SetSettingsURLs(const GURL& settings_low_dpi,
On 2014/03/25 22:31:14, dewittj wrote:
> nit: need comment on all public methods.  Ideally it'd explain in what places
in
> the UI the different URLs will be used.

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:59:
void SetAddedAppIds(std::vector<std::string> added_app_ids) {
On 2014/03/25 22:31:14, dewittj wrote:
> nit: put all inline methods beneath all non-inline methods.
> nit: set_added_app_ids if you want to keep this inlined.

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:67:
void SetRemovedAppIds(std::vector<std::string> removed_app_ids) {
On 2014/03/25 22:31:14, dewittj wrote:
> nit: set_removed_app_ids if you want to keep this inlined.
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

Oops, forgot about that guideline.  Thanks for reminding me.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info.h:75:
gfx::Image icon() {
On 2014/03/25 22:31:14, dewittj wrote:
> This method is too complex to inline.

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h:27:
struct SyncedNotificationSendingServiceSettingsData {
On 2014/03/25 22:31:14, dewittj wrote:
> nit: class comment

Done.

[dewittj, 2014-03-26 18:38:38]

lgtm, only nits remain at this point.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:563:
kSendingService1Name);
On 2014/03/26 18:12:55, Pete Williamson wrote:
> On 2014/03/25 22:31:14, dewittj wrote:
> > nit: no newline
> 
> Done.

I meant no newline between ( and kSendingService1Name);

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:572:
notifier.OnSyncedNotificationServiceEnabled(kSendingService1Name,
On 2014/03/26 18:12:55, Pete Williamson wrote:
> On 2014/03/25 22:31:14, dewittj wrote:
> > same
> 
> Done.

I meant no newline between kSendingService1Name, and true);

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:582:
notifier.OnSyncedNotificationServiceEnabled(kSendingService1Name,
On 2014/03/26 18:12:55, Pete Williamson wrote:
> On 2014/03/25 22:31:14, dewittj wrote:
> > same
> 
> Done.

I meant between kSendingService1Name, and false);

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/image_holder.cc (right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.cc:76: const SkBitmap*
bitmap) {
On 2014/03/26 18:12:55, Pete Williamson wrote:
> On 2014/03/25 22:31:14, dewittj wrote:
> > nit:no newline
> 
> Done.

This is not done.  No newline between url, and const SkBitmap* bitmap

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/image_holder.h (right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.h:50: void
set_low_dpi_url(const GURL& low_dpi_url) {
On 2014/03/26 18:12:55, Pete Williamson wrote:
> On 2014/03/25 22:31:14, dewittj wrote:
> > nit: unused, remove.
> 
> I'd really like to leave these in.  They form an integral part of the class
> interface, and without them it is hard to understand what this class is
supposed
> to do.  As accessors, I don't think it is important to remove them if they
> aren't called yet.

Can you explain how an unused function is integral?  To me it is just confusing
because now I have to grep the code to see if the URLs are being changed
mid-lifetime of this object.

Also the precedent in Chrome is to not include code that isn't used, even if you
foresee it being used sometime in the future.

[Pete Williamson, 2014-03-26 21:03:50]

Address LGTM issues, and re-order the functions in synced_notification.cc/.h to
match

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
(right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:563:
kSendingService1Name);
On 2014/03/26 18:38:39, dewittj wrote:
> On 2014/03/26 18:12:55, Pete Williamson wrote:
> > On 2014/03/25 22:31:14, dewittj wrote:
> > > nit: no newline
> > 
> > Done.
> 
> I meant no newline between ( and kSendingService1Name);

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:572:
notifier.OnSyncedNotificationServiceEnabled(kSendingService1Name,
On 2014/03/26 18:38:39, dewittj wrote:
> On 2014/03/26 18:12:55, Pete Williamson wrote:
> > On 2014/03/25 22:31:14, dewittj wrote:
> > > same
> > 
> > Done.
> 
> I meant no newline between kSendingService1Name, and true);

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:582:
notifier.OnSyncedNotificationServiceEnabled(kSendingService1Name,
On 2014/03/26 18:38:39, dewittj wrote:
> On 2014/03/26 18:12:55, Pete Williamson wrote:
> > On 2014/03/25 22:31:14, dewittj wrote:
> > > same
> > 
> > Done.
> 
> I meant between kSendingService1Name, and false);

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/image_holder.cc (right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.cc:76: const SkBitmap*
bitmap) {
On 2014/03/26 18:38:39, dewittj wrote:
> On 2014/03/26 18:12:55, Pete Williamson wrote:
> > On 2014/03/25 22:31:14, dewittj wrote:
> > > nit:no newline
> > 
> > Done.
> 
> This is not done.  No newline between url, and const SkBitmap* bitmap

Done.

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
File chrome/browser/notifications/sync_notifier/image_holder.h (right):

https://codereview.chromium.org/193773003/diff/130001/chrome/browser/notifica...
chrome/browser/notifications/sync_notifier/image_holder.h:50: void
set_low_dpi_url(const GURL& low_dpi_url) {
On 2014/03/26 18:38:39, dewittj wrote:
> On 2014/03/26 18:12:55, Pete Williamson wrote:
> > On 2014/03/25 22:31:14, dewittj wrote:
> > > nit: unused, remove.
> > 
> > I'd really like to leave these in.  They form an integral part of the class
> > interface, and without them it is hard to understand what this class is
> supposed
> > to do.  As accessors, I don't think it is important to remove them if they
> > aren't called yet.
> 
> Can you explain how an unused function is integral?  To me it is just
confusing
> because now I have to grep the code to see if the URLs are being changed
> mid-lifetime of this object.
> 
> Also the precedent in Chrome is to not include code that isn't used, even if
you
> foresee it being used sometime in the future.

Done.

[Pete Williamson, 2014-03-27 16:00:09]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2014-03-27 16:00:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/193773003/500001

[commit-bot: I haz the power, 2014-03-27 16:09:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 16:09:04]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[Pete Williamson, 2014-03-27 22:20:02]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:21:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/193773003/500001

[cmp, 2014-03-30 16:27:41]

The CQ bit was unchecked by cmp@chromium.org

[cmp, 2014-03-30 16:29:54]

The CQ bit was checked by cmp@chromium.org

[commit-bot: I haz the power, 2014-03-30 16:30:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/193773003/500001

[commit-bot: I haz the power, 2014-03-30 17:00:02]

Change committed as 260433