...
Created attachment 429793 [details] Patch
Comment on attachment 429793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429793&action=review r=me with nits > Source/WebCore/platform/sql/SQLiteDatabase.cpp:238 > + RELEASE_LOG_ERROR(SQLDatabase, "SQLiteDatabase::close: Failed to close database (%d) - %s", lastError(), lastErrorMsg()); Do you want a LOG_ERROR() too? I don't think you'll see the RELEASE_LOG_ERROR() on the bots' output, unless you get a sysdiagnose. Also, you want to use %{public}s (assuming not privacy sensitive, %{private}s otherwise) on the string so the actually gets printed out in a RELEASE_LOG. > Source/WebCore/platform/sql/SQLiteDatabase.h:62 > + enum class ShouldSetErrorState { No, Yes }; enum class ShouldSetErrorState : bool { No, Yes };
Created attachment 429842 [details] Patch for landing
Comment on attachment 429793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429793&action=review >> Source/WebCore/platform/sql/SQLiteDatabase.cpp:238 >> + RELEASE_LOG_ERROR(SQLDatabase, "SQLiteDatabase::close: Failed to close database (%d) - %s", lastError(), lastErrorMsg()); > > Do you want a LOG_ERROR() too? I don't think you'll see the RELEASE_LOG_ERROR() on the bots' output, unless you get a sysdiagnose. > > Also, you want to use %{public}s (assuming not privacy sensitive, %{private}s otherwise) on the string so the actually gets printed out in a RELEASE_LOG. The test I am investigating is an API test failed on release bot, so this is enough for me. Will add %{public}s. >> Source/WebCore/platform/sql/SQLiteDatabase.h:62 >> + enum class ShouldSetErrorState { No, Yes }; > > enum class ShouldSetErrorState : bool { No, Yes }; Okay
(In reply to Sihui Liu from comment #4) > Comment on attachment 429793 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=429793&action=review > > >> Source/WebCore/platform/sql/SQLiteDatabase.cpp:238 > >> + RELEASE_LOG_ERROR(SQLDatabase, "SQLiteDatabase::close: Failed to close database (%d) - %s", lastError(), lastErrorMsg()); > > > > Do you want a LOG_ERROR() too? I don't think you'll see the RELEASE_LOG_ERROR() on the bots' output, unless you get a sysdiagnose. > > > > Also, you want to use %{public}s (assuming not privacy sensitive, %{private}s otherwise) on the string so the actually gets printed out in a RELEASE_LOG. > > The test I am investigating is an API test failed on release bot, so this is > enough for me. > Will add %{public}s. Not sure what you mean by enough. Release logging doesn’t show in api tests output. Only way would be to get a sysdiagnose from the bots. > > >> Source/WebCore/platform/sql/SQLiteDatabase.h:62 > >> + enum class ShouldSetErrorState { No, Yes }; > > > > enum class ShouldSetErrorState : bool { No, Yes }; > > Okay
(In reply to Chris Dumez from comment #5) > (In reply to Sihui Liu from comment #4) > > Comment on attachment 429793 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=429793&action=review > > > > >> Source/WebCore/platform/sql/SQLiteDatabase.cpp:238 > > >> + RELEASE_LOG_ERROR(SQLDatabase, "SQLiteDatabase::close: Failed to close database (%d) - %s", lastError(), lastErrorMsg()); > > > > > > Do you want a LOG_ERROR() too? I don't think you'll see the RELEASE_LOG_ERROR() on the bots' output, unless you get a sysdiagnose. > > > > > > Also, you want to use %{public}s (assuming not privacy sensitive, %{private}s otherwise) on the string so the actually gets printed out in a RELEASE_LOG. > > > > The test I am investigating is an API test failed on release bot, so this is > > enough for me. > > Will add %{public}s. > > Not sure what you mean by enough. Release logging doesn’t show in api tests > output. Only way would be to get a sysdiagnose from the bots. Maybe you want a temporary WTFLogAlways() since this is a release bot? > > > > >> Source/WebCore/platform/sql/SQLiteDatabase.h:62 > > >> + enum class ShouldSetErrorState { No, Yes }; > > > > > > enum class ShouldSetErrorState : bool { No, Yes }; > > > > Okay
Committed r278150 (238194@main): <https://commits.webkit.org/238194@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 429842 [details].
<rdar://problem/78551181>