Added IndexedDBPendingConnection to group up a bunch of parameters that get

Added IndexedDBPendingConnection to group up a bunch of parameters that get passed around together.

This includes a convenience method CreateConnection in IndexedDBDatabase.cc that's not strictly needed for this change, but makes the merge of other related CLs easier later.

BUG=108012
R=cmumford,jsbell

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257568

[ericu, 2014-03-12 22:23:22]

Patch #3, which had no external dependencies.

[cmumford, 2014-03-12 22:50:23]

On 2014/03/12 22:23:22, ericu wrote:
> Patch #3, which had no external dependencies.

lgtm. Only comment is that I find the name "IndexedDBPendingConnection" to be
slightly odd in that it isn't an actual connection, but the name sort of
suggests that it is. Not sure I can suggest a better name. Maybe
IndexedDBConnectionOpenParams - but I don't like that any better.

[jsbell, 2014-03-12 23:40:48]

lgtm as well with optional suggestions.

You'll need to R or TBR someone else for the gypi change, (I often TBR=jam).

I'm fine with the IndexedDBPendingConnection name, since it at least holds
scoped_refptrs to things and thus contributes to lifetime management.

https://codereview.chromium.org/198223002/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/198223002/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:193: /* TODO(ericu):
Can you remove the commented-out code and make the TODO describe what needs to
be done here (and why), instead?

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:194:
backing_store_->GrantChildProcessPermissions(child_process_id);
Remove the commented-out code, and make the TODO describe what needs to be done
(and why) ?

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.h:275: // that of the page group,
but it's better to be conservative than sorry.
"page group" doesn't even make sense here (that's a WebKit-ism).  Remove the
comment?

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database_unittest.cc:65: 0 /* fake
child_process_id */,
For consistency, make this a const int?

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_pending_connection.cc (right):

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_pending_connection.cc:20:
child_process_id(child_process_id_in),
Dunno if it's worth it, but maybe DCHECK that child_process_id is 0 for now, and
later DCHECK that it's NOT 0 so we know we're passing it around correctly
everywhere? (Tests could be -1.) - Again, dunno if it's worth it.

[ericu, 2014-03-12 23:56:36]

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:194:
backing_store_->GrantChildProcessPermissions(child_process_id);
On 2014/03/12 23:40:49, jsbell wrote:
> Remove the commented-out code, and make the TODO describe what needs to be
done
> (and why) ?

Done.

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.h:275: // that of the page group,
but it's better to be conservative than sorry.
On 2014/03/12 23:40:49, jsbell wrote:
> "page group" doesn't even make sense here (that's a WebKit-ism).  Remove the
> comment?

Well, is this necessary or not?  Should this turn into a TODO to change it to a
raw pointer, or should we just leave it as-is?

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.h:275: // that of the page group,
but it's better to be conservative than sorry.
On 2014/03/12 23:40:49, jsbell wrote:
> "page group" doesn't even make sense here (that's a WebKit-ism).  Remove the
> comment?

Done.

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database_unittest.cc:65: 0 /* fake
child_process_id */,
On 2014/03/12 23:40:49, jsbell wrote:
> For consistency, make this a const int?

Done.

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_pending_connection.cc (right):

https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_pending_connection.cc:20:
child_process_id(child_process_id_in),
On 2014/03/12 23:40:49, jsbell wrote:
> Dunno if it's worth it, but maybe DCHECK that child_process_id is 0 for now,
and
> later DCHECK that it's NOT 0 so we know we're passing it around correctly
> everywhere? (Tests could be -1.) - Again, dunno if it's worth it.

I'm not sure a 0-check really buys us anything.
The later DCHECK makes sense to me though.

[ericu, 2014-03-12 23:58:34]

On Wed, Mar 12, 2014 at 4:56 PM,  <ericu@chromium.org> wrote:
> Remove the
>>
>> comment?
>
>
> Well, is this necessary or not?  Should this turn into a TODO to change
> it to a raw pointer, or should we just leave it as-is?

Ignore this; I deleted it, but Reitveld's having timing issues today.

>
https://codereview.chromium.org/198223002/diff/60001/content/browser/indexed_...
> content/browser/indexed_db/indexed_db_database.h:275: // that of the
> page group, but it's better to be conservative than sorry.
> On 2014/03/12 23:40:49, jsbell wrote:
>>
>> "page group" doesn't even make sense here (that's a WebKit-ism).
>
> Remove the
>>
>> comment?
>
>
> Done.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ericu, 2014-03-13 21:18:24]

On Wed, Mar 12, 2014 at 4:40 PM,  <jsbell@chromium.org> wrote:
> You'll need to R or TBR someone else for the gypi change, (I often TBR=jam).

Actually, content/OWNERS has a wildcard for the gypi for file
additions and name changes.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ericu, 2014-03-14 00:35:02]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-03-14 00:35:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/198223002/160001

[commit-bot: I haz the power, 2014-03-14 00:57:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 00:57:41]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[ericu, 2014-03-17 18:25:39]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-03-17 18:27:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/198223002/20002

[commit-bot: I haz the power, 2014-03-17 18:27:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-17 18:27:21]

Failed to apply patch for
content/browser/indexed_db/indexed_db_backing_store.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/indexed_db/indexed_db_backing_store.cc
  Hunk #1 FAILED at 834.
  1 out of 1 hunk FAILED -- saving rejects to file
content/browser/indexed_db/indexed_db_backing_store.cc.rej

Patch:       content/browser/indexed_db/indexed_db_backing_store.cc
Index: content/browser/indexed_db/indexed_db_backing_store.cc
diff --git a/content/browser/indexed_db/indexed_db_backing_store.cc
b/content/browser/indexed_db/indexed_db_backing_store.cc
index
be909b4f0b1cc63883835744faddd4ae1053ed10..e26901bc5f3ab8a088478347f924b4e74902d433
100644
--- a/content/browser/indexed_db/indexed_db_backing_store.cc
+++ b/content/browser/indexed_db/indexed_db_backing_store.cc
@@ -834,8 +834,8 @@ static void DeleteRange(LevelDBTransaction* transaction,
 leveldb::Status IndexedDBBackingStore::DeleteDatabase(
     const base::string16& name) {
   IDB_TRACE("IndexedDBBackingStore::DeleteDatabase");
-  scoped_ptr<LevelDBWriteOnlyTransaction> transaction =
-      LevelDBWriteOnlyTransaction::Create(db_.get());
+  scoped_ptr<LevelDBUncachedTransaction> transaction =
+      LevelDBUncachedTransaction::Create(db_.get());
 
   IndexedDBDatabaseMetadata metadata;
   bool success = false;

[ericu, 2014-03-17 19:29:32]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-03-17 19:29:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/198223002/190001

[commit-bot: I haz the power, 2014-03-17 21:33:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-17 21:33:06]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[ericu, 2014-03-17 21:35:32]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-03-17 21:36:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/198223002/190001

[commit-bot: I haz the power, 2014-03-18 01:21:09]

Change committed as 257568