[WebsiteSettings] API for permissions bubbles.

chrome/browser/ui/website_settings/permission_bubble_delegate.h

Index: chrome/browser/ui/website_settings/permission_bubble_delegate.h
diff --git a/chrome/browser/ui/website_settings/permission_bubble_delegate.h b/chrome/browser/ui/website_settings/permission_bubble_delegate.h
new file mode 100644
index 0000000000000000000000000000000000000000..23aeb9c5f6e2a9e96386140ce15d4ec7987ee392
--- /dev/null
+++ b/chrome/browser/ui/website_settings/permission_bubble_delegate.h
@@ -0,0 +1,64 @@
+// Copyright (c) 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.
+
+#ifndef CHROME_BROWSER_UI_WEBSITE_SETTINGS_PERMISSION_BUBBLE_DELEGATE_H_
+#define CHROME_BROWSER_UI_WEBSITE_SETTINGS_PERMISSION_BUBBLE_DELEGATE_H_
+
+// Describes the interface a feature utilizing permission bubbles should

[markusheintz_, 2014/01/07 08:12:35]

Which bubbles do you mean here? From the mocks (maybe I have missed the latest
once) I was under the impression that there will be one bubble with
implementations for each platform.

What is this class supposed to replace? There is the WebsiteSettingsUI
interface, and then there are the classes for the specific permissions / content
settings bubbles.

[Greg Billock, 2014/01/07 17:59:15]

that's right. "bubbles" here means the system as a whole.
This class is a parallel to InfoBarDelegate. 

+// implement. A class of this type is registered with the permission bubble
+// manager to receive updates about the result of the permissions request
+// from the bubble. It should live until it is unregistered or until
+// PermissionsBubbleDestroyed is called.
+// Note that no particular guarantees are made about what exact UI surface
+// is presented to the user. The delegate may be coalesced with other bubble
+// requests, or depending on the situation, not shown at all.
+class PermissionBubbleDelegate {
+ public:
+  virtual ~PermissionBubbleDelegate() {}
+
+  // Return the resource ID of an associated icon. If kNoIconID is returned, no
+  // icon will be shown.
+  virtual int GetIconID() const = 0;
+
+  // Return the prompt text for this permission. This is the central permission
+  // grant text, and must be phrased positively -- the permission bubble may
+  // coalesce different requests, and if it does, this text will be displayed
+  // next to a checkbox indicating the user grants the permission.
+  virtual base::string16 GetMessageText() const = 0;
+
+  // TODO(gbillock): Needed?
+  // Return alternative text for the accept button in the case where this single
+  // permission request is triggered in the bubble.

[groby-ooo-7-16, 2014/01/07 00:31:17]

What is an example for this alternate text?

[Greg Billock, 2014/01/07 17:59:15]

GeolocationInfoBarDelegate::GetButtonLabel for instance. Practically every
infobar has one. I'm not sure we end up wanting this, but if we required it for
infobars, we may want to account for it.

+  // If the permission request is coalesced, the text will revert to the default
+  // "Accept"-alike, so the message text must be clear enough for users to
+  // understand even if this text is not used.
+  virtual base::string16 GetAlternateAcceptButtonText() const = 0;
+
+  // Return the link text for this permission. If non-empty, a link near the
+  // |GetMessageText()| request is shown, allowing the user to get more
+  // information.
+  virtual base::string16 GetLinkText() const = 0;
+
+  // The explanatory link was clicked by the user.
+  virtual void LinkClicked() = 0;

[groby-ooo-7-16, 2014/01/07 00:31:17]

I thought we're not doing the explanation link any more?

Also, if we do support links, I'd rather have a centralized handling for them -
have the delegate specify a URL, instead of asking each implementor to add
navigation code.

[Greg Billock, 2014/01/07 17:59:15]

Yeah, I copy-pasted a couple methods in here that I'm not sure will survive,
mostly to illustrate that I think we can extend this approach to pretty readily
cover all the infobar cases without going too crazy.

+
+  // The user has granted the requested permission.
+  virtual void PermissionGranted() = 0;
+
+  // The user has denied the requested permission.
+  virtual void PermissionDenied() = 0;

[groby-ooo-7-16, 2014/01/07 00:31:17]

I really think Granted/Denied should be a callback.

[Greg Billock, 2014/01/07 17:59:15]

You mean the delegate returns a callback? Or the registration happens with
Add...(delegate, callback, callback) ?

I don't see why that'd be preferable. The caller has to implement the delegate
interface anyway.

[groby-ooo-7-16, 2014/01/07 19:44:57]

I meant registration via add(). You're probably right that the delegate might as
well host the Callback - it blurs a separation line, but might be livable. I
haven't compiled a list of current permission invocations yet to check if it's
OK to tie the two together.

[Greg Billock, 2014/01/07 20:31:19]

For infobars generically, I think that's more true, but using inheritance
delegates and then the odd callback here seems strange. And this interface is
open-ended enough that inheritance makes sense.

+
+  // The user has cancelled the permission request. This corresponds to a
+  // denial, but is segregated in case the context needs to be able to
+  // distinguish between an active refusal or an implicit refusal.
+  virtual void Cancelled() = 0;

[groby-ooo-7-16, 2014/01/07 00:31:17]

_Can_ the user cancel? The current UX  only has an OK button.

[Greg Billock, 2014/01/07 17:59:15]

The tab can be closed. Shutdown may happen. I couldn't think of a good
alternative to Cancelled(). I considered just having PermissionBubbleDestroyed()
callable without Granted/Denied. That still seems viable, but this way there's a
definite "result" step, which is nice.

+
+  // The bubble this delegate was associated with was destroyed. It is safe
+  // for the delegate to be deleted at this point -- it will receive no further
+  // message from the permission bubble system. This method will eventually be
+  // called on every delegate which is not unregistered.

[groby-ooo-7-16, 2014/01/07 00:31:17]

Could we make this a not self-deleting object? There is _always_ trouble around
the exact ownership of self-deleting things. What is the technical upside of
this added complexity?

[Greg Billock, 2014/01/07 17:59:15]

These aren't self-deleting. I mean, you could make them self-deleting, but you
probably wouldn't. Because the caller owns the delegate during the lifecycle, it
needs a clear signal when it can release the delegate. This is that.

[groby-ooo-7-16, 2014/01/07 19:44:57]

If the delegate has a callback that indicates it's safe to delete the callback,
it inevitably will be self-deleting. I question the need for the caller to
control the life cycle of the request in general.

[Greg Billock, 2014/01/07 20:31:19]

The caller is free to ignore this if it doesn't care. If you wish the PBM owned
the delegates, you can mimic that architecture with one line of code. If you
wish you just had an interface method, you can mimic that in one line of code. I
don't think this is a real differentiator.

[groby-ooo-7-16, 2014/01/07 22:28:41]

Based on numerous "what is the $#&*(@ lifetime now" around self-deleting
objects, I'm strongly opposed to their use unless there's an absolute need for
them. I'd rather not have consumers make that choice unless there's an explicit
reason for that choice.

+  virtual PermissionBubbleDestroyed() = 0;
+};
+
+#endif  // CHROME_BROWSER_UI_WEBSITE_SETTINGS_PERMISSION_BUBBLE_DELEGATE_H_