Bug 233289 – Add initial implementation for the Web Lock API

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 444611 [details]
WIP Patch

[Chris Dumez]

Created attachment 444686 [details]
WIP Patch

[Chris Dumez]

Created attachment 444702 [details]
WIP Patch

[Chris Dumez]

Created attachment 444703 [details]
WIP Patch

[Chris Dumez]

Created attachment 444705 [details]
WIP Patch

[Chris Dumez]

Created attachment 444755 [details]
WIP Patch

[Chris Dumez]

Created attachment 444757 [details]
WIP Patch

[Chris Dumez]

Created attachment 444759 [details]
WIP Patch

[Chris Dumez]

Created attachment 444763 [details]
WIP Patch

[Chris Dumez]

Created attachment 444764 [details]
WIP Patch

[Chris Dumez]

Created attachment 444772 [details]
WIP Patch

[Chris Dumez]

Created attachment 444819 [details]
WIP Patch

[Chris Dumez]

Created attachment 444823 [details]
WIP Patch

[Chris Dumez]

Created attachment 444824 [details]
WIP Patch

[Chris Dumez]

Created attachment 444830 [details]
Patch

[Chris Dumez]

Created attachment 444847 [details]
Patch

[Chris Dumez]

Created attachment 444869 [details]
Patch

[Chris Dumez]

Created attachment 444870 [details]
Patch

[Chris Dumez]

Created attachment 444878 [details]
Patch

[Chris Dumez]

Created attachment 444883 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85735279>

[Chris Dumez]

Created attachment 445277 [details]
Patch

[Geoffrey Garen]

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.

[Chris Dumez]

Created attachment 445364 [details]
Patch

[Chris Dumez]

(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.

[EWS]

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].

[Darin Adler]

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.

[Chris Dumez]

(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.