Web Locks API: Drop support for multi-resource locks. [chromium/src : master]
Joshua Bell (Gerrit)
pwnall@ - please take a look?
Note that I didn't rework the state/algorithms in the back-end in this CL. With better data structures/algorithms we can e.g. rework things to avoid walking over all locks whenever anything is released, but IMHO that makes a nice isolated follow-up.
To view, visit change 828286. To unsubscribe, or for help writing mail filters, visit settings.
Victor Costan (Gerrit)
I'm very happy to see this!
LGTM with comments addressed. (and I think you'll have to change the webexposed expectations, because of the scope -> name attribute change)
I agree that the backend optimization should be a separate CL, especially as this CL will require a Blink API OWNER review.
Patch set 1:Code-Review +1
4 comments:
File third_party/WebKit/LayoutTests/http/tests/locks/frames.html:
Given the test description, I think it still makes sense with a single-resource Lock API. So, I think the test should be modified so all 3 frames request 'r1', instead of deleting it.
I think we should keep this test (same as above).
File third_party/WebKit/LayoutTests/http/tests/locks/interfaces.html:
Patch Set #1, Line 31:
await navigator.locks.acquire('name', async l => { lock = l; });nit: Not a problem introduced in this CL, but I think "async" can be removed from this callback.
File third_party/WebKit/LayoutTests/http/tests/locks/mode-shared.html:
Patch Set #1, Line 33:
// lock was all 'shared', causing this test to time out.nit: "if the locks were not all shared" (or, if you prefer, "if any lock was exclusive")
To view, visit change 828286. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Bell (Gerrit)
Patch set 2:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"address review feedback and consolidate ack checks" https://chromium-review.googlesource.com/c/828286/2
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/828286/2
Bot data: {"action": "start", "triggered_at": "2017-12-15T20:53:01.0Z", "cq_cfg_revision": "b547f51ef97353cccb06eebcda71133909f61295", "revision": "f4b790bc151c16dd41211ea5df6bbd3911c84f52"}
Joshua Bell (Gerrit)
Thanks!
Patch Set 1: Code-Review+1
I agree that the backend optimization should be a separate CL, especially as this CL will require a Blink API OWNER review.
Only for virtual/stable which this doesn't touch.
4 comments:
Given the test description, I think it still makes sense with a single-resource Lock API. […]
Since 3 will still be blocked until 1 releases the lock, I wasn't sure this was actually testing anything differently than the previous tests.
But I guess this turns into a test that the navigated frame with pending request doesn't end up somehow getting the lock or wedging the scheduler. So adding it back in.
I think we should keep this test (same as above).
Done
File third_party/WebKit/LayoutTests/http/tests/locks/interfaces.html:
Patch Set #1, Line 31:
await navigator.locks.acquire('name', l => { lock = l; });
nit: Not a problem introduced in this CL, but I think "async" can be removed from this callback.
Done
File third_party/WebKit/LayoutTests/http/tests/locks/mode-shared.html:
Patch Set #1, Line 33:
// lock was not 'shared', causing this test to time out.
nit: "if the locks were not all shared" (or, if you prefer, "if any lock was exclusive")
Bleah, I just rewrote the comment completely. Must have been asleep. Now reads:
Since lock is held, this request would be blocked if the
lock was not 'shared', causing this test to time out.
To view, visit change 828286. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
Try jobs failed on following builders:
chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/619194)
Joshua Bell (Gerrit)
dcheng@ - can you OWNERS review the mojom change? (and CQ if it LG?) Thanks!
Joshua Bell (Gerrit)
Joshua Bell would like Daniel Cheng to review this change.
Web Locks API: Drop support for multi-resource locks.
Based on discussion on the proposal, simplify to locks representing a
single resource. Multi-resource locks can be implemented in user-space,
with appropriate care taken to avoid deadlocks.
Discussion: https://github.com/inexorabletash/web-locks/issues/20
Change-Id: I23980f3cdaf403bfbb641a36ef30bd92d665cef3
---
M content/browser/locks/lock_manager.cc
M content/browser/locks/lock_manager.h
M third_party/WebKit/LayoutTests/http/tests/locks/acquire.html
M third_party/WebKit/LayoutTests/http/tests/locks/frames.html
M third_party/WebKit/LayoutTests/http/tests/locks/ifAvailable.html
M third_party/WebKit/LayoutTests/http/tests/locks/interfaces.html
M third_party/WebKit/LayoutTests/http/tests/locks/interfaces.idl
M third_party/WebKit/LayoutTests/http/tests/locks/lock-attributes.html
M third_party/WebKit/LayoutTests/http/tests/locks/mode-exclusive.html
M third_party/WebKit/LayoutTests/http/tests/locks/mode-mixed.html
M third_party/WebKit/LayoutTests/http/tests/locks/mode-shared.html
M third_party/WebKit/LayoutTests/http/tests/locks/opaque-origin.html
M third_party/WebKit/LayoutTests/http/tests/locks/resource-names.html
M third_party/WebKit/LayoutTests/http/tests/locks/resources/iframe.html
M third_party/WebKit/LayoutTests/http/tests/locks/resources/interfaces-worker.js
M third_party/WebKit/LayoutTests/http/tests/locks/resources/worker.js
M third_party/WebKit/LayoutTests/http/tests/locks/workers.html
M third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-dedicated-worker-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-shared-worker-expected.txt
M third_party/WebKit/Source/modules/locks/Lock.cpp
M third_party/WebKit/Source/modules/locks/Lock.h
M third_party/WebKit/Source/modules/locks/Lock.idl
M third_party/WebKit/Source/modules/locks/LockManager.cpp
M third_party/WebKit/Source/modules/locks/LockManager.h
M third_party/WebKit/Source/modules/locks/LockManager.idl
M third_party/WebKit/public/platform/modules/locks/lock_manager.mojom
28 files changed, 191 insertions(+), 274 deletions(-)
Daniel Cheng (Gerrit)
LGTM with nits addressed
Patch set 2:Code-Review +1
2 comments:
File third_party/WebKit/public/platform/modules/locks/lock_manager.mojom:
Patch Set #2, Line 24:
interface LockManager {Nit: can this have a brief description of what it's supposed to do? Maybe this could link to the web spec and briefly describe what web locks are.
Patch Set #2, Line 35:
RequestLock(url.mojom.Origin origin, string name, LockMode mode, WaitMode wait, LockRequest request);Nit: wrap at 80 please.
To view, visit change 828286. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Bell (Gerrit)
Patch set 3:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Comment and wrap mojom" https://chromium-review.googlesource.com/c/828286/3
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/828286/3
Bot data: {"action": "start", "triggered_at": "2017-12-18T21:16:34.0Z", "cq_cfg_revision": "b547f51ef97353cccb06eebcda71133909f61295", "revision": "8ad0d400b1b0f11952268ca8171061ea56ca086b"}
Joshua Bell (Gerrit)
Thanks!
2 comments:
File third_party/WebKit/public/platform/modules/locks/lock_manager.mojom:
Patch Set #2, Line 24:
// TODO(jsbell): Abort() is not yet used.Nit: can this have a brief description of what it's supposed to do? Maybe this could link to the web […]
Done
Nit: wrap at 80 please.
Done
To view, visit change 828286. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
Commit Bot merged this change.
Web Locks API: Drop support for multi-resource locks.
Based on discussion on the proposal, simplify to locks representing a
single resource. Multi-resource locks can be implemented in user-space,
with appropriate care taken to avoid deadlocks.
Discussion: https://github.com/inexorabletash/web-locks/issues/20
Change-Id: I23980f3cdaf403bfbb641a36ef30bd92d665cef3
Reviewed-on: https://chromium-review.googlesource.com/828286
Commit-Queue: Joshua Bell <jsb...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Victor Costan <pwn...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524856}
28 files changed, 211 insertions(+), 276 deletions(-)