Update implementation as per feedback

atom/browser/api/atom_api_notification.cc

@@ -9,7 +9,6 @@
 
 #include "atom/browser/api/atom_api_menu.h"
 #include "atom/browser/browser.h"
-#include "atom/browser/ui/notification_delegate_adapter.h"
 #include "atom/common/api/atom_api_native_image.h"
 #include "atom/common/native_mate_converters/gfx_converter.h"
 #include "atom/common/native_mate_converters/image_converter.h"
@@ -17,6 +16,7 @@
 #include "atom/common/node_includes.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "brightray/browser/browser_client.h"
 #include "native_mate/constructor.h"
 #include "native_mate/dictionary.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -28,9 +28,6 @@ namespace atom {
 
 namespace api {
 
-int id_counter = 1;
-std::map<int, Notification*> notifications_;
-
 Notification::Notification(v8::Isolate* isolate,
                            v8::Local<v8::Object> wrapper,
                            mate::Arguments* args) {
@@ -47,13 +44,13 @@ Notification::Notification(v8::Isolate* isolate,
     opts.Get("silent", &silent_);
     opts.Get("replyPlaceholder", &reply_placeholder_);
     opts.Get("hasReply", &has_reply_);
-    id_ = id_counter++;
   }
-  notifications_[id_] = this;
   OnInitialProps();
 }
 
-Notification::~Notification() {}
+Notification::~Notification() {
+  notification_->set_delegate(nullptr);
+}
 
 // static
 mate::WrappableBase* Notification::New(mate::Arguments* args) {
@@ -64,18 +61,7 @@ mate::WrappableBase* Notification::New(mate::Arguments* args) {
   return new Notification(args->isolate(), args->GetThis(), args);
 }
 
-bool Notification::HasID(int id) {
-  return notifications_.find(id) != notifications_.end();
-}
-
-Notification* Notification::FromID(int id) {
-  return notifications_[id];
-}
-
 // Getters
-int Notification::GetID() {
-  return id_;
-}
 base::string16 Notification::GetTitle() {
   return title_;
 }
@@ -93,66 +79,54 @@ bool Notification::GetHasReply() {
 }
 
 // Setters
-void Notification::SetTitle(base::string16 new_title) {
+void Notification::SetTitle(const base::string16& new_title) {
   title_ = new_title;
-  NotifyPropsUpdated();
 }
-void Notification::SetBody(base::string16 new_body) {
+void Notification::SetBody(const base::string16& new_body) {
   body_ = new_body;
-  NotifyPropsUpdated();
 }
 void Notification::SetSilent(bool new_silent) {
   silent_ = new_silent;
-  NotifyPropsUpdated();
 }
-void Notification::SetReplyPlaceholder(base::string16 new_reply_placeholder) {
+void Notification::SetReplyPlaceholder(const base::string16& new_reply_placeholder) {
   reply_placeholder_ = new_reply_placeholder;
-  NotifyPropsUpdated();
 }
 void Notification::SetHasReply(bool new_has_reply) {
   has_reply_ = new_has_reply;
-  NotifyPropsUpdated();
 }
 
-void Notification::OnClicked() {
+void Notification::NotificationClick() {
   Emit("click");
 }
 
-void Notification::OnReplied(std::string reply) {
+void Notification::NotificationReplied(const std::string reply) {
   Emit("reply", reply);
 }
 
-void Notification::OnShown() {
+void Notification::NotificationDisplayed() {
   Emit("show");
 }
 
-void Notification::OnClosed() {
+void Notification::NotificationDestroyed() {
   Emit("close");
 }
 
-void Notification::NotifyPropsUpdated() {}
+void Notification::NotificationClosed() {}
 
 // Showing notifications
 void Notification::Show() {
-  SkBitmap image = *(new SkBitmap);
-  if (has_icon_) {
-    image = *(icon_.ToSkBitmap());
+  if (presenter_) {
+    notification_ = presenter_->CreateNotification(this);
+    if (notification_) {
+      notification_->Show(title_, body_, "", GURL(), icon_.AsBitmap(), silent_, has_reply_, reply_placeholder_);
     }
-
-  std::unique_ptr<AtomNotificationDelegateAdapter> adapter(
-      new AtomNotificationDelegateAdapter(this));
-  auto notif = presenter_->CreateNotification(adapter.get());
-  if (notif) {
-    ignore_result(adapter.release());  // it will release itself automatically.
-    GURL nullUrl = *(new GURL);
-    notif->Show(title_, body_, "", nullUrl, image, silent_, has_reply_, reply_placeholder_);
   }
 }
 
 bool initialized_ = false;
 void Notification::OnInitialProps() {
   if (!initialized_) {
-    presenter_ = brightray::NotificationPresenter::Create();
+    presenter_ = brightray::BrowserClient::Get()->GetNotificationPresenter();
     initialized_ = true;
   }
 }
@@ -164,7 +138,6 @@ void Notification::BuildPrototype(v8::Isolate* isolate,
   mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
       .MakeDestroyable()
       .SetMethod("show", &Notification::Show)
-      .SetProperty("id", &Notification::GetID)
       .SetProperty("title", &Notification::GetTitle, &Notification::SetTitle)
       .SetProperty("body", &Notification::GetBody, &Notification::SetBody)
       .SetProperty("silent", &Notification::GetSilent, &Notification::SetSilent)

atom/browser/api/atom_api_notification.h

@@ -10,9 +10,9 @@
 #include <vector>
 
 #include "atom/browser/api/trackable_object.h"
-#include "atom/browser/ui/notification_observer.h"
 #include "base/strings/utf_string_conversions.h"
 #include "brightray/browser/notification.h"
+#include "brightray/browser/notification_delegate.h"
 #include "brightray/browser/notification_presenter.h"
 #include "native_mate/handle.h"
 #include "ui/gfx/image/image.h"
@@ -22,7 +22,7 @@ namespace atom {
 namespace api {
 
 class Notification : public mate::TrackableObject<Notification>,
-                     public NotifictionObserver {
+                     public brightray::NotificationDelegate {
  public:
   static mate::WrappableBase* New(mate::Arguments* args);
   static bool HasID(int id);
@@ -31,11 +31,12 @@ class Notification : public mate::TrackableObject<Notification>,
   static void BuildPrototype(v8::Isolate* isolate,
                              v8::Local<v8::FunctionTemplate> prototype);
 
-  // NotificationObserver:
-  void OnClicked() override;
-  void OnReplied(std::string reply) override;
-  void OnShown() override;
-  void OnClosed() override;
+  // NotificationDelegate:
+  void NotificationClick() override;
+  void NotificationReplied(const std::string reply) override;
+  void NotificationDisplayed() override;
+  void NotificationDestroyed() override;
+  void NotificationClosed() override;
 
  protected:
   Notification(v8::Isolate* isolate,
@@ -47,7 +48,6 @@ class Notification : public mate::TrackableObject<Notification>,
   void Show();
 
   // Prop Getters
-  int GetID();
   base::string16 GetTitle();
   base::string16 GetBody();
   bool GetSilent();
@@ -55,26 +55,24 @@ class Notification : public mate::TrackableObject<Notification>,
   bool GetHasReply();
 
   // Prop Setters
-  void SetTitle(base::string16 new_title);
-  void SetBody(base::string16 new_body);
+  void SetTitle(const base::string16& new_title);
+  void SetBody(const base::string16& new_body);
   void SetSilent(bool new_silent);
-  void SetReplyPlaceholder(base::string16 new_reply_placeholder);
+  void SetReplyPlaceholder(const base::string16& new_reply_placeholder);
   void SetHasReply(bool new_has_reply);
 
-  void NotifyPropsUpdated();
-
  private:
-  base::string16 title_ = base::UTF8ToUTF16("");
-  base::string16 body_ = base::UTF8ToUTF16("");
+  base::string16 title_;
+  base::string16 body_;
   gfx::Image icon_;
-  base::string16 icon_path_ = base::UTF8ToUTF16("");
+  base::string16 icon_path_;
   bool has_icon_ = false;
   bool silent_ = false;
-  base::string16 reply_placeholder_ = base::UTF8ToUTF16("");
+  base::string16 reply_placeholder_;
   bool has_reply_ = false;
   brightray::NotificationPresenter* presenter_;
 
-  int id_;
+  base::WeakPtr<brightray::Notification> notification_;
 
   DISALLOW_COPY_AND_ASSIGN(Notification);
 };

atom/browser/ui/notification_delegate_adapter.cc

@@ -1,31 +0,0 @@
-// Copyright (c) 2014 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#include "atom/browser/ui/notification_delegate_adapter.h"
-
-#include "atom/browser/api/atom_api_notification.h"
-#include "brightray/browser/notification_delegate.h"
-
-namespace atom {
-
-AtomNotificationDelegateAdapter::AtomNotificationDelegateAdapter(
-    atom::api::Notification* target) {
-  observer_ = target;
-}
-void AtomNotificationDelegateAdapter::NotificationDisplayed() {
-  observer_->OnShown();
-}
-void AtomNotificationDelegateAdapter::NotificationClosed() {}
-void AtomNotificationDelegateAdapter::NotificationClick() {
-  observer_->OnClicked();
-}
-void AtomNotificationDelegateAdapter::NotificationReplied(std::string reply) {
-  observer_->OnReplied(reply);
-}
-void AtomNotificationDelegateAdapter::NotificationDestroyed() {
-  observer_->OnClosed();
-}
-void AtomNotificationDelegateAdapter::NotificationFailed() {}
-
-}  // namespace atom

atom/browser/ui/notification_delegate_adapter.h

@@ -1,29 +0,0 @@
-// Copyright (c) 2014 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#include "brightray/browser/notification_delegate.h"
-
-#include "atom/browser/api/atom_api_notification.h"
-
-#ifndef ATOM_BROWSER_UI_NOTIFICATION_DELEGATE_ADAPTER_H_
-#define ATOM_BROWSER_UI_NOTIFICATION_DELEGATE_ADAPTER_H_
-
-namespace atom {
-
-class AtomNotificationDelegateAdapter : public brightray::NotificationDelegate {
- public:
-  atom::api::Notification* observer_;
-  explicit AtomNotificationDelegateAdapter(atom::api::Notification* target);
-
-  void NotificationDisplayed();
-  void NotificationClosed();
-  void NotificationClick();
-  void NotificationDestroyed();
-  void NotificationFailed();
-  void NotificationReplied(std::string reply);
-};
-
-}  // namespace atom
-
-#endif  // ATOM_BROWSER_UI_NOTIFICATION_DELEGATE_ADAPTER_H_

atom/browser/ui/notification_observer.h

@@ -1,25 +0,0 @@
-// Copyright (c) 2014 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#ifndef ATOM_BROWSER_UI_NOTIFICATION_OBSERVER_H_
-#define ATOM_BROWSER_UI_NOTIFICATION_OBSERVER_H_
-
-#include <string>
-
-namespace atom {
-
-class NotifictionObserver {
- public:
-  virtual void OnClicked() {}
-  virtual void OnReplied(std::string reply) {}
-  virtual void OnShown() {}
-  virtual void OnClosed() {}
-
- protected:
-  virtual ~NotifictionObserver() {}
-};
-
-}  // namespace atom
-
-#endif  // ATOM_BROWSER_UI_NOTIFICATION_OBSERVER_H_

brightray/browser/browser_client.cc

@@ -36,7 +36,7 @@ BrowserClient::~BrowserClient() {
 
 NotificationPresenter* BrowserClient::GetNotificationPresenter() {
   if (!notification_presenter_) {
-    // Create a new presenter if on OS X, Linux, or Windows 8+
+    // Create a new presenter if on OS X, Linux, or Windows 7+
     notification_presenter_.reset(NotificationPresenter::Create());
   }
   return notification_presenter_.get();

brightray/browser/linux/libnotify_notification.cc

@@ -88,9 +88,9 @@ void LibnotifyNotification::Show(const base::string16& title,
                                  const std::string& tag,
                                  const GURL& icon_url,
                                  const SkBitmap& icon,
-                                 const bool silent,
-                                 const bool has_reply,
-                                 const base::string16 reply_placeholder) {
+                                 bool silent,
+                                 bool has_reply,
+                                 const base::string16& reply_placeholder) {
   notification_ = libnotify_loader_.notify_notification_new(
       base::UTF16ToUTF8(title).c_str(),
       base::UTF16ToUTF8(body).c_str(),
@@ -139,6 +139,7 @@ void LibnotifyNotification::Show(const base::string16& title,
     return;
   }
 
+  if (delegate())
     delegate()->NotificationDisplayed();
 }
 

brightray/browser/linux/libnotify_notification.h

@@ -27,9 +27,9 @@ class LibnotifyNotification : public Notification {
             const std::string& tag,
             const GURL& icon_url,
             const SkBitmap& icon,
-            const bool silent,
-            const bool has_reply,
-            const base::string16 reply_placeholder) override;
+            bool silent,
+            bool has_reply,
+            const base::string16& reply_placeholder) override;
   void Dismiss() override;
 
  private:

brightray/browser/mac/cocoa_notification.h

@@ -26,13 +26,13 @@ class CocoaNotification : public Notification {
             const std::string& tag,
             const GURL& icon_url,
             const SkBitmap& icon,
-            const bool silent,
+            bool silent,
             const bool hasReply,
             const base::string16 replyPlaceholder) override;
   void Dismiss() override;
 
   void NotificationDisplayed();
-  void NotificationReplied(std::string reply);
+  void NotificationReplied(const std::string reply);
 
   NSUserNotification* notification() const { return notification_; }
 

brightray/browser/mac/cocoa_notification.mm

@@ -28,9 +28,9 @@ void CocoaNotification::Show(const base::string16& title,
                              const std::string& tag,
                              const GURL& icon_url,
                              const SkBitmap& icon,
-                             const bool silent,
-                             const bool has_reply,
-                             const base::string16 reply_placeholder) {
+                             bool silent,
+                             bool has_reply,
+                             const base::string16& reply_placeholder) {
   notification_.reset([[NSUserNotification alloc] init]);
   [notification_ setTitle:base::SysUTF16ToNSString(title)];
   [notification_ setInformativeText:base::SysUTF16ToNSString(body)];
@@ -65,10 +65,12 @@ void CocoaNotification::Dismiss() {
 }
 
 void CocoaNotification::NotificationDisplayed() {
+  if (delegate())
     delegate()->NotificationDisplayed();
 }
 
 void CocoaNotification::NotificationReplied(const std::string reply) {
+  if (delegate())
     delegate()->NotificationReplied(reply);
 }
 

brightray/browser/notification.cc

@@ -17,20 +17,28 @@ Notification::Notification(NotificationDelegate* delegate,
 }
 
 Notification::~Notification() {
+  if (delegate())
     delegate()->NotificationDestroyed();
 }
 
+void Notification::set_delegate(NotificationDelegate* delegate) {
+  delegate_ = delegate;
+}
+
 void Notification::NotificationClicked() {
+  if (delegate())
     delegate()->NotificationClick();
   Destroy();
 }
 
 void Notification::NotificationDismissed() {
+  if (delegate())
     delegate()->NotificationClosed();
   Destroy();
 }
 
 void Notification::NotificationFailed() {
+  if (delegate())
     delegate()->NotificationFailed();
   Destroy();
 }

brightray/browser/notification.h

@@ -26,9 +26,9 @@ class Notification {
                     const std::string& tag,
                     const GURL& icon_url,
                     const SkBitmap& icon,
-                    const bool silent,
-                    const bool hasReply,
-                    const base::string16 replyPlaceholder) = 0;
+                    bool silent,
+                    bool hasReply,
+                    const base::string16& reply_placeholder) = 0;
   // Closes the notification, this instance will be destroyed after the
   // notification gets closed.
   virtual void Dismiss() = 0;
@@ -54,6 +54,7 @@ class Notification {
 
  public:
   virtual ~Notification();
+  void set_delegate(NotificationDelegate* delegate);
 
  private:
   NotificationDelegate* delegate_;

brightray/browser/notification_delegate.h

@@ -19,7 +19,7 @@ class NotificationDelegate : public content::DesktopNotificationDelegate {
   virtual void NotificationFailed() {}
 
   // Notification was replied to
-  virtual void NotificationReplied(std::string reply) {}
+  virtual void NotificationReplied(const std::string reply) {}
 };
 
 }  // namespace brightray

brightray/browser/notification_delegate_adapter.cc

@@ -30,8 +30,4 @@ void NotificationDelegateAdapter::NotificationClick() {
   delegate_->NotificationClick();
 }
 
-// void NotificationDelegateAdapter::NotificationReplied(std::string reply) {
-//   delegate_->NotificationReplied(reply);
-// }
-
 }  // namespace brightray

brightray/browser/win/win32_notification.cc

@@ -12,8 +12,8 @@ namespace brightray {
 void Win32Notification::Show(
     const base::string16& title, const base::string16& msg,
     const std::string& tag, const GURL& icon_url,
-    const SkBitmap& icon, const bool silent,
-    const bool has_reply, const base::string16 reply_placeholder) {
+    const SkBitmap& icon, bool silent,
+    bool has_reply, const base::string16& reply_placeholder) {
     auto presenter = static_cast<NotificationPresenterWin7*>(this->presenter());
     if (!presenter) return;
 

brightray/browser/win/win32_notification.h

@@ -12,8 +12,8 @@ class Win32Notification : public brightray::Notification {
     }
     void Show(const base::string16& title, const base::string16& msg,
               const std::string& tag, const GURL& icon_url,
-              const SkBitmap& icon, const bool silent,
-              const bool has_reply, const base::string16 reply_placeholder) override;
+              const SkBitmap& icon, bool silent,
+              bool has_reply, const base::string16& reply_placeholder) override;
     void Dismiss() override;
 
     const DesktopNotificationController::Notification& GetRef() const {

brightray/browser/win/windows_toast_notification.cc

@@ -89,9 +89,9 @@ void WindowsToastNotification::Show(const base::string16& title,
                                     const std::string& tag,
                                     const GURL& icon_url,
                                     const SkBitmap& icon,
-                                    const bool silent,
-                                    const bool hasReply,
-                                    const base::string16 replyPlaceholder) {
+                                    bool silent,
+                                    bool has_reply,
+                                    const base::string16& reply_placeholder) {
   auto presenter_win = static_cast<NotificationPresenterWin*>(presenter());
   std::wstring icon_path = presenter_win->SaveIconToFilesystem(icon, icon_url);
 
@@ -133,6 +133,7 @@ void WindowsToastNotification::Show(const base::string16& title,
     return;
   }
 
+  if (delegate())
     delegate()->NotificationDisplayed();
 }
 
@@ -146,7 +147,7 @@ bool WindowsToastNotification::GetToastXml(
     const std::wstring& title,
     const std::wstring& msg,
     const std::wstring& icon_path,
-    const bool silent,
+    bool silent,
     IXmlDocument** toast_xml) {
   ABI::Windows::UI::Notifications::ToastTemplateType template_type;
   if (title.empty() || msg.empty()) {

brightray/browser/win/windows_toast_notification.h

@@ -55,9 +55,9 @@ class WindowsToastNotification : public Notification {
             const std::string& tag,
             const GURL& icon_url,
             const SkBitmap& icon,
-            const bool silent,
-            const bool has_reply,
-            const base::string16 reply_placeholder) override;
+            bool silent,
+            bool has_reply,
+            const base::string16& reply_placeholder) override;
   void Dismiss() override;
 
  private:

docs/api/notification.md

@@ -6,7 +6,7 @@ Process: [Main](../glossary.md#main-process)
 
 ## Using in the renderer process
 
-If you want to use Notifications in a renderer process you should use the [HTML5 Notification API](../tutorial/notifications.md)
+If you want to show Notifications from a renderer process you should use the [HTML5 Notification API](../tutorial/notifications.md)
 
 ## Class: Notification
 

filenames.gypi

@@ -313,9 +313,6 @@
       'atom/browser/ui/message_box_gtk.cc',
       'atom/browser/ui/message_box_mac.mm',
       'atom/browser/ui/message_box_win.cc',
-      'atom/browser/ui/notification_delegate_adapter.h',
-      'atom/browser/ui/notification_delegate_adapter.cc',
-      'atom/browser/ui/notification_observer.h',
       'atom/browser/ui/tray_icon.cc',
       'atom/browser/ui/tray_icon.h',
       'atom/browser/ui/tray_icon_gtk.cc',