[WebsiteSettings] Change permission bubble API to adapt to new mocks.

chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc

Index: chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
diff --git a/chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc b/chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
index 3e5a1c1fe6ea8805440b7375bd9dfbe67d042830..f3ff7d13d3431a596894735ef8fdecb568b7f166 100644
--- a/chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
+++ b/chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
@@ -16,24 +16,29 @@ namespace {
 
 class MockRequest : public PermissionBubbleRequest {
  public:
-  MockRequest() : granted_(false), cancelled_(false), finished_(false) {}
+  explicit MockRequest(const base::string16& message)
+      : message_(message),
+        granted_(false),
+        cancelled_(false),
+        finished_(false) {}
+
   virtual ~MockRequest() {}
 
   // PermissionBubbleRequest:
-  virtual base::string16 GetMessageText() const OVERRIDE {
-    return base::ASCIIToUTF16("test");
+  virtual int GetIconID() const OVERRIDE {
+    return 5;
   }
 
-  virtual base::string16 GetMessageTextFragment() const OVERRIDE {
-    return base::ASCIIToUTF16("test");
+  virtual base::string16 GetMessageText() const OVERRIDE {
+    return message_;
   }
 
-  virtual base::string16 GetAlternateAcceptButtonText() const OVERRIDE {
-    return base::ASCIIToUTF16("button");
+  virtual base::string16 GetMessageTextFragment() const OVERRIDE {
+    return message_;
   }
 
-  virtual base::string16 GetAlternateDenyButtonText() const OVERRIDE {
-    return base::ASCIIToUTF16("button");
+  virtual bool HasUserGesture() const OVERRIDE {
+    return false;
   }
 
   virtual void PermissionGranted() OVERRIDE {
@@ -53,6 +58,7 @@ class MockRequest : public PermissionBubbleRequest {
     finished_ = true;
   }
 
+  base::string16 message_;
   bool granted_;
   bool cancelled_;
   bool finished_;
@@ -88,6 +94,10 @@ class MockView : public PermissionBubbleView {
     shown_ = false;
   }
 
+  virtual bool CanAcceptRequestUpdate() OVERRIDE {
+    return true;
+  }
+
   bool shown_;
   Delegate* delegate_;
   std::vector<PermissionBubbleRequest*> permission_requests_;
@@ -99,6 +109,11 @@ class MockView : public PermissionBubbleView {
 class PermissionBubbleManagerTest : public testing::Test {
  public:
   PermissionBubbleManagerTest();
+  virtual ~PermissionBubbleManagerTest() {}
+
+  void TearDown() {
+    manager_.reset();
+  }
 
   void ToggleAccept(int index, bool value) {
     manager_->ToggleAccept(index, value);
@@ -123,7 +138,9 @@ class PermissionBubbleManagerTest : public testing::Test {
 };
 
 PermissionBubbleManagerTest::PermissionBubbleManagerTest()
-    : manager_(new PermissionBubbleManager(NULL)),
+    : request1_(base::ASCIIToUTF16("test1")),
+      request2_(base::ASCIIToUTF16("test2")),
+      manager_(new PermissionBubbleManager(NULL)),
       ui_thread_(content::BrowserThread::UI, &message_loop_) {
   manager_->SetCoalesceIntervalForTesting(0);
 }
@@ -302,3 +319,40 @@ TEST_F(PermissionBubbleManagerTest, SequentialRequests) {
   EXPECT_FALSE(view_.shown_);
   EXPECT_TRUE(request2_.granted_);
 }
+
+TEST_F(PermissionBubbleManagerTest, SameRequestRejected) {
+  manager_->SetView(&view_);
+  manager_->AddRequest(&request1_);
+  manager_->AddRequest(&request1_);

[groby-ooo-7-16, 2014/02/25 22:27:36]

I get why we can't do that in the current framework, but I'm concerned about the
fact that re-adding an identical request _doesn't_ call RequestFinished. Isn't
that a misuse of the API that should be caught via DCHECK?

[Greg Billock, 2014/02/26 00:28:44]

It shouldn't call RequestFinished, since the manager already knows of this exact
request and is tracking it (the first successful AddRequest will eventually
result in RequestFinished). Added an EXPECT for that.

Agreed this is arguably a misuse, and it's a bit unclear to me still how much
state tracking to do in the manager vs. callers. Arguably sending dupe requests
is a misuse as well, and should DCHECK. I think that may end up being an
annoyance for some callers, since there's nothing stopping JS from just
re-calling over and over, and so if this is easy to track in the manager, it's
nice to do it there rather than in each caller. Cancellability, though, which a
couple callers need, I think I'll track there as opposed to in the manager, to
avoid RAII junk we've talked about before. If that ends up being basically all
of them, then it'll make more sense to move all request state tracking out to
callers, but so far it looks like it's more like half. The hardest number to
deal with. :-)

[groby-ooo-7-16, 2014/02/26 02:28:11]

FWIW, I think each JS request should simply generate a new C++ request object.
It simplifies the misuse case, at an arguably low cost. (Of $19.99! But wait,
there is more! ;)

That reduces identical request to "logically identical request", and callers can
now reasonably expect that _every_ AddRequest will be matched with a
RequestFinished at some point

+
+  WaitForCoalescing();
+  EXPECT_TRUE(view_.shown_);
+  ASSERT_EQ(1u, view_.permission_requests_.size());
+  EXPECT_EQ(&request1_, view_.permission_requests_[0]);
+}
+
+TEST_F(PermissionBubbleManagerTest, DuplicateRequestRejected) {
+  manager_->SetView(&view_);
+  manager_->AddRequest(&request1_);
+  MockRequest dupe_request(base::ASCIIToUTF16("test1"));
+  manager_->AddRequest(&dupe_request);
+  EXPECT_TRUE(dupe_request.finished_);
+  EXPECT_FALSE(request1_.finished_);
+}
+
+TEST_F(PermissionBubbleManagerTest, DuplicateQueuedRequest) {
+  manager_->SetView(&view_);
+  manager_->AddRequest(&request1_);
+  WaitForCoalescing();
+  manager_->AddRequest(&request2_);
+
+  MockRequest dupe_request(base::ASCIIToUTF16("test1"));
+  manager_->AddRequest(&dupe_request);
+  EXPECT_TRUE(dupe_request.finished_);
+  EXPECT_FALSE(request1_.finished_);
+
+  MockRequest dupe_request2(base::ASCIIToUTF16("test2"));
+  manager_->AddRequest(&dupe_request2);
+  EXPECT_TRUE(dupe_request2.finished_);
+  EXPECT_FALSE(request2_.finished_);
+}