...
Created attachment 434221 [details] Patch
Created attachment 434306 [details] Patch
Created attachment 434312 [details] Patch
Comment on attachment 434312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434312&action=review > Source/WebCore/Modules/indexeddb/IDBDatabase.h:67 > + TransactionOptions() : durability() { } What does it mean to initialize `durability()` when it is an enum class? Shouldn't we use IDBTransactionDurability::Default? > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1155 > + WTFLogAlways("sihuil: [%p]SQLiteIDBBackingStore::commitTransaction durability strict", this); OOPS.
Comment on attachment 434312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434312&action=review > Source/WebCore/Modules/indexeddb/IDBTransactionDurability.h:32 > +enum class IDBTransactionDurability { : uint8_t
Comment on attachment 434312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434312&action=review >> Source/WebCore/Modules/indexeddb/IDBDatabase.h:67 >> + TransactionOptions() : durability() { } > > What does it mean to initialize `durability()` when it is an enum class? Shouldn't we use IDBTransactionDurability::Default? It will give us the first value in the enumeration, so it will set it to IDBTransactionDurability::String; so I agree that we should either use IDBTransactionDurability::Default or reorder the enumeration values. But also, this can, and should, be done with member initialization syntax rather than a constructor. IDBTransactionDurability durability { IDBTransactionDurability::Default }; We should add test coverage, because the wrong default value should be observable.
Comment on attachment 434312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434312&action=review > Source/WebCore/platform/sql/SQLiteDatabase.h:100 > + enum class CheckpointMode : uint8_t { Passive, Full, Restart, Truncate }; These constants need to match the values expected by sqlite3_wal_checkpoint_v2. So reordering them or adding a new value would create a big problem. I can see think of two ways to handle this: 1) Could add four static_assert checks to make sure these values match values like SQLITE_CHECKPOINT_PASSIVE. Those would go into the implementation file, so would not pollute the header. 2) Could convert from our CheckpointMode to SQLITE_CHECKPOINT_* constants with a switch statement instead of by casting to int. Or maybe you can think of another solution. > Source/WebCore/platform/sql/SQLiteDatabase.h:101 > + int checkpoint(CheckpointMode); This should return void. If we wanted it to return a result, we would need to define what the return value means. Just passing through the integer from the sqlite3_wal_checkpoint_v2 call doesn’t seem reasonable from an encapsulation point of view. Better, all of this should be private. I don’t see any clients outside SQLiteDatabase.cpp. Maybe not members at all. This entire thing could be kept inside the implementation file without touching the header.
(In reply to Chris Dumez from comment #4) > Comment on attachment 434312 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434312&action=review > > > Source/WebCore/Modules/indexeddb/IDBDatabase.h:67 > > + TransactionOptions() : durability() { } > > What does it mean to initialize `durability()` when it is an enum class? > Shouldn't we use IDBTransactionDurability::Default? Yes, it should be IDBTransactionDurability::Default. > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1155 > > + WTFLogAlways("sihuil: [%p]SQLiteIDBBackingStore::commitTransaction durability strict", this); > > OOPS. OOPS, will remove.
(In reply to Darin Adler from comment #6) > Comment on attachment 434312 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434312&action=review > > >> Source/WebCore/Modules/indexeddb/IDBDatabase.h:67 > >> + TransactionOptions() : durability() { } > > > > What does it mean to initialize `durability()` when it is an enum class? Shouldn't we use IDBTransactionDurability::Default? > > It will give us the first value in the enumeration, so it will set it to > IDBTransactionDurability::String; so I agree that we should either use > IDBTransactionDurability::Default or reorder the enumeration values. But > also, this can, and should, be done with member initialization syntax rather > than a constructor. > > IDBTransactionDurability durability { IDBTransactionDurability::Default > }; > > We should add test coverage, because the wrong default value should be > observable. Will add.
(In reply to Darin Adler from comment #7) > Comment on attachment 434312 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434312&action=review > > > Source/WebCore/platform/sql/SQLiteDatabase.h:100 > > + enum class CheckpointMode : uint8_t { Passive, Full, Restart, Truncate }; > > These constants need to match the values expected by > sqlite3_wal_checkpoint_v2. So reordering them or adding a new value would > create a big problem. I can see think of two ways to handle this: > > 1) Could add four static_assert checks to make sure these values match > values like SQLITE_CHECKPOINT_PASSIVE. Those would go into the > implementation file, so would not pollute the header. > 2) Could convert from our CheckpointMode to SQLITE_CHECKPOINT_* constants > with a switch statement instead of by casting to int. > > Or maybe you can think of another solution. 2) sounds good to me. Will change. > > > Source/WebCore/platform/sql/SQLiteDatabase.h:101 > > + int checkpoint(CheckpointMode); > > This should return void. If we wanted it to return a result, we would need > to define what the return value means. Just passing through the integer from > the sqlite3_wal_checkpoint_v2 call doesn’t seem reasonable from an > encapsulation point of view. Okay, will change to void. > > Better, all of this should be private. I don’t see any clients outside > SQLiteDatabase.cpp. Maybe not members at all. This entire thing could be > kept inside the implementation file without touching the header. SQLiteIDBBackingStore uses this to force writing data back to file.
Created attachment 434455 [details] Patch
Comment on attachment 434455 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434455&action=review > Source/WebCore/platform/sql/SQLiteDatabase.h:100 > + enum class CheckpointMode : uint8_t { Passive, Full, Restart, Truncate }; Since WebKit is only using Full and Truncate, can we leave Passive and Restart out for now? We can add then if we find we need the modes.
Created attachment 434492 [details] Patch for landing
(In reply to Darin Adler from comment #12) > Comment on attachment 434455 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434455&action=review > > > Source/WebCore/platform/sql/SQLiteDatabase.h:100 > > + enum class CheckpointMode : uint8_t { Passive, Full, Restart, Truncate }; > > Since WebKit is only using Full and Truncate, can we leave Passive and > Restart out for now? We can add then if we find we need the modes. Sure, removed.
Committed r280415 (240055@main): <https://commits.webkit.org/240055@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434492 [details].
<rdar://problem/81251758>