Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
265773002:
[Media,Geolocation] Add permission bubble cancellation. (Closed)
|
Created:
6 years, 7 months ago by Greg Billock Modified:
6 years, 7 months ago CC:
chromium-reviews, fischman+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Media,Geolocation] Add permission bubble cancellation.
BUG=332115, 342562
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273068
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Rebase #Patch Set 4 : Fix rebase #
Total comments: 13
Patch Set 5 : iterator, checks #
Total comments: 10
Patch Set 6 : dcheck #Patch Set 7 : use ScopedPtrHashMap #Patch Set 8 : rebase #
Messages
Total messages: 25 (0 generated)
Looks ok to me, but do we have tests that already exercise this code?
Could you add a bug number? I haven't reviewed any of this work before, so I don't really get what this change is doing.
On 2014/05/08 17:34:22, Victoria Kirst wrote: > Could you add a bug number? I haven't reviewed any of this work before, so I > don't really get what this change is doing. oops! The context is replacing permissions infobars with bubbles. For a couple, that means we need the ability to cancel outstanding requests. The intention is that this follows a fairly parallel path with infobar cancellation.
On 2014/05/08 10:16:21, Michael van Ouwerkerk wrote: > Looks ok to me, but do we have tests that already exercise this code? There are cancel tests, but to exercise the permission pathway you have to run them with the flag on. I'll make sure to do that, but it doesn't really show up in the bots directly. I could duplicate the tests with the flag set on and off.
On 2014/05/08 19:01:55, Greg Billock wrote: > On 2014/05/08 10:16:21, Michael van Ouwerkerk wrote: > > Looks ok to me, but do we have tests that already exercise this code? > > There are cancel tests, but to exercise the permission pathway you have to run > them with the flag on. I'll make sure to do that, but it doesn't really show up > in the bots directly. I could duplicate the tests with the flag set on and off. It sounds like a good idea to run them both with the flag on and off. That way, there's less chance of bit rot for the path that is disabled by default.
On 2014/05/08 19:07:11, Michael van Ouwerkerk wrote: > On 2014/05/08 19:01:55, Greg Billock wrote: > > On 2014/05/08 10:16:21, Michael van Ouwerkerk wrote: > > > Looks ok to me, but do we have tests that already exercise this code? > > > > There are cancel tests, but to exercise the permission pathway you have to run > > them with the flag on. I'll make sure to do that, but it doesn't really show > up > > in the bots directly. I could duplicate the tests with the flag set on and > off. > > It sounds like a good idea to run them both with the flag on and off. That way, > there's less chance of bit rot for the path that is disabled by default. I looked into this. The tests are pretty deeply embedded with the infobar. I think they'll need a rework before we switch to the bubble anyway.
lgtm, keep going then :-)
On 2014/05/12 10:29:33, Michael van Ouwerkerk wrote: > lgtm, keep going then :-) vrk, any other suggestions?
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/265773002/20001
On 2014/05/15 17:49:43, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/gbillock@chromium.org/265773002/20001 Ami could you have a look for media/?
Only looked at media. https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_midi_permission_context.cc (right): https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:103: void MidiPermissionRequest::Cancelled() { NOTREACHED? https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:107: // Deletes 'this'. IWBN to ensure that this is the only way ~MPR() can be called; feel like putting a bool is_finished_; member on MPR that is ctor'd to false, set to true here, and DCHECK'd in the dtor? https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:118: DCHECK(!permission_queue_controller_); Would you DCHECK(pending_requests_.empty()); or would you expect that it's possible that there are still pending requests, in which case my linked_ptr comment is actually a bug-fix (leak)? https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:207: pending_requests_[id.ToString()] = request; This depends on id.ToString() being unique. Is that true? Would you mind greatly bool inserted = pending_requests_.insert(make_pair(id.ToString(), request)).second; DCHECK(inserted) << "Duplicate id: " << id; ? https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:264: for (it = pending_requests_.begin(); it != pending_requests_.end(); it++) { This is a map; why are you iterating?? Put another way: you don't seem to use the fact that the map is keyed by ID. This might as well be a set of pairs of id & pointer. If you do want to keep it as a map, then l.280-287 simplify to: std::map<std::string, MidiPermissionRequest*>::iterator it = pending_requests_.find(id.ToString()); MidiPermissionRequest* cancelling = it != pending_requests_.end() ? it->second : NULL; https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:270: } NOTREACHED() << "Missing request!"; ? https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_midi_permission_context.h (right): https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.h:90: std::map<std::string, MidiPermissionRequest*> pending_requests_; IIUC this owns the pointers; it is poor form to use a raw pointer for an owned pointer. This looks like a good use for linked_ptr. https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.h:90: std::map<std::string, MidiPermissionRequest*> pending_requests_; If it was me I'd typedef this type to save some chars in the .cc file.
https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... File chrome/browser/media/chrome_midi_permission_context.cc (right): https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:103: void MidiPermissionRequest::Cancelled() { On 2014/05/20 17:09:53, Ami Fischman wrote: > NOTREACHED? This is invoked if the user cancels the permission bubble. I'm changing this to match infobar behavior for now instead of treating it as a rejected permission request, but it's available if you want to do something different. https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:107: // Deletes 'this'. sounds good, done. On 2014/05/20 17:09:53, Ami Fischman wrote: > IWBN to ensure that this is the only way ~MPR() can be called; feel like putting > a > bool is_finished_; > member on MPR that is ctor'd to false, set to true here, and DCHECK'd in the > dtor? https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:207: pending_requests_[id.ToString()] = request; On 2014/05/20 17:09:53, Ami Fischman wrote: > This depends on id.ToString() being unique. Is that true? Would you mind > greatly > bool inserted = pending_requests_.insert(make_pair(id.ToString(), > request)).second; > DCHECK(inserted) << "Duplicate id: " << id; > ? It would be nice to key the map on PermissionRequestID, but that's not possible. :-( I toyed with trying to make that work, but I think this ends up being fine. The reason is that the bridge_id_ of the PRID is unique per-instance and is included in the string. (And indeed, the operation of the class is predicated on being able to tell PRIDs apart.) Adding a DCHECK on it seems fine, though. https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:264: for (it = pending_requests_.begin(); it != pending_requests_.end(); it++) { This iteration is searching by value. The one below could change, though. On map vs list<pair>, I think a map is more intelligible. On 2014/05/20 17:09:53, Ami Fischman wrote: > This is a map; why are you iterating?? > > Put another way: you don't seem to use the fact that the map is keyed by ID. > This might as well be a set of pairs of id & pointer. > > If you do want to keep it as a map, then l.280-287 simplify to: > std::map<std::string, MidiPermissionRequest*>::iterator it = > pending_requests_.find(id.ToString()); > MidiPermissionRequest* cancelling = > it != pending_requests_.end() ? it->second : NULL; > https://codereview.chromium.org/265773002/diff/60001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:270: } On 2014/05/20 17:09:53, Ami Fischman wrote: > NOTREACHED() << "Missing request!"; > ? Yes. That's correct.
https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... File chrome/browser/media/chrome_midi_permission_context.cc (right): https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:107: void MidiPermissionRequest::Cancelled() { On 2014/05/21 19:12:34, Greg Billock wrote: > On 2014/05/20 17:09:53, Ami Fischman wrote: > > NOTREACHED? > > This is invoked if the user cancels the permission bubble. I'm changing this to > match infobar behavior for now instead of treating it as a rejected permission > request, but it's available if you want to do something different. // This is called if the info-bar is dismissed. Permission is neither granted nor denied and we hope JS can deal with that. ? (I suspect such a hope is poorly-founded; I can easily imagine use-cases where the JS effectively blocks the app's functionality until the permission is granted/denied and it would be good to give the JS a hint that that grant is never coming) https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:123: DCHECK(!permission_queue_controller_); Did you miss this comment? Would you DCHECK(pending_requests_.empty()); or would you expect that it's possible that there are still pending requests, in which case my linked_ptr comment is actually a bug-fix (leak)? https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... File chrome/browser/media/chrome_midi_permission_context.h (right): https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.h:90: std::map<std::string, MidiPermissionRequest*> pending_requests_; I think you missed the comments I made on this file in PS#4.
https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... File chrome/browser/media/chrome_midi_permission_context.cc (right): https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:107: void MidiPermissionRequest::Cancelled() { On 2014/05/21 20:15:13, Ami Fischman wrote: > On 2014/05/21 19:12:34, Greg Billock wrote: > > On 2014/05/20 17:09:53, Ami Fischman wrote: > > > NOTREACHED? > > > > This is invoked if the user cancels the permission bubble. I'm changing this > to > > match infobar behavior for now instead of treating it as a rejected permission > > request, but it's available if you want to do something different. > > // This is called if the info-bar is dismissed. Permission is neither granted > nor denied and we hope JS can deal with that. > ? > > (I suspect such a hope is poorly-founded; I can easily imagine use-cases where > the JS effectively blocks the app's functionality until the permission is > granted/denied and it would be good to give the JS a hint that that grant is > never coming) Yeah, me too. That's why I put in initially that cancellation would look like denial. In retrospect, that seemed wrong since the dialog can cancel on tab close and such. I don't know what the JS looks like for this call, or if there's a good way to signal back a "request processed but still in limbo" kind of thing. https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:123: DCHECK(!permission_queue_controller_); On 2014/05/21 20:15:13, Ami Fischman wrote: > Did you miss this comment? > Would you > DCHECK(pending_requests_.empty()); > or would you expect that it's possible that there are still pending requests, in > which case my linked_ptr comment is actually a bug-fix (leak)? Ah, yes, I missed responding to that. Style guide is "never use linked_ptr", but all requests are guaranteed to have RequestFinished called, so there's no leak. (The permission bubble mgr is per-WC and this context is per-profile.)
https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... File chrome/browser/media/chrome_midi_permission_context.cc (right): https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:107: void MidiPermissionRequest::Cancelled() { On 2014/05/21 22:46:42, Greg Billock wrote: > On 2014/05/21 20:15:13, Ami Fischman wrote: > > On 2014/05/21 19:12:34, Greg Billock wrote: > > > On 2014/05/20 17:09:53, Ami Fischman wrote: > > > > NOTREACHED? > > > > > > This is invoked if the user cancels the permission bubble. I'm changing this > > to > > > match infobar behavior for now instead of treating it as a rejected > permission > > > request, but it's available if you want to do something different. > > > > // This is called if the info-bar is dismissed. Permission is neither granted > > nor denied and we hope JS can deal with that. > > ? > > > > (I suspect such a hope is poorly-founded; I can easily imagine use-cases where > > the JS effectively blocks the app's functionality until the permission is > > granted/denied and it would be good to give the JS a hint that that grant is > > never coming) > > Yeah, me too. That's why I put in initially that cancellation would look like > denial. In retrospect, that seemed wrong since the dialog can cancel on tab > close and such. I don't know what the JS looks like for this call, or if there's > a good way to signal back a "request processed but still in limbo" kind of > thing. Shouldn't you find out before reverting the behavior? https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:123: DCHECK(!permission_queue_controller_); On 2014/05/21 22:46:42, Greg Billock wrote: > On 2014/05/21 20:15:13, Ami Fischman wrote: > > Did you miss this comment? > > Would you > > DCHECK(pending_requests_.empty()); > > or would you expect that it's possible that there are still pending requests, > in > > which case my linked_ptr comment is actually a bug-fix (leak)? > > Ah, yes, I missed responding to that. Style guide is "never use linked_ptr", Where does it say that? > but > all requests are guaranteed to have RequestFinished called, so there's no leak. > (The permission bubble mgr is per-WC and this context is per-profile.) Then can you add the DCHECK?
https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... File chrome/browser/media/chrome_midi_permission_context.cc (right): https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:107: void MidiPermissionRequest::Cancelled() { On 2014/05/21 23:47:34, Ami Fischman wrote: > On 2014/05/21 22:46:42, Greg Billock wrote: > > On 2014/05/21 20:15:13, Ami Fischman wrote: > > > On 2014/05/21 19:12:34, Greg Billock wrote: > > > > On 2014/05/20 17:09:53, Ami Fischman wrote: > > > > > NOTREACHED? > > > > > > > > This is invoked if the user cancels the permission bubble. I'm changing > this > > > to > > > > match infobar behavior for now instead of treating it as a rejected > > permission > > > > request, but it's available if you want to do something different. > > > > > > // This is called if the info-bar is dismissed. Permission is neither > granted > > > nor denied and we hope JS can deal with that. > > > ? > > > > > > (I suspect such a hope is poorly-founded; I can easily imagine use-cases > where > > > the JS effectively blocks the app's functionality until the permission is > > > granted/denied and it would be good to give the JS a hint that that grant is > > > never coming) > > > > Yeah, me too. That's why I put in initially that cancellation would look like > > denial. In retrospect, that seemed wrong since the dialog can cancel on tab > > close and such. I don't know what the JS looks like for this call, or if > there's > > a good way to signal back a "request processed but still in limbo" kind of > > thing. > > Shouldn't you find out before reverting the behavior? This (the change in behavior) is flagged off currently. Before it turns on we need to coordinate with callers to make sure the termination cases are handled the way they want. I'm trying to set them all to mimic the infobar to make that easier. https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:123: DCHECK(!permission_queue_controller_); On 2014/05/21 23:47:34, Ami Fischman wrote: > On 2014/05/21 22:46:42, Greg Billock wrote: > > On 2014/05/21 20:15:13, Ami Fischman wrote: > > > Did you miss this comment? > > > Would you > > > DCHECK(pending_requests_.empty()); > > > or would you expect that it's possible that there are still pending > requests, > > in > > > which case my linked_ptr comment is actually a bug-fix (leak)? > > > > Ah, yes, I missed responding to that. Style guide is "never use linked_ptr", > > Where does it say that? http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml But I have no particular style agenda here. I'm not opposed to using linked_ptr in the container -- it's kind of the sweet spot for it IMO. Mostly just telling you the story of how we got where we are. :-) I want the code to look how you want it to look. > > > but > > all requests are guaranteed to have RequestFinished called, so there's no > leak. > > (The permission bubble mgr is per-WC and this context is per-profile.) > > Then can you add the DCHECK? sure. that's a good plan regardless of the container type.
LGTM % linked_ptr. https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... File chrome/browser/media/chrome_midi_permission_context.cc (right): https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... chrome/browser/media/chrome_midi_permission_context.cc:123: DCHECK(!permission_queue_controller_); On 2014/05/22 01:50:00, Greg Billock wrote: > On 2014/05/21 23:47:34, Ami Fischman wrote: > > On 2014/05/21 22:46:42, Greg Billock wrote: > > > On 2014/05/21 20:15:13, Ami Fischman wrote: > > > > Did you miss this comment? > > > > Would you > > > > DCHECK(pending_requests_.empty()); > > > > or would you expect that it's possible that there are still pending > > requests, > > > in > > > > which case my linked_ptr comment is actually a bug-fix (leak)? > > > > > > Ah, yes, I missed responding to that. Style guide is "never use linked_ptr", > > > > Where does it say that? > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml This is an unfortunate divergence between chromium style and google3 style; the latter has embraced much of C++11, including std::unique_ptr. Unfortunately chromium has not done so, making that section of the style guide inapplicable to chromium (there is no std::unique_ptr in C++98, which is effectively what chromium uses). https://code.google.com/p/google-styleguide/source/diff?spec=svn112&r=112&for... Will raise on chromium-dev. Until we move to C++11 there is no std:: alternative to linked_ptr, so I think you should use linked_ptr here :)
On 2014/05/22 02:56:33, Ami Fischman wrote: > LGTM % linked_ptr. > > https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... > File chrome/browser/media/chrome_midi_permission_context.cc (right): > > https://codereview.chromium.org/265773002/diff/80001/chrome/browser/media/chr... > chrome/browser/media/chrome_midi_permission_context.cc:123: > DCHECK(!permission_queue_controller_); > On 2014/05/22 01:50:00, Greg Billock wrote: > > On 2014/05/21 23:47:34, Ami Fischman wrote: > > > On 2014/05/21 22:46:42, Greg Billock wrote: > > > > On 2014/05/21 20:15:13, Ami Fischman wrote: > > > > > Did you miss this comment? > > > > > Would you > > > > > DCHECK(pending_requests_.empty()); > > > > > or would you expect that it's possible that there are still pending > > > requests, > > > > in > > > > > which case my linked_ptr comment is actually a bug-fix (leak)? > > > > > > > > Ah, yes, I missed responding to that. Style guide is "never use > linked_ptr", > > > > > > Where does it say that? > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml > > This is an unfortunate divergence between chromium style and google3 style; the > latter has embraced much of C++11, including std::unique_ptr. Unfortunately > chromium has not done so, making that section of the style guide inapplicable to > chromium (there is no std::unique_ptr in C++98, which is effectively what > chromium uses). > https://code.google.com/p/google-styleguide/source/diff?spec=svn112&r=112&for... > > Will raise on chromium-dev. > > Until we move to C++11 there is no std:: alternative to linked_ptr, so I think > you should use linked_ptr here :) switched to ScopedPtrHashMap
PS#7 LGTM (I love that the scoped_ptr.get() highlights that the request is owned by the hashmap but the bubblemanager gets a raw pointer to it anyway; might be learnings for changing some of the bubble-related API surface around)
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/265773002/130001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/9584)
Message was sent while issue was closed.
Change committed as 273068 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
