Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2573573002:
Switch PermissionStatus events to an observer model. (Closed)
|
Created:
4 years ago by Reilly Grant (use Gerrit) Modified:
3 years, 11 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, jam, mlamouri+watch-blink_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch PermissionStatus events to an observer model.
This change replaces the PermissionService::GetNextPermissionChange
method with a PermissionObserver interface that more closely matches the
permission subscription model that exists in the browser process as it
is unnecessary for the client to throttle these low-frequency updates.
This also makes it cheaper (1 message vs. 2) to send an update to the
renderer.
BUG=677774
Committed: https://crrev.com/86dfcea3340fe31e9b44123163b90501df9ffde8
Cr-Commit-Position: refs/heads/master@{#441491}
Patch Set 1 #Patch Set 2 : Rebased. #
Total comments: 2
Patch Set 3 : Remove call to registerPreFinalizer(). #
Total comments: 10
Patch Set 4 : Addresses mlamouri@'s feedback. #Patch Set 5 : Rebased. #Messages
Total messages: 43 (25 generated)
The CQ bit was checked by reillyg@chromium.org to run a CQ dry run
reillyg@chromium.org changed reviewers: + mlamouri@chromium.org, tsepez@chromium.org
tsepez@, IPC security review. mlamouri@, general review.
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
mojom itself LGTM, but still obviously lotsa red bots.
Rebased.
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.
mlamouri@chromium.org changed reviewers: + blundell@chromium.org
This was designed based on the constraints and recommendations from the mojo folks at the time. I'm not sure who should have a look at this wrt to the mojo design. +blundell@ who might know.
On 2016/12/13 13:35:56, mlamouri wrote: > This was designed based on the constraints and recommendations from the mojo > folks at the time. I'm not sure who should have a look at this wrt to the mojo > design. +blundell@ who might know. This design is the current recommendation. I did a similar refactor to the USB mojom a while ago and am just getting around to this one.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2573573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp (right): https://codereview.chromium.org/2573573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:44: ThreadState::current()->registerPreFinalizer(this); We removed this method. Now the registration is not needed. USING_PRE_FINALIZER is enough to make it work.
Hi Reilly, Are you referring to the relatively recent chromium-mojo@ thread on push vs. pull (I unfortunately can't find the link)? If so, the ending recommendation was that the right pattern was situation-dependent, so I would just add some justification to the CL description here on why the ability for the client to throttle updates isn't necessary in this case. Thanks, Colin
Description was changed from ========== Switch PermissionStatus events to an observer model. This change replaces the PermissionService::GetNextPermissionChange method with a PermissionObserver interface that more closely matches the permission subscription model that exists in the browser process. This also makes it cheaper (1 message vs. 2) to send an update to the renderer. BUG=None ========== to ========== Switch PermissionStatus events to an observer model. This change replaces the PermissionService::GetNextPermissionChange method with a PermissionObserver interface that more closely matches the permission subscription model that exists in the browser process as it is unnecessary for the client to throttle these updates. This also makes it cheaper (1 message vs. 2) to send an update to the renderer. BUG=None ==========
Description was changed from ========== Switch PermissionStatus events to an observer model. This change replaces the PermissionService::GetNextPermissionChange method with a PermissionObserver interface that more closely matches the permission subscription model that exists in the browser process as it is unnecessary for the client to throttle these updates. This also makes it cheaper (1 message vs. 2) to send an update to the renderer. BUG=None ========== to ========== Switch PermissionStatus events to an observer model. This change replaces the PermissionService::GetNextPermissionChange method with a PermissionObserver interface that more closely matches the permission subscription model that exists in the browser process as it is unnecessary for the client to throttle these low-frequency updates. This also makes it cheaper (1 message vs. 2) to send an update to the renderer. BUG=None ==========
I've updated the description to clarify that these updates are low-frequency so there is no concern about needing to be able to throttle them. https://codereview.chromium.org/2573573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp (right): https://codereview.chromium.org/2573573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:44: ThreadState::current()->registerPreFinalizer(this); On 2016/12/14 02:00:33, haraken wrote: > > We removed this method. Now the registration is not needed. USING_PRE_FINALIZER > is enough to make it work. Done.
On 2016/12/15 23:13:26, Reilly Grant wrote: > I've updated the description to clarify that these updates are low-frequency so > there is no concern about needing to be able to throttle them. > > https://codereview.chromium.org/2573573002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp (right): > > https://codereview.chromium.org/2573573002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:44: > ThreadState::current()->registerPreFinalizer(this); > On 2016/12/14 02:00:33, haraken wrote: > > > > We removed this method. Now the registration is not needed. > USING_PRE_FINALIZER > > is enough to make it work. > > Done. oilpan part LGTM
Thanks! I agree that the switch to observer definitely fits the recommendations from that thread.
lgtm with comments applied https://codereview.chromium.org/2573573002/diff/40001/content/browser/permiss... File content/browser/permissions/permission_service_context.cc (right): https://codereview.chromium.org/2573573002/diff/40001/content/browser/permiss... content/browser/permissions/permission_service_context.cc:47: int id_; What's `id_` initial value? Please set a value here and add a DCHECK in `OnConnectionError`. https://codereview.chromium.org/2573573002/diff/40001/content/browser/permiss... content/browser/permissions/permission_service_context.cc:141: for (const auto& service : services_) why? https://codereview.chromium.org/2573573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp (right): https://codereview.chromium.org/2573573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:45: PermissionStatus::~PermissionStatus() {} = default; https://codereview.chromium.org/2573573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:106: if (m_status != status) { You should ideally prefer early returns: ``` if (m_status == status) return; ``` https://codereview.chromium.org/2573573002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/permissions/permission.mojom (right): https://codereview.chromium.org/2573573002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/permissions/permission.mojom:57: AddPermissionObserver(PermissionDescriptor permission, It would be good to add a comment. You should be able to re-use most of what you removed above. Also, can you explain that the observe will be auto-removed when the connection drops? There is no `RemovePermissionObserver` and at a first glance it's unclear if it's on purpose or a bug.
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...
https://codereview.chromium.org/2573573002/diff/40001/content/browser/permiss... File content/browser/permissions/permission_service_context.cc (right): https://codereview.chromium.org/2573573002/diff/40001/content/browser/permiss... content/browser/permissions/permission_service_context.cc:47: int id_; On 2016/12/19 13:37:45, mlamouri wrote: > What's `id_` initial value? Please set a value here and add a DCHECK in > `OnConnectionError`. Done. https://codereview.chromium.org/2573573002/diff/40001/content/browser/permiss... content/browser/permissions/permission_service_context.cc:141: for (const auto& service : services_) On 2016/12/19 13:37:45, mlamouri wrote: > why? services_ changed from a ScopedVector to a std::vector<std::unique_ptr> which changes the type of the iterator. https://codereview.chromium.org/2573573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp (right): https://codereview.chromium.org/2573573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:45: PermissionStatus::~PermissionStatus() {} On 2016/12/19 13:37:45, mlamouri wrote: > = default; Done. https://codereview.chromium.org/2573573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:106: if (m_status != status) { On 2016/12/19 13:37:45, mlamouri wrote: > You should ideally prefer early returns: > ``` > if (m_status == status) > return; > ``` Done. https://codereview.chromium.org/2573573002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/permissions/permission.mojom (right): https://codereview.chromium.org/2573573002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/permissions/permission.mojom:57: AddPermissionObserver(PermissionDescriptor permission, On 2016/12/19 13:37:45, mlamouri wrote: > It would be good to add a comment. You should be able to re-use most of what you > removed above. Also, can you explain that the observe will be auto-removed when > the connection drops? There is no `RemovePermissionObserver` and at a first > glance it's unclear if it's on purpose or a bug. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Switch PermissionStatus events to an observer model. This change replaces the PermissionService::GetNextPermissionChange method with a PermissionObserver interface that more closely matches the permission subscription model that exists in the browser process as it is unnecessary for the client to throttle these low-frequency updates. This also makes it cheaper (1 message vs. 2) to send an update to the renderer. BUG=None ========== to ========== Switch PermissionStatus events to an observer model. This change replaces the PermissionService::GetNextPermissionChange method with a PermissionObserver interface that more closely matches the permission subscription model that exists in the browser process as it is unnecessary for the client to throttle these low-frequency updates. This also makes it cheaper (1 message vs. 2) to send an update to the renderer. BUG=677774 ==========
The CQ bit was checked by reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, haraken@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2573573002/#ps80001 (title: "Rebased.")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by reillyg@chromium.org
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": 80001, "attempt_start_ts": 1483563160554510,
"parent_rev": "c619a2c9ebfc8f53aa1d5c5c658db8e13a4e0150", "commit_rev":
"f3a2fc2fc9c9f16277a569212a91fb234d3a7eab"}
Message was sent while issue was closed.
Description was changed from ========== Switch PermissionStatus events to an observer model. This change replaces the PermissionService::GetNextPermissionChange method with a PermissionObserver interface that more closely matches the permission subscription model that exists in the browser process as it is unnecessary for the client to throttle these low-frequency updates. This also makes it cheaper (1 message vs. 2) to send an update to the renderer. BUG=677774 ========== to ========== Switch PermissionStatus events to an observer model. This change replaces the PermissionService::GetNextPermissionChange method with a PermissionObserver interface that more closely matches the permission subscription model that exists in the browser process as it is unnecessary for the client to throttle these low-frequency updates. This also makes it cheaper (1 message vs. 2) to send an update to the renderer. BUG=677774 Review-Url: https://codereview.chromium.org/2573573002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Switch PermissionStatus events to an observer model. This change replaces the PermissionService::GetNextPermissionChange method with a PermissionObserver interface that more closely matches the permission subscription model that exists in the browser process as it is unnecessary for the client to throttle these low-frequency updates. This also makes it cheaper (1 message vs. 2) to send an update to the renderer. BUG=677774 Review-Url: https://codereview.chromium.org/2573573002 ========== to ========== Switch PermissionStatus events to an observer model. This change replaces the PermissionService::GetNextPermissionChange method with a PermissionObserver interface that more closely matches the permission subscription model that exists in the browser process as it is unnecessary for the client to throttle these low-frequency updates. This also makes it cheaper (1 message vs. 2) to send an update to the renderer. BUG=677774 Committed: https://crrev.com/86dfcea3340fe31e9b44123163b90501df9ffde8 Cr-Commit-Position: refs/heads/master@{#441491} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/86dfcea3340fe31e9b44123163b90501df9ffde8 Cr-Commit-Position: refs/heads/master@{#441491} |
