[webkit-reviews] review granted: [Bug 226157] Make CheckedLock the default Lock : [Attachment 429493] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 226157: Make CheckedLock the default Lock
https://bugs.webkit.org/show_bug.cgi?id=226157

Attachment 429493: Patch

https://bugs.webkit.org/attachment.cgi?id=429493&action=review




--- Comment #15 from Darin Adler <darin at apple.com> ---
Comment on attachment 429493
  --> https://bugs.webkit.org/attachment.cgi?id=429493
Patch

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

Seems like a good start. I couldn’t really discern the pattern of where we use
UncheckedLock explicitly in this patch, even though you explained the strategy
and rationale, but I suppose the main point is switching to the checking from
CheckedLock almost everywhere that is *not* in the patch.

> Source/WTF/wtf/Lock.h:216
> +using WTF::assertIsHeld;

Just occurred to me that we might not need this because of argument dependent
lookup <https://en.cppreference.com/w/cpp/language/adl>. If the argument is in
the WTF namespace, then the function is looked for in the WTF namespace without
requiring a WTF:: prefix, so this using line solves a problem that does not
exist.

> Source/WebCore/html/canvas/WebGLObject.h:31
> +#include <wtf/Lock.h>

Could we use Forward.h here instead, and remove the forward declaration of
AbstractLocker below?