[webkit-reviews] review denied: [Bug 60197] IndexedDB result of deleting a record should be true or false : [Attachment 112285] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 24 17:31:41 PDT 2011
David Levin <levin at chromium.org> has denied Joshua Bell <jsbell at chromium.org>'s
request for review:
Bug 60197: IndexedDB result of deleting a record should be true or false
https://bugs.webkit.org/show_bug.cgi?id=60197
Attachment 112285: Patch
https://bugs.webkit.org/attachment.cgi?id=112285&action=review
------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=112285&action=review
It looks like you are calling a v8 bindings only method "trueValue" from
generic code "Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp".
> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1958
> + DEFINE_STATIC_LOCAL(RefPtr<SerializedScriptValue>, trueValue, (0));
Given that this function may be called from different threads (putting it on
SerializedScriptValue implies that somewhat since it may be used from workers).
This doesn't look like a safe way of doing this.
Does the result really need to be cached? Is there some perf test that
indicates this?
(I see that nullValue does this, but I wonder why and I'm concerned about it as
well.)
If nothing else, we should add asserts in these methods to verify that they are
only used on the main thread, but personally I'd prefer that these
non-threadsafe methods not be in SerializedScriptValue (and if caching is
needed perhaps it should happen in code that is known to be single threaded.)
> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1959
> + if (!trueValue) {
Prefer "fail fast":
if (trueValue)
return trueValue.get();
More information about the webkit-reviews
mailing list