Send notification to users upon receiving sms messages

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

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

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

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

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_SMS_OBSERVER_H_
+#define ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_SMS_OBSERVER_H_
+
+#include <stddef.h>
+

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

nit: no blank line

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

Done.

+#include <string>
+
+#include "ash/common/system/tray/system_tray_item.h"
+#include "base/macros.h"
+#include "base/values.h"
+#include "chromeos/network/network_sms_handler.h"
+
+namespace ash {
+
+class SmsObserver : public chromeos::NetworkSmsHandler::Observer {

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

Please include some class-level documentation here.

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

Done.

+ public:
+  SmsObserver();
+  ~SmsObserver() override;
+
+  void Shutdown();
+
+  // chromeos::NetworkSmsHandler::Observer:
+  void MessageReceived(const base::DictionaryValue& message) override;
+
+ protected:
+  // After receiving a text message with valid number and non-empty content,
+  // send the |message| to notification center to display to users.
+  void Update(const base::DictionaryValue* message,

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

Why did you choose to make this protected rather than private?

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

Done.

+              std::string message_text,
+              std::string message_number);
+
+ private:
+  base::ListValue messages_;

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

Please include a brief description for each member.

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

I forget to delete this messages_ variable. It's not needed in the new approach.

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

Acknowledged.

+
+  // Used to create notification identifier.
+  uint32_t message_id_;
+  static const char kNotificationId[];

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

I don't think you need to have this in the .h

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

I don't think i need it, but i see this pattern for other notification id, such
as screen_layout_observer.h, tray_supervised_user.h....

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

It's good to keep the header file as slim as possible, and unless there is a
good reason to expose the constant here I'd rather it just be declared in the
.cc.

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

Agree. I will add it in this function as you suggested earlier.

+
+  DISALLOW_COPY_AND_ASSIGN(SmsObserver);
+};
+
+}  // namespace ash
+
+#endif  // ASH_COMMON_SYSTEM_CHROMEOS_NETWORK_SMS_OBSERVER_H_