Send notification to users upon receiving sms messages

Send notification to users upon receiving sms messages

After receiving a sms message, instead of creating a notification view bubble, 
a notification in notification center containing the sms message is sent to user. 

Remove notification view implementation because it is only used to display 
sms messages.

This CL is a split from https://codereview.chromium.org/2583393002/. Please
Refer to it for some discussion details. 

BUG=630641
TEST=system_tray_unittest

Review-Url: https://codereview.chromium.org/2690543003
Cr-Commit-Position: refs/heads/master@{#450875}
Committed: https://chromium.googlesource.com/chromium/src/+/90cfee2509916bf517552bd6f69acb9d5086aeaf

[yiyix, 2017-02-10 07:05:34]

Description was changed from

==========
update notification


update notification

BUG=
==========

to

==========
Send notification to users upon receiving sms messages

After receiving a sms message, instead of creating a notification view bubble, 
a notification in notification center containing the sms message is sent to
user. 

Remove notification view implementation because it is only used to display 
sms messages.

BUG=630641
TEST=system_tray_unittest
==========

[yiyix, 2017-02-10 07:05:52]

The CQ bit was checked by yiyix@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 07:06:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yiyix, 2017-02-10 07:08:38]

Description was changed from

==========
Send notification to users upon receiving sms messages

After receiving a sms message, instead of creating a notification view bubble, 
a notification in notification center containing the sms message is sent to
user. 

Remove notification view implementation because it is only used to display 
sms messages.

BUG=630641
TEST=system_tray_unittest
==========

to

==========
Send notification to users upon receiving sms messages

After receiving a sms message, instead of creating a notification view bubble, 
a notification in notification center containing the sms message is sent to
user. 

Remove notification view implementation because it is only used to display 
sms messages.

This CL is a split from https://codereview.chromium.org/2583393002/. Please
Refer to it for some discussion details. 

BUG=630641
TEST=system_tray_unittest
==========

[commit-bot: I haz the power, 2017-02-10 08:12:59]

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

[commit-bot: I haz the power, 2017-02-10 08:13:00]

Dry run: This issue passed the CQ dry run.

[yiyix, 2017-02-10 15:59:17]

yiyix@chromium.org changed reviewers:
+ jamescook@chromium.org, tdanderson@chromium.org

[yiyix, 2017-02-10 15:59:18]

This patch contains the logic to send sms messages to notification center only.
Removing notification view will be submitted after this patch. Please refer to
https://codereview.chromium.org/2583393002/ for discussion details. 

@tdanderson & @jamescook, could you please review this patch. 

@tdanderson, thank you for reviewing my original patch.

[yiyix, 2017-02-10 15:59:45]

yiyix@chromium.org changed reviewers:
+ stevenjb@chromium.org

[yiyix, 2017-02-10 16:00:42]

On 2017/02/10 15:59:18, yiyix wrote:
> This patch contains the logic to send sms messages to notification center
only.
> Removing notification view will be submitted after this patch. Please refer to
> https://codereview.chromium.org/2583393002/ for discussion details. 
> 
> @tdanderson & @jamescook, could you please review this patch. 
> 
> @tdanderson, thank you for reviewing my original patch.

@stevenjb, Thank you for review sms_observer classes in my other patch. I have
addressed your comments in this patch.

[stevenjb, 2017-02-10 17:15:23]

Thanks for simplifying this CL! LGTM w/ one request.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/sms_observer.cc (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:71: NET_LOG_ERROR("SMS
message contains no content.", "");
While we are moving this code, could you change these to the newer, preferred
format, e.g.:

NET_LOG(ERROR) << "SMS message contains no content.";

Thanks!

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:90: "");
This would be:

NTE_LOG(DEBUG) << "Received SMS from: " << message_number << " with text: " <<
message_text;

[James Cook, 2017-02-10 17:36:38]

My biggest question is where SmsObserver should live. ShelfWidget is mostly a
view class (in the MVC sense) and it seems like the observer is more a
data-model thing.

I like how you're using composition instead of directly subclassing the
NetworkSmsObserver, tho.

https://codereview.chromium.org/2690543003/diff/1/ash/ash_chromeos_strings.grdp
File ash/ash_chromeos_strings.grdp (left):

https://codereview.chromium.org/2690543003/diff/1/ash/ash_chromeos_strings.gr...
ash/ash_chromeos_strings.grdp:276: </message>
Thanks for remembering to clean up the strings and resources!

https://codereview.chromium.org/2690543003/diff/1/ash/common/shelf/shelf_widg...
File ash/common/shelf/shelf_widget.h (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/shelf/shelf_widg...
ash/common/shelf/shelf_widget.h:128: std::unique_ptr<SmsObserver> sms_observer_;
Why does SmsObserver live here? It seems like some class in the notification
system should subclass SmsObserver or should own an SmsObserver.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/sms_observer.cc (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:4: 
#include sms_observer.h here

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:7: #include
"ash/common/material_design/material_design_controller.h"
Is this needed?

Are there other unused includes below?

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:31: void SendMessage(const
base::DictionaryValue* message,
put utility functions in an anonymous namespace

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:32: std::string message_text,
const std::string& ?

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:36:
std::unique_ptr<message_center::Notification> notification;
instead of reset() below, just do notification =
base::MakeUnique<Notification>(params...) here

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:48:
message_center::MessageCenter::Get()->AddNotification(
optional: I'm OK with "using message_center::Foo" and "using
chromeos::NetworkHandler" above if it makes line wrapping better

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:92: SendMessage(&message,
message_text, message_number, message_id_);
SendMessage() would be better if it was called ShowMessage() or
ShowNotification()

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/sms_observer.h (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.h:10: #include <string>
not needed?

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.h:11: #include
"ash/common/system/tray/system_tray_item.h"
not needed?

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.h:30: uint32_t message_id_;
nit: initialize in header (in general, if all members can be initialized in the
header I prefer to do it there)

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/sms_observer_unittest.cc (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017 unless this is mostly a move

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:5: #include
"ash/common/system/chromeos/network/sms_observer.h"
nit: blank line below

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:34:
std::unique_ptr<base::DictionaryValue> CreateMessage(
put in anonymous namespace

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:35: const char*
kDefaultNumber,
default_number (and below)

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:38:
std::unique_ptr<base::DictionaryValue> sms(new base::DictionaryValue());
prefer MakeUnique

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:56:
sms_observer->MessageReceived(*CreateMessage("000-000-0000",
nit: put result of CreateMessage in a local var. It's hard to reason about
memory management if the unique_ptr is a temporary

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:64:
EXPECT_EQ((*notifications.begin())->title(),
nit: revert EXPECT_EQ parameters so expected value is first (and below)

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:72:
message_center::MessageCenter::Get()->GetVisibleNotifications().size());
optional: using message_center::MessageCenter if it makes wrapping better

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/system_no...
File ash/common/system/system_notifier.cc (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/system_no...
ash/common/system/system_notifier.cc:23: kNotifierPower, kNotifierSms,
Steven, do we want SMS to show at the login screen? The lock screen? It kinda
seems privacy-sensitive.

[stevenjb, 2017-02-10 17:48:26]

https://codereview.chromium.org/2690543003/diff/1/ash/common/shelf/shelf_widg...
File ash/common/shelf/shelf_widget.h (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/shelf/shelf_widg...
ash/common/shelf/shelf_widget.h:128: std::unique_ptr<SmsObserver> sms_observer_;
On 2017/02/10 17:36:37, James Cook wrote:
> Why does SmsObserver live here? It seems like some class in the notification
> system should subclass SmsObserver or should own an SmsObserver.

That's a good point. A more natural place for the to live would be:

chrome/browser/chromeos/net/

Alongside network_state_notifier.cc

Which is owned by NetworkConnectDelegateChromeOS which is owned by DBusServices
in chrome_browser_main_chromeos.cc.

Long term all of that may need to move to Ash, but for now I think it makes
sense for this to be owned by chrome_browser_main_chromeos.cc and instantiated
alongside NetworkConnectDelegateChromeOS.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/system_no...
File ash/common/system/system_notifier.cc (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/system_no...
ash/common/system/system_notifier.cc:23: kNotifierPower, kNotifierSms,
On 2017/02/10 17:36:38, James Cook wrote:
> Steven, do we want SMS to show at the login screen? The lock screen? It kinda
> seems privacy-sensitive.

Good catch. No, we do not.

[James Cook, 2017-02-10 17:51:44]

Are you sure it should go in chrome/browser? That will complicate mustash. Maybe
just hang it off ash/shell ?

[stevenjb, 2017-02-10 18:26:08]

As I mentioned, much of the code in chrome/browser/chromeos/net/ is going
to also need to move at some point. Right now as far as I am aware we don't
have a good home for any of it, so it seems like keeping it all together
until we do makes it more likely that it will end up in the same place
eventually, which would be ideal.

Specifically, I am referring to code that observers network state and
spawns message center notifications; both NetworkStateNotifier and
NetworkPortalNotifcation do something along those lines).

If you have a suggestion for a new home (and I can't think of anyone else
who might :) ) that would also be fine by me. Hopefully it will be the
obvious location when we move the other classes :)




On Fri, Feb 10, 2017 at 9:51 AM, <jamescook@chromium.org> wrote:

> Are you sure it should go in chrome/browser? That will complicate mustash.
> Maybe
> just hang it off ash/shell ?
>
>
> https://codereview.chromium.org/2690543003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[James Cook, 2017-02-10 18:28:34]

If it doesn't have chrome/browser dependencies I would leave the file itself
where it is right now and hang the object off of ash::Shell.

[stevenjb, 2017-02-10 18:41:10]

On 2017/02/10 18:28:34, James Cook wrote:
> If it doesn't have chrome/browser dependencies I would leave the file itself
> where it is right now and hang the object off of ash::Shell.

sftm

[yiyix, 2017-02-13 23:34:01]

The sms_observer is now initialized in ash/shell. I think it is a better home
for this observer as well. 

Sorry, I did many different trials, i forget to clean up the includes list. 

@jamescook and stevenjb, could you please take another look? Thank you very much

https://codereview.chromium.org/2690543003/diff/1/ash/ash_chromeos_strings.grdp
File ash/ash_chromeos_strings.grdp (left):

https://codereview.chromium.org/2690543003/diff/1/ash/ash_chromeos_strings.gr...
ash/ash_chromeos_strings.grdp:276: </message>
On 2017/02/10 17:36:37, James Cook wrote:
> Thanks for remembering to clean up the strings and resources!

:)

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/sms_observer.cc (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:4: 
On 2017/02/10 17:36:38, James Cook wrote:
> #include sms_observer.h here

So sorry, I missed this obvious include.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:7: #include
"ash/common/material_design/material_design_controller.h"
On 2017/02/10 17:36:37, James Cook wrote:
> Is this needed?
> 
> Are there other unused includes below?

Sorry, after addressing comments and experiments, i forget to remove the
includes.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:31: void SendMessage(const
base::DictionaryValue* message,
On 2017/02/10 17:36:38, James Cook wrote:
> put utility functions in an anonymous namespace

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:32: std::string message_text,
On 2017/02/10 17:36:37, James Cook wrote:
> const std::string& ?

You are right, constant makes more sense.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:36:
std::unique_ptr<message_center::Notification> notification;
On 2017/02/10 17:36:38, James Cook wrote:
> instead of reset() below, just do notification =
> base::MakeUnique<Notification>(params...) here

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:48:
message_center::MessageCenter::Get()->AddNotification(
On 2017/02/10 17:36:38, James Cook wrote:
> optional: I'm OK with "using message_center::Foo" and "using
> chromeos::NetworkHandler" above if it makes line wrapping better

As we have discussed offline, i will use NetworkHandler instead of
chromeos::NetworkHandler.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:71: NET_LOG_ERROR("SMS
message contains no content.", "");
On 2017/02/10 17:15:23, stevenjb wrote:
> While we are moving this code, could you change these to the newer, preferred
> format, e.g.:
> 
> NET_LOG(ERROR) << "SMS message contains no content.";
> 
> Thanks!

I can update it as a part of this cl for sure.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:90: "");
On 2017/02/10 17:15:23, stevenjb wrote:
> This would be:
> 
> NTE_LOG(DEBUG) << "Received SMS from: " << message_number << " with text: " <<
> message_text;

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.cc:92: SendMessage(&message,
message_text, message_number, message_id_);
On 2017/02/10 17:36:37, James Cook wrote:
> SendMessage() would be better if it was called ShowMessage() or
> ShowNotification()

I will use ShowNotification(), so it's clear that it's will be in the
notification center.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/sms_observer.h (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.h:10: #include <string>
On 2017/02/10 17:36:38, James Cook wrote:
> not needed?

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer.h:11: #include
"ash/common/system/tray/system_tray_item.h"
On 2017/02/10 17:36:38, James Cook wrote:
> not needed?

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/sms_observer_unittest.cc (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/02/10 17:36:38, James Cook wrote:
> 2017 unless this is mostly a move

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:5: #include
"ash/common/system/chromeos/network/sms_observer.h"
On 2017/02/10 17:36:38, James Cook wrote:
> nit: blank line below

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:34:
std::unique_ptr<base::DictionaryValue> CreateMessage(
On 2017/02/10 17:36:38, James Cook wrote:
> put in anonymous namespace

Done. Is it true that unless it's class specific helper function and mentioned
in the .h file, we always want to include it in the anonymous namespace?

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:35: const char*
kDefaultNumber,
On 2017/02/10 17:36:38, James Cook wrote:
> default_number (and below)

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:38:
std::unique_ptr<base::DictionaryValue> sms(new base::DictionaryValue());
On 2017/02/10 17:36:38, James Cook wrote:
> prefer MakeUnique

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:56:
sms_observer->MessageReceived(*CreateMessage("000-000-0000",
On 2017/02/10 17:36:38, James Cook wrote:
> nit: put result of CreateMessage in a local var. It's hard to reason about
> memory management if the unique_ptr is a temporary

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:64:
EXPECT_EQ((*notifications.begin())->title(),
On 2017/02/10 17:36:38, James Cook wrote:
> nit: revert EXPECT_EQ parameters so expected value is first (and below)

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:72:
message_center::MessageCenter::Get()->GetVisibleNotifications().size());
On 2017/02/10 17:36:38, James Cook wrote:
> optional: using message_center::MessageCenter if it makes wrapping better

Done.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/system_no...
File ash/common/system/system_notifier.cc (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/system_no...
ash/common/system/system_notifier.cc:23: kNotifierPower, kNotifierSms,
On 2017/02/10 17:48:26, stevenjb wrote:
> On 2017/02/10 17:36:38, James Cook wrote:
> > Steven, do we want SMS to show at the login screen? The lock screen? It
kinda
> > seems privacy-sensitive.
> 
> Good catch. No, we do not.

Good catch! Thanks

[stevenjb, 2017-02-14 00:47:21]

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/sms_observer.cc (right):

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer.cc:37:
message_center::MessageCenter::Get()->AddNotification(
Now that this is owned by ash::Shell, I don't think it can assume that
message_center::MessageCenter::Get() is not null on startup or shutdown, we need
to test that it is valid.

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/sms_observer.h (right):

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer.h:11: #include "base/values.h"
Whouldn't need this either, DictionaryValue can be forward declared:

namespace base {
class DictionaryValue;
}

[James Cook, 2017-02-14 02:04:38]

Code looks fine, just nits.

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/sms_observer_unittest.cc (right):

https://codereview.chromium.org/2690543003/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/sms_observer_unittest.cc:34:
std::unique_ptr<base::DictionaryValue> CreateMessage(
On 2017/02/13 23:34:00, yiyix wrote:
> On 2017/02/10 17:36:38, James Cook wrote:
> > put in anonymous namespace
> 
> Done. Is it true that unless it's class specific helper function and mentioned
> in the .h file, we always want to include it in the anonymous namespace?

Yes. Unless you need to access it from outside the .cc file, or it's part of the
class, it should be in an anonymous namespace. This helps prevent polluting the
global namespace and it also makes things a little bit easier on the linker (it
doesn't have as many symbols to resolve).

https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Stat...

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/sms_observer.cc (right):

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer.cc:4: #include
"ash/common/system/chromeos/network/sms_observer.h"
nit: blank line above

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer.cc:20: void
ShowNotification(const base::DictionaryValue* message,
put in an anonymous namespace

What I usually do is:

namespace ash {
namespace {

void UtilityFunction() {
 ...
}

}  // namespace

... class...

}  // namespace ash

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/sms_observer_unittest.cc (right):

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer_unittest.cc:22:
std::unique_ptr<base::DictionaryValue> CreateMessage(
anonymous namespace again

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer_unittest.cc:38: namespace test {
super nit: Don't use this namespace. "namespace test" is for test helpers, not
for the tests themselves. Basically, it's for things that live in src/ash/test.

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer_unittest.cc:47: return
shell->sms_observer_.get();
nit: just inline Shell::GetInstance()->sms_observer_.get()

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_tray.cc (left):

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray.cc:295:
AddTrayItem(base::MakeUnique<TraySms>(this));
Hooray that this is going away!

https://codereview.chromium.org/2690543003/diff/20001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2690543003/diff/20001/ash/shell.h#newcode532
ash/shell.h:532: // Listens for new sms messages and display.
nit: "displays them" or "shows notifications".

[yiyix, 2017-02-15 19:41:39]

@jamescook and stevenjb: Could you please take another look at this change?

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/sms_observer.cc (right):

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer.cc:4: #include
"ash/common/system/chromeos/network/sms_observer.h"
On 2017/02/14 02:04:38, James Cook wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer.cc:20: void
ShowNotification(const base::DictionaryValue* message,
On 2017/02/14 02:04:38, James Cook wrote:
> put in an anonymous namespace
> 
> What I usually do is:
> 
> namespace ash {
> namespace {
> 
> void UtilityFunction() {
>  ...
> }
> 
> }  // namespace
> 
> ... class...
> 
> }  // namespace ash

Thanks. I will adapt to this form.

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer.cc:37:
message_center::MessageCenter::Get()->AddNotification(
On 2017/02/14 00:47:21, stevenjb wrote:
> Now that this is owned by ash::Shell, I don't think it can assume that
> message_center::MessageCenter::Get() is not null on startup or shutdown, we
need
> to test that it is valid.

would be a good check. Thanks.

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/sms_observer.h (right):

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer.h:11: #include "base/values.h"
On 2017/02/14 00:47:21, stevenjb wrote:
> Whouldn't need this either, DictionaryValue can be forward declared:
> 
> namespace base {
> class DictionaryValue;
> }

Done.

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/sms_observer_unittest.cc (right):

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer_unittest.cc:22:
std::unique_ptr<base::DictionaryValue> CreateMessage(
On 2017/02/14 02:04:38, James Cook wrote:
> anonymous namespace again

Done.

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer_unittest.cc:38: namespace test {
On 2017/02/14 02:04:38, James Cook wrote:
> super nit: Don't use this namespace. "namespace test" is for test helpers, not
> for the tests themselves. Basically, it's for things that live in
src/ash/test.

Done.

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer_unittest.cc:47: return
shell->sms_observer_.get();
On 2017/02/14 02:04:38, James Cook wrote:
> nit: just inline Shell::GetInstance()->sms_observer_.get()

sorry, i was experimenting different ways to get shell forget to update it.

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_tray.cc (left):

https://codereview.chromium.org/2690543003/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray.cc:295:
AddTrayItem(base::MakeUnique<TraySms>(this));
On 2017/02/14 02:04:38, James Cook wrote:
> Hooray that this is going away!

:)

https://codereview.chromium.org/2690543003/diff/20001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2690543003/diff/20001/ash/shell.h#newcode532
ash/shell.h:532: // Listens for new sms messages and display.
On 2017/02/14 02:04:38, James Cook wrote:
> nit: "displays them" or "shows notifications".

Done.

[James Cook, 2017-02-15 19:49:48]

LGTM. Nice patch.

https://codereview.chromium.org/2690543003/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/sms_observer.cc (right):

https://codereview.chromium.org/2690543003/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/sms_observer.cc:21: namespace {
nit: blank line below

[yiyix, 2017-02-16 02:40:44]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2017-02-16 02:40:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2690543003/#ps60001
(title: "fix nits")

[commit-bot: I haz the power, 2017-02-16 02:42:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 05:38:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487212844841140,
"parent_rev": "127b436b7ab9c1043923e1e6c1a3a57a202908b3", "commit_rev":
"90cfee2509916bf517552bd6f69acb9d5086aeaf"}

[commit-bot: I haz the power, 2017-02-16 05:39:30]

Description was changed from

==========
Send notification to users upon receiving sms messages

After receiving a sms message, instead of creating a notification view bubble, 
a notification in notification center containing the sms message is sent to
user. 

Remove notification view implementation because it is only used to display 
sms messages.

This CL is a split from https://codereview.chromium.org/2583393002/. Please
Refer to it for some discussion details. 

BUG=630641
TEST=system_tray_unittest
==========

to

==========
Send notification to users upon receiving sms messages

After receiving a sms message, instead of creating a notification view bubble, 
a notification in notification center containing the sms message is sent to
user. 

Remove notification view implementation because it is only used to display 
sms messages.

This CL is a split from https://codereview.chromium.org/2583393002/. Please
Refer to it for some discussion details. 

BUG=630641
TEST=system_tray_unittest

Review-Url: https://codereview.chromium.org/2690543003
Cr-Commit-Position: refs/heads/master@{#450875}
Committed:
https://chromium.googlesource.com/chromium/src/+/90cfee2509916bf517552bd6f69a...
==========

[commit-bot: I haz the power, 2017-02-16 05:39:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/90cfee2509916bf517552bd6f69a...

[Evan Stade, 2017-02-16 19:21:12]

Hi Yi,

happy this landed! we should be able to delete a lot more NotificationView code
now because I think SMS was the only user of that. Is this on your radar?

https://cs.chromium.org/chromium/src/ash/common/system/tray/system_tray.cc?rc...

[stevenjb, 2017-02-16 19:31:16]

Indeed! I would love to see that code go away!

crbug.com/685845 is tracking this.



On Thu, Feb 16, 2017 at 11:21 AM, <estade@chromium.org> wrote:

> Hi Yi,
>
> happy this landed! we should be able to delete a lot more NotificationView
> code
> now because I think SMS was the only user of that. Is this on your radar?
>
> https://cs.chromium.org/chromium/src/ash/common/
> system/tray/system_tray.cc?rcl=7b23d4a6d90df4d29fe04165c07da7
> ff7d655e0e&l=614
>
> https://codereview.chromium.org/2690543003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yiyix, 2017-02-16 21:41:13]

On 2017/02/16 19:21:12, Evan Stade wrote:
> Hi Yi,
> 
> happy this landed! we should be able to delete a lot more NotificationView
code
> now because I think SMS was the only user of that. Is this on your radar?
> 
>
https://cs.chromium.org/chromium/src/ash/common/system/tray/system_tray.cc?rc...

Hi, in my previous CL, I included the deletion of the notification view and this
CL https://codereview.chromium.org/2583393002/, however the cl became too
large..... 

I will be updating the second part of that CL soon.

[yiyix, 2017-02-16 21:41:37]

On 2017/02/16 19:31:16, stevenjb wrote:
> Indeed! I would love to see that code go away!
> 
> crbug.com/685845 is tracking this.
> 
> 
> 
> On Thu, Feb 16, 2017 at 11:21 AM, <mailto:estade@chromium.org> wrote:
> 
> > Hi Yi,
> >
> > happy this landed! we should be able to delete a lot more NotificationView
> > code
> > now because I think SMS was the only user of that. Is this on your radar?
> >
> > https://cs.chromium.org/chromium/src/ash/common/
> > system/tray/system_tray.cc?rcl=7b23d4a6d90df4d29fe04165c07da7
> > ff7d655e0e&l=614
> >
> > https://codereview.chromium.org/2690543003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Could I assign the owner to me?

[stevenjb, 2017-02-16 23:05:10]

sgtm!

On Thu, Feb 16, 2017 at 1:41 PM, <yiyix@chromium.org> wrote:

> On 2017/02/16 19:31:16, stevenjb wrote:
> > Indeed! I would love to see that code go away!
> >
> > crbug.com/685845 is tracking this.
> >
> >
> >
> > On Thu, Feb 16, 2017 at 11:21 AM, <mailto:estade@chromium.org> wrote:
> >
> > > Hi Yi,
> > >
> > > happy this landed! we should be able to delete a lot more
> NotificationView
> > > code
> > > now because I think SMS was the only user of that. Is this on your
> radar?
> > >
> > > https://cs.chromium.org/chromium/src/ash/common/
> > > system/tray/system_tray.cc?rcl=7b23d4a6d90df4d29fe04165c07da7
> > > ff7d655e0e&l=614
> > >
> > > https://codereview.chromium.org/2690543003/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> Could I assign the owner to me?
>
> https://codereview.chromium.org/2690543003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2017-02-17 21:23:47]

thanks.


-- Evan Stade

On Thu, Feb 16, 2017 at 4:05 PM, Steven Bennetts <stevenjb@chromium.org>
wrote:

> sgtm!
>
> On Thu, Feb 16, 2017 at 1:41 PM, <yiyix@chromium.org> wrote:
>
>> On 2017/02/16 19:31:16, stevenjb wrote:
>> > Indeed! I would love to see that code go away!
>> >
>> > crbug.com/685845 is tracking this.
>> >
>> >
>> >
>> > On Thu, Feb 16, 2017 at 11:21 AM, <mailto:estade@chromium.org> wrote:
>> >
>> > > Hi Yi,
>> > >
>> > > happy this landed! we should be able to delete a lot more
>> NotificationView
>> > > code
>> > > now because I think SMS was the only user of that. Is this on your
>> radar?
>> > >
>> > > https://cs.chromium.org/chromium/src/ash/common/
>> > > system/tray/system_tray.cc?rcl=7b23d4a6d90df4d29fe04165c07da7
>> > > ff7d655e0e&l=614
>> > >
>> > > https://codereview.chromium.org/2690543003/
>> > >
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>> Groups
>> > "Chromium-reviews" group.
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>> Could I assign the owner to me?
>>
>> https://codereview.chromium.org/2690543003/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.