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

chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc

 // 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 "base/command_line.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_manager.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_request.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_view.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 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 int GetIconID() const OVERRIDE {
+    return 5;
+  }
+
   virtual base::string16 GetMessageText() const OVERRIDE {
-    return base::ASCIIToUTF16("test");
+    return message_;
   }
 
   virtual base::string16 GetMessageTextFragment() const OVERRIDE {
-    return base::ASCIIToUTF16("test");
+    return message_;
   }
 
-  virtual base::string16 GetAlternateAcceptButtonText() const OVERRIDE {
-    return base::ASCIIToUTF16("button");
-  }
-
-  virtual base::string16 GetAlternateDenyButtonText() const OVERRIDE {
-    return base::ASCIIToUTF16("button");
+  virtual bool HasUserGesture() const OVERRIDE {
+    return false;
   }
 
   virtual void PermissionGranted() OVERRIDE {
     granted_ = true;
   }
 
   virtual void PermissionDenied() OVERRIDE {
     granted_ = false;
   }
 
   virtual void Cancelled() OVERRIDE {
     granted_ = false;
     cancelled_ = true;
   }
 
   virtual void RequestFinished() OVERRIDE {
     finished_ = true;
   }
 
+  base::string16 message_;
   bool granted_;
   bool cancelled_;
   bool finished_;
 };
 
 class MockView : public PermissionBubbleView {
  public:
   MockView() : shown_(false), delegate_(NULL) {}
   ~MockView() {}
 
@@ skipping 15 matching lines @@
       bool customization_state_) OVERRIDE {
     shown_ = true;
     permission_requests_ = requests;
     permission_states_ = accept_state;
   }
 
   virtual void Hide() OVERRIDE {
     shown_ = false;
   }
 
+  virtual bool CanAcceptRequestUpdate() OVERRIDE {
+    return true;
+  }
+
   bool shown_;
   Delegate* delegate_;
   std::vector<PermissionBubbleRequest*> permission_requests_;
   std::vector<bool> permission_states_;
 };
 
 }  // namespace
 
 class PermissionBubbleManagerTest : public testing::Test {
  public:
   PermissionBubbleManagerTest();
+  virtual ~PermissionBubbleManagerTest() {}
+
+  void TearDown() {
+    manager_.reset();
+  }
 
   void ToggleAccept(int index, bool value) {
     manager_->ToggleAccept(index, value);
   }
 
   void Accept() {
     manager_->Accept();
   }
 
   void WaitForCoalescing() {
     base::MessageLoop::current()->RunUntilIdle();
   }
 
  protected:
   MockRequest request1_;
   MockRequest request2_;
   MockView view_;
   scoped_ptr<PermissionBubbleManager> manager_;
 
   base::MessageLoop message_loop_;
   content::TestBrowserThread ui_thread_;
 };
 
 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);
 }
 
 TEST_F(PermissionBubbleManagerTest, TestFlag) {
   EXPECT_FALSE(PermissionBubbleManager::Enabled());
   CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnablePermissionsBubbles);
   EXPECT_TRUE(PermissionBubbleManager::Enabled());
 }
@@ skipping 158 matching lines @@
 
   EXPECT_FALSE(view_.shown_);
 
   manager_->AddRequest(&request2_);
   WaitForCoalescing();
   EXPECT_TRUE(view_.shown_);
   Accept();
   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_);
+}