Handles iframe permissions requests separately, in a subsequent bubble.

Handles iframe permissions requests separately, in a subsequent bubble.

If the requests are received before the bubble is shown, and there are main
frame requests pending, they will not be shown with the main frame requests.

Once the main frame requests have been handled, the iframe requests will be
shown in a new bubble.

All requests are flushed from the queues when a page is reloaded.

BUG=369685
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273137

[leng, 2014-05-19 18:53:25]

It just occurred to me:
Could an iframe request ever come from a user gesture?

[Greg Billock, 2014-05-21 17:09:19]

Yeah, I think an iframe request can come with a user gesture in general.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:105: bool
is_main_frame = request->GetRequestingHostname() == request_url_;
Do we need to extract just the hostport from the request_url_ to do this?

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:109: if
(ExistingRequest(request, requests_, &exact_duplicate) ||
Maybe exact_duplicate --> same_object ?

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:133:
accept_states_.push_back(true);
should this only execute if is_main_frame, right?

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:136:
TriggerShowBubble();
Should this be ScheduleShowBubble, right? (and was it an error before?) to let
any simultaneous pending registrations complete? Kind of a race condition, so
maybe no observable difference...

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:215: if
(request_url_has_loaded_) {
no braces

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:228:
details.type == content::NAVIGATION_TYPE_EXISTING_PAGE) {
Makes sense. We then get the re-notifications for any permissions requests
issued by the page, right?

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:317:
accept_states_.resize(requests_.size(), true);
Maybe a comment that the 'true' here is setting their value? I felt like I
wanted to look that up to make sure I had it right.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:360: for (iter =
queue.begin();
fits on one line?

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:364: if
(exact_duplicate)
we can stipulate that CHECK(exact_duplicate)

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager.h (right):

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.h:106: bool*
exact_dupcliate);
sp

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:415:
WaitForCoalescing();
Maybe try a request with a url like www.google.com/some/url

Also, should this be checking that view_.shown_ iwth request1_, and then after
closing that, that iframe_request_ is shown?

[leng, 2014-05-22 00:08:47]

I've modified it to give user gesture requests precedence, even for iframe
requests.

PTAL, thanks!

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:105: bool
is_main_frame = request->GetRequestingHostname() == request_url_;
On 2014/05/21 17:09:19, Greg Billock wrote:
> Do we need to extract just the hostport from the request_url_ to do this?

I have no idea.  I don't know enough about main frame vs. iframe requests.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:109: if
(ExistingRequest(request, requests_, &exact_duplicate) ||
On 2014/05/21 17:09:19, Greg Billock wrote:
> Maybe exact_duplicate --> same_object ?

Good call.  Done.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:133:
accept_states_.push_back(true);
On 2014/05/21 17:09:19, Greg Billock wrote:
> should this only execute if is_main_frame, right?

Done.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:136:
TriggerShowBubble();
On 2014/05/21 17:09:19, Greg Billock wrote:
> Should this be ScheduleShowBubble, right? (and was it an error before?) to let
> any simultaneous pending registrations complete? Kind of a race condition, so
> maybe no observable difference...

I see no harm in making it ScheduleShowBubble(), and a slight possible upside if
there is a race.  Changed.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:215: if
(request_url_has_loaded_) {
On 2014/05/21 17:09:19, Greg Billock wrote:
> no braces

Done.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:228:
details.type == content::NAVIGATION_TYPE_EXISTING_PAGE) {
On 2014/05/21 17:09:19, Greg Billock wrote:
> Makes sense. We then get the re-notifications for any permissions requests
> issued by the page, right?

Correct.  I tested it manually.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:317:
accept_states_.resize(requests_.size(), true);
On 2014/05/21 17:09:19, Greg Billock wrote:
> Maybe a comment that the 'true' here is setting their value? I felt like I
> wanted to look that up to make sure I had it right.

Done.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:360: for (iter =
queue.begin();
On 2014/05/21 17:09:19, Greg Billock wrote:
> fits on one line?

Done.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:364: if
(exact_duplicate)
On 2014/05/21 17:09:19, Greg Billock wrote:
> we can stipulate that CHECK(exact_duplicate)

Done.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager.h (right):

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.h:106: bool*
exact_dupcliate);
On 2014/05/21 17:09:19, Greg Billock wrote:
> sp

Done.

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:415:
WaitForCoalescing();
On 2014/05/21 17:09:19, Greg Billock wrote:
> Maybe try a request with a url like http://www.google.com/some/url
> 
> Also, should this be checking that view_.shown_ iwth request1_, and then after
> closing that, that iframe_request_ is shown?

I assumed you meant the url to be the iframe url...  done.

I also fleshed out the tests some more.  For some reason, I stopped partway.

[Greg Billock, 2014-05-22 02:06:31]

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:105: bool
is_main_frame = request->GetRequestingHostname() == request_url_;
On 2014/05/22 00:08:47, leng wrote:
> On 2014/05/21 17:09:19, Greg Billock wrote:
> > Do we need to extract just the hostport from the request_url_ to do this?
> 
> I have no idea.  I don't know enough about main frame vs. iframe requests.

I think GetLastCommittedURL() is the full url, and GetRequestingHostname() is
just the hostname portion of the url (formatted as a url), so the root of the
security context. So in our test cases of url == just a host it would work, but
I'm suspicious it won't for urls with paths.

https://codereview.chromium.org/292453009/diff/60001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/292453009/diff/60001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:68:
iframe_request_("iframe", GURL("http://www.google.com/some/url")) {}
Shall we have an off-domain iframe request test as well? I know there's like 2^N
possibilities here...

[leng, 2014-05-22 23:57:04]

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/292453009/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:105: bool
is_main_frame = request->GetRequestingHostname() == request_url_;
On 2014/05/22 02:06:31, Greg Billock wrote:
> On 2014/05/22 00:08:47, leng wrote:
> > On 2014/05/21 17:09:19, Greg Billock wrote:
> > > Do we need to extract just the hostport from the request_url_ to do this?
> > 
> > I have no idea.  I don't know enough about main frame vs. iframe requests.
> 
> I think GetLastCommittedURL() is the full url, and GetRequestingHostname() is
> just the hostname portion of the url (formatted as a url), so the root of the
> security context. So in our test cases of url == just a host it would work,
but
> I'm suspicious it won't for urls with paths.

Got it.  I changed it to use GetOrigin() for both - we only care about same
domain vs. other domain, right?

https://codereview.chromium.org/292453009/diff/60001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/292453009/diff/60001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:68:
iframe_request_("iframe", GURL("http://www.google.com/some/url")) {}
On 2014/05/22 02:06:31, Greg Billock wrote:
> Shall we have an off-domain iframe request test as well? I know there's like
2^N
> possibilities here...

I added a bunch, and made it clearer.  (I hope!)

[Greg Billock, 2014-05-27 20:04:55]

Looking good. Just one little policy question left.

https://codereview.chromium.org/292453009/diff/80001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/292453009/diff/80001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:452:
manager_->AddRequest(&iframe_request_same_domain_);
Sorry, one other thing I thought of: we want same-domain iframe requests to
coalesce with the main frame request, right? I don't remember talking about that
specifically, but it seems like it'd be good (the obverse would look weird, but
presumably be pretty uncommon).

Want to modify this test so WaitForCoalescing and WaitForFrameLoad() co-occur
and then we expect view_permission_requests_.size() == 2?

[leng, 2014-05-27 23:03:55]

https://codereview.chromium.org/292453009/diff/80001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/292453009/diff/80001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:452:
manager_->AddRequest(&iframe_request_same_domain_);
On 2014/05/27 20:04:55, Greg Billock wrote:
> Sorry, one other thing I thought of: we want same-domain iframe requests to
> coalesce with the main frame request, right? I don't remember talking about
that
> specifically, but it seems like it'd be good (the obverse would look weird,
but
> presumably be pretty uncommon).
> 
> Want to modify this test so WaitForCoalescing and WaitForFrameLoad() co-occur
> and then we expect view_permission_requests_.size() == 2?

I made the same assumption - that same-domain iframe requests will coalesce with
the main frame request.  The code currently works like that, though
DocumentOnLoadCompletedInMainFrame() and DocumentLoadedInFrame() will both
schedule a call to show the bubble - they don't wait for each other.

For the tests, I added a view_.permission_requeusts_.size() == 2 check to an
earlier test, where WaitForCoalescing() and WaitForFrameLoad() are contiguous.

[Greg Billock, 2014-05-27 23:07:55]

lgtm

https://codereview.chromium.org/292453009/diff/80001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/292453009/diff/80001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:452:
manager_->AddRequest(&iframe_request_same_domain_);
ok, cool

On 2014/05/27 23:03:55, leng wrote:
> On 2014/05/27 20:04:55, Greg Billock wrote:
> > Sorry, one other thing I thought of: we want same-domain iframe requests to
> > coalesce with the main frame request, right? I don't remember talking about
> that
> > specifically, but it seems like it'd be good (the obverse would look weird,
> but
> > presumably be pretty uncommon).
> > 
> > Want to modify this test so WaitForCoalescing and WaitForFrameLoad()
co-occur
> > and then we expect view_permission_requests_.size() == 2?
> 
> I made the same assumption - that same-domain iframe requests will coalesce
with
> the main frame request.  The code currently works like that, though
> DocumentOnLoadCompletedInMainFrame() and DocumentLoadedInFrame() will both
> schedule a call to show the bubble - they don't wait for each other.
> 
> For the tests, I added a view_.permission_requeusts_.size() == 2 check to an
> earlier test, where WaitForCoalescing() and WaitForFrameLoad() are contiguous.

[leng, 2014-05-27 23:31:56]

The CQ bit was checked by leng@chromium.org

[commit-bot: I haz the power, 2014-05-27 23:33:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leng@chromium.org/292453009/100001

[commit-bot: I haz the power, 2014-05-28 03:12:42]

Change committed as 273137