Make sure bubbles in Views default to close before their RenderFrameHosts.

components/bubble/bubble_manager.cc

 // Copyright 2015 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/bubble/bubble_manager.h"
 
 #include <utility>
 #include <vector>
 
 #include "components/bubble/bubble_controller.h"
@@ skipping 29 matching lines @@
       break;
   }
 
   return bubble_ref;
 }
 
 bool BubbleManager::CloseBubble(BubbleReference bubble,
                                 BubbleCloseReason reason) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_NE(manager_state_, ITERATING_BUBBLES);
-  return CloseAllMatchingBubbles(bubble.get(), reason);
+  return CloseAllMatchingBubbles(bubble.get(), nullptr, reason);
 }
 
 void BubbleManager::CloseAllBubbles(BubbleCloseReason reason) {
   // The following close reasons don't make sense for multiple bubbles:
   DCHECK_NE(reason, BUBBLE_CLOSE_ACCEPTED);
   DCHECK_NE(reason, BUBBLE_CLOSE_CANCELED);
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_NE(manager_state_, ITERATING_BUBBLES);
-  CloseAllMatchingBubbles(nullptr, reason);
+  CloseAllMatchingBubbles(nullptr, nullptr, reason);
 }
 
 void BubbleManager::UpdateAllBubbleAnchors() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_NE(manager_state_, ITERATING_BUBBLES);
 
   // Guard against bubbles being added or removed while iterating the bubbles.
   ManagerState original_state = manager_state_;
   manager_state_ = ITERATING_BUBBLES;
   for (auto controller : controllers_)
@@ skipping 12 matching lines @@
 
 void BubbleManager::FinalizePendingRequests() {
   // Return if already "Finalized".
   if (manager_state_ == NO_MORE_BUBBLES)
     return;
 
   manager_state_ = NO_MORE_BUBBLES;
   CloseAllBubbles(BUBBLE_CLOSE_FORCED);
 }
 
-bool BubbleManager::CloseAllMatchingBubbles(BubbleController* match,
-                                            BubbleCloseReason reason) {
+void BubbleManager::CloseBubblesOwnedBy(const content::RenderFrameHost* frame) {
+  CloseAllMatchingBubbles(nullptr, frame, BUBBLE_CLOSE_FRAME_DESTROYED);
+}
+
+bool BubbleManager::CloseAllMatchingBubbles(
+    BubbleController* match,
+    const content::RenderFrameHost* frame,
+    BubbleCloseReason reason) {
   ScopedVector<BubbleController> close_queue;
 
   // Guard against bubbles being added or removed while iterating the bubbles.
   ManagerState original_state = manager_state_;
   manager_state_ = ITERATING_BUBBLES;
   for (auto iter = controllers_.begin(); iter != controllers_.end();) {
-    if ((!match || match == *iter) && (*iter)->ShouldClose(reason)) {
+    bool matches = true;

[hcarmona, 2016/02/05 22:48:24]

If both |match| and |frame| are passed in, should only bubbles that match both
conditions be closed?

Or does it make sense to close any bubble that matches either condition?

Or should only |match| or |frame| be specified, but not both?

[Jeffrey Yasskin, 2016/02/08 23:59:09]

Right now, only one is ever specified. I lean toward matching both conditions
(so that's what's implemented), but since we don't have any use cases for
passing both, there's no way to tell. We could instead pass in a lambda to
control what matches, but that seems like overengineering for now.

[hcarmona, 2016/02/09 00:31:05]

Agreed, lambda would be over-engineering.

Can we add a DCHECK(!(bubble && owner)) if we don't expect |bubble| and |owner|
to both be passed in. This way, if they're both passed in, whoever changes it
will need to decide AND or OR as opposed to us deciding arbitrarily right now.

[Peter Kasting, 2016/02/09 00:42:41]

Hmm.  I'm a little uneasy about using DCHECK like this when there's no real
reason why callers can't or even shouldn't do this.

I think the implemented behavior (force both to match if you specify both) is
probably OK, so I would suggest simply making sure the API comments are clear
about this case.  If someone passes both and doesn't read the comments, and has
a reason to want a different behavior, but doesn't check to see that their code
works, they kinda deserve what they get.

[hcarmona, 2016/02/09 00:59:44]

I can't think of a reason we would specify a specific bubble to close
and a frame. If we have a frame, all bubbles in that frame need to
decide to close. And if we want to close a specific bubble, then it
doesn't matter what frame that bubble is in, it should get the close
event. ANDing these two checks means that either 1 bubble will be asked
to close or no bubbles will be asked. Neither of those results seems
like desired behavior. We should either OR the condition checks, or
forbid filtering by both at the same time to avoid unwanted behavior.

+    if (match && match != *iter)
+      matches = false;
+    if (frame && !(*iter)->OwningFrameIs(frame))
+      matches = false;
+
+    if (matches && (*iter)->ShouldClose(reason)) {

[Peter Kasting, 2016/02/06 04:13:08]

Nit: Shorter and, IMO, more readable due to omitting the temp:

    if ((!bubble || bubble == *i) && (!owner || (*i)->OwningFrameIs(owner)) &&
        (*i)->ShouldClose(reason)) {

This renames the params as I suggested in the header and renames |iter| to |i|
as well (I think |i| and |it| are both better names than |iter|).

[Jeffrey Yasskin, 2016/02/08 23:59:09]

I had some trouble decoding the long boolean expression, which is why I changed
it to a temporary variable, but I also like getting my patch approved, so I've
changed it back.

       close_queue.push_back(*iter);
       iter = controllers_.weak_erase(iter);
     } else {
       ++iter;
     }
   }
   manager_state_ = original_state;
 
   for (auto controller : close_queue) {
     controller->DoClose();
 
     FOR_EACH_OBSERVER(BubbleManagerObserver, observers_,
                       OnBubbleClosed(controller->AsWeakPtr(), reason));
   }
 
   return !close_queue.empty();
 }