Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2842013002:
Use a mojo::StrongBindingSet to manage PermissionServiceImpls (Closed)
|
Created:
3 years, 8 months ago by Reilly Grant (use Gerrit) Modified:
3 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse a mojo::StrongBindingSet to manage PermissionServiceImpls
mojo::StrongBindingSet replaces the pattern of manually managing a list
of Mojo service implementations and registering connection error
handlers to remove entries from the list when their pipes are closed.
BUG=None
Review-Url: https://codereview.chromium.org/2842013002
Cr-Commit-Position: refs/heads/master@{#470992}
Committed: https://chromium.googlesource.com/chromium/src/+/b743d73b67249433eead4af1f526d77acc31e0cc
Patch Set 1 #
Total comments: 4
Patch Set 2 : CancelPendingOperations -> CloseBindings #Patch Set 3 : Rebased #
Messages
Total messages: 32 (18 generated)
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reillyg@chromium.org changed reviewers: + mlamouri@chromium.org
PTAL
mlamouri@chromium.org changed reviewers: + raymes@chromium.org
+raymes
Overall this looks like a nice simplification but had a question. https://codereview.chromium.org/2842013002/diff/1/content/browser/permissions... File content/browser/permissions/permission_service_context.cc (left): https://codereview.chromium.org/2842013002/diff/1/content/browser/permissions... content/browser/permissions/permission_service_context.cc:147: service->CancelPendingOperations(); Please bear with my ignorance because my mojo-foo isn't very good. Previously this looks like it would have cancelled the operations but the interfaces would still have been bound to the message pipes? However now it looks like the interfaces will be unbound. Is that significant?
https://codereview.chromium.org/2842013002/diff/1/content/browser/permissions... File content/browser/permissions/permission_service_context.cc (left): https://codereview.chromium.org/2842013002/diff/1/content/browser/permissions... content/browser/permissions/permission_service_context.cc:147: service->CancelPendingOperations(); On 2017/04/28 02:46:00, raymes wrote: > Please bear with my ignorance because my mojo-foo isn't very good. > > Previously this looks like it would have cancelled the operations but the > interfaces would still have been bound to the message pipes? However now it > looks like the interfaces will be unbound. Is that significant? It's a good question. Yes, before the requests would be cancelled and now the pipes are closed. The reason I did this is because mojo::BindingSet doesn't provide a way to iterate over the interface implementations to run a method, only to close the pipes. The reason I believe this is okay is that the situations in which we call this method (RFH changed, RFH deleted and navigation) are all cases in which the renderer side of the pipe will be closed anyways.
lgtm Thanks for the explanation. That makes sense. What happens if we needed to iterate over the interfaces in the future? https://codereview.chromium.org/2842013002/diff/1/content/browser/permissions... File content/browser/permissions/permission_service_context.cc (right): https://codereview.chromium.org/2842013002/diff/1/content/browser/permissions... content/browser/permissions/permission_service_context.cc:129: void PermissionServiceContext::CancelPendingOperations( Maybe we should call this CloseBindings (or something) instead of CancelPendingOperations?
I talked to rockot@ about adding a method to iterate over a mojo::StrongBindingSet. It's certainly possible so we can look into it if it ever comes up. https://codereview.chromium.org/2842013002/diff/1/content/browser/permissions... File content/browser/permissions/permission_service_context.cc (right): https://codereview.chromium.org/2842013002/diff/1/content/browser/permissions... content/browser/permissions/permission_service_context.cc:129: void PermissionServiceContext::CancelPendingOperations( On 2017/04/30 23:25:24, raymes wrote: > Maybe we should call this CloseBindings (or something) instead of > CancelPendingOperations? Done.
CancelPendingOperations -> CloseBindings
The CQ bit was checked by reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2842013002/#ps20001 (title: "CancelPendingOperations -> CloseBindings")
The CQ bit was unchecked by reillyg@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
reillyg@chromium.org changed reviewers: + timvolodine@chromium.org
+timvolodine@ for OWNERS review since mlamouri@ is OOO.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/01 20:06:54, Reilly Grant wrote: > +timvolodine@ for OWNERS review since mlamouri@ is OOO. lgtm, provided mlamouri@ is fine with this )
lgtm
Rebased
The CQ bit was checked by reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, timvolodine@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2842013002/#ps40001 (title: "Rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494518313719890,
"parent_rev": "67fdcd138ad6a6a8c27848b668355f8c571a89a1", "commit_rev":
"b743d73b67249433eead4af1f526d77acc31e0cc"}
Message was sent while issue was closed.
Description was changed from ========== Use a mojo::StrongBindingSet to manage PermissionServiceImpls mojo::StrongBindingSet replaces the pattern of manually managing a list of Mojo service implementations and registering connection error handlers to remove entries from the list when their pipes are closed. BUG=None ========== to ========== Use a mojo::StrongBindingSet to manage PermissionServiceImpls mojo::StrongBindingSet replaces the pattern of manually managing a list of Mojo service implementations and registering connection error handlers to remove entries from the list when their pipes are closed. BUG=None Review-Url: https://codereview.chromium.org/2842013002 Cr-Commit-Position: refs/heads/master@{#470992} Committed: https://chromium.googlesource.com/chromium/src/+/b743d73b67249433eead4af1f526... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b743d73b67249433eead4af1f526... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
