Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
121113007:
[WebsiteSettings] API for permissions bubbles. (Closed)
|
Created:
6 years, 11 months ago by Greg Billock Modified:
6 years, 11 months ago CC:
chromium-reviews, markusheintz_ Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[WebsiteSettings] API for permissions bubbles.
This introduces a new API for clients to prepare permission bubbles
for display to the user. Currently Follows the API of the InfoBar system
pretty closely.
BUG=332115
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245396
Patch Set 1 #
Total comments: 39
Patch Set 2 : comment #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : Add view, gypi #Patch Set 7 : Flesh out impl #Patch Set 8 : . #
Total comments: 4
Patch Set 9 : . #Patch Set 10 : . #Patch Set 11 : destructor #
Messages
Total messages: 25 (0 generated)
Quick first pass. Also, it'd be great to have a few common use cases: How to request a specific permission (bubble), under what circumstances does the caller need the delegate pointer at all, etc. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_delegate.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:32: // permission request is triggered in the bubble. What is an example for this alternate text? https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:44: virtual void LinkClicked() = 0; 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:50: virtual void PermissionDenied() = 0; I really think Granted/Denied should be a callback. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:55: virtual void Cancelled() = 0; _Can_ the user cancel? The current UX only has an OK button. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:60: // called on every delegate which is not unregistered. 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? https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.cc:23: } Let's do this with STL - delegates_.erase(std::remove(delegates_.begin(), delegates_.end(), delegate), delegates_.end()) https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.cc:33: content::WebContents* web_contents) { I'm aware InfoBarService does this, but I don't think this paranoia is warranted. What possible case is this covering? https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_manager.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:22: // the associated WebContents. Why should permission requests outlive their WebContents? Aren't they implicitly tied to WebContents? https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:31: // to outlive the PermissionBubbleManager. Why? What is the benefit for the caller to keep ownership? https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:32: virtual void AddPermissionBubbleDelegate(PermissionBubbleDelegate* delegate); This is requesting a permission - adding the delegate is merely an implementation detail. I'd prefer the name to reflect the intent, not the implementation. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:34: // Remove a consumer delegate from the permission bubble. In which case would this happen?
https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_delegate.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:8: // Describes the interface a feature utilizing permission bubbles should 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.
Could we have a bug to track the actual feature work? Would be a great place to attache mocks too.
https://code.google.com/p/chromium/issues/detail?id=332115 Ruby, do you want to turn this into the feature review bug/tracking bug/launch bug/whatever? Let's link the design doc there and put mocks and such as Mark suggests. On Tue, Jan 7, 2014 at 12:13 AM, <markusheintz@chromium.org> wrote: > Could we have a bug to track the actual feature work? Would be a great > place to > attache mocks too. > > https://codereview.chromium.org/121113007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_delegate.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:8: // Describes the interface a feature utilizing permission bubbles should On 2014/01/07 08:12:35, markusheintz_ wrote: > 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. that's right. "bubbles" here means the system as a whole. > 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. This class is a parallel to InfoBarDelegate. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:32: // permission request is triggered in the bubble. On 2014/01/07 00:31:17, groby wrote: > What is an example for this alternate text? 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:44: virtual void LinkClicked() = 0; On 2014/01/07 00:31:17, groby wrote: > 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. 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:50: virtual void PermissionDenied() = 0; On 2014/01/07 00:31:17, groby wrote: > I really think Granted/Denied should be a callback. 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:55: virtual void Cancelled() = 0; On 2014/01/07 00:31:17, groby wrote: > _Can_ the user cancel? The current UX only has an OK button. 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:60: // called on every delegate which is not unregistered. On 2014/01/07 00:31:17, groby wrote: > 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? 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_manager.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:22: // the associated WebContents. On 2014/01/07 00:31:17, groby wrote: > Why should permission requests outlive their WebContents? Aren't they implicitly > tied to WebContents? I botched this. I mean the "WebContents' PBM" but that's a bit of a mouthfull. I'll reword. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:31: // to outlive the PermissionBubbleManager. On 2014/01/07 00:31:17, groby wrote: > Why? What is the benefit for the caller to keep ownership? We need some sort of pathway to get back to them. They have to implement an interface which is kept alive the life of the bubble anyway, so consolidating it there is convenient and hard to screw up. In the infobar system, the delegates are owned by the infobar, so they all have another layer of indirection to reconnect to their target object, with auto-implicit lifetime arrangements. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:32: virtual void AddPermissionBubbleDelegate(PermissionBubbleDelegate* delegate); On 2014/01/07 00:31:17, groby wrote: > This is requesting a permission - adding the delegate is merely an > implementation detail. I'd prefer the name to reflect the intent, not the > implementation. Yeah, I toyed with a couple more focused names, but didn't find anything I liked better. One reason is that this is intended to be UI-only -- it's not intended to encompass all the permissions policy and fussing about with the profile and such. If we can abstract that to some functional helper, that's more where the "RequestPermission" type call belongs. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:34: // Remove a consumer delegate from the permission bubble. On 2014/01/07 00:31:17, groby wrote: > In which case would this happen? Not sure, but if history is any guide, it's sure to.
Yep, that works. Looks like it's a public bug - the link to the PRD is: https://docs.google.com/a/google.com/document/d/15jzaoL7JVJFlLt5nIO4Q1JNPwSh_... . Thanks, Ruby On Tue, Jan 7, 2014 at 9:21 AM, Greg Billock <gbillock@chromium.org> wrote: > https://code.google.com/p/chromium/issues/detail?id=332115 > > Ruby, do you want to turn this into the feature review bug/tracking > bug/launch bug/whatever? Let's link the design doc there and put mocks and > such as Mark suggests. > > > On Tue, Jan 7, 2014 at 12:13 AM, <markusheintz@chromium.org> wrote: > >> Could we have a bug to track the actual feature work? Would be a great >> place to >> attache mocks too. >> >> https://codereview.chromium.org/121113007/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
You could make it private, but I don't know that we'd want to. On Tue, Jan 7, 2014 at 11:30 AM, Ruby Lee <rubylee@google.com> wrote: > Yep, that works. Looks like it's a public bug - the link to the PRD is: > https://docs.google.com/a/google.com/document/d/15jzaoL7JVJFlLt5nIO4Q1JNPwSh_... > . > > Thanks, > Ruby > > > On Tue, Jan 7, 2014 at 9:21 AM, Greg Billock <gbillock@chromium.org>wrote: > >> https://code.google.com/p/chromium/issues/detail?id=332115 >> >> Ruby, do you want to turn this into the feature review bug/tracking >> bug/launch bug/whatever? Let's link the design doc there and put mocks and >> such as Mark suggests. >> >> >> On Tue, Jan 7, 2014 at 12:13 AM, <markusheintz@chromium.org> wrote: >> >>> Could we have a bug to track the actual feature work? Would be a great >>> place to >>> attache mocks too. >>> >>> https://codereview.chromium.org/121113007/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_delegate.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:50: virtual void PermissionDenied() = 0; On 2014/01/07 17:59:15, Greg Billock wrote: > On 2014/01/07 00:31:17, groby wrote: > > I really think Granted/Denied should be a callback. > > 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. 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:60: // called on every delegate which is not unregistered. On 2014/01/07 17:59:15, Greg Billock wrote: > On 2014/01/07 00:31:17, groby wrote: > > 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? > > 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. 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_manager.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:22: // the associated WebContents. On 2014/01/07 17:59:15, Greg Billock wrote: > On 2014/01/07 00:31:17, groby wrote: > > Why should permission requests outlive their WebContents? Aren't they > implicitly > > tied to WebContents? > > I botched this. I mean the "WebContents' PBM" but that's a bit of a mouthfull. > I'll reword. Since the requests are managed by the PBM, I'm not sure why they should outlive it. Again, this goes to ownership model - I don't think permission requests need to be owned by anybody but the PBM. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:31: // to outlive the PermissionBubbleManager. On 2014/01/07 17:59:15, Greg Billock wrote: > On 2014/01/07 00:31:17, groby wrote: > > Why? What is the benefit for the caller to keep ownership? > > We need some sort of pathway to get back to them. They have to implement an > interface which is kept alive the life of the bubble anyway, so consolidating it > there is convenient and hard to screw up. > > In the infobar system, the delegates are owned by the infobar, so they all have > another layer of indirection to reconnect to their target object, with > auto-implicit lifetime arrangements. Why do we need to get back to them? What is the use case there? And I'm all for keeping it alive during the bubble duration - that's why PBM should own them. And manage the bubble, so it can delete the delegates when the bubble goes away. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:32: virtual void AddPermissionBubbleDelegate(PermissionBubbleDelegate* delegate); On 2014/01/07 17:59:15, Greg Billock wrote: > On 2014/01/07 00:31:17, groby wrote: > > This is requesting a permission - adding the delegate is merely an > > implementation detail. I'd prefer the name to reflect the intent, not the > > implementation. > > Yeah, I toyed with a couple more focused names, but didn't find anything I liked > better. One reason is that this is intended to be UI-only -- it's not intended > to encompass all the permissions policy and fussing about with the profile and > such. If we can abstract that to some functional helper, that's more where the > "RequestPermission" type call belongs. Let's not bikeshed on it right now (mostly because I don't have a good name :), but the final version hopefully won't talk about type names. (I still like "AddPermissionRequest", because that's what it does. Maybe "AddUserPermissionRequest" or somesuch) https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:34: // Remove a consumer delegate from the permission bubble. On 2014/01/07 17:59:15, Greg Billock wrote: > On 2014/01/07 00:31:17, groby wrote: > > In which case would this happen? > > Not sure, but if history is any guide, it's sure to. Well, let's add it when it is :) (More lifecycle - if PBM owns the requests, no need to retract them)
As background, the reason I'm attempting to keep scope very small for this UI piece and have a platform-independent view interface on one side and a feature-independent interface on the other is that I think this class is going to grow up to be rather complicated. It's going to have to handle questions about view display per-webcontents, about coalescing, about timings, and so on. Since there's a fair bit of complexity that is going to end up in it, I would like to sharply limit the amount of stuff it is responsible for in order to make that responsibilities the focus of the class. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_delegate.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:50: virtual void PermissionDenied() = 0; On 2014/01/07 19:44:57, groby wrote: > On 2014/01/07 17:59:15, Greg Billock wrote: > > On 2014/01/07 00:31:17, groby wrote: > > > I really think Granted/Denied should be a callback. > > > > 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. > > 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. 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:60: // called on every delegate which is not unregistered. On 2014/01/07 19:44:57, groby wrote: > On 2014/01/07 17:59:15, Greg Billock wrote: > > On 2014/01/07 00:31:17, groby wrote: > > > 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? > > > > 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. > > 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. 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_manager.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:22: // the associated WebContents. On 2014/01/07 19:44:57, groby wrote: > On 2014/01/07 17:59:15, Greg Billock wrote: > > On 2014/01/07 00:31:17, groby wrote: > > > Why should permission requests outlive their WebContents? Aren't they > > implicitly > > > tied to WebContents? > > > > I botched this. I mean the "WebContents' PBM" but that's a bit of a mouthfull. > > I'll reword. > > Since the requests are managed by the PBM, I'm not sure why they should outlive > it. Again, this goes to ownership model - I don't think permission requests need > to be owned by anybody but the PBM. Take this case: tab 1 requests a permission. User switches to tab 2. User switches back to tab 1. User allows the permission request. Something in the scope of tab 1 has to remain alive during this entire process to collect the response to the permission request. Something has to re-populate the bubble contents after the tab change. Currently in infobars the delegate does this, and the forwards responses to an inner member. This works basically the same, but leaves that whole object graph within the client. So the UI code handles the UI object graph, the client code handles the response object graph. The manager is the API mediator. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:31: // to outlive the PermissionBubbleManager. On 2014/01/07 19:44:57, groby wrote: > On 2014/01/07 17:59:15, Greg Billock wrote: > > On 2014/01/07 00:31:17, groby wrote: > > > Why? What is the benefit for the caller to keep ownership? > > > > We need some sort of pathway to get back to them. They have to implement an > > interface which is kept alive the life of the bubble anyway, so consolidating > it > > there is convenient and hard to screw up. > > > > In the infobar system, the delegates are owned by the infobar, so they all > have > > another layer of indirection to reconnect to their target object, with > > auto-implicit lifetime arrangements. > > Why do we need to get back to them? What is the use case there? The "user allowed your permission request" use case. That's universally true, otherwise, the client wouldn't even bother calling in the first place. > And I'm all for keeping it alive during the bubble duration - that's why PBM > should own them. And manage the bubble, so it can delete the delegates when the > bubble goes away. Then you have the infobar architecture, which gets the job done, but with N separate lifetime arrangements between delegate and client. That makes more sense for infobars, since a lot of the delegates aren't about requests, but basically go off and call other global browser functions. For permissions, where each invocation is associated with a particular request, there's no need to double-joint it. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:32: virtual void AddPermissionBubbleDelegate(PermissionBubbleDelegate* delegate); On 2014/01/07 19:44:57, groby wrote: > On 2014/01/07 17:59:15, Greg Billock wrote: > > On 2014/01/07 00:31:17, groby wrote: > > > This is requesting a permission - adding the delegate is merely an > > > implementation detail. I'd prefer the name to reflect the intent, not the > > > implementation. > > > > Yeah, I toyed with a couple more focused names, but didn't find anything I > liked > > better. One reason is that this is intended to be UI-only -- it's not intended > > to encompass all the permissions policy and fussing about with the profile and > > such. If we can abstract that to some functional helper, that's more where the > > "RequestPermission" type call belongs. > > Let's not bikeshed on it right now (mostly because I don't have a good name :), > but the final version hopefully won't talk about type names. (I still like > "AddPermissionRequest", because that's what it does. Maybe > "AddUserPermissionRequest" or somesuch) I think you may have a different idea of the scope of this class. The scope I'm envisioning for it is extremely confined: show the permissions UI, and that's it. If there's profile policy negotiation to be done, or permission policy investigative work to do, the caller has to do it. There may be an abstraction that's useful for that, but this code doesn't do it. It just puts the pixels on the screen when it is told. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:34: // Remove a consumer delegate from the permission bubble. On 2014/01/07 19:44:57, groby wrote: > On 2014/01/07 17:59:15, Greg Billock wrote: > > On 2014/01/07 00:31:17, groby wrote: > > > In which case would this happen? > > > > Not sure, but if history is any guide, it's sure to. > > Well, let's add it when it is :) (More lifecycle - if PBM owns the requests, no > need to retract them) Sure, but then retraction becomes awkward, and you end up with ReplaceInfoBar type of architecture, where you crawl through the existing delegates, using homebrew RTTI to figure out if you've ever added an equivalent one before. That's specifically the thing I'm avoiding here -- that logic must all be external, and if you decide you hate the permission you asked for, and want a new one, then you just Remove(old); Add(new).
Overall, I'm thinking my mental model of the architecture is significantly different from yours. I'm happy to continue hashing this out here, but if you'd prefer me to write a doc (or a proposal API) to explain my model, please LMK. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_delegate.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_delegate.h:60: // called on every delegate which is not unregistered. On 2014/01/07 20:31:19, Greg Billock wrote: > On 2014/01/07 19:44:57, groby wrote: > > On 2014/01/07 17:59:15, Greg Billock wrote: > > > On 2014/01/07 00:31:17, groby wrote: > > > > 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? > > > > > > 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. > > > > 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. > > 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. 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. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... File chrome/browser/ui/website_settings/permission_bubble_manager.h (right): https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:22: // the associated WebContents. On 2014/01/07 20:31:19, Greg Billock wrote: > On 2014/01/07 19:44:57, groby wrote: > > On 2014/01/07 17:59:15, Greg Billock wrote: > > > On 2014/01/07 00:31:17, groby wrote: > > > > Why should permission requests outlive their WebContents? Aren't they > > > implicitly > > > > tied to WebContents? > > > > > > I botched this. I mean the "WebContents' PBM" but that's a bit of a > mouthfull. > > > I'll reword. > > > > Since the requests are managed by the PBM, I'm not sure why they should > outlive > > it. Again, this goes to ownership model - I don't think permission requests > need > > to be owned by anybody but the PBM. > > Take this case: tab 1 requests a permission. User switches to tab 2. User > switches back to tab 1. User allows the permission request. > > Something in the scope of tab 1 has to remain alive during this entire process > to collect the response to the permission request. Something has to re-populate > the bubble contents after the tab change. Currently in infobars the delegate > does this, and the forwards responses to an inner member. This works basically > the same, but leaves that whole object graph within the client. So the UI code > handles the UI object graph, the client code handles the response object graph. > The manager is the API mediator. PBM is in the scope of tab1, no? It's tied to WebContents. And I don't _want_ the graph within the client, because it puts burden on the implementers. The use case, as I see it, is that we have various facilities that a) Ask for user permissions to do something b) Do something as the user reacts. There's no need for them to manage the UI object graph directly, because they shouldn't be concerned with is. It's a query and a callback. https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:31: // to outlive the PermissionBubbleManager. On 2014/01/07 20:31:19, Greg Billock wrote: > On 2014/01/07 19:44:57, groby wrote: > > On 2014/01/07 17:59:15, Greg Billock wrote: > > > On 2014/01/07 00:31:17, groby wrote: > > > > Why? What is the benefit for the caller to keep ownership? > > > > > > We need some sort of pathway to get back to them. They have to implement an > > > interface which is kept alive the life of the bubble anyway, so > consolidating > > it > > > there is convenient and hard to screw up. > > > > > > In the infobar system, the delegates are owned by the infobar, so they all > > have > > > another layer of indirection to reconnect to their target object, with > > > auto-implicit lifetime arrangements. > > > > Why do we need to get back to them? What is the use case there? > > The "user allowed your permission request" use case. That's universally true, > otherwise, the client wouldn't even bother calling in the first place. > > > And I'm all for keeping it alive during the bubble duration - that's why PBM > > should own them. And manage the bubble, so it can delete the delegates when > the > > bubble goes away. > > Then you have the infobar architecture, which gets the job done, but with N > separate lifetime arrangements between delegate and client. That makes more > sense for infobars, since a lot of the delegates aren't about requests, but > basically go off and call other global browser functions. For permissions, where > each invocation is associated with a particular request, there's no need to > double-joint it. But the caller doesn't need the object for the pathway back. There will be a OnCancel/OnSuccess on the delegate, which needs to talk to the caller. When does the caller need to talk to the delegate? I feel I'm missing something here - please help me find it :) As I see it, the caller does something akin to AskUserPermissionForContent(web_content, new SpecificPermissionBubbleDelegate(some_state_associated_with_request)); The delegate will do the right thing for both cancel and success. It will utilize the state associated with the request to do that, whatever that state is. What I fail to see is where the delegate is ever kept around. The current InfoBar architecture doesn't keep the delegate around, either? https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:32: virtual void AddPermissionBubbleDelegate(PermissionBubbleDelegate* delegate); On 2014/01/07 20:31:19, Greg Billock wrote: > On 2014/01/07 19:44:57, groby wrote: > > On 2014/01/07 17:59:15, Greg Billock wrote: > > > On 2014/01/07 00:31:17, groby wrote: > > > > This is requesting a permission - adding the delegate is merely an > > > > implementation detail. I'd prefer the name to reflect the intent, not the > > > > implementation. > > > > > > Yeah, I toyed with a couple more focused names, but didn't find anything I > > liked > > > better. One reason is that this is intended to be UI-only -- it's not > intended > > > to encompass all the permissions policy and fussing about with the profile > and > > > such. If we can abstract that to some functional helper, that's more where > the > > > "RequestPermission" type call belongs. > > > > Let's not bikeshed on it right now (mostly because I don't have a good name > :), > > but the final version hopefully won't talk about type names. (I still like > > "AddPermissionRequest", because that's what it does. Maybe > > "AddUserPermissionRequest" or somesuch) > > I think you may have a different idea of the scope of this class. The scope I'm > envisioning for it is extremely confined: show the permissions UI, and that's > it. If there's profile policy negotiation to be done, or permission policy > investigative work to do, the caller has to do it. There may be an abstraction > that's useful for that, but this code doesn't do it. It just puts the pixels on > the screen when it is told. I think my idea of the scope of the _delegate_ is what differs. The work of doing policy work is on it. (That's why I suggested in my original e-mail that we have a Context/Controller that's passed in to the permission request) https://codereview.chromium.org/121113007/diff/1/chrome/browser/ui/website_se... chrome/browser/ui/website_settings/permission_bubble_manager.h:34: // Remove a consumer delegate from the permission bubble. On 2014/01/07 20:31:19, Greg Billock wrote: > On 2014/01/07 19:44:57, groby wrote: > > On 2014/01/07 17:59:15, Greg Billock wrote: > > > On 2014/01/07 00:31:17, groby wrote: > > > > In which case would this happen? > > > > > > Not sure, but if history is any guide, it's sure to. > > > > Well, let's add it when it is :) (More lifecycle - if PBM owns the requests, > no > > need to retract them) > > Sure, but then retraction becomes awkward, and you end up with ReplaceInfoBar > type of architecture, where you crawl through the existing delegates, using > homebrew RTTI to figure out if you've ever added an equivalent one before. > That's specifically the thing I'm avoiding here -- that logic must all be > external, and if you decide you hate the permission you asked for, and want a > new one, then you just Remove(old); Add(new). > But that means that this is handled individually by each delegate. I don't think policies are wildly different, and having each delegate implement this increases the burden on the API consumer. And that is what currently happens in the current infobar architecture - all the delegates call ReplaceInfoBar. There's no need for it. All that needs to happen in PBM is: * see if there's an existing infobar with equivalent type. if (newDelegate->IsEquivalent(otherDelegate)...) - yes, it's a bit of RTTI, but no casting/switching on types. * If there is, ask the new infobar to subsume the old one, replacing the old delegate with the new one. otherDelegate->Merge(newDelegate) * Potentially update the UI This allows delegates to implement specific logic around replacement/mergin without being concerned what the visual expression is. (I.e no need to Add/Remove other delegates)
I saw the doc you had before -- it looked pretty similar to this, but it now sounds like you want to do a whole permission-checking protocol inside this class. I haven't formed an opinion about whether that is a thing that can be abstracted or not, but I'm quite certain it doesn't belong in this class. If there's a profile-checking, defaults-inspecting thing to be written, it should be written separately. The process this class intends to abstract is permission coalescing. There's a separate cross-platform abstraction that's kind of hiding behind this one in a view interface. I'm making a CL for that right now. If there's an additional layer of profile-checking abstraction we can put in front of it, that's fine, but it shouldn't be included in this interface in my opinion. If you think responsibilities should be sliced differently, yeah, write up a doc about it. If it helps to understand, consider the PermissionBubbleDelegate to be composed of two halves: the GetMessageText half that drives the UI and the PermissionAccepted half that consumes the result. It would be possible to pass the two halves separately to the API, but they'd always go together, and I'm having trouble seeing why it's an advantage for the client to have them separate -- if such a need arises for some client (and it hasn't for infobars), they can make a trivial forwarder to do it. On Tue, Jan 7, 2014 at 2:28 PM, <groby@chromium.org> wrote: > Overall, I'm thinking my mental model of the architecture is significantly > different from yours. I'm happy to continue hashing this out here, but if > you'd > prefer me to write a doc (or a proposal API) to explain my model, please > LMK. > > > > > > https://codereview.chromium.org/121113007/diff/1/chrome/ > browser/ui/website_settings/permission_bubble_delegate.h > File chrome/browser/ui/website_settings/permission_bubble_delegate.h > (right): > > https://codereview.chromium.org/121113007/diff/1/chrome/ > browser/ui/website_settings/permission_bubble_delegate.h#newcode60 > chrome/browser/ui/website_settings/permission_bubble_delegate.h:60: // > called on every delegate which is not unregistered. > On 2014/01/07 20:31:19, Greg Billock wrote: > >> On 2014/01/07 19:44:57, groby wrote: >> > On 2014/01/07 17:59:15, Greg Billock wrote: >> > > On 2014/01/07 00:31:17, groby wrote: >> > > > 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? >> > > >> > > 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. > >> > >> > 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. >> > > 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. >> > > 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. > > > https://codereview.chromium.org/121113007/diff/1/chrome/ > browser/ui/website_settings/permission_bubble_manager.h > File chrome/browser/ui/website_settings/permission_bubble_manager.h > (right): > > https://codereview.chromium.org/121113007/diff/1/chrome/ > browser/ui/website_settings/permission_bubble_manager.h#newcode22 > chrome/browser/ui/website_settings/permission_bubble_manager.h:22: // > the associated WebContents. > On 2014/01/07 20:31:19, Greg Billock wrote: > >> On 2014/01/07 19:44:57, groby wrote: >> > On 2014/01/07 17:59:15, Greg Billock wrote: >> > > On 2014/01/07 00:31:17, groby wrote: >> > > > Why should permission requests outlive their WebContents? Aren't >> > they > >> > > implicitly >> > > > tied to WebContents? >> > > >> > > I botched this. I mean the "WebContents' PBM" but that's a bit of >> > a > >> mouthfull. >> > > I'll reword. >> > >> > Since the requests are managed by the PBM, I'm not sure why they >> > should > >> outlive >> > it. Again, this goes to ownership model - I don't think permission >> > requests > >> need >> > to be owned by anybody but the PBM. >> > > Take this case: tab 1 requests a permission. User switches to tab 2. >> > User > >> switches back to tab 1. User allows the permission request. >> > > Something in the scope of tab 1 has to remain alive during this entire >> > process > >> to collect the response to the permission request. Something has to >> > re-populate > >> the bubble contents after the tab change. Currently in infobars the >> > delegate > >> does this, and the forwards responses to an inner member. This works >> > basically > >> the same, but leaves that whole object graph within the client. So the >> > UI code > >> handles the UI object graph, the client code handles the response >> > object graph. > >> The manager is the API mediator. >> > > PBM is in the scope of tab1, no? It's tied to WebContents. > > And I don't _want_ the graph within the client, because it puts burden > on the implementers. The use case, as I see it, is that we have various > facilities that > a) Ask for user permissions to do something > b) Do something as the user reacts. > > There's no need for them to manage the UI object graph directly, because > they shouldn't be concerned with is. It's a query and a callback. > > > https://codereview.chromium.org/121113007/diff/1/chrome/ > browser/ui/website_settings/permission_bubble_manager.h#newcode31 > chrome/browser/ui/website_settings/permission_bubble_manager.h:31: // to > outlive the PermissionBubbleManager. > On 2014/01/07 20:31:19, Greg Billock wrote: > >> On 2014/01/07 19:44:57, groby wrote: >> > On 2014/01/07 17:59:15, Greg Billock wrote: >> > > On 2014/01/07 00:31:17, groby wrote: >> > > > Why? What is the benefit for the caller to keep ownership? >> > > >> > > We need some sort of pathway to get back to them. They have to >> > implement an > >> > > interface which is kept alive the life of the bubble anyway, so >> consolidating >> > it >> > > there is convenient and hard to screw up. >> > > >> > > In the infobar system, the delegates are owned by the infobar, so >> > they all > >> > have >> > > another layer of indirection to reconnect to their target object, >> > with > >> > > auto-implicit lifetime arrangements. >> > >> > Why do we need to get back to them? What is the use case there? >> > > The "user allowed your permission request" use case. That's >> > universally true, > >> otherwise, the client wouldn't even bother calling in the first place. >> > > > And I'm all for keeping it alive during the bubble duration - that's >> > why PBM > >> > should own them. And manage the bubble, so it can delete the >> > delegates when > >> the >> > bubble goes away. >> > > Then you have the infobar architecture, which gets the job done, but >> > with N > >> separate lifetime arrangements between delegate and client. That makes >> > more > >> sense for infobars, since a lot of the delegates aren't about >> > requests, but > >> basically go off and call other global browser functions. For >> > permissions, where > >> each invocation is associated with a particular request, there's no >> > need to > >> double-joint it. >> > > But the caller doesn't need the object for the pathway back. There will > be a OnCancel/OnSuccess on the delegate, which needs to talk to the > caller. When does the caller need to talk to the delegate? I feel I'm > missing something here - please help me find it :) > > As I see it, the caller does something akin to > AskUserPermissionForContent(web_content, new > SpecificPermissionBubbleDelegate(some_state_associated_with_request)); > > The delegate will do the right thing for both cancel and success. It > will utilize the state associated with the request to do that, whatever > that state is. > > What I fail to see is where the delegate is ever kept around. The > current InfoBar architecture doesn't keep the delegate around, either? > > > https://codereview.chromium.org/121113007/diff/1/chrome/ > browser/ui/website_settings/permission_bubble_manager.h#newcode32 > chrome/browser/ui/website_settings/permission_bubble_manager.h:32: > virtual void AddPermissionBubbleDelegate(PermissionBubbleDelegate* > delegate); > On 2014/01/07 20:31:19, Greg Billock wrote: > >> On 2014/01/07 19:44:57, groby wrote: >> > On 2014/01/07 17:59:15, Greg Billock wrote: >> > > On 2014/01/07 00:31:17, groby wrote: >> > > > This is requesting a permission - adding the delegate is merely >> > an > >> > > > implementation detail. I'd prefer the name to reflect the >> > intent, not the > >> > > > implementation. >> > > >> > > Yeah, I toyed with a couple more focused names, but didn't find >> > anything I > >> > liked >> > > better. One reason is that this is intended to be UI-only -- it's >> > not > >> intended >> > > to encompass all the permissions policy and fussing about with the >> > profile > >> and >> > > such. If we can abstract that to some functional helper, that's >> > more where > >> the >> > > "RequestPermission" type call belongs. >> > >> > Let's not bikeshed on it right now (mostly because I don't have a >> > good name > >> :), >> > but the final version hopefully won't talk about type names. (I >> > still like > >> > "AddPermissionRequest", because that's what it does. Maybe >> > "AddUserPermissionRequest" or somesuch) >> > > I think you may have a different idea of the scope of this class. The >> > scope I'm > >> envisioning for it is extremely confined: show the permissions UI, and >> > that's > >> it. If there's profile policy negotiation to be done, or permission >> > policy > >> investigative work to do, the caller has to do it. There may be an >> > abstraction > >> that's useful for that, but this code doesn't do it. It just puts the >> > pixels on > >> the screen when it is told. >> > > I think my idea of the scope of the _delegate_ is what differs. The work > of doing policy work is on it. (That's why I suggested in my original > e-mail that we have a Context/Controller that's passed in to the > permission request) > > > https://codereview.chromium.org/121113007/diff/1/chrome/ > browser/ui/website_settings/permission_bubble_manager.h#newcode34 > chrome/browser/ui/website_settings/permission_bubble_manager.h:34: // > Remove a consumer delegate from the permission bubble. > On 2014/01/07 20:31:19, Greg Billock wrote: > >> On 2014/01/07 19:44:57, groby wrote: >> > On 2014/01/07 17:59:15, Greg Billock wrote: >> > > On 2014/01/07 00:31:17, groby wrote: >> > > > In which case would this happen? >> > > >> > > Not sure, but if history is any guide, it's sure to. >> > >> > Well, let's add it when it is :) (More lifecycle - if PBM owns the >> > requests, > >> no >> > need to retract them) >> > > Sure, but then retraction becomes awkward, and you end up with >> > ReplaceInfoBar > >> type of architecture, where you crawl through the existing delegates, >> > using > >> homebrew RTTI to figure out if you've ever added an equivalent one >> > before. > >> That's specifically the thing I'm avoiding here -- that logic must all >> > be > >> external, and if you decide you hate the permission you asked for, and >> > want a > >> new one, then you just Remove(old); Add(new). >> > > > But that means that this is handled individually by each delegate. I > don't think policies are wildly different, and having each delegate > implement this increases the burden on the API consumer. And that is > what currently happens in the current infobar architecture - all the > delegates call ReplaceInfoBar. There's no need for it. > > All that needs to happen in PBM is: > * see if there's an existing infobar with equivalent type. if > (newDelegate->IsEquivalent(otherDelegate)...) - yes, it's a bit of RTTI, > but no casting/switching on types. > * If there is, ask the new infobar to subsume the old one, replacing the > old delegate with the new one. otherDelegate->Merge(newDelegate) > * Potentially update the UI > > This allows delegates to implement specific logic around > replacement/mergin without being concerned what the visual expression > is. (I.e no need to Add/Remove other delegates) > > https://codereview.chromium.org/121113007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
No, I don't want to handle the actual checking inside the class :) I'll try whip up a writeup. On Tue, Jan 7, 2014 at 3:58 PM, Greg Billock <gbillock@chromium.org> wrote: > I saw the doc you had before -- it looked pretty similar to this, but it > now sounds like you want to do a whole permission-checking protocol inside > this class. I haven't formed an opinion about whether that is a thing that > can be abstracted or not, but I'm quite certain it doesn't belong in this > class. If there's a profile-checking, defaults-inspecting thing to be > written, it should be written separately. > > The process this class intends to abstract is permission coalescing. > There's a separate cross-platform abstraction that's kind of hiding behind > this one in a view interface. I'm making a CL for that right now. If > there's an additional layer of profile-checking abstraction we can put in > front of it, that's fine, but it shouldn't be included in this interface in > my opinion. > > If you think responsibilities should be sliced differently, yeah, write up > a doc about it. > > If it helps to understand, consider the PermissionBubbleDelegate to be > composed of two halves: the GetMessageText half that drives the UI and the > PermissionAccepted half that consumes the result. It would be possible to > pass the two halves separately to the API, but they'd always go together, > and I'm having trouble seeing why it's an advantage for the client to have > them separate -- if such a need arises for some client (and it hasn't for > infobars), they can make a trivial forwarder to do it. > > > > On Tue, Jan 7, 2014 at 2:28 PM, <groby@chromium.org> wrote: > >> Overall, I'm thinking my mental model of the architecture is significantly >> different from yours. I'm happy to continue hashing this out here, but if >> you'd >> prefer me to write a doc (or a proposal API) to explain my model, please >> LMK. >> >> >> >> >> >> https://codereview.chromium.org/121113007/diff/1/chrome/ >> browser/ui/website_settings/permission_bubble_delegate.h >> File chrome/browser/ui/website_settings/permission_bubble_delegate.h >> (right): >> >> https://codereview.chromium.org/121113007/diff/1/chrome/ >> browser/ui/website_settings/permission_bubble_delegate.h#newcode60 >> chrome/browser/ui/website_settings/permission_bubble_delegate.h:60: // >> called on every delegate which is not unregistered. >> On 2014/01/07 20:31:19, Greg Billock wrote: >> >>> On 2014/01/07 19:44:57, groby wrote: >>> > On 2014/01/07 17:59:15, Greg Billock wrote: >>> > > On 2014/01/07 00:31:17, groby wrote: >>> > > > 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? >>> > > >>> > > 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. >> >>> > >>> > 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. >>> >> >> 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. >>> >> >> 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. >> >> >> https://codereview.chromium.org/121113007/diff/1/chrome/ >> browser/ui/website_settings/permission_bubble_manager.h >> File chrome/browser/ui/website_settings/permission_bubble_manager.h >> (right): >> >> https://codereview.chromium.org/121113007/diff/1/chrome/ >> browser/ui/website_settings/permission_bubble_manager.h#newcode22 >> chrome/browser/ui/website_settings/permission_bubble_manager.h:22: // >> the associated WebContents. >> On 2014/01/07 20:31:19, Greg Billock wrote: >> >>> On 2014/01/07 19:44:57, groby wrote: >>> > On 2014/01/07 17:59:15, Greg Billock wrote: >>> > > On 2014/01/07 00:31:17, groby wrote: >>> > > > Why should permission requests outlive their WebContents? Aren't >>> >> they >> >>> > > implicitly >>> > > > tied to WebContents? >>> > > >>> > > I botched this. I mean the "WebContents' PBM" but that's a bit of >>> >> a >> >>> mouthfull. >>> > > I'll reword. >>> > >>> > Since the requests are managed by the PBM, I'm not sure why they >>> >> should >> >>> outlive >>> > it. Again, this goes to ownership model - I don't think permission >>> >> requests >> >>> need >>> > to be owned by anybody but the PBM. >>> >> >> Take this case: tab 1 requests a permission. User switches to tab 2. >>> >> User >> >>> switches back to tab 1. User allows the permission request. >>> >> >> Something in the scope of tab 1 has to remain alive during this entire >>> >> process >> >>> to collect the response to the permission request. Something has to >>> >> re-populate >> >>> the bubble contents after the tab change. Currently in infobars the >>> >> delegate >> >>> does this, and the forwards responses to an inner member. This works >>> >> basically >> >>> the same, but leaves that whole object graph within the client. So the >>> >> UI code >> >>> handles the UI object graph, the client code handles the response >>> >> object graph. >> >>> The manager is the API mediator. >>> >> >> PBM is in the scope of tab1, no? It's tied to WebContents. >> >> And I don't _want_ the graph within the client, because it puts burden >> on the implementers. The use case, as I see it, is that we have various >> facilities that >> a) Ask for user permissions to do something >> b) Do something as the user reacts. >> >> There's no need for them to manage the UI object graph directly, because >> they shouldn't be concerned with is. It's a query and a callback. >> >> >> https://codereview.chromium.org/121113007/diff/1/chrome/ >> browser/ui/website_settings/permission_bubble_manager.h#newcode31 >> chrome/browser/ui/website_settings/permission_bubble_manager.h:31: // to >> outlive the PermissionBubbleManager. >> On 2014/01/07 20:31:19, Greg Billock wrote: >> >>> On 2014/01/07 19:44:57, groby wrote: >>> > On 2014/01/07 17:59:15, Greg Billock wrote: >>> > > On 2014/01/07 00:31:17, groby wrote: >>> > > > Why? What is the benefit for the caller to keep ownership? >>> > > >>> > > We need some sort of pathway to get back to them. They have to >>> >> implement an >> >>> > > interface which is kept alive the life of the bubble anyway, so >>> consolidating >>> > it >>> > > there is convenient and hard to screw up. >>> > > >>> > > In the infobar system, the delegates are owned by the infobar, so >>> >> they all >> >>> > have >>> > > another layer of indirection to reconnect to their target object, >>> >> with >> >>> > > auto-implicit lifetime arrangements. >>> > >>> > Why do we need to get back to them? What is the use case there? >>> >> >> The "user allowed your permission request" use case. That's >>> >> universally true, >> >>> otherwise, the client wouldn't even bother calling in the first place. >>> >> >> > And I'm all for keeping it alive during the bubble duration - that's >>> >> why PBM >> >>> > should own them. And manage the bubble, so it can delete the >>> >> delegates when >> >>> the >>> > bubble goes away. >>> >> >> Then you have the infobar architecture, which gets the job done, but >>> >> with N >> >>> separate lifetime arrangements between delegate and client. That makes >>> >> more >> >>> sense for infobars, since a lot of the delegates aren't about >>> >> requests, but >> >>> basically go off and call other global browser functions. For >>> >> permissions, where >> >>> each invocation is associated with a particular request, there's no >>> >> need to >> >>> double-joint it. >>> >> >> But the caller doesn't need the object for the pathway back. There will >> be a OnCancel/OnSuccess on the delegate, which needs to talk to the >> caller. When does the caller need to talk to the delegate? I feel I'm >> missing something here - please help me find it :) >> >> As I see it, the caller does something akin to >> AskUserPermissionForContent(web_content, new >> SpecificPermissionBubbleDelegate(some_state_associated_with_request)); >> >> The delegate will do the right thing for both cancel and success. It >> will utilize the state associated with the request to do that, whatever >> that state is. >> >> What I fail to see is where the delegate is ever kept around. The >> current InfoBar architecture doesn't keep the delegate around, either? >> >> >> https://codereview.chromium.org/121113007/diff/1/chrome/ >> browser/ui/website_settings/permission_bubble_manager.h#newcode32 >> chrome/browser/ui/website_settings/permission_bubble_manager.h:32: >> virtual void AddPermissionBubbleDelegate(PermissionBubbleDelegate* >> delegate); >> On 2014/01/07 20:31:19, Greg Billock wrote: >> >>> On 2014/01/07 19:44:57, groby wrote: >>> > On 2014/01/07 17:59:15, Greg Billock wrote: >>> > > On 2014/01/07 00:31:17, groby wrote: >>> > > > This is requesting a permission - adding the delegate is merely >>> >> an >> >>> > > > implementation detail. I'd prefer the name to reflect the >>> >> intent, not the >> >>> > > > implementation. >>> > > >>> > > Yeah, I toyed with a couple more focused names, but didn't find >>> >> anything I >> >>> > liked >>> > > better. One reason is that this is intended to be UI-only -- it's >>> >> not >> >>> intended >>> > > to encompass all the permissions policy and fussing about with the >>> >> profile >> >>> and >>> > > such. If we can abstract that to some functional helper, that's >>> >> more where >> >>> the >>> > > "RequestPermission" type call belongs. >>> > >>> > Let's not bikeshed on it right now (mostly because I don't have a >>> >> good name >> >>> :), >>> > but the final version hopefully won't talk about type names. (I >>> >> still like >> >>> > "AddPermissionRequest", because that's what it does. Maybe >>> > "AddUserPermissionRequest" or somesuch) >>> >> >> I think you may have a different idea of the scope of this class. The >>> >> scope I'm >> >>> envisioning for it is extremely confined: show the permissions UI, and >>> >> that's >> >>> it. If there's profile policy negotiation to be done, or permission >>> >> policy >> >>> investigative work to do, the caller has to do it. There may be an >>> >> abstraction >> >>> that's useful for that, but this code doesn't do it. It just puts the >>> >> pixels on >> >>> the screen when it is told. >>> >> >> I think my idea of the scope of the _delegate_ is what differs. The work >> of doing policy work is on it. (That's why I suggested in my original >> e-mail that we have a Context/Controller that's passed in to the >> permission request) >> >> >> https://codereview.chromium.org/121113007/diff/1/chrome/ >> browser/ui/website_settings/permission_bubble_manager.h#newcode34 >> chrome/browser/ui/website_settings/permission_bubble_manager.h:34: // >> Remove a consumer delegate from the permission bubble. >> On 2014/01/07 20:31:19, Greg Billock wrote: >> >>> On 2014/01/07 19:44:57, groby wrote: >>> > On 2014/01/07 17:59:15, Greg Billock wrote: >>> > > On 2014/01/07 00:31:17, groby wrote: >>> > > > In which case would this happen? >>> > > >>> > > Not sure, but if history is any guide, it's sure to. >>> > >>> > Well, let's add it when it is :) (More lifecycle - if PBM owns the >>> >> requests, >> >>> no >>> > need to retract them) >>> >> >> Sure, but then retraction becomes awkward, and you end up with >>> >> ReplaceInfoBar >> >>> type of architecture, where you crawl through the existing delegates, >>> >> using >> >>> homebrew RTTI to figure out if you've ever added an equivalent one >>> >> before. >> >>> That's specifically the thing I'm avoiding here -- that logic must all >>> >> be >> >>> external, and if you decide you hate the permission you asked for, and >>> >> want a >> >>> new one, then you just Remove(old); Add(new). >>> >> >> >> But that means that this is handled individually by each delegate. I >> don't think policies are wildly different, and having each delegate >> implement this increases the burden on the API consumer. And that is >> what currently happens in the current infobar architecture - all the >> delegates call ReplaceInfoBar. There's no need for it. >> >> All that needs to happen in PBM is: >> * see if there's an existing infobar with equivalent type. if >> (newDelegate->IsEquivalent(otherDelegate)...) - yes, it's a bit of RTTI, >> but no casting/switching on types. >> * If there is, ask the new infobar to subsume the old one, replacing the >> old delegate with the new one. otherDelegate->Merge(newDelegate) >> * Potentially update the UI >> >> This allows delegates to implement specific logic around >> replacement/mergin without being concerned what the visual expression >> is. (I.e no need to Add/Remove other delegates) >> >> https://codereview.chromium.org/121113007/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Still writing up details, but after experimentation with APIs, the _one_ major difference is really around lifetimes, so I'll raise this separately here. In my opinion, there is no need for clients to hold on to PermissionBubbleDelegates'. The PermissionBubbleManager controls their liftetime. There's also no need for the delegates to know when the bubble is destroyed - for logical purposes, the bubble is persistent until the user confirms or denies the request. The fact that it is temporarily invisible on tab switches is a detail for the PermissionBubbleManager, and there's no need to surface it further.
On 2014/01/08 17:59:03, groby wrote: > Still writing up details, but after experimentation with APIs, the _one_ major > difference is really around lifetimes, so I'll raise this separately here. > > In my opinion, there is no need for clients to hold on to > PermissionBubbleDelegates'. The PermissionBubbleManager controls their > liftetime. There's also no need for the delegates to know when the bubble is > destroyed - for logical purposes, the bubble is persistent until the user > confirms or denies the request. The fact that it is temporarily invisible on tab > switches is a detail for the PermissionBubbleManager, and there's no need to > surface it further. Yeah, that's an unfortunate name. I meant 'destroyed' to mean something different than 'hidden' since the idea is to signal that there's no more calls coming, not that the bubble was hidden/shown, but I agree it is confusing. Making the delegates owned by the manager would solve that, but at the cost of the other complications I've mentioned.
On 2014/01/09 22:11:10, Greg Billock wrote: > On 2014/01/08 17:59:03, groby wrote: > > Still writing up details, but after experimentation with APIs, the _one_ major > > difference is really around lifetimes, so I'll raise this separately here. > > > > In my opinion, there is no need for clients to hold on to > > PermissionBubbleDelegates'. The PermissionBubbleManager controls their > > liftetime. There's also no need for the delegates to know when the bubble is > > destroyed - for logical purposes, the bubble is persistent until the user > > confirms or denies the request. The fact that it is temporarily invisible on > tab > > switches is a detail for the PermissionBubbleManager, and there's no need to > > surface it further. > > Yeah, that's an unfortunate name. I meant 'destroyed' to mean something > different than 'hidden' since the idea is to signal that there's no more calls > coming, not that the bubble was hidden/shown, but I agree it is confusing. > Making the delegates owned by the manager would solve that, but at the cost of > the other complications I've mentioned. Any other views on this? If we're aimed in the right direction I'd like to get it committed -- I'm sure there will be adjustments as we go. (e.g. perhaps we need a default enabled/disabled indicator in the API delegate...)
This has a lot of reviewers. As always when there are several reviewers, please explicitly say what each person is to review.
On 2014/01/14 00:53:18, Peter Kasting wrote: > This has a lot of reviewers. As always when there are several reviewers, please > explicitly say what each person is to review. Yeah, I think of design scope reviews like this as pretty open for review, but markusheintz is the owner of this directory and the official reviewer.
I have not looked into the alternative Rachel described. From my perspective this LGTM to move forward. https://codereview.chromium.org/121113007/diff/310007/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_bubble_delegate.h (right): https://codereview.chromium.org/121113007/diff/310007/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_bubble_delegate.h:24: virtual int GetIconID() const = 0; The mocks I know don't have an icons. So I'm not sure if this is necessary. https://codereview.chromium.org/121113007/diff/310007/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right): https://codereview.chromium.org/121113007/diff/310007/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_bubble_manager.cc:21: accept_state_.push_back(false); // make default state a delegate property? Nit: Is this comment a TODO? If so please mark it as TODO. I would prefer if the default is deny because a click outside the bubble will close it and we should not grant any permissions by accident. So I would not make this a delegate property.
I have not looked into the alternative Rachel described. From my perspective this LGTM to move forward.
After local discussion with Greg I've decided not to pursue the alternative. I still believe our lifecycle management is a bad choice, but it is quite likely this is essentially a matter of opinion, not functionality. Given the number of callees for this APIs is in the low 10's, I'm OK with going ahead with this and fixing it after the fact if lifecycle management really is the problem I imagine it to be.
Thanks, Markus. I think we'll evolve this a bit as we hit realities. This'll let us start building platform-specific code and discover what those'll be, though. https://codereview.chromium.org/121113007/diff/310007/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_bubble_delegate.h (right): https://codereview.chromium.org/121113007/diff/310007/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_bubble_delegate.h:24: virtual int GetIconID() const = 0; On 2014/01/15 23:25:30, markusheintz_ wrote: > The mocks I know don't have an icons. So I'm not sure if this is necessary. Talked with Alex today. They'd like to get icons, so I'm going to go ahead and leave it in for now, but I agree it isn't clear it'll stay. https://codereview.chromium.org/121113007/diff/310007/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right): https://codereview.chromium.org/121113007/diff/310007/chrome/browser/ui/websi... chrome/browser/ui/website_settings/permission_bubble_manager.cc:21: accept_state_.push_back(false); // make default state a delegate property? On 2014/01/15 23:25:30, markusheintz_ wrote: > Nit: Is this comment a TODO? If so please mark it as TODO. > > I would prefer if the default is deny because a click outside the bubble will > close it and we should not grant any permissions by accident. So I would not > make this a delegate property. The current plan is for these bubbles to be modal-y and require interaction to close. I'll mark it a TODO -- I don't know if it'll end up a necessary option or not. Fairly obvious to add, though.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/121113007/450001
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/121113007/720001
Message was sent while issue was closed.
Change committed as 245396 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
