Bug 228289 – Implement IDBTransaction.durability

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Sihui Liu]

...

[Sihui Liu]

Created attachment 434221 [details]
Patch

[Sihui Liu]

Created attachment 434306 [details]
Patch

[Sihui Liu]

Created attachment 434312 [details]
Patch

[Chris Dumez]

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.

[Alex Christensen]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Sihui Liu]

(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.

[Sihui Liu]

(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.

[Sihui Liu]

(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.

[Sihui Liu]

Created attachment 434455 [details]
Patch

[Darin Adler]

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.

[Sihui Liu]

Created attachment 434492 [details]
Patch for landing

[Sihui Liu]

(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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/81251758>