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

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

This adds cancellation ability for the clients that will need it. Adds
user gesture awareness to the API. Removes button customization. Adds icons.
Some code reorganization as well.

R=leng@google.com
BUG=332115
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255889

[Greg Billock, 2014-02-25 19:07:40]

On 2014/02/22 01:31:58, Greg Billock wrote:

leng will be gone for a while. Can one of you take a look? Interesting bits are
in website_settings view/manager/request.

[groby-ooo-7-16, 2014-02-25 22:27:36]

Does what it says on the box. Mostly questions around APIs, plus a few nits.

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

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:51:
requests_.clear();
Why clear? The dtor does that already?

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:71:
request->RequestFinished();
Is that really "RequestFinished"? Shouldn't that be treated as a still pending
request?

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:75: for
(requests_iter = queued_requests_.begin();
This is a lot of duplicated code - worth factoring out an internal
AddRequest(std::vector<>, Request*) ?

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:87: if
(bubble_showing_) {
It's technically outside of this CL's scope, but: Why not ask the view_ if it's
showing?

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:104: }
Why add this if it doesn't do anything yet?

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:326:
manager_->AddRequest(&request1_);
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?

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_request.h (right):

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_request.h:23: // Must be a
valid icon of size 16x16. (TODO(gbillock): tbd size)
So is size tbd or 16x16? :)

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_request.h:24: virtual int
GetIconID() const = 0;
I'm wondering if it's worth making a const icon_id_ member on the base class and
passing the ID in the ctor

Slightly less boilerplate for derived classes, but an impure base class.

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_request.h:32: // as a
heading, so like "Location" or "Camera". The permission bubble may
"for example" or "e.g." instead of "so like"

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_view.h (right):

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_view.h:41: //
|CanAcceptRequestUpdate()| is true.
nit: Don't we usually only use that on variable names?

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_view.h:47: // Returns true
if the view can accept a new Show() command to coalesce requests.
More than 80 chars per line

[Greg Billock, 2014-02-26 00:28:44]

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

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:51:
requests_.clear();
On 2014/02/25 22:27:36, groby wrote:
> Why clear? The dtor does that already?

Just gives me a warm fuzzy. :-) I'll take it out.

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:71:
request->RequestFinished();
On 2014/02/25 22:27:36, groby wrote:
> Is that really "RequestFinished"? Shouldn't that be treated as a still pending
> request?

This is basically rejecting requests that the user won't be able to distinguish.
I need to add a TODO here to worry about different requesting hosts, though. We
don't have an overall plan how to deal with those yet...

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:87: if
(bubble_showing_) {
On 2014/02/25 22:27:36, groby wrote:
> It's technically outside of this CL's scope, but: Why not ask the view_ if
it's
> showing?

The way I have the handshake working right now, the toolkit makes and owns the
view implementation, which then gets told to the manager. The manager is in
charge of figuring out whether the bubble should be shown, since there's only
one view class per browser, and the decision about bubble showing is per-tab
(the manager is per-tab).

IOW, the same view impl should be shown in tab 1 and not in tab 2 (but with
different contents).

At one point I played with calling this bubble_should_be_shown_, which is
arguably a bit better of a name.

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:104: }
On 2014/02/25 22:27:36, groby wrote:
> Why add this if it doesn't do anything yet?

I was attempting to keep the scope of the CL down. This'll require more than
trivial work at the call sites, so my plan is to do it when I do that. Could
remove the method signature for now, though...

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:326:
manager_->AddRequest(&request1_);
On 2014/02/25 22:27:36, groby wrote:
> 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?

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. :-)

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_request.h (right):

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_request.h:23: // Must be a
valid icon of size 16x16. (TODO(gbillock): tbd size)
On 2014/02/25 22:27:36, groby wrote:
> So is size tbd or 16x16? :)

:-) This is more like "when we decide, write that size here instead of 16x16"

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_request.h:24: virtual int
GetIconID() const = 0;
On 2014/02/25 22:27:36, groby wrote:
> I'm wondering if it's worth making a const icon_id_ member on the base class
and
> passing the ID in the ctor
> 
> Slightly less boilerplate for derived classes, but an impure base class.

Yeah, the same suggestion was made for the strings as well. The pure interface
is nice, so I've left it like that for now.

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_request.h:32: // as a
heading, so like "Location" or "Camera". The permission bubble may
On 2014/02/25 22:27:36, groby wrote:
> "for example" or "e.g." instead of "so like"

Done.

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_view.h (right):

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_view.h:41: //
|CanAcceptRequestUpdate()| is true.
On 2014/02/25 22:27:36, groby wrote:
> nit: Don't we usually only use that on variable names?

Done.

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_view.h:47: // Returns true
if the view can accept a new Show() command to coalesce requests.
On 2014/02/25 22:27:36, groby wrote:
> More than 80 chars per line

Done.

[groby-ooo-7-16, 2014-02-26 02:28:11]

LGTM from my POV, but you'll need to find an OWNER.

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/176053002/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:326:
manager_->AddRequest(&request1_);
On 2014/02/26 00:28:44, Greg Billock wrote:
> On 2014/02/25 22:27:36, groby wrote:
> > 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?
> 
> 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. :-)

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

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

https://codereview.chromium.org/176053002/diff/60001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:66: //
TODO(gbillock): worry about the requesting host name as well.
I think I already suggested it offline - I wonder if requests should simply
generate a digest, based on whatever their identifying characteristics are.
(Granted, it'd need to be a perfect hash for security reasons. The sec. team
won't like "probably won't collide" ;)

[Greg Billock, 2014-02-26 23:37:41]

+pkasting for browser.cc owners
+fischman for media owners
+timvolodine for geoloc owners
+dewittj for notifications owners

(head's up: another couple changes incoming to plumb through the user gesture in
a few of these cases -- I'll try to split those up into smaller bites.)

[Peter Kasting, 2014-02-26 23:40:02]

On 2014/02/26 23:37:41, Greg Billock wrote:
> +pkasting for browser.cc owners

LGTM.  If you have multiple files that have trivial changes like this you're
welcome to TBR or get a chrome/ root owner to approve all at once.

[Ami GONE FROM CHROMIUM, 2014-02-26 23:59:01]

media RS LGTM

[dewittj, 2014-02-27 00:19:43]

https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (left):

https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:141: return
l10n_util::GetStringUTF16(IDS_NOTIFICATION_PERMISSION_YES);
Does this mean that Yes/No is the default?

https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:146: return false;
Chrome requires that the requestPermission method be called in response to a
user gesture (i.e. in onClick handlers).  So this could probably just return
true?

[Greg Billock, 2014-02-27 00:39:43]

https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (left):

https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:141: return
l10n_util::GetStringUTF16(IDS_NOTIFICATION_PERMISSION_YES);
On 2014/02/27 00:19:43, dewittj wrote:
> Does this mean that Yes/No is the default?

Current plan is that the bubbles will always say "Allow" and "Block" with no
override possible.

https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:146: return false;
On 2014/02/27 00:19:43, dewittj wrote:
> Chrome requires that the requestPermission method be called in response to a
> user gesture (i.e. in onClick handlers).  So this could probably just return
> true?

Yes. I discovered this doing the plumbing. :-) I think the subsequent CL takes
out this TODO and adds that note, but I can just as well do it here.

[Greg Billock, 2014-02-27 19:01:18]

On 2014/02/27 00:39:43, Greg Billock wrote:
>
https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
> File chrome/browser/notifications/desktop_notification_service.cc (left):
> 
>
https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
> chrome/browser/notifications/desktop_notification_service.cc:141: return
> l10n_util::GetStringUTF16(IDS_NOTIFICATION_PERMISSION_YES);
> On 2014/02/27 00:19:43, dewittj wrote:
> > Does this mean that Yes/No is the default?
> 
> Current plan is that the bubbles will always say "Allow" and "Block" with no
> override possible.
> 
>
https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
> File chrome/browser/notifications/desktop_notification_service.cc (right):
> 
>
https://codereview.chromium.org/176053002/diff/70001/chrome/browser/notificat...
> chrome/browser/notifications/desktop_notification_service.cc:146: return
false;
> On 2014/02/27 00:19:43, dewittj wrote:
> > Chrome requires that the requestPermission method be called in response to a
> > user gesture (i.e. in onClick handlers).  So this could probably just return
> > true?
> 
> Yes. I discovered this doing the plumbing. :-) I think the subsequent CL takes
> out this TODO and adds that note, but I can just as well do it here.

+rdsmith for the downloads changes

[Greg Billock, 2014-03-03 16:15:42]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-03 16:15:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/176053002/140001

[commit-bot: I haz the power, 2014-03-03 16:48:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-03 16:48:18]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Greg Billock, 2014-03-05 20:06:32]

On 2014/03/03 16:48:18, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

+thakis for RPH and quota

timvolodine, rdsmith, dewittj please have a look

[dewittj, 2014-03-05 20:48:22]

c/b/notifications lgtm

[Nico, 2014-03-05 22:43:41]

first 3 files lgtm

[Greg Billock, 2014-03-06 18:10:44]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-06 18:12:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/176053002/140001

[commit-bot: I haz the power, 2014-03-06 20:54:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/176053002/140001

[commit-bot: I haz the power, 2014-03-06 21:42:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/176053002/140001

[commit-bot: I haz the power, 2014-03-06 22:17:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/176053002/140001

[commit-bot: I haz the power, 2014-03-07 05:03:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 05:03:49]

Retried try job too often on mac_rel for step(s) telemetry_perf_unittests,
telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Greg Billock, 2014-03-07 18:26:16]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-07 18:26:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/176053002/140001

[Greg Billock, 2014-03-07 18:26:58]

The CQ bit was unchecked by gbillock@chromium.org

[Greg Billock, 2014-03-08 04:14:49]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-08 10:40:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/176053002/160001

[commit-bot: I haz the power, 2014-03-08 12:07:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-08 12:07:34]

Try jobs failed on following builders: mac_chromium_compile_dbg

[Greg Billock, 2014-03-08 16:53:44]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-08 16:54:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/176053002/180001

[commit-bot: I haz the power, 2014-03-08 17:37:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-08 17:37:28]

Try jobs failed on following builders: mac_chromium_compile_dbg

[Greg Billock, 2014-03-09 22:33:33]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-09 22:33:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/176053002/220001

[commit-bot: I haz the power, 2014-03-10 06:39:56]

Change committed as 255889