Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1260193009:
renderer: implement multiple permission requesting (Closed)
|
Created:
5 years, 4 months ago by Lalit Maganti Modified:
5 years, 3 months ago CC:
raymes Base URL:
https://chromium.googlesource.com/chromium/src.git@permissions-request-multiple Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement renderer side of multiple permissions request.
This CL implements navigator.permissions.request() with multiple permissions
on the renderer side (permission_dispatcher and thread_proxy implementation).
On the browser side, it gets the mojo service to return the current permission
statuses in order for the call to "work". It does not implement a proper
permission request UI yet.
The browser side logic is being implemented in:
https://codereview.chromium.org/1316863010/
BUG=516626
Committed: https://crrev.com/d0b65b68ecad02a4e677b854a107ae267c8ad93e
Cr-Commit-Position: refs/heads/master@{#347446}
Patch Set 1 #Patch Set 2 : Fix remaining stuff todo #Patch Set 3 : Working implementation #Patch Set 4 : Merge patches #
Total comments: 10
Patch Set 5 : Move default behaviour code from Blink to Chromium #Patch Set 6 : Cleanup #Patch Set 7 : Reflect blink changes #Patch Set 8 : Split MIDI permission checking into another patch #Patch Set 9 : Fix crash when duplicate permissions are requested #Patch Set 10 : Rebase on midi change #Patch Set 11 : #
Total comments: 28
Patch Set 12 : Unify code paths between single and multiple in several places #Patch Set 13 : Rebase on top of other change #
Total comments: 20
Patch Set 14 : Fix review comments #
Total comments: 38
Patch Set 15 : Address review comments #Patch Set 16 : Split off all browser sections #
Total comments: 4
Patch Set 17 : Address review comments #Dependent Patchsets: Messages
Total messages: 26 (5 generated)
lalitmaganti@gmail.com changed reviewers: + lalitmaganti@gmail.com
https://codereview.chromium.org/1260193009/diff/50017/chrome/browser/permissi... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/1260193009/diff/50017/chrome/browser/permissi... chrome/browser/permissions/permission_manager.cc:81: struct PermissionManager::PendingResponse { Move to header and keep implementation here. https://codereview.chromium.org/1260193009/diff/50017/chrome/browser/permissi... chrome/browser/permissions/permission_manager.cc:91: struct PermissionManager::PendingResponses { Ditto. https://codereview.chromium.org/1260193009/diff/50017/chrome/browser/permissi... chrome/browser/permissions/permission_manager.cc:93: size_t count; Make this immutable and another field. https://codereview.chromium.org/1260193009/diff/50017/chrome/browser/permissi... chrome/browser/permissions/permission_manager.cc:161: PendingResponses *pending_responses = scoped_response.get(); Move * https://codereview.chromium.org/1260193009/diff/50017/chrome/browser/permissi... chrome/browser/permissions/permission_manager.cc:207: callback.Run(status); Move this after next line https://codereview.chromium.org/1260193009/diff/50017/chrome/browser/permissi... chrome/browser/permissions/permission_manager.cc:246: for (size_t i = 0; i < pending_responses->count; ++i) { This is totally wrong - we need to store actual and current count since this is modifiable. https://codereview.chromium.org/1260193009/diff/50017/chrome/browser/permissi... File chrome/browser/permissions/permission_manager.h (right): https://codereview.chromium.org/1260193009/diff/50017/chrome/browser/permissi... chrome/browser/permissions/permission_manager.h:76: struct PendingResponse; Should be removed https://codereview.chromium.org/1260193009/diff/50017/chrome/browser/permissi... chrome/browser/permissions/permission_manager.h:96: base::WeakPtrFactory<PermissionManager> weak_ptr_factory_; Comment about this being the last. https://codereview.chromium.org/1260193009/diff/50017/content/child/permissio... File content/child/permissions/permission_dispatcher.cc (right): https://codereview.chromium.org/1260193009/diff/50017/content/child/permissio... content/child/permissions/permission_dispatcher.cc:102: PermissionDispatcher::MultipleCallbackInformation::MultipleCallbackInformation( Consider merging this with the Callback information class. https://codereview.chromium.org/1260193009/diff/50017/content/child/permissio... content/child/permissions/permission_dispatcher.cc:267: void PermissionDispatcher::RunMultiCallbackOnWorkerThread( Template method?
Mounir: If you could have a quick look that would be awesome :)
lalitm@google.com changed reviewers: + mlamouri@chromium.org - lalitmaganti@gmail.com
In order to facilitate the review, do you think you can split? For example, you could have a CL that just adds the new MIDI permission?
Have split off the midi code into a separate patch as you suggested.
Quite a big CL. I haven't reviewed all of it. I had a deep look into the renderer/ bits and an overview of the browser/ side. I will look into it a bit more after my first comments are applied. https://codereview.chromium.org/1260193009/diff/190001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/1260193009/diff/190001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:262: if (--pending_responses->pending == 0) { shrug... please don't do that :) https://codereview.chromium.org/1260193009/diff/190001/content/browser/permis... File content/browser/permissions/permission_pending_multiple_request.h (right): https://codereview.chromium.org/1260193009/diff/190001/content/browser/permis... content/browser/permissions/permission_pending_multiple_request.h:32: PermissionsStatusCallback callback; This is not allowed. You must have the callback private (renamed to callback_) and have accessors. https://codereview.chromium.org/1260193009/diff/190001/content/browser/permis... content/browser/permissions/permission_pending_multiple_request.h:35: int request_count; style: request_count_ https://codereview.chromium.org/1260193009/diff/190001/content/browser/permis... File content/browser/permissions/permission_pending_single_request.h (right): https://codereview.chromium.org/1260193009/diff/190001/content/browser/permis... content/browser/permissions/permission_pending_single_request.h:19: class PermissionPendingSingleRequest : public PermissionPendingRequest { Could that be simplified to be a special case of multiple permission? In other words, you drop the class specialization and have only one class for permission request pendings which will take one or more permission requests. WDYT? https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... File content/child/permissions/permission_dispatcher.cc (right): https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:291: } nit: no { } https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:356: scoped_ptr<blink::WebVector<blink::WebPermissionStatus>> status( nit: statuses https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:360: } nit: remove { } https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:380: status.release())); wouldn't status.release be enough? https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... File content/child/permissions/permission_dispatcher.h (right): https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.h:94: static void RunMultiCallbackOnWorkerThread( I think it's find to overload RunCallbackOnWorkerThread. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.h:149: ~CallbackInformation() {} nit: = default. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.h:170: using SingleCallbackInfo = CallbackInformation<blink::WebPermissionCallback>; I dislike the name to be honest. Maybe you could come up with a better one. Otherwise simply using the full name? https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.h:174: using MultiCallbackInfo = CallbackInformation<blink::WebPermissionsCallback>; ditto. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.h:175: using MultipleCallbackMap = IDMap<MultiCallbackInfo, IDMapOwnPointer>; Again, the name isn't clear. At least, add a comment. https://codereview.chromium.org/1260193009/diff/190001/content/public/browser... File content/public/browser/permission_manager.h (right): https://codereview.chromium.org/1260193009/diff/190001/content/public/browser... content/public/browser/permission_manager.h:57: int request_id) = 0; Would it make sense to change CancelPermission to that or is PermissionType required for something? (I assume in both case we can pass the origin).
Much of my comments are now irrelevant because of the unification of single and multi code paths. https://codereview.chromium.org/1260193009/diff/190001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/1260193009/diff/190001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:262: if (--pending_responses->pending == 0) { On 2015/08/18 13:37:15, Mounir Lamouri wrote: > shrug... please don't do that :) Haha I thought this might be a bit amusing :) Changed to permission_responses->pending == 1. https://codereview.chromium.org/1260193009/diff/190001/content/browser/permis... File content/browser/permissions/permission_pending_multiple_request.h (right): https://codereview.chromium.org/1260193009/diff/190001/content/browser/permis... content/browser/permissions/permission_pending_multiple_request.h:32: PermissionsStatusCallback callback; On 2015/08/18 13:37:15, Mounir Lamouri wrote: > This is not allowed. You must have the callback private (renamed to callback_) > and have accessors. The reason I did this was because I remembered that in Manifest there were public fields and it was OK. But then I realised that manifest was actually just a storage class and it's also a struct rather than a class. Done. https://codereview.chromium.org/1260193009/diff/190001/content/browser/permis... content/browser/permissions/permission_pending_multiple_request.h:35: int request_count; On 2015/08/18 13:37:15, Mounir Lamouri wrote: > style: request_count_ Done. https://codereview.chromium.org/1260193009/diff/190001/content/browser/permis... File content/browser/permissions/permission_pending_single_request.h (right): https://codereview.chromium.org/1260193009/diff/190001/content/browser/permis... content/browser/permissions/permission_pending_single_request.h:19: class PermissionPendingSingleRequest : public PermissionPendingRequest { On 2015/08/18 13:37:15, Mounir Lamouri wrote: > Could that be simplified to be a special case of multiple permission? In other > words, you drop the class specialization and have only one class for permission > request pendings which will take one or more permission requests. WDYT? I'm split about this. It would vastly reduce code complexity but would unnecessarily introduce overhead by using vectors everywhere rather than plain objects. I think it would definitely be good to discuss this further. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... File content/child/permissions/permission_dispatcher.cc (right): https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:291: } On 2015/08/18 13:37:15, Mounir Lamouri wrote: > nit: no { } Done. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:356: scoped_ptr<blink::WebVector<blink::WebPermissionStatus>> status( On 2015/08/18 13:37:15, Mounir Lamouri wrote: > nit: statuses Done. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:360: } On 2015/08/18 13:37:15, Mounir Lamouri wrote: > nit: remove { } Done. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:380: status.release())); On 2015/08/18 13:37:15, Mounir Lamouri wrote: > wouldn't status.release be enough? Although that would make so much sense, WebPtrs don't have implicit conversion implemented. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... File content/child/permissions/permission_dispatcher.h (right): https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.h:94: static void RunMultiCallbackOnWorkerThread( On 2015/08/18 13:37:15, Mounir Lamouri wrote: > I think it's find to overload RunCallbackOnWorkerThread. Can't actually do this because I use it in a callback and bind does not support overloading. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.h:149: ~CallbackInformation() {} On 2015/08/18 13:37:15, Mounir Lamouri wrote: > nit: = default. Removed class. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.h:170: using SingleCallbackInfo = CallbackInformation<blink::WebPermissionCallback>; On 2015/08/18 13:37:15, Mounir Lamouri wrote: > I dislike the name to be honest. Maybe you could come up with a better one. > Otherwise simply using the full name? The reason I put the using is because the lines become far too long without them. I'll attempt to come up with a better name. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.h:174: using MultiCallbackInfo = CallbackInformation<blink::WebPermissionsCallback>; On 2015/08/18 13:37:15, Mounir Lamouri wrote: > ditto. Acknowledged. https://codereview.chromium.org/1260193009/diff/190001/content/child/permissi... content/child/permissions/permission_dispatcher.h:175: using MultipleCallbackMap = IDMap<MultiCallbackInfo, IDMapOwnPointer>; On 2015/08/18 13:37:15, Mounir Lamouri wrote: > Again, the name isn't clear. At least, add a comment. Acknowledged. https://codereview.chromium.org/1260193009/diff/190001/content/public/browser... File content/public/browser/permission_manager.h (right): https://codereview.chromium.org/1260193009/diff/190001/content/public/browser... content/public/browser/permission_manager.h:57: int request_id) = 0; On 2015/08/18 13:37:15, Mounir Lamouri wrote: > Would it make sense to change CancelPermission to that or is PermissionType > required for something? (I assume in both case we can pass the origin). In CancelPermission, it is used to retrieve the context. In any case, with unification of code paths this has changed.
I had a look at the CL except for the chrome PermissionManager. I'm still not a big fan of the namings but it looks quite good. It is a shame we have to introduce so much complexity but I really have no idea what else we could do here. Also, you should know that you will have to update the WebView PermissionManager. https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... File content/browser/permissions/permission_pending_request.cc (right): https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... content/browser/permissions/permission_pending_request.cc:25: statuses[i] = PERMISSION_STATUS_ASK; DENIED is usually what we do as default value. https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... File content/browser/permissions/permission_service_impl.cc (right): https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:112: if (permissions.is_null()) { When would that happen? https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:128: } style: remove { } https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:139: } ditto https://codereview.chromium.org/1260193009/diff/230001/content/child/permissi... File content/child/permissions/permission_dispatcher.cc (left): https://codereview.chromium.org/1260193009/diff/230001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:224: nit: revert that change https://codereview.chromium.org/1260193009/diff/230001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:245: ditto https://codereview.chromium.org/1260193009/diff/230001/content/child/permissi... File content/child/permissions/permission_dispatcher.cc (right): https://codereview.chromium.org/1260193009/diff/230001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:287: batch_pending_callbacks_.insert(batch_pending_callbacks_.end(), callback); I guess this needs to be rebased. https://codereview.chromium.org/1260193009/diff/230001/content/common/permiss... File content/common/permission_service.mojom (right): https://codereview.chromium.org/1260193009/diff/230001/content/common/permiss... content/common/permission_service.mojom:27: RequestBatchPermission(array<PermissionName> permission, string origin, bool user_gesture) Would Mojo be happy if we were to do overloading? or event RequestPermissions(). RequestBatchPermission() as an public method name isn't great. https://codereview.chromium.org/1260193009/diff/230001/content/public/browser... File content/public/browser/permission_manager.h (right): https://codereview.chromium.org/1260193009/diff/230001/content/public/browser... content/public/browser/permission_manager.h:24: using BatchRequestCallback = nit: I'm not sure which one I prefer between BatchRequestCallback and MultiRequestCallback. You prefer Batch? https://codereview.chromium.org/1260193009/diff/230001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_permission_manager.cc (right): https://codereview.chromium.org/1260193009/diff/230001/content/shell/browser/... content/shell/browser/layout_test/layout_test_permission_manager.cc:89: PERMISSION_STATUS_DENIED); nit: I would move the two arguments on this second line.
Responding to comments that don't need action - the others I will fix up once manifest stuff is done. https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... File content/browser/permissions/permission_pending_request.cc (right): https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... content/browser/permissions/permission_pending_request.cc:25: statuses[i] = PERMISSION_STATUS_ASK; On 2015/08/21 10:24:40, Mounir Lamouri wrote: > DENIED is usually what we do as default value. I just followed what was previously in permission_service_impl - there the previous way was to do ASK. I can change it to DENITED if there was no motive behind ASK https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... File content/browser/permissions/permission_service_impl.cc (right): https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:112: if (permissions.is_null()) { On 2015/08/21 10:24:41, Mounir Lamouri wrote: > When would that happen? Compromised renderer process is the only thing I can think of. https://codereview.chromium.org/1260193009/diff/230001/content/common/permiss... File content/common/permission_service.mojom (right): https://codereview.chromium.org/1260193009/diff/230001/content/common/permiss... content/common/permission_service.mojom:27: RequestBatchPermission(array<PermissionName> permission, string origin, bool user_gesture) On 2015/08/21 10:24:41, Mounir Lamouri wrote: > Would Mojo be happy if we were to do overloading? or event RequestPermissions(). > RequestBatchPermission() as an public method name isn't great. No Mojo doesn't support overloading which is why I had to do this :(. Yeah I really don't like either the Multiple or Batch prefix. Although I have no idea what would be a good name either. https://codereview.chromium.org/1260193009/diff/230001/content/public/browser... File content/public/browser/permission_manager.h (right): https://codereview.chromium.org/1260193009/diff/230001/content/public/browser... content/public/browser/permission_manager.h:24: using BatchRequestCallback = On 2015/08/21 10:24:41, Mounir Lamouri wrote: > nit: I'm not sure which one I prefer between BatchRequestCallback and > MultiRequestCallback. You prefer Batch? I really don't like either of them. I'd rather try and come up with a replacement but between the two I think batch edges it for me.
https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... File content/browser/permissions/permission_service_impl.cc (right): https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:128: } On 2015/08/21 10:24:41, Mounir Lamouri wrote: > style: remove { } Done. https://codereview.chromium.org/1260193009/diff/230001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:139: } On 2015/08/21 10:24:41, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1260193009/diff/230001/content/child/permissi... File content/child/permissions/permission_dispatcher.cc (left): https://codereview.chromium.org/1260193009/diff/230001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:224: On 2015/08/21 10:24:41, Mounir Lamouri wrote: > nit: revert that change Done. https://codereview.chromium.org/1260193009/diff/230001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:245: On 2015/08/21 10:24:41, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1260193009/diff/230001/content/child/permissi... File content/child/permissions/permission_dispatcher.cc (right): https://codereview.chromium.org/1260193009/diff/230001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:287: batch_pending_callbacks_.insert(batch_pending_callbacks_.end(), callback); On 2015/08/21 10:24:41, Mounir Lamouri wrote: > I guess this needs to be rebased. Done. https://codereview.chromium.org/1260193009/diff/230001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_permission_manager.cc (right): https://codereview.chromium.org/1260193009/diff/230001/content/shell/browser/... content/shell/browser/layout_test/layout_test_permission_manager.cc:89: PERMISSION_STATUS_DENIED); On 2015/08/21 10:24:41, Mounir Lamouri wrote: > nit: I would move the two arguments on this second line. Done.
I had another look at the content/renderer/ content/child/ and content/public/ changes. I will need to look more deeply at the content/browser/ (ie. mojo service) and chrome/ changes. I think what I looked at was fine except for namings. https://codereview.chromium.org/1260193009/diff/250001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.h (right): https://codereview.chromium.org/1260193009/diff/250001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.h:41: void RequestPermission( RequestPermissions ? https://codereview.chromium.org/1260193009/diff/250001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.h:47: const BatchRequestCallback& callback) override; use the real callback type? https://codereview.chromium.org/1260193009/diff/250001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.h:74: = base::ScopedPtrHashMap<int, scoped_ptr<PendingResponses>>; nit: = on previous line. https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... File content/browser/permissions/permission_pending_request.cc (right): https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... content/browser/permissions/permission_pending_request.cc:15: int request_count) : style: I think ':' should be on the next line https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... File content/browser/permissions/permission_pending_request.h (right): https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... content/browser/permissions/permission_pending_request.h:10: #include "content/common/permission_service.mojom.h" Do you really need that? https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... content/browser/permissions/permission_pending_request.h:11: #include "third_party/mojo/src/mojo/public/cpp/bindings/binding.h" and that? https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... content/browser/permissions/permission_pending_request.h:12: #include "url/gurl.h" and that? https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... content/browser/permissions/permission_pending_request.h:18: class PermissionPendingRequest { Why isn't that part of the mojo service implementation? It seems that it could have stayed there. Not that I'm against having it in its own file but it seems to be a fairly simple class. https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... File content/child/permissions/permission_dispatcher.h (right): https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... content/child/permissions/permission_dispatcher.h:92: using PendingBatchCallbackMap = Name-wise, what about: PermissionCallbackMap and PermissionsCallbackMap. Again, the 's' is subtle but maybe the type will help. https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... content/child/permissions/permission_dispatcher.h:98: static void RunCallbackOnWorkerThread( Rename to RunPermissionCallbackOnWorkerThread? https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... content/child/permissions/permission_dispatcher.h:101: static void RunMultiCallbackOnWorkerThread( RunPermissionsCallbackOnWorkerThread? https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... content/child/permissions/permission_dispatcher.h:130: void OnPermissionsResponse(int worker_thread_id, Rename OnRequestPermissionsResponse() https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... content/child/permissions/permission_dispatcher.h:148: PendingBatchCallbackMap batch_pending_callbacks_; // Pending callbacks for query(), revoke() and request() single permission. PermissionCallbackMap permission_callbacks_; // Pending callbacks for request() multiple permissions. PermissionsCallbackMap permissions_callback_; (You can also rename all that PendingPermissionCallbacks and PendingPermissionsCallbacks. It might be slightly better?) https://codereview.chromium.org/1260193009/diff/250001/content/common/permiss... File content/common/permission_service.mojom (right): https://codereview.chromium.org/1260193009/diff/250001/content/common/permiss... content/common/permission_service.mojom:27: RequestBatchPermission(array<PermissionName> permission, string origin, bool user_gesture) Rename to RequestPermissions() https://codereview.chromium.org/1260193009/diff/250001/content/public/browser... File content/public/browser/permission_manager.h (right): https://codereview.chromium.org/1260193009/diff/250001/content/public/browser... content/public/browser/permission_manager.h:24: using BatchRequestCallback = Let's drop "Batch" has a name. It's really odd. Also, could you remove the using from the public space of the class? https://codereview.chromium.org/1260193009/diff/250001/content/public/browser... content/public/browser/permission_manager.h:46: virtual void RequestPermission( I would tend to say that you can add an 's' here. I know the name difference is subtle but one method takes a vector<> and not the other so callers shouldn't be mistaken. Ideally, both should be named "Request()" but that's not going to happen ;) https://codereview.chromium.org/1260193009/diff/250001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_permission_manager.cc (right): https://codereview.chromium.org/1260193009/diff/250001/content/shell/browser/... content/shell/browser/layout_test/layout_test_permission_manager.cc:70: const RequestCallback& callback) { nit: keep the plain callback type. https://codereview.chromium.org/1260193009/diff/250001/content/shell/browser/... content/shell/browser/layout_test/layout_test_permission_manager.cc:88: std::vector<PermissionStatus> statuses( nit: if you want, you can rename statuses to result. https://codereview.chromium.org/1260193009/diff/250001/content/shell/browser/... content/shell/browser/layout_test/layout_test_permission_manager.cc:90: for (size_t i = 0; i < permissions.size(); i++) { nit: you can probably do something among those lines: for (const auto& permission : permissions) { result.push_back(GetPermissionStatus(permission, requesting_origin, WebContents::...()); } https://codereview.chromium.org/1260193009/diff/250001/content/shell/browser/... File content/shell/browser/shell_permission_manager.cc (right): https://codereview.chromium.org/1260193009/diff/250001/content/shell/browser/... content/shell/browser/shell_permission_manager.cc:40: statuses[i] = permissions[i] == PermissionType::GEOLOCATION Same as above.
Change seems to be coming down in size slowly. https://codereview.chromium.org/1260193009/diff/250001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.h (right): https://codereview.chromium.org/1260193009/diff/250001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.h:41: void RequestPermission( On 2015/09/02 11:36:41, Mounir Lamouri wrote: > RequestPermissions ? Done. https://codereview.chromium.org/1260193009/diff/250001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.h:47: const BatchRequestCallback& callback) override; On 2015/09/02 11:36:41, Mounir Lamouri wrote: > use the real callback type? Makes the line too long and breaking it up looks ugly in most ways. Anyway, I've tried to do so. https://codereview.chromium.org/1260193009/diff/250001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.h:74: = base::ScopedPtrHashMap<int, scoped_ptr<PendingResponses>>; On 2015/09/02 11:36:41, Mounir Lamouri wrote: > nit: = on previous line. Done. https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... File content/browser/permissions/permission_pending_request.cc (right): https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... content/browser/permissions/permission_pending_request.cc:15: int request_count) : On 2015/09/02 11:36:41, Mounir Lamouri wrote: > style: I think ':' should be on the next line Done. https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... File content/browser/permissions/permission_pending_request.h (right): https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... content/browser/permissions/permission_pending_request.h:10: #include "content/common/permission_service.mojom.h" On 2015/09/02 11:36:41, Mounir Lamouri wrote: > Do you really need that? No. But I do need the Mojo callback file. Yay for getting rid of transitive dependencies. https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... content/browser/permissions/permission_pending_request.h:11: #include "third_party/mojo/src/mojo/public/cpp/bindings/binding.h" On 2015/09/02 11:36:41, Mounir Lamouri wrote: > and that? And no. https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... content/browser/permissions/permission_pending_request.h:12: #include "url/gurl.h" On 2015/09/02 11:36:41, Mounir Lamouri wrote: > and that? And no :) https://codereview.chromium.org/1260193009/diff/250001/content/browser/permis... content/browser/permissions/permission_pending_request.h:18: class PermissionPendingRequest { On 2015/09/02 11:36:41, Mounir Lamouri wrote: > Why isn't that part of the mojo service implementation? It seems that it could > have stayed there. Not that I'm against having it in its own file but it seems > to be a fairly simple class. TBH it could probably be moved back to permisison_manager. At one point it was much more complex than it is now. I don't mind either way - I can move it back if you prefer. https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... File content/child/permissions/permission_dispatcher.h (right): https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... content/child/permissions/permission_dispatcher.h:92: using PendingBatchCallbackMap = On 2015/09/02 11:36:41, Mounir Lamouri wrote: > Name-wise, what about: PermissionCallbackMap and PermissionsCallbackMap. Again, > the 's' is subtle but maybe the type will help. Done. https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... content/child/permissions/permission_dispatcher.h:98: static void RunCallbackOnWorkerThread( On 2015/09/02 11:36:41, Mounir Lamouri wrote: > Rename to RunPermissionCallbackOnWorkerThread? Done. https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... content/child/permissions/permission_dispatcher.h:101: static void RunMultiCallbackOnWorkerThread( On 2015/09/02 11:36:41, Mounir Lamouri wrote: > RunPermissionsCallbackOnWorkerThread? Done. https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... content/child/permissions/permission_dispatcher.h:130: void OnPermissionsResponse(int worker_thread_id, On 2015/09/02 11:36:41, Mounir Lamouri wrote: > Rename OnRequestPermissionsResponse() Done. https://codereview.chromium.org/1260193009/diff/250001/content/child/permissi... content/child/permissions/permission_dispatcher.h:148: PendingBatchCallbackMap batch_pending_callbacks_; On 2015/09/02 11:36:41, Mounir Lamouri wrote: > // Pending callbacks for query(), revoke() and request() single permission. > PermissionCallbackMap permission_callbacks_; > > // Pending callbacks for request() multiple permissions. > PermissionsCallbackMap permissions_callback_; > > (You can also rename all that PendingPermissionCallbacks and > PendingPermissionsCallbacks. It might be slightly better?) Done. https://codereview.chromium.org/1260193009/diff/250001/content/common/permiss... File content/common/permission_service.mojom (right): https://codereview.chromium.org/1260193009/diff/250001/content/common/permiss... content/common/permission_service.mojom:27: RequestBatchPermission(array<PermissionName> permission, string origin, bool user_gesture) On 2015/09/02 11:36:41, Mounir Lamouri wrote: > Rename to RequestPermissions() Done. https://codereview.chromium.org/1260193009/diff/250001/content/public/browser... File content/public/browser/permission_manager.h (right): https://codereview.chromium.org/1260193009/diff/250001/content/public/browser... content/public/browser/permission_manager.h:24: using BatchRequestCallback = On 2015/09/02 11:36:41, Mounir Lamouri wrote: > Let's drop "Batch" has a name. It's really odd. > > Also, could you remove the using from the public space of the class? Done. https://codereview.chromium.org/1260193009/diff/250001/content/public/browser... content/public/browser/permission_manager.h:46: virtual void RequestPermission( On 2015/09/02 11:36:41, Mounir Lamouri wrote: > I would tend to say that you can add an 's' here. I know the name difference is > subtle but one method takes a vector<> and not the other so callers shouldn't be > mistaken. > > Ideally, both should be named "Request()" but that's not going to happen ;) Done. https://codereview.chromium.org/1260193009/diff/250001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_permission_manager.cc (right): https://codereview.chromium.org/1260193009/diff/250001/content/shell/browser/... content/shell/browser/layout_test/layout_test_permission_manager.cc:70: const RequestCallback& callback) { On 2015/09/02 11:36:42, Mounir Lamouri wrote: > nit: keep the plain callback type. Done. https://codereview.chromium.org/1260193009/diff/250001/content/shell/browser/... content/shell/browser/layout_test/layout_test_permission_manager.cc:88: std::vector<PermissionStatus> statuses( On 2015/09/02 11:36:42, Mounir Lamouri wrote: > nit: if you want, you can rename statuses to result. Done.
I was thinking of something in order to reduce the size patch and cut it in pieces. Could you remove all the code from the browser process to another CL? Basically, the Mojo service implementation of RequestPermissions() would have a NOTIMPLEMENTED() and will immediately return with the current status of all the permissions being requested. I think we could get that CL to land fairly quickly and have the other (smaller) CL being reviewed more thoroughly. WDYT?
On 2015/09/04 10:39:09, Mounir Lamouri wrote: > I was thinking of something in order to reduce the size patch and cut it in > pieces. > > Could you remove all the code from the browser process to another CL? Basically, > the Mojo service implementation of RequestPermissions() would have a > NOTIMPLEMENTED() and will immediately return with the current status of all the > permissions being requested. I think we could get that CL to land fairly quickly > and have the other (smaller) CL being reviewed more thoroughly. WDYT? That sounds like a very good idea to me as most of the headache in this CL is in permission_manager.
Please improve the CL description: """ Implement renderer side of multiple permissions request. This CL implements navigator.permissions.request() with multiple permissions on the renderer side (permission_dispatcher and thread_proxy implementation). On the browser side, it gets the mojo service to return the current permission statuses in order for the call to "work". It does not implement a proper permission request UI yet. The browser side logic is being implemented in: https://codereview.chromium.org/1316863010/ BUG=516626 """ Feel free to change the wording obviously.
Message looks good to me so used as is.
lgtm with comments applied. https://codereview.chromium.org/1260193009/diff/290001/content/browser/permis... File content/browser/permissions/permission_service_impl.cc (right): https://codereview.chromium.org/1260193009/diff/290001/content/browser/permis... content/browser/permissions/permission_service_impl.cc:141: mojo::Array<PermissionStatus> result(permissions.size()); add: TODO(lalitm,mlamouri): this is returning the current permission statuses in order for the call to successfully return. It will be changed later. See htts://crbug.com/516626 https://codereview.chromium.org/1260193009/diff/290001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1260193009/diff/290001/content/content_browse... content/content_browser.gypi:1104: 'browser/permissions/permission_pending_request.h', I doubt you need that.
lalitm@google.com changed reviewers: + tsepez@chromium.org
tsepez@: could you please take a look at the permission_service mojo change? https://chromiumcodereview.appspot.com/1260193009/diff/290001/content/browser... File content/browser/permissions/permission_service_impl.cc (right): https://chromiumcodereview.appspot.com/1260193009/diff/290001/content/browser... content/browser/permissions/permission_service_impl.cc:141: mojo::Array<PermissionStatus> result(permissions.size()); On 2015/09/04 15:04:13, Mounir Lamouri wrote: > add: > TODO(lalitm,mlamouri): this is returning the current permission statuses in > order for the call to successfully return. It will be changed later. See > htts://crbug.com/516626 Done. https://chromiumcodereview.appspot.com/1260193009/diff/290001/content/content... File content/content_browser.gypi (right): https://chromiumcodereview.appspot.com/1260193009/diff/290001/content/content... content/content_browser.gypi:1104: 'browser/permissions/permission_pending_request.h', On 2015/09/04 15:04:13, Mounir Lamouri wrote: > I doubt you need that. Done.
lgtm
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1260193009/#ps310001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1260193009/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1260193009/310001
Message was sent while issue was closed.
Committed patchset #17 (id:310001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/d0b65b68ecad02a4e677b854a107ae267c8ad93e Cr-Commit-Position: refs/heads/master@{#347446} |
