[WebModals] New API for browser-scoped popup management.

components/web_modal/popup_manager_unittest.cc

Index: components/web_modal/popup_manager_unittest.cc
diff --git a/components/web_modal/popup_manager_unittest.cc b/components/web_modal/popup_manager_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1a159cabd6ac18845a63d7016a17ce777dc99ff9
--- /dev/null
+++ b/components/web_modal/popup_manager_unittest.cc
@@ -0,0 +1,146 @@
+// Copyright 2014 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.
+
+#include "components/web_modal/popup_manager.h"
+
+#include <map>
+
+#include "base/memory/scoped_ptr.h"
+#include "components/web_modal/single_popup_manager.h"
+#include "content/public/test/test_renderer_host.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace web_modal {
+
+// Tracks persistent state changes of the native WC-modal dialog manager.
+class NativePopupTracker {
+ public:
+  enum DialogState {

[Finnur, 2014/05/16 12:38:14]

PopupState?

+    UNKNOWN,
+    NOT_SHOWN,
+    SHOWN,
+    HIDDEN,
+    CLOSED
+  };
+
+  NativePopupTracker() : state_(UNKNOWN), was_shown_(false) {}
+
+  void SetState(DialogState state) {
+    state_ = state;
+    if (state_ == SHOWN)
+      was_shown_ = true;
+  }
+

[Finnur, 2014/05/16 12:38:14]

private:
... and ...
  DISALLOW_COPY_AND_ASSIGN()

+  DialogState state_;
+  bool was_shown_;
+};
+
+class TestNativePopupManager : public SinglePopupManager {
+ public:
+  TestNativePopupManager(
+      NativePopup dialog,
+      SinglePopupManagerDelegate* delegate,
+      NativePopupTracker* tracker)
+      : delegate_(delegate),
+        dialog_(dialog),
+        tracker_(tracker) {
+    if (tracker_)

[Finnur, 2014/05/16 12:38:14]

You seem guaranteed to have tracker_ until you stop tracking so why all the if
checks? Are you getting Show/Hide/Close calls after tracking stops? Seems at
least the if check here is not needed, no?

+      tracker_->SetState(NativePopupTracker::NOT_SHOWN);
+  }
+
+  virtual content::WebContents* IsBoundToWebContents() OVERRIDE {
+    return NULL;
+  }
+
+  virtual void Show() OVERRIDE {
+    if (tracker_)
+      tracker_->SetState(NativePopupTracker::SHOWN);
+  }
+  virtual void Hide() OVERRIDE {
+    if (tracker_)
+      tracker_->SetState(NativePopupTracker::HIDDEN);
+  }

[Finnur, 2014/05/16 12:38:14]

Nit: Spacing is a little inconsistent here (sometimes space between functions,
sometimes not).

+  virtual void Close() OVERRIDE {
+    if (tracker_)
+      tracker_->SetState(NativePopupTracker::CLOSED);
+    delegate_->WillClose(dialog_);
+  }
+  virtual void Focus() OVERRIDE {
+  }
+  virtual void Pulse() OVERRIDE {
+  }
+  virtual NativePopup popup() OVERRIDE {
+    return dialog_;
+  }
+
+  virtual bool HasUserGesture() OVERRIDE {
+    return false;
+  }
+  virtual bool MayBeOverlapped() OVERRIDE {
+    return false;
+  }
+
+  void StopTracking() {
+    tracker_ = NULL;
+  }
+
+ private:
+  SinglePopupManagerDelegate* delegate_;
+  NativePopup dialog_;

[Finnur, 2014/05/16 12:38:14]

popup_?

+  NativePopupTracker* tracker_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestNativePopupManager);
+};
+
+class PopupManagerTest : public content::RenderViewHostTestHarness {
+ public:
+  PopupManagerTest()
+      : next_dialog_id(1),
+        manager(NULL) {
+  }
+
+  virtual void SetUp() {
+    content::RenderViewHostTestHarness::SetUp();
+
+    manager = new PopupManager();
+  }
+
+  virtual void TearDown() {
+    content::RenderViewHostTestHarness::TearDown();
+    manager->CloseAllPopups();
+    delete manager;

[Finnur, 2014/05/16 12:38:14]

The OCD part of me wants to see TearDown be last, since Setup is first above. :)

+  }
+
+ protected:
+  NativePopup MakeFakeDialog() {
+    // PopupManager treats the NativePopup as an opaque type, so creating
+    // fake NativePopups using reinterpret_cast is valid.
+    return reinterpret_cast<NativePopup>(next_dialog_id++);
+  }
+
+  int next_dialog_id;
+  PopupManager* manager;

[Finnur, 2014/05/16 12:38:14]

nit: Missing underscore for members. Also, use scoped_ptr?

+
+  DISALLOW_COPY_AND_ASSIGN(PopupManagerTest);

[Finnur, 2014/05/16 12:38:14]

nit: Should you not have private: above this?

+};
+
+// Test that the dialog is shown immediately when the delegate indicates the web
+// contents is visible.
+TEST_F(PopupManagerTest, WebContentsVisible) {
+  // Dialog should be shown while WebContents is visible.
+  const NativePopup dialog = MakeFakeDialog();
+
+  NativePopupTracker tracker;
+  TestNativePopupManager* native_manager =
+      new TestNativePopupManager(dialog, manager, &tracker);
+  manager->ShowPopup(scoped_ptr<SinglePopupManager>(native_manager).Pass());
+
+  EXPECT_EQ(NativePopupTracker::SHOWN, tracker.state_);
+  EXPECT_TRUE(manager->IsPopupActiveInCurrentWebContents());
+  EXPECT_TRUE(tracker.was_shown_);
+
+  native_manager->StopTracking();
+}
+
+}  // namespace web_modal