Do not expire permission bubbles for in-page navigations.

chrome/browser/ui/website_settings/permission_bubble_manager.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 "chrome/browser/ui/website_settings/permission_bubble_manager.h"
 
 #include "base/command_line.h"
 #include "base/metrics/user_metrics_action.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_request.h"
 #include "chrome/common/chrome_switches.h"
@@ skipping 255 matching lines @@
     const content::LoadCommittedDetails& details) {
   // No permissions requests pending.
   if (request_url_.is_empty())
     return;
 
   // If we have navigated to a new url or reloaded the page...
   // GetAsReferrer strips fragment and username/password, meaning
   // the navigation is really to the same page.
   if ((request_url_.GetAsReferrer() !=
        web_contents()->GetLastCommittedURL().GetAsReferrer()) ||
-      details.type == content::NAVIGATION_TYPE_EXISTING_PAGE) {
+      (details.type == content::NAVIGATION_TYPE_EXISTING_PAGE &&
+       !details.is_in_page)) {

[mlamouri (slow - plz ping), 2015/08/04 11:56:38]

Why not use ::DidNavigateAnyFrame() to merge this and
main_frame_has_fully_loaded_ reset?

[Avi (use Gerrit), 2015/08/04 14:28:16]

Because I don't know this code well, and I was trying to make the smallest
change to keep the intent the same. If you know this code better, and think
that's a better choice, please do so.

[hcarmona, 2015/08/04 23:10:44]

It looks like using GetAsReferrer to compare the request and previous
URLs is trying to achieve the same as details.is_in_page.

I think it might be best to change this if condition to:

if (!details.is_in_page ||
    details.type == content::NAVIGATION_TYPE_SAME_PAGE ||
    details.type == content::NAVIGATION_TYPE_EXISTING_PAGE)

Because we want to clear permissions if we navigate to a different URL,
explicitly navigate to the same page, or refresh the page.

[Avi (use Gerrit), 2015/08/05 15:11:48]

But that's different behavior. The bug calls out going back over an in-page
navigation which is EXISTING_PAGE + is_in_page.

If you want to argue that the current behavior is incorrect, that's fine with
me; I'm no expert. Go make a CL to do it. But my change _restored_ the behavior
that this had before my change.

[hcarmona, 2015/08/05 18:51:04]

Interesting. I didn't know there were cases where the permission bubbles
should survive back/forward navigation, but it does makes sense.

I can make a change to get to the correct behavior, but I want to
clarify what that is exactly. Are there other cases I'm not aware of?
go/when-to-dismiss-permissions

     // Kill off existing bubble and cancel any pending requests.
     CancelPendingQueues();
     FinalizeBubble();
   }
 }
 
 void PermissionBubbleManager::WebContentsDestroyed() {
   // If the web contents has been destroyed, treat the bubble as cancelled.
   CancelPendingQueues();
   FinalizeBubble();
@@ skipping 181 matching lines @@
     case DENY_ALL:
       Deny();
       break;
     case DISMISS:
       Closing();
       break;
     case NONE:
       NOTREACHED();
   }
 }