Bug 208393

Created attachment 392021 [details] Patch

Comment on attachment 392021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392021&action=review > Source/JavaScriptCore/runtime/CacheableIdentifier.h:50 > + enum DoNotKeepAliveTag { DoNotKeepAlive }; This isn't the right name. We definitely want both to be "kept" alive. But, this is just to indicate that it's a non-cell pointer we don't need to visit. Not sure what the right name is, but "DoNotKeepAlive" is super misleading. Maybe something like "OwnedByCodeBlock"?

Comment on attachment 392021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392021&action=review >> Source/JavaScriptCore/runtime/CacheableIdentifier.h:50 >> + enum DoNotKeepAliveTag { DoNotKeepAlive }; > > This isn't the right name. We definitely want both to be "kept" alive. But, this is just to indicate that it's a non-cell pointer we don't need to visit. Not sure what the right name is, but "DoNotKeepAlive" is super misleading. > > Maybe something like "OwnedByCodeBlock"? How about defining static factory functions? Like, CacheableIdentifier::createWithoutKeepingAlive(const Identifier&), and hiding the constructor.

Comment on attachment 392021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392021&action=review >>> Source/JavaScriptCore/runtime/CacheableIdentifier.h:50 >>> + enum DoNotKeepAliveTag { DoNotKeepAlive }; >> >> This isn't the right name. We definitely want both to be "kept" alive. But, this is just to indicate that it's a non-cell pointer we don't need to visit. Not sure what the right name is, but "DoNotKeepAlive" is super misleading. >> >> Maybe something like "OwnedByCodeBlock"? > > How about defining static factory functions? Like, > > CacheableIdentifier::createWithoutKeepingAlive(const Identifier&), and hiding the constructor. CacheableIdentifier::createFromIdentifierOwnedByCodeBlock ?

Created attachment 392026 [details] Patch

Comment on attachment 392026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392026&action=review r=me > Source/JavaScriptCore/runtime/CacheableIdentifier.h:50 > inline CacheableIdentifier(JSCell* identifier); Personally I like making this constructor hidden too, and defining "createFromCell(JSCell*)` factory function instead, but it's up to you :).

Comment on attachment 392026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392026&action=review >> Source/JavaScriptCore/runtime/CacheableIdentifier.h:50 >> inline CacheableIdentifier(JSCell* identifier); > > Personally I like making this constructor hidden too, and defining "createFromCell(JSCell*)` factory function instead, but it's up to you :). +1 It's weird to have one path use constructor, and one use the static factory thingy.

r=me too

Created attachment 392169 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 392169 [details]: editing/spelling/spellcheck-paste-continuous-disabled.html bug 208016 (authors: g.czajkowski@samsung.com and mark.lam@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 392169 [details] Patch Clearing flags on attachment: 392169 Committed r257728: <https://trac.webkit.org/changeset/257728>

All reviewed patches have been landed. Closing bug.

Comment on attachment 392169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392169&action=review > Source/JavaScriptCore/runtime/CacheableIdentifier.h:87 > + explicit inline CacheableIdentifier(const Identifier&); why make this explicit but not "createFromIdentifierOwnedByCodeBlock" since that's the API of this class (since these are private ctors)