Bug 182320 – Canonicalize aquiring the JSCell lock.

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Canonicalize aquiring the JSCell lock.

[Keith Miller]

Created attachment 332741 [details]
Patch

[Keith Miller]

Created attachment 332762 [details]
Patch

[Keith Miller]

Created attachment 332763 [details]
Patch

[Saam Barati]

Comment on attachment 332763 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCell.h:124
> +    // It's also recommended that cells are locked with the holdJSCellLock() method to help keep

Why not make those private and friend lockholder then? Or you could do what I suggest below and make these private and people can call lock/unlock on the wrapper type

> Source/JavaScriptCore/runtime/JSCell.h:126
> +    Locker<JSCell> holdJSCellLock() { return holdLock(*this); }

Not in love with the name here. What if you had some method named “cellLock” and it returned to you some wrapper that exposes lock/unlock methods. You could do:
auto locker = holdLock(cell->cellLock())
But I’m not sure if that’s much better

[Keith Miller]

Comment on attachment 332763 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSCell.h:124
>> +    // It's also recommended that cells are locked with the holdJSCellLock() method to help keep
> 
> Why not make those private and friend lockholder then? Or you could do what I suggest below and make these private and people can call lock/unlock on the wrapper type

You can't really do that. If you friend Locker then anyone can make a Locker that can lock a JSCell so we're in the same boat. You also can't make a subclass of Locker that you friend because the base class needs access to lock/unlock.

>> Source/JavaScriptCore/runtime/JSCell.h:126
>> +    Locker<JSCell> holdJSCellLock() { return holdLock(*this); }
> 
> Not in love with the name here. What if you had some method named “cellLock” and it returned to you some wrapper that exposes lock/unlock methods. You could do:
> auto locker = holdLock(cell->cellLock())
> But I’m not sure if that’s much better

Yeah, I guess that's a bit better in that you can't just holdLock(cell).

[Keith Miller]

Created attachment 332773 [details]
Patch

[Keith Miller]

Created attachment 332775 [details]
Patch

[Michael Saboff]

Comment on attachment 332775 [details]
Patch

r=me after build failures.

[Keith Miller]

(In reply to Michael Saboff from comment #8)
> Comment on attachment 332775 [details]
> Patch
> 
> r=me after build failures.

Looks like ToT is broken...

[WebKit Commit Bot]

Comment on attachment 332775 [details]
Patch

Clearing flags on attachment: 332775

Committed r227906: <https://trac.webkit.org/changeset/227906>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37077485>

[Saam Barati]

Comment on attachment 332775 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCell.h:288
> +class JSCellLock : public JSCell {

nice. This patch LGTM too. I think this API works out nicely.