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

Issue 1071553002: Optimizes the operation of DeleteDatabase when no backing store exists. (Closed)

Created:
5 years, 8 months ago by payal.pandey
Modified:
5 years, 5 months ago
Reviewers:
jsbell, cmumford
CC:
chromium-reviews, darin-cc_chromium.org, dgrogan, jam, jsbell+idb_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Optimizes the operation of DeleteDatabase when no backing store exists. When deleting the non existing database in a origin with no existing backing store, then in DeleteDatabse(), early exiting before OpenBackingStore, so that it wont results in multiple transaction as earlier. BUG=460236 Committed: https://crrev.com/a4c618a142515773709688bfe3c5753e589afd43 Cr-Commit-Position: refs/heads/master@{#326473}

Patch Set 1 #

Total comments: 3

Patch Set 2 : Optimizes the operation of DeleteDatabase when no backing store exists. #

Total comments: 1

Patch Set 3 : Optimizes the operation of DeleteDatabase when no backing store exists. #

Total comments: 7

Patch Set 4 : Optimizes the operation of DeleteDatabase when no backing store exists. #

Total comments: 6

Patch Set 5 : Incorporated review comments for browser tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -12 lines) Patch
M content/browser/indexed_db/indexed_db_browsertest.cc View 1 2 3 4 1 chunk +4 lines, -12 lines 0 comments Download
M content/browser/indexed_db/indexed_db_factory_impl.cc View 1 2 3 1 chunk +20 lines, -0 lines 0 comments Download

Messages

Total messages: 37 (8 generated)
payal.pandey
please have a look and review..
5 years, 8 months ago (2015-04-08 12:31:25 UTC) #2
cmumford
https://codereview.chromium.org/1071553002/diff/1/content/browser/indexed_db/indexed_db_factory_impl.cc File content/browser/indexed_db/indexed_db_factory_impl.cc (right): https://codereview.chromium.org/1071553002/diff/1/content/browser/indexed_db/indexed_db_factory_impl.cc#newcode240 content/browser/indexed_db/indexed_db_factory_impl.cc:240: IndexedDBBackingStoreMap::iterator it2 = backing_store_map_.find(origin_url); Just call IsBackingStoreOpen(origin_url) https://codereview.chromium.org/1071553002/diff/1/content/browser/indexed_db/indexed_db_factory_impl.cc#newcode241 content/browser/indexed_db/indexed_db_factory_impl.cc:241: ...
5 years, 8 months ago (2015-04-08 15:14:35 UTC) #4
cmumford
BTW I don't believe this CL satisfies the issue requirements. backing_store_map_ is only a map ...
5 years, 8 months ago (2015-04-08 16:49:25 UTC) #5
payal.pandey
5 years, 8 months ago (2015-04-10 11:44:54 UTC) #6
payal.pandey
Modified the early exit after OpenBackingStore and before Database creation in DeleteDatabase() And instead of ...
5 years, 8 months ago (2015-04-10 11:51:37 UTC) #7
cmumford
This change breaks some content browser tests (content_browsertests) and Blink layout tests so this change ...
5 years, 8 months ago (2015-04-10 14:58:15 UTC) #10
payal.pandey
Modified the CL as per review comment: > This change breaks some content browser tests ...
5 years, 8 months ago (2015-04-13 14:12:37 UTC) #11
payal.pandey
5 years, 8 months ago (2015-04-13 14:12:52 UTC) #12
jsbell
https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed_db/indexed_db_factory_impl.cc File content/browser/indexed_db/indexed_db_factory_impl.cc (right): https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed_db/indexed_db_factory_impl.cc#newcode265 content/browser/indexed_db/indexed_db_factory_impl.cc:265: "indexedDB.webkitGetDatabaseNames."); This should be "indexedDB.deleteDatabase" https://codereview.chromium.org/1071553002/diff/80001/content/browser/indexed_db/indexed_db_factory_impl.cc#newcode272 content/browser/indexed_db/indexed_db_factory_impl.cc:272: if(std::find(names.begin(), names.end(), ...
5 years, 8 months ago (2015-04-13 16:32:40 UTC) #13
payal.pandey
Incorporated review comments.
5 years, 8 months ago (2015-04-15 09:41:49 UTC) #14
payal.pandey
On 2015/04/15 09:41:49, payal.pandey wrote: > Incorporated review comments. Please review the patch..
5 years, 8 months ago (2015-04-16 05:03:40 UTC) #15
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1071553002/100001
5 years, 8 months ago (2015-04-16 15:58:05 UTC) #17
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/56837)
5 years, 8 months ago (2015-04-16 16:08:48 UTC) #19
cmumford
I don't believe this change will fix the bug, and is also harmful. There's only ...
5 years, 8 months ago (2015-04-16 17:22:48 UTC) #20
cmumford
Discussed with another dev here (jsbell), and this CL may be OK. We're going to ...
5 years, 8 months ago (2015-04-16 20:10:58 UTC) #21
payal.pandey
On 2015/04/16 20:10:58, cmumford wrote: > Discussed with another dev here (jsbell), and this CL ...
5 years, 8 months ago (2015-04-20 06:53:25 UTC) #22
payal.pandey
when can I expect for its final response
5 years, 8 months ago (2015-04-20 06:55:29 UTC) #23
cmumford
On 2015/04/20 at 06:55:29, payal.pandey wrote: > when can I expect for its final response ...
5 years, 8 months ago (2015-04-20 15:26:24 UTC) #24
jsbell
Yes, I'm sorry for the delay. The code looks correct in context, given that it ...
5 years, 8 months ago (2015-04-20 16:27:59 UTC) #25
jsbell
Yes, I'm sorry for the delay. The code looks correct in context, given that it ...
5 years, 8 months ago (2015-04-20 16:27:59 UTC) #26
payal.pandey
On 2015/04/20 16:27:59, jsbell wrote: > Yes, I'm sorry for the delay. The code looks ...
5 years, 8 months ago (2015-04-21 08:33:52 UTC) #27
payal.pandey
I wondered, if you please look in this CL, today, whether it can be closed.
5 years, 8 months ago (2015-04-22 13:21:16 UTC) #28
jsbell
Thanks for your patience! lgtm if the test comments are fixed. https://codereview.chromium.org/1071553002/diff/100001/content/browser/indexed_db/indexed_db_browsertest.cc File content/browser/indexed_db/indexed_db_browsertest.cc (right): ...
5 years, 8 months ago (2015-04-22 21:30:07 UTC) #29
jsbell
On 2015/04/21 08:33:52, payal.pandey wrote: > Sounds great.. for next optimization opportunity. > I would ...
5 years, 8 months ago (2015-04-22 21:35:59 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1071553002/120001
5 years, 8 months ago (2015-04-23 05:42:21 UTC) #33
commit-bot: I haz the power
Committed patchset #5 (id:120001)
5 years, 8 months ago (2015-04-23 07:40:39 UTC) #34
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/a4c618a142515773709688bfe3c5753e589afd43 Cr-Commit-Position: refs/heads/master@{#326473}
5 years, 8 months ago (2015-04-23 07:41:37 UTC) #35
Jeffrey Yasskin
A revert of this CL (patchset #5 id:120001) has been created in https://codereview.chromium.org/1106703003/ by jyasskin@chromium.org. ...
5 years, 8 months ago (2015-04-23 23:03:11 UTC) #36
jsbell
5 years, 5 months ago (2015-06-29 22:19:39 UTC) #37
Message was sent while issue was closed.
Attempting reland over at https://codereview.chromium.org/1216573008/

Powered by Google App Engine
This is Rietveld 408576698