-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make SQLiteTransaction notify SQLiteDatabaseTracker on macOS #15700
Make SQLiteTransaction notify SQLiteDatabaseTracker on macOS #15700
Conversation
EWS run on previous version of this PR (hash 8dc15e1) |
I did not change the |
8dc15e1
to
63729d5
Compare
EWS run on current version of this PR (hash 63729d5) |
@@ -27,10 +27,7 @@ | |||
#include "SQLiteTransaction.h" | |||
|
|||
#include "SQLiteDatabase.h" | |||
|
|||
#if PLATFORM(IOS_FAMILY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be #if PLATFORM(COCOA)
? Or does this stuff exist/work on GTK/Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the tracker code, it seems cross-platform, so this seems right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQLiteDatabaseTracker builds fine on all platforms but is a no-op on some platforms. It doesn't hurt to run this code everywhere and dropping the #if
checks makes the code nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -27,10 +27,7 @@ | |||
#include "SQLiteTransaction.h" | |||
|
|||
#include "SQLiteDatabase.h" | |||
|
|||
#if PLATFORM(IOS_FAMILY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the tracker code, it seems cross-platform, so this seems right.
https://bugs.webkit.org/show_bug.cgi?id=259062 rdar://112027513 Reviewed by Chris Dumez and Brent Fulgham. In 265791@main we enabled SQLiteDatabaseTracker on macOS, but it still does nothing because SQLiteTransaction doesn't notify it. Change SQLiteTransaction to notify the tracker about ongoing transactions on macOS as well. * Source/WebCore/platform/sql/SQLiteTransaction.cpp: (WebCore::SQLiteTransaction::begin): (WebCore::SQLiteTransaction::commit): (WebCore::SQLiteTransaction::rollback): (WebCore::SQLiteTransaction::stop): Canonical link: https://commits.webkit.org/265918@main
63729d5
to
722cf99
Compare
Committed 265918@main (722cf99): https://commits.webkit.org/265918@main Reviewed commits have been landed. Closing PR #15700 and removing active labels. |
722cf99
63729d5