Enable and disable Synced Notification Client Services

Enable and disable Synced Notification Client Services

Synced Notifications can be sent from web services.  This checkin will allow us to enable and disable synced notifications from specific service sources.  For example, sending services could include mail, calendar, and Google+.  These notifications will be displayed to the user as Rich Notifications.

We are developing a protocol to send the names, app ids, and icons for these services as a new synced data type.  However, until that protocol is ready, we are hardcoding the names and icons of the services into chrome.  We will remove the hardcoded names and icons once the new sync datatype is ready (hopefully during the next milestone).

This change also supplies the name of our first client (Google+) and an icon to use in the synced notifications settings dialog.  Due to licensing restrictions, we are checking in one icon for chromium (basically a copy of the default favicon), and another one for chrome proper (this will be a separate checkin to the chrome specific codebase).

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

[Pete Williamson, 2013-07-18 18:08:12]

thakis@ please do OWNERS review of the resource changes.  Feel free to contact
me if my explanation in the bug does not answer all your questions about what we
are doing and why (I am so close to the problem I may have left out some
important context).

dewittj@, dimich@ please review the files under chrome/browser/notifications

[dewittj, 2013-07-18 21:22:25]

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.h (right):

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.h:74: std::string
GetSendingServiceName() const;
this is ambiguously named, since it's being used as an internal identifier.

https://codereview.chromium.org/19056002/diff/2001/ui/base/strings/ui_strings...
File ui/base/strings/ui_strings.grd (right):

https://codereview.chromium.org/19056002/diff/2001/ui/base/strings/ui_strings...
ui/base/strings/ui_strings.grd:501: <message
name="IDS_MESSAGE_CENTER_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME" desc="The name
of the first source for synced notifications.">
This should probably be in chrome/app/chromium_strings.grd and
chrome/app/google_chrome_strings.grd, not ui_strings, since it's a chrome
concept.

https://codereview.chromium.org/19056002/diff/2001/ui/resources/ui_resources.grd
File ui/resources/ui_resources.grd (right):

https://codereview.chromium.org/19056002/diff/2001/ui/resources/ui_resources....
ui/resources/ui_resources.grd:424: <structure type="chrome_scaled_image"
name="IDR_TEMPORARY_GOOGLE_PLUS_ICON" file="common/ic_gplus_color_16.png" />
I don't think ui/resources is the right place for this.  Should be somewhere in
chrome/browser, like chrome/app/theme/theme_resources.grd

[Nico, 2013-07-18 21:51:52]

BUG= line looks wrong (links to
https://code.google.com/p/chromium/issues/detail?id=254412).

[Nico, 2013-07-18 21:56:03]

CL description looks great (except for the wrong BUG= line), thanks. I agree
with dewittj's comments about ui/ probably not being the right place for the new
resources, but other than that this is fine.

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:297: //
service.  Until then, we use hardcoded service icons for all services.
Is there a bug for this TODO? If not, there probably should be (and this comment
should link to it)

https://codereview.chromium.org/19056002/diff/2001/ui/base/strings/ui_strings...
File ui/base/strings/ui_strings.grd (right):

https://codereview.chromium.org/19056002/diff/2001/ui/base/strings/ui_strings...
ui/base/strings/ui_strings.grd:501: <message
name="IDS_MESSAGE_CENTER_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME" desc="The name
of the first source for synced notifications.">
On 2013/07/18 21:22:25, dewittj wrote:
> This should probably be in chrome/app/chromium_strings.grd and
> chrome/app/google_chrome_strings.grd, not ui_strings, since it's a chrome
> concept.

No, strings that are common to chrome and chromium go in
chrome/app/generated_resources.grd (but yes, ui/ doesn't look right)

https://codereview.chromium.org/19056002/diff/2001/ui/resources/ui_resources.grd
File ui/resources/ui_resources.grd (right):

https://codereview.chromium.org/19056002/diff/2001/ui/resources/ui_resources....
ui/resources/ui_resources.grd:424: <structure type="chrome_scaled_image"
name="IDR_TEMPORARY_GOOGLE_PLUS_ICON" file="common/ic_gplus_color_16.png" />
On 2013/07/18 21:22:25, dewittj wrote:
> I don't think ui/resources is the right place for this.  Should be somewhere
in
> chrome/browser, like chrome/app/theme/theme_resources.grd

+1

[Pete Williamson, 2013-07-18 22:01:24]

On 2013/07/18 21:51:52, Nico wrote:
> BUG= line looks wrong (links to
> https://code.google.com/p/chromium/issues/detail?id=254412).

BUG= line updated

[Dmitry Titov, 2013-07-18 22:30:52]

Great! Agree with previous reviewers, couple small comments:

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:297: //
service.  Until then, we use hardcoded service icons for all services.
On 2013/07/18 21:56:03, Nico wrote:
> Is there a bug for this TODO? If not, there probably should be (and this
comment
> should link to it)

I guess it should be the same bug as for the previous TODO in this method,
mentioned there too.

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:377: //
TODO(petewil): Maybe faster to swap with last element and then erase it.
if enabled_sending_services_was a std::list, then simple erase() would be just
enough. find() would work the same, so why not replace with a list and remove
TODO?

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.cc:26: //
TODO(petewil): remove this hardcoding once we have the synced notification
This also needs a bug number on creating a way to download a list of sending
services.

[Pete Williamson, 2013-07-19 18:50:13]

CR fixes per DewittJ, Thakis, and Dimich

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:297: //
service.  Until then, we use hardcoded service icons for all services.
On 2013/07/18 21:56:03, Nico wrote:
> Is there a bug for this TODO? If not, there probably should be (and this
comment
> should link to it)

Done.

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:297: //
service.  Until then, we use hardcoded service icons for all services.
On 2013/07/18 22:30:52, Dmitry Titov wrote:
> On 2013/07/18 21:56:03, Nico wrote:
> > Is there a bug for this TODO? If not, there probably should be (and this
> comment
> > should link to it)
> 
> I guess it should be the same bug as for the previous TODO in this method,
> mentioned there too.

Pervious TODO now links to bug

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:377: //
TODO(petewil): Maybe faster to swap with last element and then erase it.
On 2013/07/18 22:30:52, Dmitry Titov wrote:
> if enabled_sending_services_was a std::list, then simple erase() would be just
> enough. find() would work the same, so why not replace with a list and remove
> TODO?

A vector has better locality of reference.  Since I don't expect this to be a
frequent operation, I just removed the TODO, which was a note to myself about a
potential optimization.

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.cc:26: //
TODO(petewil): remove this hardcoding once we have the synced notification
On 2013/07/18 22:30:52, Dmitry Titov wrote:
> This also needs a bug number on creating a way to download a list of sending
> services.

Done.

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.h (right):

https://codereview.chromium.org/19056002/diff/2001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.h:74: std::string
GetSendingServiceName() const;
On 2013/07/18 21:22:25, dewittj wrote:
> this is ambiguously named, since it's being used as an internal identifier.

Renamed to GetSendingServiceId()

https://codereview.chromium.org/19056002/diff/2001/ui/base/strings/ui_strings...
File ui/base/strings/ui_strings.grd (right):

https://codereview.chromium.org/19056002/diff/2001/ui/base/strings/ui_strings...
ui/base/strings/ui_strings.grd:501: <message
name="IDS_MESSAGE_CENTER_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME" desc="The name
of the first source for synced notifications.">
On 2013/07/18 21:56:03, Nico wrote:
> On 2013/07/18 21:22:25, dewittj wrote:
> > This should probably be in chrome/app/chromium_strings.grd and
> > chrome/app/google_chrome_strings.grd, not ui_strings, since it's a chrome
> > concept.
> 
> No, strings that are common to chrome and chromium go in
> chrome/app/generated_resources.grd (but yes, ui/ doesn't look right)

Moved to generated_resources.grd.

https://codereview.chromium.org/19056002/diff/2001/ui/resources/ui_resources.grd
File ui/resources/ui_resources.grd (right):

https://codereview.chromium.org/19056002/diff/2001/ui/resources/ui_resources....
ui/resources/ui_resources.grd:424: <structure type="chrome_scaled_image"
name="IDR_TEMPORARY_GOOGLE_PLUS_ICON" file="common/ic_gplus_color_16.png" />
On 2013/07/18 21:56:03, Nico wrote:
> On 2013/07/18 21:22:25, dewittj wrote:
> > I don't think ui/resources is the right place for this.  Should be somewhere
> in
> > chrome/browser, like chrome/app/theme/theme_resources.grd
> 
> +1

Done, moved to chrome/app/theme/theme_resources.grd

[Nico, 2013-07-19 19:29:38]

resource changes lgtm

https://codereview.chromium.org/19056002/diff/15001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/19056002/diff/15001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:15684: +    <message
name="IDS_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME" desc="The name of the first
service source for synced notifications.">
nit: I doubt a translator translating this string wil know what a "service
source" is, why it's important that this is the first, and even "synced
notification" is probably incomprehensible. "Product name shown in the settings
UI" (or similar) is probably better.

[dewittj, 2013-07-19 19:43:06]

lgtm with some nits.

https://codereview.chromium.org/19056002/diff/15001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/19056002/diff/15001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:283: //
https://code.google.com/p/chromium/issues/detail?id=248337
I'd prefer something like:
See crbug.com/248337.

at the end of line 282.  Same with the below bug reference.

https://codereview.chromium.org/19056002/diff/15001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/19056002/diff/15001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification.cc:28: //
https://code.google.com/p/chromium/issues/detail?id=248337
Same bug reference comment.

[Dmitry Titov, 2013-07-19 20:25:45]

lgtm

[Dmitry Titov, 2013-07-19 20:25:46]

lgtm

[Dmitry Titov, 2013-07-19 20:25:46]

lgtm

[Dmitry Titov, 2013-07-19 20:27:28]

Apparently the send form does not have multiple-click protection :-)

[commit-bot: I haz the power, 2013-07-19 20:58:27]

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

[commit-bot: I haz the power, 2013-07-19 20:58:29]

Can't process patch for file
chrome/app/theme/default_100_percent/common/ic_gplus_color_16.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[commit-bot: I haz the power, 2013-07-22 21:48:07]

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

[commit-bot: I haz the power, 2013-07-22 23:59:30]

Retried try job too often on linux_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-07-23 00:52:47]

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

[commit-bot: I haz the power, 2013-07-23 01:59:08]

Retried try job too often on linux_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-07-23 17:47:32]

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

[commit-bot: I haz the power, 2013-07-23 18:35:04]

Change committed as 213175

[Nico, 2014-02-06 04:09:39]

On Thu, Jul 18, 2013 at 11:08 AM, <petewil@chromium.org> wrote:

> Reviewers: dewittj, Dmitry Titov, Nico,
>
> Message:
> thakis@ please do OWNERS review of the resource changes.  Feel free to
> contact
> me if my explanation in the bug does not answer all your questions about
> what we
> are doing and why (I am so close to the problem I may have left out some
> important context).
>
> dewittj@, dimich@ please review the files under
> chrome/browser/notifications
>
> Description:
> Enable and disable Synced Notification Client Services
>
> Synced Notifications can be sent from web services.  This checkin will
> allow us
> to enable and disable synced notifications from specific service sources.
>  For
> example, sending services could include mail, calendar, and Google+.  These
> notifications will be displayed to the user as Rich Notifications.
>
> We are developing a protocol to send the names, app ids, and icons for
> these
> services as a new synced data type.  However, until that protocol is
> ready, we
> are hardcoding the names and icons of the services into chrome.  We will
> remove
> the hardcoded names and icons once the new sync datatype is ready
> (hopefully
> during the next milestone).
>

Is this the case now? It's half a year later, and the name seems to still
be hardcoded (but the bug it links to has been closed as dupe and the bug
it's duped against is marked fixed.)


>
> This change also supplies the name of our first client (Google+) and an
> icon to
> use in the synced notifications settings dialog.  Due to licensing
> restrictions,
> we are checking in one icon for chromium (basically a copy of the default
> favicon), and another one for chrome proper (this will be a separate
> checkin to
> the chrome specific codebase).
>
> BUG=254412
>
> Please review this at https://codereview.chromium.org/19056002/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
>   M chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
>   M chrome/browser/notifications/sync_notifier/synced_notification.h
>   M chrome/browser/notifications/sync_notifier/synced_notification.cc
>   M ui/base/strings/ui_strings.grd
>   A + ui/resources/default_100_percent/common/ic_gplus_color_16.png
>   A + ui/resources/default_200_percent/common/ic_gplus_color_16.png
>   M ui/resources/ui_resources.grd
>
>
> Index: chrome/browser/notifications/sync_notifier/chrome_notifier_
> service.cc
> diff --git
a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
> b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
> index 84643d3ffbba454b18558a3e46e2025a3f699283..
> 1198b38fbe9f7384a3405b704ecfa22b0b370bbb 100644
> --- a/chrome/browser/notifications/sync_notifier/
> chrome_notifier_service.cc
> +++ b/chrome/browser/notifications/sync_notifier/
> chrome_notifier_service.cc
> @@ -8,11 +8,15 @@
>
>  #include "chrome/browser/notifications/sync_notifier/chrome_notifier_
> service.h"
>
> +#include <string>
> +#include <vector>
> +
>  #include "chrome/browser/notifications/desktop_notification_service.h"
>  #include "chrome/browser/notifications/desktop_notification_service_
> factory.h"
>  #include "chrome/browser/notifications/notification.h"
>  #include "chrome/browser/notifications/notification_ui_manager.h"
>  #include "chrome/browser/profiles/profile.h"
> +#include "grit/ui_resources.h"
>  #include "grit/ui_strings.h"
>  #include "sync/api/sync_change.h"
>  #include "sync/api/sync_change_processor.h"
> @@ -21,13 +25,14 @@
>  #include "sync/protocol/synced_notification_specifics.pb.h"
>  #include "third_party/WebKit/public/web/WebTextDirection.h"
>  #include "ui/base/l10n/l10n_util.h"
> +#include "ui/base/resource/resource_bundle.h"
>  #include "ui/message_center/notifier_settings.h"
>  #include "url/gurl.h"
>
>  namespace notifier {
>  namespace {
>
> -const char kSampleSyncedNotificationServiceId[] =
> "sample-synced-service";
> +const char kFirstSyncedNotificationServiceId[] = "Google+";
>
>  }
>
> @@ -35,7 +40,8 @@ bool ChromeNotifierService::avoid_bitmap_fetching_for_test_
> = false;
>
>  ChromeNotifierService::ChromeNotifierService(Profile* profile,
>                                               NotificationUIManager*
> manager)
> -    : profile_(profile), notification_manager_(manager) {}
> +    : profile_(profile), notification_manager_(manager) {
> +}
>  ChromeNotifierService::~ChromeNotifierService() {}
>
>  // Methods from BrowserContextKeyedService.
> @@ -270,20 +276,29 @@ void ChromeNotifierService::
> GetSyncedNotificationServices(
>    // TODO(mukai|petewil): Check the profile's eligibility before adding
> the
>    // sample app.
>
> -  // Currently we just use kSampleSyncedNotificationServiceId as a place
> holder.
> -  // TODO(petewil): Really obtain the list of apps from the server and
> create
> -  // the list of ids here.
> +  // TODO(petewil): Really obtain the list of synced notification sending
> +  // services from the server and create the list of ids here.  Until
> then, we
> +  // are hardcoding the service names.  Once that is done, remove this
> +  // hardcoding.
>    DesktopNotificationService* desktop_notification_service =
>        DesktopNotificationServiceFactory::GetForProfile(profile_);
>    message_center::NotifierId notifier_id(
>        message_center::NotifierId::SYNCED_NOTIFICATION_SERVICE,
> -      kSampleSyncedNotificationServiceId);
> -  notifiers->push_back(new message_center::Notifier(
> +      kFirstSyncedNotificationServiceId);
> +  message_center::Notifier* notifier_service = new
> message_center::Notifier(
>        notifier_id,
>        l10n_util::GetStringUTF16(
> -          IDS_MESSAGE_CENTER_SAMPLE_SYNCED_NOTIFICATION_SERVICE_NAME),
> -      desktop_notification_service->IsNotifierEnabled(notifier_id)));
> -  // TODO(mukai): Add icon for the sample app.
> +          IDS_MESSAGE_CENTER_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME),
> +      desktop_notification_service->IsNotifierEnabled(notifier_id));
> +
> +  // Add icons for our sending services.
> +  // TODO(petewil): Replace this temporary hardcoding with a new sync
> datatype
> +  // to dynamically get the name and icon for each synced notification
> sending
> +  // service.  Until then, we use hardcoded service icons for all
> services.
> +  notifier_service->icon = ui::ResourceBundle::GetSharedInstance().
> +      GetImageNamed(IDR_TEMPORARY_GOOGLE_PLUS_ICON);
> +
> +  notifiers->push_back(notifier_service);
>  }
>
>  void ChromeNotifierService::MarkNotificationAsDismissed(
> @@ -311,6 +326,15 @@ void
ChromeNotifierService::Add(scoped_ptr<SyncedNotification>
> notification) {
>    // Take ownership of the object and put it into our local storage.
>    notification_data_.push_back(notification.release());
>
> +  // If the user is not interested in this type of notification, ignore
> it.
> +  std::vector<std::string>::iterator iter =
> +      find(enabled_sending_services_.begin(),
> +           enabled_sending_services_.end(),
> +           notification_copy->GetSendingServiceName());
> +  if (iter == enabled_sending_services_.end()) {
> +    return;
> +  }
> +
>    Display(notification_copy);
>  }
>
> @@ -320,6 +344,7 @@ void ChromeNotifierService::AddForTest(
>    }
>
>  void ChromeNotifierService::Display(SyncedNotification* notification) {
> +
>    // Set up to fetch the bitmaps.
>    notification->QueueBitmapFetchJobs(notification_manager_,
>                                            this,
> @@ -337,7 +362,24 @@ void ChromeNotifierService::Display(SyncedNotification*
> notification) {
>
>  void ChromeNotifierService::OnSyncedNotificationServiceEnabled(
>      const std::string& notifier_id, bool enabled) {
> -  // TODO(petewil): start/stop syncing
> +  std::vector<std::string>::iterator iter;
> +
> +  iter = find(enabled_sending_services_.begin(),
> +              enabled_sending_services_.end(),
> +              notifier_id);
> +
> +  // Add the notifier_id if it is enabled and not already there.
> +  if (iter == enabled_sending_services_.end() && enabled) {
> +    enabled_sending_services_.push_back(notifier_id);
> +    // TODO(petewil) Check now for any outstanding notifications.
> +  // Remove the notifier_id if it is disabled and present.
> +  } else if (iter != enabled_sending_services_.end() && !enabled) {
> +    // TODO(petewil): Maybe faster to swap with last element and then
> erase it.
> +    enabled_sending_services_.erase(iter);
> +  }
> +
> +  // Otherwise, nothing to do, we can exit.
> +  return;
>  }
>
>  }  // namespace notifier
> Index: chrome/browser/notifications/sync_notifier/chrome_notifier_
> service.h
> diff --git
a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
> b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
> index b423703af79e1fb7be96344e81a7b02828740c9a..
> 13a21f0ec7542502424054b5036143581faa8510 100644
> --- a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
> +++ b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
> @@ -98,6 +98,7 @@ class ChromeNotifierService : public
> syncer::SyncableService,
>    Profile* const profile_;
>    NotificationUIManager* const notification_manager_;
>    scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
> +  std::vector<std::string> enabled_sending_services_;
>    static bool avoid_bitmap_fetching_for_test_;
>
>    // TODO(petewil): Consider whether a map would better suit our data.
> Index: chrome/browser/notifications/sync_notifier/synced_notification.cc
> diff --git
a/chrome/browser/notifications/sync_notifier/synced_notification.cc
> b/chrome/browser/notifications/sync_notifier/synced_notification.cc
> index 851c5d68ac105da38c1426ee8ece1706c6df0fe3..
> 21ed5e9cb585b4f13dbcd42bc8b36a17161c71bc 100644
> --- a/chrome/browser/notifications/sync_notifier/synced_notification.cc
> +++ b/chrome/browser/notifications/sync_notifier/synced_notification.cc
> @@ -22,6 +22,12 @@
>  namespace {
>  const char kExtensionScheme[] = "chrome-extension://";
>
> +// The name of our first synced notification service.
> +// TODO(petewil): remove this hardcoding once we have the synced
> notification
> +// signalling sync data type set up to provide this.
> +const char kFirstSyncedNotificationServiceId[] = "Google+";
> +
> +
>  // Today rich notifications only supports two buttons, make sure we don't
>  // try to supply them with more than this number of buttons.
>  const unsigned int kMaxNotificationButtonIndex = 2;
> @@ -470,7 +476,9 @@ int SyncedNotification::GetPriority() const {
>      return message_center::DEFAULT_PRIORITY;
>    } else if (protobuf_priority ==
>               sync_pb::CoalescedSyncedNotification_Priority_HIGH) {
> -    return message_center::HIGH_PRIORITY;
> +    // High priority synced notifications are considered default priority
> in
> +    // Chrome.
> +    return message_center::DEFAULT_PRIORITY;
>    } else {
>      // Complain if this is a new priority we have not seen before.
>      DCHECK(protobuf_priority <
> @@ -575,4 +583,12 @@ std::string SyncedNotification::
> GetContainedNotificationMessage(
>        collapsed_info(index).simple_collapsed_layout().description();
>  }
>
> +std::string SyncedNotification::GetSendingServiceName() const {
> +  // TODO(petewil): We are building a new protocol (a new sync datatype)
> to send
> +  // the service name and icon from the server.  For now this method is
> +  // hardcoded to the name of our first service using synced
> notifications.
> +  // Once the new protocol is built, remove this hardcoding.
> +  return kFirstSyncedNotificationServiceId;
> +}
> +
>  }  // namespace notifier
> Index: chrome/browser/notifications/sync_notifier/synced_notification.h
> diff --git a/chrome/browser/notifications/sync_notifier/synced_notification.h
> b/chrome/browser/notifications/sync_notifier/synced_notification.h
> index fd3ee552cec80369f6e3c9d3919cd050bef287c2..
> 0bbcb2903ce376d7458a3f3710a02b835e26a5c9 100644
> --- a/chrome/browser/notifications/sync_notifier/synced_notification.h
> +++ b/chrome/browser/notifications/sync_notifier/synced_notification.h
> @@ -71,6 +71,7 @@ class SyncedNotification : public
> NotificationBitmapFetcherDelegate {
>    size_t GetButtonCount() const;
>    std::string GetContainedNotificationTitle(int index) const;
>    std::string GetContainedNotificationMessage(int index) const;
> +  std::string GetSendingServiceName() const;
>
>
>    bool EqualsIgnoringReadState(const SyncedNotification& other) const;
> Index: ui/base/strings/ui_strings.grd
> diff --git a/ui/base/strings/ui_strings.grd b/ui/base/strings/ui_strings.
> grd
> index fd1cac9db281590776d222b0512d33389b1c5312..
> 237aad1e451ce5456805e2057898c7c3fb2e351f 100644
> --- a/ui/base/strings/ui_strings.grd
> +++ b/ui/base/strings/ui_strings.grd
> @@ -498,8 +498,8 @@ need to be translated for each locale.-->
>        <message name="IDS_MESSAGE_CENTER_TOOLTIP_UNREAD" desc="Tooltip
> for notification tray icon with unread notifications">
>          <ph name="product">$1<ex>Chrome</ex></ph> - Notifications (<ph
> name="QUANTITY">$2<ex>3</ex></ph> unread)
>        </message>
> -      <message
name="IDS_MESSAGE_CENTER_SAMPLE_SYNCED_NOTIFICATION_SERVICE_NAME"
> desc="The name of the only source for synced notifications. TODO and note
> for translators: this text must be replaced by the real name by the branch
> point.  See crbug.com/254617">
> -        Sample app
> +      <message
name="IDS_MESSAGE_CENTER_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME"
> desc="The name of the first source for synced notifications.">
> +        Google+
>        </message>
>        <message name="IDS_MESSAGE_CENTER_NOTIFIER_SCREENSHOT_NAME"
> desc="The name of screenshot notifier that is a system componet">
>          Screenshot
> Index: ui/resources/default_100_percent/common/ic_gplus_color_16.png
> diff --git a/ui/resources/default_100_percent/common/default_favicon.png
> b/ui/resources/default_100_percent/common/ic_gplus_color_16.png
> similarity index 100%
> copy from ui/resources/default_100_percent/common/default_favicon.png
> copy to ui/resources/default_100_percent/common/ic_gplus_color_16.png
> Index: ui/resources/default_200_percent/common/ic_gplus_color_16.png
> diff --git a/ui/resources/default_200_percent/common/default_favicon.png
> b/ui/resources/default_200_percent/common/ic_gplus_color_16.png
> similarity index 100%
> copy from ui/resources/default_200_percent/common/default_favicon.png
> copy to ui/resources/default_200_percent/common/ic_gplus_color_16.png
> Index: ui/resources/ui_resources.grd
> diff --git a/ui/resources/ui_resources.grd b/ui/resources/ui_resources.grd
> index 1f853bd15a08472ba18d9d57e454ae7a5e1df99e..
> 4d50648e33a0219d480de5e4971deb67db6ada09 100644
> --- a/ui/resources/ui_resources.grd
> +++ b/ui/resources/ui_resources.grd
> @@ -421,6 +421,7 @@
>          <structure type="chrome_scaled_image" name="IDR_RESTORE_H"
> file="restore_hover.png" />
>          <structure type="chrome_scaled_image" name="IDR_RESTORE_P"
> file="restore_pressed.png" />
>        </if>
> +      <structure type="chrome_scaled_image"
name="IDR_TEMPORARY_GOOGLE_PLUS_ICON"
> file="common/ic_gplus_color_16.png" />
>        <structure type="chrome_scaled_image"
name="IDR_TEXTBUTTON_HOVER_BOTTOM"
> file="common/textbutton_hover_bottom.png" />
>        <structure type="chrome_scaled_image"
name="IDR_TEXTBUTTON_HOVER_BOTTOM_LEFT"
> file="common/textbutton_hover_bottom_left.png" />
>        <structure type="chrome_scaled_image"
name="IDR_TEXTBUTTON_HOVER_BOTTOM_RIGHT"
> file="common/textbutton_hover_bottom_right.png" />
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[petewil, 2014-02-06 17:55:11]

Yep, it is currently under work.  We took a detour to investigate a desk
band. Sorry this has taken so long to get to.

Here is the first changelist for the "unhardcoding" work.
https://codereview.chromium.org/143973006/

I'm prearing a second changelist now, and a third changelist will be
required before removing the hardcoding is complete.


On Wed, Feb 5, 2014 at 8:09 PM, Nico Weber <thakis@chromium.org> wrote:

> On Thu, Jul 18, 2013 at 11:08 AM, <petewil@chromium.org> wrote:
>
>> Reviewers: dewittj, Dmitry Titov, Nico,
>>
>> Message:
>> thakis@ please do OWNERS review of the resource changes.  Feel free to
>> contact
>> me if my explanation in the bug does not answer all your questions about
>> what we
>> are doing and why (I am so close to the problem I may have left out some
>> important context).
>>
>> dewittj@, dimich@ please review the files under
>> chrome/browser/notifications
>>
>> Description:
>> Enable and disable Synced Notification Client Services
>>
>> Synced Notifications can be sent from web services.  This checkin will
>> allow us
>> to enable and disable synced notifications from specific service sources.
>>  For
>> example, sending services could include mail, calendar, and Google+.
>>  These
>> notifications will be displayed to the user as Rich Notifications.
>>
>> We are developing a protocol to send the names, app ids, and icons for
>> these
>> services as a new synced data type.  However, until that protocol is
>> ready, we
>> are hardcoding the names and icons of the services into chrome.  We will
>> remove
>> the hardcoded names and icons once the new sync datatype is ready
>> (hopefully
>> during the next milestone).
>>
>
> Is this the case now? It's half a year later, and the name seems to still
> be hardcoded (but the bug it links to has been closed as dupe and the bug
> it's duped against is marked fixed.)
>
>
>>
>> This change also supplies the name of our first client (Google+) and an
>> icon to
>> use in the synced notifications settings dialog.  Due to licensing
>> restrictions,
>> we are checking in one icon for chromium (basically a copy of the default
>> favicon), and another one for chrome proper (this will be a separate
>> checkin to
>> the chrome specific codebase).
>>
>> BUG=254412
>>
>> Please review this at https://codereview.chromium.org/19056002/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files:
>>   M chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
>>   M chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
>>   M chrome/browser/notifications/sync_notifier/synced_notification.h
>>   M chrome/browser/notifications/sync_notifier/synced_notification.cc
>>   M ui/base/strings/ui_strings.grd
>>   A + ui/resources/default_100_percent/common/ic_gplus_color_16.png
>>   A + ui/resources/default_200_percent/common/ic_gplus_color_16.png
>>   M ui/resources/ui_resources.grd
>>
>>
>> Index: chrome/browser/notifications/sync_notifier/chrome_notifier_
>> service.cc
>> diff --git
a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
>> b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
>> index 84643d3ffbba454b18558a3e46e2025a3f699283..
>> 1198b38fbe9f7384a3405b704ecfa22b0b370bbb 100644
>> --- a/chrome/browser/notifications/sync_notifier/
>> chrome_notifier_service.cc
>> +++ b/chrome/browser/notifications/sync_notifier/
>> chrome_notifier_service.cc
>> @@ -8,11 +8,15 @@
>>
>>  #include "chrome/browser/notifications/sync_notifier/chrome_notifier_
>> service.h"
>>
>> +#include <string>
>> +#include <vector>
>> +
>>  #include "chrome/browser/notifications/desktop_notification_service.h"
>>  #include "chrome/browser/notifications/desktop_notification_service_
>> factory.h"
>>  #include "chrome/browser/notifications/notification.h"
>>  #include "chrome/browser/notifications/notification_ui_manager.h"
>>  #include "chrome/browser/profiles/profile.h"
>> +#include "grit/ui_resources.h"
>>  #include "grit/ui_strings.h"
>>  #include "sync/api/sync_change.h"
>>  #include "sync/api/sync_change_processor.h"
>> @@ -21,13 +25,14 @@
>>  #include "sync/protocol/synced_notification_specifics.pb.h"
>>  #include "third_party/WebKit/public/web/WebTextDirection.h"
>>  #include "ui/base/l10n/l10n_util.h"
>> +#include "ui/base/resource/resource_bundle.h"
>>  #include "ui/message_center/notifier_settings.h"
>>  #include "url/gurl.h"
>>
>>  namespace notifier {
>>  namespace {
>>
>> -const char kSampleSyncedNotificationServiceId[] =
>> "sample-synced-service";
>> +const char kFirstSyncedNotificationServiceId[] = "Google+";
>>
>>  }
>>
>> @@ -35,7 +40,8 @@ bool
ChromeNotifierService::avoid_bitmap_fetching_for_test_
>> = false;
>>
>>  ChromeNotifierService::ChromeNotifierService(Profile* profile,
>>                                               NotificationUIManager*
>> manager)
>> -    : profile_(profile), notification_manager_(manager) {}
>> +    : profile_(profile), notification_manager_(manager) {
>> +}
>>  ChromeNotifierService::~ChromeNotifierService() {}
>>
>>  // Methods from BrowserContextKeyedService.
>> @@ -270,20 +276,29 @@ void ChromeNotifierService::
>> GetSyncedNotificationServices(
>>    // TODO(mukai|petewil): Check the profile's eligibility before adding
>> the
>>    // sample app.
>>
>> -  // Currently we just use kSampleSyncedNotificationServiceId as a
>> place holder.
>> -  // TODO(petewil): Really obtain the list of apps from the server and
>> create
>> -  // the list of ids here.
>> +  // TODO(petewil): Really obtain the list of synced notification sending
>> +  // services from the server and create the list of ids here.  Until
>> then, we
>> +  // are hardcoding the service names.  Once that is done, remove this
>> +  // hardcoding.
>>    DesktopNotificationService* desktop_notification_service =
>>        DesktopNotificationServiceFactory::GetForProfile(profile_);
>>    message_center::NotifierId notifier_id(
>>        message_center::NotifierId::SYNCED_NOTIFICATION_SERVICE,
>> -      kSampleSyncedNotificationServiceId);
>> -  notifiers->push_back(new message_center::Notifier(
>> +      kFirstSyncedNotificationServiceId);
>> +  message_center::Notifier* notifier_service = new
>> message_center::Notifier(
>>        notifier_id,
>>        l10n_util::GetStringUTF16(
>> -          IDS_MESSAGE_CENTER_SAMPLE_SYNCED_NOTIFICATION_SERVICE_NAME),
>> -      desktop_notification_service->IsNotifierEnabled(notifier_id)));
>> -  // TODO(mukai): Add icon for the sample app.
>> +          IDS_MESSAGE_CENTER_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME),
>> +      desktop_notification_service->IsNotifierEnabled(notifier_id));
>> +
>> +  // Add icons for our sending services.
>> +  // TODO(petewil): Replace this temporary hardcoding with a new sync
>> datatype
>> +  // to dynamically get the name and icon for each synced notification
>> sending
>> +  // service.  Until then, we use hardcoded service icons for all
>> services.
>> +  notifier_service->icon = ui::ResourceBundle::GetSharedInstance().
>> +      GetImageNamed(IDR_TEMPORARY_GOOGLE_PLUS_ICON);
>> +
>> +  notifiers->push_back(notifier_service);
>>  }
>>
>>  void ChromeNotifierService::MarkNotificationAsDismissed(
>> @@ -311,6 +326,15 @@ void
ChromeNotifierService::Add(scoped_ptr<SyncedNotification>
>> notification) {
>>    // Take ownership of the object and put it into our local storage.
>>    notification_data_.push_back(notification.release());
>>
>> +  // If the user is not interested in this type of notification, ignore
>> it.
>> +  std::vector<std::string>::iterator iter =
>> +      find(enabled_sending_services_.begin(),
>> +           enabled_sending_services_.end(),
>> +           notification_copy->GetSendingServiceName());
>> +  if (iter == enabled_sending_services_.end()) {
>> +    return;
>> +  }
>> +
>>    Display(notification_copy);
>>  }
>>
>> @@ -320,6 +344,7 @@ void ChromeNotifierService::AddForTest(
>>    }
>>
>>  void ChromeNotifierService::Display(SyncedNotification* notification) {
>> +
>>    // Set up to fetch the bitmaps.
>>    notification->QueueBitmapFetchJobs(notification_manager_,
>>                                            this,
>> @@ -337,7 +362,24 @@ void ChromeNotifierService::Display(SyncedNotification*
>> notification) {
>>
>>  void ChromeNotifierService::OnSyncedNotificationServiceEnabled(
>>      const std::string& notifier_id, bool enabled) {
>> -  // TODO(petewil): start/stop syncing
>> +  std::vector<std::string>::iterator iter;
>> +
>> +  iter = find(enabled_sending_services_.begin(),
>> +              enabled_sending_services_.end(),
>> +              notifier_id);
>> +
>> +  // Add the notifier_id if it is enabled and not already there.
>> +  if (iter == enabled_sending_services_.end() && enabled) {
>> +    enabled_sending_services_.push_back(notifier_id);
>> +    // TODO(petewil) Check now for any outstanding notifications.
>> +  // Remove the notifier_id if it is disabled and present.
>> +  } else if (iter != enabled_sending_services_.end() && !enabled) {
>> +    // TODO(petewil): Maybe faster to swap with last element and then
>> erase it.
>> +    enabled_sending_services_.erase(iter);
>> +  }
>> +
>> +  // Otherwise, nothing to do, we can exit.
>> +  return;
>>  }
>>
>>  }  // namespace notifier
>> Index: chrome/browser/notifications/sync_notifier/chrome_notifier_
>> service.h
>> diff --git
a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
>> b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
>> index b423703af79e1fb7be96344e81a7b02828740c9a..
>> 13a21f0ec7542502424054b5036143581faa8510 100644
>> --- a/chrome/browser/notifications/sync_notifier/
>> chrome_notifier_service.h
>> +++ b/chrome/browser/notifications/sync_notifier/
>> chrome_notifier_service.h
>> @@ -98,6 +98,7 @@ class ChromeNotifierService : public
>> syncer::SyncableService,
>>    Profile* const profile_;
>>    NotificationUIManager* const notification_manager_;
>>    scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
>> +  std::vector<std::string> enabled_sending_services_;
>>    static bool avoid_bitmap_fetching_for_test_;
>>
>>    // TODO(petewil): Consider whether a map would better suit our data.
>> Index: chrome/browser/notifications/sync_notifier/synced_notification.cc
>> diff --git
a/chrome/browser/notifications/sync_notifier/synced_notification.cc
>> b/chrome/browser/notifications/sync_notifier/synced_notification.cc
>> index 851c5d68ac105da38c1426ee8ece1706c6df0fe3..
>> 21ed5e9cb585b4f13dbcd42bc8b36a17161c71bc 100644
>> --- a/chrome/browser/notifications/sync_notifier/synced_notification.cc
>> +++ b/chrome/browser/notifications/sync_notifier/synced_notification.cc
>> @@ -22,6 +22,12 @@
>>  namespace {
>>  const char kExtensionScheme[] = "chrome-extension://";
>>
>> +// The name of our first synced notification service.
>> +// TODO(petewil): remove this hardcoding once we have the synced
>> notification
>> +// signalling sync data type set up to provide this.
>> +const char kFirstSyncedNotificationServiceId[] = "Google+";
>> +
>> +
>>  // Today rich notifications only supports two buttons, make sure we don't
>>  // try to supply them with more than this number of buttons.
>>  const unsigned int kMaxNotificationButtonIndex = 2;
>> @@ -470,7 +476,9 @@ int SyncedNotification::GetPriority() const {
>>      return message_center::DEFAULT_PRIORITY;
>>    } else if (protobuf_priority ==
>>               sync_pb::CoalescedSyncedNotification_Priority_HIGH) {
>> -    return message_center::HIGH_PRIORITY;
>> +    // High priority synced notifications are considered default
>> priority in
>> +    // Chrome.
>> +    return message_center::DEFAULT_PRIORITY;
>>    } else {
>>      // Complain if this is a new priority we have not seen before.
>>      DCHECK(protobuf_priority <
>> @@ -575,4 +583,12 @@ std::string SyncedNotification::
>> GetContainedNotificationMessage(
>>        collapsed_info(index).simple_collapsed_layout().description();
>>  }
>>
>> +std::string SyncedNotification::GetSendingServiceName() const {
>> +  // TODO(petewil): We are building a new protocol (a new sync datatype)
>> to send
>> +  // the service name and icon from the server.  For now this method is
>> +  // hardcoded to the name of our first service using synced
>> notifications.
>> +  // Once the new protocol is built, remove this hardcoding.
>> +  return kFirstSyncedNotificationServiceId;
>> +}
>> +
>>  }  // namespace notifier
>> Index: chrome/browser/notifications/sync_notifier/synced_notification.h
>> diff --git
a/chrome/browser/notifications/sync_notifier/synced_notification.h
>> b/chrome/browser/notifications/sync_notifier/synced_notification.h
>> index fd3ee552cec80369f6e3c9d3919cd050bef287c2..
>> 0bbcb2903ce376d7458a3f3710a02b835e26a5c9 100644
>> --- a/chrome/browser/notifications/sync_notifier/synced_notification.h
>> +++ b/chrome/browser/notifications/sync_notifier/synced_notification.h
>> @@ -71,6 +71,7 @@ class SyncedNotification : public
>> NotificationBitmapFetcherDelegate {
>>    size_t GetButtonCount() const;
>>    std::string GetContainedNotificationTitle(int index) const;
>>    std::string GetContainedNotificationMessage(int index) const;
>> +  std::string GetSendingServiceName() const;
>>
>>
>>    bool EqualsIgnoringReadState(const SyncedNotification& other) const;
>> Index: ui/base/strings/ui_strings.grd
>> diff --git a/ui/base/strings/ui_strings.grd b/ui/base/strings/ui_strings.
>> grd
>> index fd1cac9db281590776d222b0512d33389b1c5312..
>> 237aad1e451ce5456805e2057898c7c3fb2e351f 100644
>> --- a/ui/base/strings/ui_strings.grd
>> +++ b/ui/base/strings/ui_strings.grd
>> @@ -498,8 +498,8 @@ need to be translated for each locale.-->
>>        <message name="IDS_MESSAGE_CENTER_TOOLTIP_UNREAD" desc="Tooltip
>> for notification tray icon with unread notifications">
>>          <ph name="product">$1<ex>Chrome</ex></ph> - Notifications (<ph
>> name="QUANTITY">$2<ex>3</ex></ph> unread)
>>        </message>
>> -      <message
name="IDS_MESSAGE_CENTER_SAMPLE_SYNCED_NOTIFICATION_SERVICE_NAME"
>> desc="The name of the only source for synced notifications. TODO and note
>> for translators: this text must be replaced by the real name by the branch
>> point.  See crbug.com/254617">
>> -        Sample app
>> +      <message
name="IDS_MESSAGE_CENTER_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME"
>> desc="The name of the first source for synced notifications.">
>> +        Google+
>>        </message>
>>        <message name="IDS_MESSAGE_CENTER_NOTIFIER_SCREENSHOT_NAME"
>> desc="The name of screenshot notifier that is a system componet">
>>          Screenshot
>> Index: ui/resources/default_100_percent/common/ic_gplus_color_16.png
>> diff --git a/ui/resources/default_100_percent/common/default_favicon.png
>> b/ui/resources/default_100_percent/common/ic_gplus_color_16.png
>> similarity index 100%
>> copy from ui/resources/default_100_percent/common/default_favicon.png
>> copy to ui/resources/default_100_percent/common/ic_gplus_color_16.png
>> Index: ui/resources/default_200_percent/common/ic_gplus_color_16.png
>> diff --git a/ui/resources/default_200_percent/common/default_favicon.png
>> b/ui/resources/default_200_percent/common/ic_gplus_color_16.png
>> similarity index 100%
>> copy from ui/resources/default_200_percent/common/default_favicon.png
>> copy to ui/resources/default_200_percent/common/ic_gplus_color_16.png
>> Index: ui/resources/ui_resources.grd
>> diff --git a/ui/resources/ui_resources.grd b/ui/resources/ui_resources.
>> grd
>> index 1f853bd15a08472ba18d9d57e454ae7a5e1df99e..
>> 4d50648e33a0219d480de5e4971deb67db6ada09 100644
>> --- a/ui/resources/ui_resources.grd
>> +++ b/ui/resources/ui_resources.grd
>> @@ -421,6 +421,7 @@
>>          <structure type="chrome_scaled_image" name="IDR_RESTORE_H"
>> file="restore_hover.png" />
>>          <structure type="chrome_scaled_image" name="IDR_RESTORE_P"
>> file="restore_pressed.png" />
>>        </if>
>> +      <structure type="chrome_scaled_image"
name="IDR_TEMPORARY_GOOGLE_PLUS_ICON"
>> file="common/ic_gplus_color_16.png" />
>>        <structure type="chrome_scaled_image"
name="IDR_TEXTBUTTON_HOVER_BOTTOM"
>> file="common/textbutton_hover_bottom.png" />
>>        <structure type="chrome_scaled_image"
name="IDR_TEXTBUTTON_HOVER_BOTTOM_LEFT"
>> file="common/textbutton_hover_bottom_left.png" />
>>        <structure type="chrome_scaled_image"
name="IDR_TEXTBUTTON_HOVER_BOTTOM_RIGHT"
>> file="common/textbutton_hover_bottom_right.png" />
>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-02-06 17:59:46]

Good to hear it's under development, thanks!


On Thu, Feb 6, 2014 at 9:55 AM, Pete Williamson <petewil@google.com> wrote:

> Yep, it is currently under work.  We took a detour to investigate a desk
> band. Sorry this has taken so long to get to.
>
> Here is the first changelist for the "unhardcoding" work.
> https://codereview.chromium.org/143973006/
>
> I'm prearing a second changelist now, and a third changelist will be
> required before removing the hardcoding is complete.
>
>
> On Wed, Feb 5, 2014 at 8:09 PM, Nico Weber <thakis@chromium.org> wrote:
>
>> On Thu, Jul 18, 2013 at 11:08 AM, <petewil@chromium.org> wrote:
>>
>>> Reviewers: dewittj, Dmitry Titov, Nico,
>>>
>>> Message:
>>> thakis@ please do OWNERS review of the resource changes.  Feel free to
>>> contact
>>> me if my explanation in the bug does not answer all your questions about
>>> what we
>>> are doing and why (I am so close to the problem I may have left out some
>>> important context).
>>>
>>> dewittj@, dimich@ please review the files under
>>> chrome/browser/notifications
>>>
>>> Description:
>>> Enable and disable Synced Notification Client Services
>>>
>>> Synced Notifications can be sent from web services.  This checkin will
>>> allow us
>>> to enable and disable synced notifications from specific service
>>> sources.  For
>>> example, sending services could include mail, calendar, and Google+.
>>>  These
>>> notifications will be displayed to the user as Rich Notifications.
>>>
>>> We are developing a protocol to send the names, app ids, and icons for
>>> these
>>> services as a new synced data type.  However, until that protocol is
>>> ready, we
>>> are hardcoding the names and icons of the services into chrome.  We will
>>> remove
>>> the hardcoded names and icons once the new sync datatype is ready
>>> (hopefully
>>> during the next milestone).
>>>
>>
>> Is this the case now? It's half a year later, and the name seems to still
>> be hardcoded (but the bug it links to has been closed as dupe and the bug
>> it's duped against is marked fixed.)
>>
>>
>>>
>>> This change also supplies the name of our first client (Google+) and an
>>> icon to
>>> use in the synced notifications settings dialog.  Due to licensing
>>> restrictions,
>>> we are checking in one icon for chromium (basically a copy of the default
>>> favicon), and another one for chrome proper (this will be a separate
>>> checkin to
>>> the chrome specific codebase).
>>>
>>> BUG=254412
>>>
>>> Please review this at https://codereview.chromium.org/19056002/
>>>
>>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>>
>>> Affected files:
>>>   M chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
>>>   M chrome/browser/notifications/sync_notifier/chrome_notifier_
>>> service.cc
>>>   M chrome/browser/notifications/sync_notifier/synced_notification.h
>>>   M chrome/browser/notifications/sync_notifier/synced_notification.cc
>>>   M ui/base/strings/ui_strings.grd
>>>   A + ui/resources/default_100_percent/common/ic_gplus_color_16.png
>>>   A + ui/resources/default_200_percent/common/ic_gplus_color_16.png
>>>   M ui/resources/ui_resources.grd
>>>
>>>
>>> Index: chrome/browser/notifications/sync_notifier/chrome_notifier_
>>> service.cc
>>> diff --git
a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
>>> b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
>>> index 84643d3ffbba454b18558a3e46e2025a3f699283..
>>> 1198b38fbe9f7384a3405b704ecfa22b0b370bbb 100644
>>> --- a/chrome/browser/notifications/sync_notifier/
>>> chrome_notifier_service.cc
>>> +++ b/chrome/browser/notifications/sync_notifier/
>>> chrome_notifier_service.cc
>>> @@ -8,11 +8,15 @@
>>>
>>>  #include "chrome/browser/notifications/sync_notifier/chrome_notifier_
>>> service.h"
>>>
>>> +#include <string>
>>> +#include <vector>
>>> +
>>>  #include "chrome/browser/notifications/desktop_notification_service.h"
>>>  #include "chrome/browser/notifications/desktop_notification_service_
>>> factory.h"
>>>  #include "chrome/browser/notifications/notification.h"
>>>  #include "chrome/browser/notifications/notification_ui_manager.h"
>>>  #include "chrome/browser/profiles/profile.h"
>>> +#include "grit/ui_resources.h"
>>>  #include "grit/ui_strings.h"
>>>  #include "sync/api/sync_change.h"
>>>  #include "sync/api/sync_change_processor.h"
>>> @@ -21,13 +25,14 @@
>>>  #include "sync/protocol/synced_notification_specifics.pb.h"
>>>  #include "third_party/WebKit/public/web/WebTextDirection.h"
>>>  #include "ui/base/l10n/l10n_util.h"
>>> +#include "ui/base/resource/resource_bundle.h"
>>>  #include "ui/message_center/notifier_settings.h"
>>>  #include "url/gurl.h"
>>>
>>>  namespace notifier {
>>>  namespace {
>>>
>>> -const char kSampleSyncedNotificationServiceId[] =
>>> "sample-synced-service";
>>> +const char kFirstSyncedNotificationServiceId[] = "Google+";
>>>
>>>  }
>>>
>>> @@ -35,7 +40,8 @@ bool
ChromeNotifierService::avoid_bitmap_fetching_for_test_
>>> = false;
>>>
>>>  ChromeNotifierService::ChromeNotifierService(Profile* profile,
>>>                                               NotificationUIManager*
>>> manager)
>>> -    : profile_(profile), notification_manager_(manager) {}
>>> +    : profile_(profile), notification_manager_(manager) {
>>> +}
>>>  ChromeNotifierService::~ChromeNotifierService() {}
>>>
>>>  // Methods from BrowserContextKeyedService.
>>> @@ -270,20 +276,29 @@ void ChromeNotifierService::
>>> GetSyncedNotificationServices(
>>>    // TODO(mukai|petewil): Check the profile's eligibility before adding
>>> the
>>>    // sample app.
>>>
>>> -  // Currently we just use kSampleSyncedNotificationServiceId as a
>>> place holder.
>>> -  // TODO(petewil): Really obtain the list of apps from the server and
>>> create
>>> -  // the list of ids here.
>>> +  // TODO(petewil): Really obtain the list of synced notification
>>> sending
>>> +  // services from the server and create the list of ids here.  Until
>>> then, we
>>> +  // are hardcoding the service names.  Once that is done, remove this
>>> +  // hardcoding.
>>>    DesktopNotificationService* desktop_notification_service =
>>>        DesktopNotificationServiceFactory::GetForProfile(profile_);
>>>    message_center::NotifierId notifier_id(
>>>        message_center::NotifierId::SYNCED_NOTIFICATION_SERVICE,
>>> -      kSampleSyncedNotificationServiceId);
>>> -  notifiers->push_back(new message_center::Notifier(
>>> +      kFirstSyncedNotificationServiceId);
>>> +  message_center::Notifier* notifier_service = new
>>> message_center::Notifier(
>>>        notifier_id,
>>>        l10n_util::GetStringUTF16(
>>> -          IDS_MESSAGE_CENTER_SAMPLE_SYNCED_NOTIFICATION_SERVICE_NAME),
>>> -      desktop_notification_service->IsNotifierEnabled(notifier_id)));
>>> -  // TODO(mukai): Add icon for the sample app.
>>> +          IDS_MESSAGE_CENTER_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME),
>>> +      desktop_notification_service->IsNotifierEnabled(notifier_id));
>>> +
>>> +  // Add icons for our sending services.
>>> +  // TODO(petewil): Replace this temporary hardcoding with a new sync
>>> datatype
>>> +  // to dynamically get the name and icon for each synced notification
>>> sending
>>> +  // service.  Until then, we use hardcoded service icons for all
>>> services.
>>> +  notifier_service->icon = ui::ResourceBundle::GetSharedInstance().
>>> +      GetImageNamed(IDR_TEMPORARY_GOOGLE_PLUS_ICON);
>>> +
>>> +  notifiers->push_back(notifier_service);
>>>  }
>>>
>>>  void ChromeNotifierService::MarkNotificationAsDismissed(
>>> @@ -311,6 +326,15 @@ void
ChromeNotifierService::Add(scoped_ptr<SyncedNotification>
>>> notification) {
>>>    // Take ownership of the object and put it into our local storage.
>>>    notification_data_.push_back(notification.release());
>>>
>>> +  // If the user is not interested in this type of notification, ignore
>>> it.
>>> +  std::vector<std::string>::iterator iter =
>>> +      find(enabled_sending_services_.begin(),
>>> +           enabled_sending_services_.end(),
>>> +           notification_copy->GetSendingServiceName());
>>> +  if (iter == enabled_sending_services_.end()) {
>>> +    return;
>>> +  }
>>> +
>>>    Display(notification_copy);
>>>  }
>>>
>>> @@ -320,6 +344,7 @@ void ChromeNotifierService::AddForTest(
>>>    }
>>>
>>>  void ChromeNotifierService::Display(SyncedNotification* notification) {
>>> +
>>>    // Set up to fetch the bitmaps.
>>>    notification->QueueBitmapFetchJobs(notification_manager_,
>>>                                            this,
>>> @@ -337,7 +362,24 @@ void
ChromeNotifierService::Display(SyncedNotification*
>>> notification) {
>>>
>>>  void ChromeNotifierService::OnSyncedNotificationServiceEnabled(
>>>      const std::string& notifier_id, bool enabled) {
>>> -  // TODO(petewil): start/stop syncing
>>> +  std::vector<std::string>::iterator iter;
>>> +
>>> +  iter = find(enabled_sending_services_.begin(),
>>> +              enabled_sending_services_.end(),
>>> +              notifier_id);
>>> +
>>> +  // Add the notifier_id if it is enabled and not already there.
>>> +  if (iter == enabled_sending_services_.end() && enabled) {
>>> +    enabled_sending_services_.push_back(notifier_id);
>>> +    // TODO(petewil) Check now for any outstanding notifications.
>>> +  // Remove the notifier_id if it is disabled and present.
>>> +  } else if (iter != enabled_sending_services_.end() && !enabled) {
>>> +    // TODO(petewil): Maybe faster to swap with last element and then
>>> erase it.
>>> +    enabled_sending_services_.erase(iter);
>>> +  }
>>> +
>>> +  // Otherwise, nothing to do, we can exit.
>>> +  return;
>>>  }
>>>
>>>  }  // namespace notifier
>>> Index: chrome/browser/notifications/sync_notifier/chrome_notifier_
>>> service.h
>>> diff --git
a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
>>> b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.h
>>> index b423703af79e1fb7be96344e81a7b02828740c9a..
>>> 13a21f0ec7542502424054b5036143581faa8510 100644
>>> --- a/chrome/browser/notifications/sync_notifier/
>>> chrome_notifier_service.h
>>> +++ b/chrome/browser/notifications/sync_notifier/
>>> chrome_notifier_service.h
>>> @@ -98,6 +98,7 @@ class ChromeNotifierService : public
>>> syncer::SyncableService,
>>>    Profile* const profile_;
>>>    NotificationUIManager* const notification_manager_;
>>>    scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
>>> +  std::vector<std::string> enabled_sending_services_;
>>>    static bool avoid_bitmap_fetching_for_test_;
>>>
>>>    // TODO(petewil): Consider whether a map would better suit our data.
>>> Index: chrome/browser/notifications/sync_notifier/synced_notification.cc
>>> diff --git
a/chrome/browser/notifications/sync_notifier/synced_notification.cc
>>> b/chrome/browser/notifications/sync_notifier/synced_notification.cc
>>> index 851c5d68ac105da38c1426ee8ece1706c6df0fe3..
>>> 21ed5e9cb585b4f13dbcd42bc8b36a17161c71bc 100644
>>> --- a/chrome/browser/notifications/sync_notifier/synced_notification.cc
>>> +++ b/chrome/browser/notifications/sync_notifier/synced_notification.cc
>>> @@ -22,6 +22,12 @@
>>>  namespace {
>>>  const char kExtensionScheme[] = "chrome-extension://";
>>>
>>> +// The name of our first synced notification service.
>>> +// TODO(petewil): remove this hardcoding once we have the synced
>>> notification
>>> +// signalling sync data type set up to provide this.
>>> +const char kFirstSyncedNotificationServiceId[] = "Google+";
>>> +
>>> +
>>>  // Today rich notifications only supports two buttons, make sure we
>>> don't
>>>  // try to supply them with more than this number of buttons.
>>>  const unsigned int kMaxNotificationButtonIndex = 2;
>>> @@ -470,7 +476,9 @@ int SyncedNotification::GetPriority() const {
>>>      return message_center::DEFAULT_PRIORITY;
>>>    } else if (protobuf_priority ==
>>>               sync_pb::CoalescedSyncedNotification_Priority_HIGH) {
>>> -    return message_center::HIGH_PRIORITY;
>>> +    // High priority synced notifications are considered default
>>> priority in
>>> +    // Chrome.
>>> +    return message_center::DEFAULT_PRIORITY;
>>>    } else {
>>>      // Complain if this is a new priority we have not seen before.
>>>      DCHECK(protobuf_priority <
>>> @@ -575,4 +583,12 @@ std::string SyncedNotification::
>>> GetContainedNotificationMessage(
>>>        collapsed_info(index).simple_collapsed_layout().description();
>>>  }
>>>
>>> +std::string SyncedNotification::GetSendingServiceName() const {
>>> +  // TODO(petewil): We are building a new protocol (a new sync
>>> datatype) to send
>>> +  // the service name and icon from the server.  For now this method is
>>> +  // hardcoded to the name of our first service using synced
>>> notifications.
>>> +  // Once the new protocol is built, remove this hardcoding.
>>> +  return kFirstSyncedNotificationServiceId;
>>> +}
>>> +
>>>  }  // namespace notifier
>>> Index: chrome/browser/notifications/sync_notifier/synced_notification.h
>>> diff --git
a/chrome/browser/notifications/sync_notifier/synced_notification.h
>>> b/chrome/browser/notifications/sync_notifier/synced_notification.h
>>> index fd3ee552cec80369f6e3c9d3919cd050bef287c2..
>>> 0bbcb2903ce376d7458a3f3710a02b835e26a5c9 100644
>>> --- a/chrome/browser/notifications/sync_notifier/synced_notification.h
>>> +++ b/chrome/browser/notifications/sync_notifier/synced_notification.h
>>> @@ -71,6 +71,7 @@ class SyncedNotification : public
>>> NotificationBitmapFetcherDelegate {
>>>    size_t GetButtonCount() const;
>>>    std::string GetContainedNotificationTitle(int index) const;
>>>    std::string GetContainedNotificationMessage(int index) const;
>>> +  std::string GetSendingServiceName() const;
>>>
>>>
>>>    bool EqualsIgnoringReadState(const SyncedNotification& other) const;
>>> Index: ui/base/strings/ui_strings.grd
>>> diff --git a/ui/base/strings/ui_strings.grd
>>> b/ui/base/strings/ui_strings.grd
>>> index fd1cac9db281590776d222b0512d33389b1c5312..
>>> 237aad1e451ce5456805e2057898c7c3fb2e351f 100644
>>> --- a/ui/base/strings/ui_strings.grd
>>> +++ b/ui/base/strings/ui_strings.grd
>>> @@ -498,8 +498,8 @@ need to be translated for each locale.-->
>>>        <message name="IDS_MESSAGE_CENTER_TOOLTIP_UNREAD" desc="Tooltip
>>> for notification tray icon with unread notifications">
>>>          <ph name="product">$1<ex>Chrome</ex></ph> - Notifications (<ph
>>> name="QUANTITY">$2<ex>3</ex></ph> unread)
>>>        </message>
>>> -      <message
name="IDS_MESSAGE_CENTER_SAMPLE_SYNCED_NOTIFICATION_SERVICE_NAME"
>>> desc="The name of the only source for synced notifications. TODO and note
>>> for translators: this text must be replaced by the real name by the branch
>>> point.  See crbug.com/254617">
>>> -        Sample app
>>> +      <message
name="IDS_MESSAGE_CENTER_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME"
>>> desc="The name of the first source for synced notifications.">
>>> +        Google+
>>>        </message>
>>>        <message name="IDS_MESSAGE_CENTER_NOTIFIER_SCREENSHOT_NAME"
>>> desc="The name of screenshot notifier that is a system componet">
>>>          Screenshot
>>> Index: ui/resources/default_100_percent/common/ic_gplus_color_16.png
>>> diff --git a/ui/resources/default_100_percent/common/default_favicon.png
>>> b/ui/resources/default_100_percent/common/ic_gplus_color_16.png
>>> similarity index 100%
>>> copy from ui/resources/default_100_percent/common/default_favicon.png
>>> copy to ui/resources/default_100_percent/common/ic_gplus_color_16.png
>>> Index: ui/resources/default_200_percent/common/ic_gplus_color_16.png
>>> diff --git a/ui/resources/default_200_percent/common/default_favicon.png
>>> b/ui/resources/default_200_percent/common/ic_gplus_color_16.png
>>> similarity index 100%
>>> copy from ui/resources/default_200_percent/common/default_favicon.png
>>> copy to ui/resources/default_200_percent/common/ic_gplus_color_16.png
>>> Index: ui/resources/ui_resources.grd
>>> diff --git a/ui/resources/ui_resources.grd b/ui/resources/ui_resources.
>>> grd
>>> index 1f853bd15a08472ba18d9d57e454ae7a5e1df99e..
>>> 4d50648e33a0219d480de5e4971deb67db6ada09 100644
>>> --- a/ui/resources/ui_resources.grd
>>> +++ b/ui/resources/ui_resources.grd
>>> @@ -421,6 +421,7 @@
>>>          <structure type="chrome_scaled_image" name="IDR_RESTORE_H"
>>> file="restore_hover.png" />
>>>          <structure type="chrome_scaled_image" name="IDR_RESTORE_P"
>>> file="restore_pressed.png" />
>>>        </if>
>>> +      <structure type="chrome_scaled_image"
name="IDR_TEMPORARY_GOOGLE_PLUS_ICON"
>>> file="common/ic_gplus_color_16.png" />
>>>        <structure type="chrome_scaled_image"
name="IDR_TEXTBUTTON_HOVER_BOTTOM"
>>> file="common/textbutton_hover_bottom.png" />
>>>        <structure type="chrome_scaled_image"
name="IDR_TEXTBUTTON_HOVER_BOTTOM_LEFT"
>>> file="common/textbutton_hover_bottom_left.png" />
>>>        <structure type="chrome_scaled_image"
name="IDR_TEXTBUTTON_HOVER_BOTTOM_RIGHT"
>>> file="common/textbutton_hover_bottom_right.png" />
>>>
>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.