Send notification to users upon receiving sms messages

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

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+

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

#include sms_observer.h here

[yiyix, 2017/02/13 23:34:00]

So sorry, I missed this obvious include.

+#include <utility>
+
+#include "ash/common/material_design/material_design_controller.h"

[James Cook, 2017/02/10 17:36:37]

Is this needed?

Are there other unused includes below?

[yiyix, 2017/02/13 23:34:00]

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

+#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 {
+
+// Send the |message| to notification center to display to users. Note that each
+// notification will be assigned with different |message_id| as notification id.
+void SendMessage(const base::DictionaryValue* message,

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

put utility functions in an anonymous namespace

[yiyix, 2017/02/13 23:34:00]

Done.

+                 std::string message_text,

[James Cook, 2017/02/10 17:36:37]

const std::string& ?

[yiyix, 2017/02/13 23:34:00]

You are right, constant makes more sense.

+                 std::string message_number,
+                 int message_id) {
+  const char kNotificationId[] = "chrome://network/sms";
+  std::unique_ptr<message_center::Notification> notification;

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

instead of reset() below, just do notification =
base::MakeUnique<Notification>(params...) here

[yiyix, 2017/02/13 23:34:00]

Done.

+
+  notification.reset(new message_center::Notification(
+      message_center::NOTIFICATION_TYPE_SIMPLE,
+      kNotificationId + std::to_string(message_id),
+      base::ASCIIToUTF16(message_number), base::ASCIIToUTF16(message_text),
+      gfx::Image(
+          CreateVectorIcon(kSystemMenuSmsIcon, kMenuIconSize, kMenuIconColor)),
+      base::string16(), GURL(),
+      message_center::NotifierId(message_center::NotifierId::APPLICATION,
+                                 system_notifier::kNotifierSms),
+      message_center::RichNotificationData(), nullptr));
+  message_center::MessageCenter::Get()->AddNotification(

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

optional: I'm OK with "using message_center::Foo" and "using
chromeos::NetworkHandler" above if it makes line wrapping better

[yiyix, 2017/02/13 23:34:00]

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

+      std::move(notification));
+}
+
+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() {
+  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.", "");

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

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!

[yiyix, 2017/02/13 23:34:00]

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

+    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,
+      "");

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

This would be:

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

[yiyix, 2017/02/13 23:34:00]

Done.

+  message_id_++;
+  SendMessage(&message, message_text, message_number, message_id_);

[James Cook, 2017/02/10 17:36:37]

SendMessage() would be better if it was called ShowMessage() or
ShowNotification()

[yiyix, 2017/02/13 23:34:00]

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

+}
+
+}  // namespace ash