Created attachment 267998 [details] Patch v1

Comment on attachment 267998 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=267998&action=review > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382 > + serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier()); You should use downcast here instead of static_cast. > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.h:92 > + virtual bool dispatchEvent(Event&) override final; No need to specify virtual here. > Source/WebCore/Modules/indexeddb/client/IDBVersionChangeEventImpl.h:51 > + virtual bool isVersionChangeEvent() const override final { return true; } Ditto. > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:350 > + if (!databaseConnection) > + return; > + > + databaseConnection->didFireVersionChangeEvent(requestIdentifier); It'd be more concise to write: if (auto databaseConnection = m_databaseConnections.get(databaseConnectionIdentifier)) databaseConnection->didFireVersionChangeEvent(requestIdentifier); > Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:377 > + RefPtr<InProcessIDBServer> self(this); > + RunLoop::current().dispatch([this, self, databaseConnectionIdentifier, requestIdentifier] { > + m_server->didFireVersionChangeEvent(databaseConnectionIdentifier, requestIdentifier); > + }); Could you use self->m_server so you don't have to capture this? > Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.h:75 > + virtual void didFireVersionChangeEvent(uint64_t databaseConnectionIdentifier, const IDBResourceIdentifier& requestIdentifier) override final; No need for virtual here, or for any other function in this file that's already marked override. > Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.h:94 > - virtual void fireVersionChangeEvent(IDBServer::UniqueIDBDatabaseConnection&, uint64_t requestedVersion) override final; > + virtual void fireVersionChangeEvent(IDBServer::UniqueIDBDatabaseConnection&, const IDBResourceIdentifier& requestIdentifier, uint64_t requestedVersion) override final; Ditto.

I'll do *some* of the "virtual not needed"s, but not all (where the rest of the file is already littered with them. (In reply to comment #2) > Comment on attachment 267998 [details] > Patch v1 > > > Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:377 > > + RefPtr<InProcessIDBServer> self(this); > > + RunLoop::current().dispatch([this, self, databaseConnectionIdentifier, requestIdentifier] { > > + m_server->didFireVersionChangeEvent(databaseConnectionIdentifier, requestIdentifier); > > + }); > > Could you use self->m_server so you don't have to capture this? Nope, need to capture this to have access to private variables.

Forgot to reply to this one: (In reply to comment #2) > Comment on attachment 267998 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267998&action=review > > > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382 > > + serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier()); > > You should use downcast here instead of static_cast. It's an upcast, not a downcast. =/

(In reply to comment #4) > Forgot to reply to this one: > > (In reply to comment #2) > > Comment on attachment 267998 [details] > > Patch v1 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=267998&action=review > > > > > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382 > > > + serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier()); > > > > You should use downcast here instead of static_cast. > > It's an upcast, not a downcast. =/ IDBVersionChangeEvent is a subclass of Event, right? This is what downcast is for.

Comment on attachment 267998 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=267998&action=review >>>> Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382 >>>> + serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier()); >>> >>> You should use downcast here instead of static_cast. >> >> It's an upcast, not a downcast. =/ > > IDBVersionChangeEvent is a subclass of Event, right? This is what downcast is for. Arguably this isn't necessary since you're already calling isVersionChangeEvent(), but it wouldn't hurt to have downcast's assertions.

(In reply to comment #6) > (In reply to comment #4) > > Forgot to reply to this one: > > > > (In reply to comment #2) > > > Comment on attachment 267998 [details] > > > Patch v1 > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=267998&action=review > > > > > > > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382 > > > > + serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier()); > > > > > > You should use downcast here instead of static_cast. > > > > It's an upcast, not a downcast. =/ > > IDBVersionChangeEvent is a subclass of Event, right? This is what downcast > is for. It's not a direct subclass, but it is a descendent class. I guess I misunderstand the direction. Regardless, I changed it to downcast<> and it wouldn't compile. Maybe I just don't understand how downcast<> is supposed to be used.

(In reply to comment #8) > (In reply to comment #6) > > (In reply to comment #4) > > > Forgot to reply to this one: > > > > > > (In reply to comment #2) > > > > Comment on attachment 267998 [details] > > > > Patch v1 > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=267998&action=review > > > > > > > > > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382 > > > > > + serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier()); > > > > > > > > You should use downcast here instead of static_cast. > > > > > > It's an upcast, not a downcast. =/ > > > > IDBVersionChangeEvent is a subclass of Event, right? This is what downcast > > is for. > > It's not a direct subclass, but it is a descendent class. > > I guess I misunderstand the direction. > > Regardless, I changed it to downcast<> and it wouldn't compile. Maybe I just > don't understand how downcast<> is supposed to be used. The build failure I got was caused by: static_assert(std::is_void<ExpectedType>::value, "Missing TypeCastTraits specialization"); in TypeCasts.h

And yes, the comment above that assert does say exactly how to fix it.

Created attachment 268045 [details] Downcast followup

(In reply to comment #11) > Created attachment 268045 [details] > Downcast followup 👍

Neither CQ nor EWS seem willing to chew on this patch... will land manually.

Followup landed in https://trac.webkit.org/changeset/194468

*** Bug 71130 has been marked as a duplicate of this bug. ***