Do not expire permission bubbles for in-page navigations.

Do not expire permission bubbles for in-page navigations.

When the IN_PAGE classification was removed in r337562, such navigations were folded into EXISTING_PAGE. The PermissionBubbleManager was checking for EXISTING_PAGE with the understanding that it did not include in-page navigations; restore that understanding.

BUG=516309
TEST=as in bug

Committed: https://crrev.com/4e2c120b7ff86303421ef94a36d0f8de4bc558ea
Cr-Commit-Position: refs/heads/master@{#341554}

[Avi (use Gerrit), 2015-08-03 16:52:13]

avi@chromium.org changed reviewers:
+ felt@chromium.org

[Avi (use Gerrit), 2015-08-03 16:52:14]

Adrienne, something for you.

BTW, I searched the code, and PermissionBubbleManager is the only code that
explicitly listens for EXISTING_PAGE in this way.

[felt, 2015-08-03 17:15:28]

+hector and mounir as an FYI since you were both touching this code

Thanks Avi, lgtm.

[Avi (use Gerrit), 2015-08-03 17:33:45]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2015-08-03 17:35:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1266003004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1266003004/1

[commit-bot: I haz the power, 2015-08-03 18:13:18]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-08-03 18:14:55]

Patchset 1 (id:??) landed as
https://crrev.com/4e2c120b7ff86303421ef94a36d0f8de4bc558ea
Cr-Commit-Position: refs/heads/master@{#341554}

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

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

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

https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:277:
!details.is_in_page)) {
Why not use ::DidNavigateAnyFrame() to merge this and
main_frame_has_fully_loaded_ reset?

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

https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:277:
!details.is_in_page)) {
On 2015/08/04 11:56:38, Mounir Lamouri wrote:
> Why not use ::DidNavigateAnyFrame() to merge this and
> main_frame_has_fully_loaded_ reset?

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]

hcarmona@chromium.org changed reviewers:
+ hcarmona@chromium.org

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

https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:277:
!details.is_in_page)) {
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]

https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:277:
!details.is_in_page)) {
On 2015/08/04 23:10:44, Hector Carmona wrote:
> 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.

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]

https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:277:
!details.is_in_page)) {
On 2015/08/05 15:11:48, Avi wrote:
> On 2015/08/04 23:10:44, Hector Carmona wrote:
> > 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.
> 
> 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.

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

[Avi (use Gerrit), 2015-08-05 19:07:12]

On 2015/08/05 18:51:04, Hector Carmona wrote:
>
https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
> File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):
> 
>
https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
> chrome/browser/ui/website_settings/permission_bubble_manager.cc:277:
> !details.is_in_page)) {
> On 2015/08/05 15:11:48, Avi wrote:
> > On 2015/08/04 23:10:44, Hector Carmona wrote:
> > > 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.
> > 
> > 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.
> 
> 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

I don't know. I'm not a permissions person, I'm a navigation person who broke
this little bit of code.

[Avi (use Gerrit), 2015-08-05 19:31:16]

On 2015/08/05 19:07:12, Avi wrote:
> On 2015/08/05 18:51:04, Hector Carmona wrote:
> >
>
https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
> > File chrome/browser/ui/website_settings/permission_bubble_manager.cc
(right):
> > 
> >
>
https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
> > chrome/browser/ui/website_settings/permission_bubble_manager.cc:277:
> > !details.is_in_page)) {
> > On 2015/08/05 15:11:48, Avi wrote:
> > > On 2015/08/04 23:10:44, Hector Carmona wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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
> 
> I don't know. I'm not a permissions person, I'm a navigation person who broke
> this little bit of code.

BTW, I'm interested in exploring a rewrite of this part of the code with you, as
I'm investigating the possibility of removing NavigationType.

[hcarmona, 2015-08-05 21:00:47]

On 2015/08/05 19:31:16, Avi wrote:
> On 2015/08/05 19:07:12, Avi wrote:
> > On 2015/08/05 18:51:04, Hector Carmona wrote:
> > >
> >
>
https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
> > > File chrome/browser/ui/website_settings/permission_bubble_manager.cc
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1266003004/diff/1/chrome/browser/ui/website_s...
> > > chrome/browser/ui/website_settings/permission_bubble_manager.cc:277:
> > > !details.is_in_page)) {
> > > On 2015/08/05 15:11:48, Avi wrote:
> > > > On 2015/08/04 23:10:44, Hector Carmona wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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
> > 
> > I don't know. I'm not a permissions person, I'm a navigation person who
broke
> > this little bit of code.
> 
> BTW, I'm interested in exploring a rewrite of this part of the code with you,
as
> I'm investigating the possibility of removing NavigationType.

That sounds good. We're in the process of refactoring bubbles, so
updating this code would be great if we can remove some of the
complexity.