Send notification to users upon receiving sms messages

ash/common/shelf/shelf_widget.cc

Index: ash/common/shelf/shelf_widget.cc
diff --git a/ash/common/shelf/shelf_widget.cc b/ash/common/shelf/shelf_widget.cc
index d735e5897c17a0ddbd815142ed7698def34e053b..167143a872307e0654a6cd3ee36487744709538d 100644
--- a/ash/common/shelf/shelf_widget.cc
+++ b/ash/common/shelf/shelf_widget.cc
@@ -15,6 +15,7 @@
 #include "ash/common/shelf/shelf_view.h"
 #include "ash/common/shelf/wm_shelf.h"
 #include "ash/common/shelf/wm_shelf_util.h"
+#include "ash/common/system/chromeos/network/sms_observer.h"
 #include "ash/common/system/status_area_layout_manager.h"
 #include "ash/common/system/status_area_widget.h"
 #include "ash/common/wm_lookup.h"
@@ -219,6 +220,7 @@ ShelfWidget::ShelfWidget(WmWindow* shelf_container, WmShelf* wm_shelf)
       delegate_view_(new DelegateView(wm_shelf, this)),
       shelf_view_(nullptr),
       background_animator_(SHELF_BACKGROUND_DEFAULT, wm_shelf_),
+      sms_observer_(nullptr),
       activating_as_fallback_(false) {
   background_animator_.AddObserver(this);
   background_animator_.AddObserver(delegate_view_);
@@ -271,6 +273,7 @@ void ShelfWidget::CreateStatusAreaWidget(WmWindow* status_container) {
   background_animator_.AddObserver(status_area_widget_);
   status_container->SetLayoutManager(
       base::MakeUnique<StatusAreaLayoutManager>(this));
+  sms_observer_ = new SmsObserver();

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

Is there a specific reason why you chose to instantiate |sms_observer_| here
rather than in the ctor or initializer list?

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

sorry, it's a mistake. 

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

Acknowledged.

 }
 
 void ShelfWidget::SetPaintsBackground(
@@ -385,6 +388,7 @@ void ShelfWidget::Shutdown() {
     status_area_widget_ = nullptr;
   }
 
+  sms_observer_->Shutdown();

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

Is there a reason why you need to introduce a Shutdown() method and call it at
this specific place? It seems if you make ShelfWidget own SmsObserver, then
de-registering the observer can be done in the SmsObserver dtor rather than
needing a separate Shutdown() method.

If left as-is, I suggest guarding against a null |sms_observer_|.

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

I originally didn't create the shutdown method, then i keep receive this failure
as I exit chrome in my local build:
[105378:105378:0202/152205.133528:FATAL:observer_list.h(338)] Check failed:
ObserverListBase<ObserverType>::size() == 0U (1 vs. 0)

From the stack trace, I believe it calls shutdown first then it calls the dtor.
So i added this line to de-register sms_observer properly. I have added the
check as you have recommended.

[yiyix, 2017/02/02 21:24:19]

Sorry, after analyzing again, I understand that the dtor is not called. So I
updated it to unique_ptr and removed the unnecessary shutdown method.

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

Thanks for looking into this in more detail, what you have in PS2 looks good!

   CloseNow();
 }