[webkit-reviews] review denied: [Bug 192270] Let JSDOMGlobalObject in uniqueIDBDatabase be deleted if no database is using it : [Attachment 356268] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 192270: Let JSDOMGlobalObject in uniqueIDBDatabase be deleted if no
database is using it
https://bugs.webkit.org/show_bug.cgi?id=192270

Attachment 356268: Patch

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




--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 356268
  --> https://bugs.webkit.org/attachment.cgi?id=356268
Patch

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

layout test failure looks likely related.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:90
> +    , m_serializationContext(nullptr)

Not needed, the RefPtr<> default constructor does the right thing.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:967
> +void
UniqueIDBDatabase::sharedIDBSerializationContext(RefPtr<IDBSerializationContext
>& context)

Why isn't this returning the context instead of using an out-parameter? This
looks confusing.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:974
> +    if (!s_context.get().get())

Doesn't "if (!s_context)" work? At the very least "if (!s_context.get())"
should.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1032
> +	   sharedIDBSerializationContext(m_serializationContext);

This looks super confusing.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h:80
> +    IDBSerializationContext();

Constructor should be private and this class should have a create() factory
static function. This is the convention for RefCounted classes. Also, I'd swap
the base classes order.