Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(208)

Issue 12096073: Implement sql::Connection::RazeAndClose(). (Closed)

Created:
7 years, 10 months ago by Scott Hess - ex-Googler
Modified:
7 years, 10 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, erikwright (departed), browser-components-watch_chromium.org, Aaron Boodman, darin-cc_chromium.org, chromium-apps-reviews_chromium.org
Visibility:
Public.

Description

Implement sql::Connection::RazeAndClose(). Raze() clears out the database, but cannot be called within a transaction. Close() can only be called once all statements have cleared. Error callbacks happen while executing statements (thus often in a transaction, and at least one statement is outstanding). RazeAndClose() forcibly breaks outstanding transactions, calls Raze() to clear the database, then calls Close() to close the handle. All future operations against the database should fail safely (without affecting storage and without crashing). BUG=166419, 136846 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181152

Patch Set 1 #

Total comments: 12

Patch Set 2 : Execute() and untracked changes, unit testsing. #

Total comments: 2

Patch Set 3 : Fix Open() to warn after RazeOnClose() w/o Close(). #

Patch Set 4 : Disable DEATH test on android and ios. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+261 lines, -76 lines) Patch
M chrome/browser/extensions/activity_database.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/extensions/activity_database.cc View 1 1 chunk +1 line, -3 lines 0 comments Download
M chrome/browser/history/thumbnail_database.cc View 1 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/net/sqlite_persistent_cookie_store.cc View 1 chunk +1 line, -2 lines 0 comments Download
M sql/connection.h View 1 2 8 chunks +39 lines, -10 lines 0 comments Download
M sql/connection.cc View 1 2 18 chunks +103 lines, -50 lines 0 comments Download
M sql/connection_unittest.cc View 1 2 3 3 chunks +106 lines, -4 lines 0 comments Download
M sql/statement.cc View 1 3 chunks +7 lines, -5 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
Scott Hess - ex-Googler
I've been allowing perfect to be the enemy of good (imagine me circling round and ...
7 years, 10 months ago (2013-01-31 00:26:49 UTC) #1
pkotwicz
LGTM with nits and comments The comments are the type of things that I would ...
7 years, 10 months ago (2013-01-31 19:22:00 UTC) #2
erikwright (departed)
Cookie side LGTM. Let me know if you don't have enough coverage of the sql/ ...
7 years, 10 months ago (2013-01-31 19:24:32 UTC) #3
Scott Hess - ex-Googler
Apologies for the delay, the Execute*() issue required stepping back and rethinking a little to ...
7 years, 10 months ago (2013-02-05 01:20:25 UTC) #4
Scott Hess - ex-Googler
Apologies for having re-baselined before uploading. The delta-1 changes outside of sql/ aren't from my ...
7 years, 10 months ago (2013-02-05 01:31:55 UTC) #5
pkotwicz
LGTM https://codereview.chromium.org/12096073/diff/6001/sql/connection_unittest.cc File sql/connection_unittest.cc (right): https://codereview.chromium.org/12096073/diff/6001/sql/connection_unittest.cc#newcode293 sql/connection_unittest.cc:293: ASSERT_FALSE(db().is_open()); Nit: Should you call db().Close() here for ...
7 years, 10 months ago (2013-02-06 17:58:57 UTC) #6
Scott Hess - ex-Googler
https://codereview.chromium.org/12096073/diff/6001/sql/connection_unittest.cc File sql/connection_unittest.cc (right): https://codereview.chromium.org/12096073/diff/6001/sql/connection_unittest.cc#newcode293 sql/connection_unittest.cc:293: ASSERT_FALSE(db().is_open()); On 2013/02/06 17:58:57, pkotwicz wrote: > Nit: Should ...
7 years, 10 months ago (2013-02-06 19:19:29 UTC) #7
Scott Hess - ex-Googler
Brett, OWNERS review for chrome/history/ and extensions/ . This all is an attempt to pull ...
7 years, 10 months ago (2013-02-06 19:20:34 UTC) #8
brettw
lgtm rubberstamp based on shess' review.
7 years, 10 months ago (2013-02-06 20:25:49 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shess@chromium.org/12096073/10004
7 years, 10 months ago (2013-02-06 20:36:21 UTC) #10
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 10 months ago (2013-02-06 22:22:49 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shess@chromium.org/12096073/20001
7 years, 10 months ago (2013-02-06 22:26:54 UTC) #12
commit-bot: I haz the power
7 years, 10 months ago (2013-02-07 02:35:51 UTC) #13
Message was sent while issue was closed.
Change committed as 181152

Powered by Google App Engine
This is Rietveld 408576698