| Summary: | Remove unneeded MemoryIDBBackingStore::create | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||
| Component: | WebCore Misc. | Assignee: | Darin Adler <darin> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | achristensen, alecflett, beidson, cdumez, ews-watchlist, jsbell, rbuis, sam, sihui_liu, webkit-bug-importer, youennf | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Darin Adler
2019-12-20 11:20:00 PST
Created attachment 386229 [details]
Patch
Created attachment 386230 [details]
Patch
Looks good! Probably we can move functions in SQLiteIDBBackingStore to private too Comment on attachment 386230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386230&action=review > Source/WebCore/ChangeLog:7 > + Optionally something like "Covered by existing tests" can be added. > Source/WebCore/ChangeLog:10 > + instead of MemoryIDBBackingStore::create. I am guessing there are quite a few places in WebKit where we can do the same. > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.h:45 > static std::unique_ptr<MemoryIDBBackingStore> create(PAL::SessionID, const IDBDatabaseIdentifier&); This can now be removed as well, right? Comment on attachment 386230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386230&action=review >> Source/WebCore/ChangeLog:10 >> + instead of MemoryIDBBackingStore::create. > > I am guessing there are quite a few places in WebKit where we can do the same. Some, but maybe not as many as you think. We mostly use create functions when it’s more than just a makeUnique, for example for reference counted objects, where we want to prevent the mistake of allocating on the stack. There’s no reason to use create functions in cases like this one. >> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.h:45 >> static std::unique_ptr<MemoryIDBBackingStore> create(PAL::SessionID, const IDBDatabaseIdentifier&); > > This can now be removed as well, right? Yes, oops, good catch. I should have removed that! Anyone interested in reviewing? As you can see, this is passing all the tests on EWS. I will make changes based on Rob’s comments. Comment on attachment 386230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386230&action=review > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.h:50 > IDBError getOrEstablishDatabaseInfo(IDBDatabaseInfo&) final; s/~MemoryIDBBackingStore() final;/~MemoryIDBBackingStore();/ as well now the class is final. Committed r254414: <https://trac.webkit.org/changeset/254414> |