Send notification to users upon receiving sms messages

ash/common/system/chromeos/network/sms_observer.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be

[tdanderson, 2017/01/26 22:19:06]

nit: update header - no (c) and update year.

[yiyix, 2017/02/02 20:43:56]

Done.

+// found in the LICENSE file.
+
+#include <utility>
+
+#include "ash/common/material_design/material_design_controller.h"
+#include "ash/common/metrics/user_metrics_action.h"
+#include "ash/common/system/chromeos/network/sms_observer.h"
+#include "ash/common/system/system_notifier.h"
+#include "ash/common/system/tray/tray_constants.h"
+#include "ash/common/wm_shell.h"
+#include "ash/resources/vector_icons/vector_icons.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chromeos/network/network_event_log.h"
+#include "chromeos/network/network_handler.h"
+#include "grit/ash_resources.h"
+#include "grit/ash_strings.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/paint_vector_icon.h"
+#include "ui/message_center/message_center.h"
+#include "ui/views/view.h"
+
+namespace ash {
+
+const char SmsObserver::kNotificationId[] = "chrome://network/sms";
+
+SmsObserver::SmsObserver() : message_id_(0) {
+  // TODO(armansito): SMS could be a special case for cellular that requires a
+  // user (perhaps the owner) to be logged in. If that is the case, then an
+  // additional check should be done before subscribing for SMS notifications.
+  if (chromeos::NetworkHandler::IsInitialized())
+    chromeos::NetworkHandler::Get()->network_sms_handler()->AddObserver(this);
+}
+
+SmsObserver::~SmsObserver() {}
+
+void SmsObserver::Shutdown() {
+  if (chromeos::NetworkHandler::IsInitialized()) {
+    chromeos::NetworkHandler::Get()->network_sms_handler()->RemoveObserver(
+        this);
+  }
+}
+
+void SmsObserver::MessageReceived(const base::DictionaryValue& message) {
+  std::string message_text;
+  if (!message.GetStringWithoutPathExpansion(
+          chromeos::NetworkSmsHandler::kTextKey, &message_text)) {
+    NET_LOG_ERROR("SMS message contains no content.", "");
+    return;
+  }
+  // TODO(armansito): A message might be due to a special "Message Waiting"
+  // state that the message is in. Once SMS handling moves to shill, such
+  // messages should be filtered there so that this check becomes unnecessary.
+  if (message_text.empty()) {
+    NET_LOG_DEBUG("SMS has empty content text. Ignoring.", "");
+    return;
+  }
+  std::string message_number;
+  if (!message.GetStringWithoutPathExpansion(
+          chromeos::NetworkSmsHandler::kNumberKey, &message_number)) {
+    NET_LOG_DEBUG("SMS contains no number. Ignoring.", "");
+    return;
+  }
+
+  NET_LOG_DEBUG(
+      "Received SMS from: " + message_number + " with text: " + message_text,
+      "");
+  message_id_++;

[tdanderson, 2017/01/26 22:19:06]

This member is initialized to 0 when SmsObserver is created, and then you're
incrementing by one each time a message is sent. What happens if you send an SMS
to the notification center with ID of 0, SmsObserver is destroyed, SmsObserver
is re-instantiated, and then you send a second SMS to the notification center,
also with an ID of 0?

After a quick search through the codebase I can't find any pattern of generating
notification IDs using a counter in this way; do you know of any? It seems like
what you might want instead is to generate a GUID like in
extension_welcome_notification.cc ?

[yiyix, 2017/02/02 20:43:56]

It is how download notification is generated.
https://cs.chromium.org/chromium/src/chrome/browser/download/notification/dow...
tag is referring to the notification id in our case. To display all downloading
item, tag is a strictly increasing uint32_t and initialized at 0
(https://cs.chromium.org/chromium/src/content/browser/download/download_item_i...).
To create a different set of id for SmsObserver, i set the notificationid to
chrome://network/sms + number.

If SmsObserver is destroyed, then ShelfWidget is destroyed. All notification
will be closed. Creating another notification with id=chrome://network/sms0 will
not override the previous sms notification, so i don't think it will be a
problem.

[tdanderson, 2017/02/07 00:15:45]

Can you double check that you linked to the right code? Following from the links
you've given it looks as though each download item gets a unique ID provided by
the underlying DownloadItem object. I don't see anywhere that keeps track of its
own counter and increments by 1 each time a notification is sent.
Perhaps the views are destroyed, but I'm not sure that the underlying
message_center model is destroyed. In other words, I think there may be the
possibility of dispatching an SMS notification with a duplicate ID.

Anyhow, I would feel much better if you could ask a message_center owner (such
as stevenjb@) to review this portion of the CL once all other comments have been
addressed.

[yiyix, 2017/02/07 22:03:21]

As we have discussed offline, The logic where it increases the download id is
here:
https://cs.chromium.org/chromium/src/chrome/browser/download/chrome_download_...

As we have agreed, after addressing all comments from this cl, i will ask
@stevenjb to review this particular class.

+  Update(&message, message_text, message_number);
+}
+
+void SmsObserver::Update(const base::DictionaryValue* message,
+                         std::string message_text,
+                         std::string message_number) {
+  std::unique_ptr<message_center::Notification> notification;

[tdanderson, 2017/01/26 22:19:06]

I don't think you need to declare this so early, I suggest moving it to just
above line 92

[yiyix, 2017/02/02 20:43:56]

Sure. Declare the variable before using it. I was thinking that there is no
crash/early return between here and l92, so it will get initialized anyways.

[tdanderson, 2017/02/07 00:15:45]

Thanks.

+  gfx::Image* image;
+  if (MaterialDesignController::UseMaterialDesignSystemIcons()) {
+    image = new gfx::Image(
+        CreateVectorIcon(kSystemMenuSmsIcon, kMenuIconSize, kMenuIconColor));
+  } else {

[tdanderson, 2017/01/26 23:12:50]

Also, you can land this with just lines 82-83. We haven't been bothering to
update the non-md code paths for anything 'new'. I think line 87 is the only
place that would have still used IDR_AURA_UBER_TRAY_SMS, so you should remove
that asset as well.

[yiyix, 2017/02/02 20:43:56]

Yeah~ Finally, we can land code with md path only! Thanks.

[tdanderson, 2017/02/07 00:15:45]

:)

+    image = new gfx::Image(
+        *ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
+            IDR_AURA_UBER_TRAY_SMS));
+  }
+
+  std::string notification_id = kNotificationId + std::to_string(message_id_);

[tdanderson, 2017/01/26 22:19:06]

Looks like you don't need this as a named local variable, so just inline this on
line 93

[yiyix, 2017/02/02 20:43:56]

I want to keep it as a class variable so other resources can use this
notification id to filter sms notifications in the future. 

[tdanderson, 2017/02/07 00:15:45]

I meant that you don't need |notification_id| as a named variable. I was
suggesting that you just put 'kNotificationId + std::to_string(message_id_)' in
place of 'notification_id' on line 93.

[yiyix, 2017/02/07 22:03:21]

Done.

+  LOG(ERROR) << notification_id;

[tdanderson, 2017/01/26 22:19:06]

Left over from debugging?

[yiyix, 2017/02/02 20:43:56]

sorry, i missed this line.

[tdanderson, 2017/02/07 00:15:45]

Acknowledged.

+  notification.reset(new message_center::Notification(
+      message_center::NOTIFICATION_TYPE_SIMPLE, notification_id,
+      base::ASCIIToUTF16(message_number), base::ASCIIToUTF16(message_text),
+      *image, base::string16(), GURL(),
+      message_center::NotifierId(message_center::NotifierId::APPLICATION,
+                                 system_notifier::kNotifierSms),
+      message_center::RichNotificationData(), nullptr));

[tdanderson, 2017/01/26 22:19:06]

Has this CL received approval from UX in terms of the notification type used,
behavior, how the notifications remain in the notification center until
explicitly closed, how multiple message from the same number are handled, etc?

[yiyix, 2017/02/02 20:43:56]

Nope, i only talked with sebastien on hangout. how could i get approval from ux?
Do you know their ldaps?

[tdanderson, 2017/02/07 00:15:45]

Approval from Sebastien is enough in this case, thanks.

[yiyix, 2017/02/07 22:03:21]

I will ask him to approve it.

+  message_center::MessageCenter::Get()->AddNotification(
+      std::move(notification));
+}
+
+}  // namespace ash