Bug 233289

Summary: Add initial implementation for the Web Lock API
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, benjamin, cmarcelo, darin, esprehn+autocc, ews-watchlist, ggaren, gyuyoung.kim, kangil.han, kondapallykalyan, mark.lam, ryuan.choi, sergio, webkit-bug-importer, youennf, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=233655
Bug Depends on:    
Bug Blocks: 232436, 233719    
Attachments:
Description Flags
WIP Patch
ews-feeder: commit-queue-
WIP Patch
ews-feeder: commit-queue-
WIP Patch
none
WIP Patch
none
WIP Patch
ews-feeder: commit-queue-
WIP Patch
none
WIP Patch
none
WIP Patch
ews-feeder: commit-queue-
WIP Patch
none
WIP Patch
ews-feeder: commit-queue-
WIP Patch
ews-feeder: commit-queue-
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2021-11-17 15:58:33 PST
Add initial implementation for the Web Lock API. For now, it only coordinates locks within one WebProcess to keep things the patch as small as possible. Support for multiple processes with be added in a follow-up.
Comment 1 Chris Dumez 2021-11-17 17:02:03 PST
Created attachment 444611 [details]
WIP Patch
Comment 2 Chris Dumez 2021-11-18 09:00:35 PST
Created attachment 444686 [details]
WIP Patch
Comment 3 Chris Dumez 2021-11-18 10:52:31 PST
Created attachment 444702 [details]
WIP Patch
Comment 4 Chris Dumez 2021-11-18 10:54:41 PST
Created attachment 444703 [details]
WIP Patch
Comment 5 Chris Dumez 2021-11-18 11:20:59 PST
Created attachment 444705 [details]
WIP Patch
Comment 6 Chris Dumez 2021-11-18 16:27:23 PST
Created attachment 444755 [details]
WIP Patch
Comment 7 Chris Dumez 2021-11-18 16:45:17 PST
Created attachment 444757 [details]
WIP Patch
Comment 8 Chris Dumez 2021-11-18 16:49:07 PST
Created attachment 444759 [details]
WIP Patch
Comment 9 Chris Dumez 2021-11-18 17:42:21 PST
Created attachment 444763 [details]
WIP Patch
Comment 10 Chris Dumez 2021-11-18 18:02:36 PST
Created attachment 444764 [details]
WIP Patch
Comment 11 Chris Dumez 2021-11-18 19:27:53 PST
Created attachment 444772 [details]
WIP Patch
Comment 12 Chris Dumez 2021-11-19 07:43:59 PST
Created attachment 444819 [details]
WIP Patch
Comment 13 Chris Dumez 2021-11-19 08:48:27 PST
Created attachment 444823 [details]
WIP Patch
Comment 14 Chris Dumez 2021-11-19 08:59:25 PST
Created attachment 444824 [details]
WIP Patch
Comment 15 Chris Dumez 2021-11-19 10:11:52 PST
Created attachment 444830 [details]
Patch
Comment 16 Chris Dumez 2021-11-19 13:02:06 PST
Created attachment 444847 [details]
Patch
Comment 17 Chris Dumez 2021-11-19 16:40:35 PST
Created attachment 444869 [details]
Patch
Comment 18 Chris Dumez 2021-11-19 16:47:55 PST
Created attachment 444870 [details]
Patch
Comment 19 Chris Dumez 2021-11-19 18:13:13 PST
Created attachment 444878 [details]
Patch
Comment 20 Chris Dumez 2021-11-19 23:13:41 PST
Created attachment 444883 [details]
Patch
Comment 21 Radar WebKit Bug Importer 2021-11-24 15:59:19 PST
<rdar://problem/85735279>
Comment 22 Chris Dumez 2021-11-29 07:29:48 PST
Created attachment 445277 [details]
Patch
Comment 23 Geoffrey Garen 2021-11-29 15:49:23 PST
Comment on attachment 445277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445277&action=review

r=me

> Source/WTF/wtf/Deque.h:576
> +inline size_t Deque<T, inlineCapacity>::removeAllMatching(const Func& func)
>  {
> +    size_t removedCount = 0;
>      size_t count = size();
>      while (count--) {
>          T value = takeFirst();
>          if (!func(value))
>              append(WTFMove(value));
> +        else
> +            ++removedCount;
>      }
> +    return removedCount;
>  }

I wonder if we should just return oldSize - size() here. Simplifies the loop a bit.

> Source/WebCore/Modules/web-locks/WebLockRegistry.cpp:80
> +    bool shouldPrepend = false;

Might be clearer as "bool shouldPrepend = steal;", rather than using a branch below.
Comment 24 Chris Dumez 2021-11-29 16:28:04 PST
Created attachment 445364 [details]
Patch
Comment 25 Chris Dumez 2021-11-29 16:30:21 PST
(In reply to Geoffrey Garen from comment #23)
> Comment on attachment 445277 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445277&action=review
> 
> r=me
> 
> > Source/WTF/wtf/Deque.h:576
> > +inline size_t Deque<T, inlineCapacity>::removeAllMatching(const Func& func)
> >  {
> > +    size_t removedCount = 0;
> >      size_t count = size();
> >      while (count--) {
> >          T value = takeFirst();
> >          if (!func(value))
> >              append(WTFMove(value));
> > +        else
> > +            ++removedCount;
> >      }
> > +    return removedCount;
> >  }
> 
> I wonder if we should just return oldSize - size() here. Simplifies the loop
> a bit.
> 
> > Source/WebCore/Modules/web-locks/WebLockRegistry.cpp:80
> > +    bool shouldPrepend = false;
> 
> Might be clearer as "bool shouldPrepend = steal;", rather than using a
> branch below.

Good ideas, thanks. I made those changes.
Comment 26 EWS 2021-11-29 21:24:45 PST
Committed r286284 (244643@main): <https://commits.webkit.org/244643@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445364 [details].
Comment 27 Darin Adler 2021-11-30 09:31:53 PST
Comment on attachment 445364 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445364&action=review

> Source/WebCore/bindings/js/JSDOMConvertPromise.h:54
> +                auto& scriptController = *downcast<WorkerGlobalScope>(*scriptExecutionContext).script();

Just noticed that this is a function returning a pointer, but we are unconditionally dereferencing. Can we change this to return a reference? Or should we add a null check? Or a comment explaining why.

Not super-important, but mysteriously missing null checks are not my favorite.
Comment 28 Chris Dumez 2021-11-30 13:05:19 PST
(In reply to Darin Adler from comment #27)
> Comment on attachment 445364 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445364&action=review
> 
> > Source/WebCore/bindings/js/JSDOMConvertPromise.h:54
> > +                auto& scriptController = *downcast<WorkerGlobalScope>(*scriptExecutionContext).script();
> 
> Just noticed that this is a function returning a pointer, but we are
> unconditionally dereferencing. Can we change this to return a reference? Or
> should we add a null check? Or a comment explaining why.
> 
> Not super-important, but mysteriously missing null checks are not my
> favorite.

Will follow-up in https://bugs.webkit.org/show_bug.cgi?id=233655 to add a null check since the script controller can in theory be null.