Port messages sent by WebIDBDatabaseImpl to Mojo.

Port messages sent by WebIDBDatabaseImpl to Mojo.

This is part two of the series of patches that converts the IPC messages
sent by WebIDBFactoryImpl, WebIDBDatabaseImpl and WebIDBCursorImpl to
Mojo messages.

This is a more mechanical change because the conversion of messages sent
by WebIDBFactoryImpl added most of the necessary infrastructure.

BUG=627484
Committed: https://crrev.com/963ec6df64089bef6867ccdc608101d5fc06d5c0
Cr-Commit-Position: refs/heads/master@{#430110}

[Reilly Grant (use Gerrit), 2016-10-26 19:33:31]

reillyg@chromium.org changed reviewers:
+ cmumford@chromium.org, dcheng@chromium.org, rockot@chromium.org

[Reilly Grant (use Gerrit), 2016-10-26 19:33:32]

cmumford@ for IndexedDB review.
dcheng@ for IPC security review.
rockot@ for //mojo/public/cpp/bindings/lib/handle_interface_serialization.h

[Reilly Grant (use Gerrit), 2016-10-26 19:33:35]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 19:34:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 19:48:55]

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

[commit-bot: I haz the power, 2016-10-26 19:48:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Reilly Grant (use Gerrit), 2016-10-26 23:47:37]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 23:48:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 00:36:17]

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

[commit-bot: I haz the power, 2016-10-27 00:36:18]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ken Rockot(use gerrit already), 2016-10-27 04:05:46]

On 2016/10/26 at 19:33:32, reillyg wrote:
> rockot@ for //mojo/public/cpp/bindings/lib/handle_interface_serialization.h

Said file LGTM

[Reilly Grant (use Gerrit), 2016-10-27 21:25:37]

jam@ because he's likely curious about this patch and because I need a //content
owner for browser/BUILD.gn and test/BUILD.gn.

[Reilly Grant (use Gerrit), 2016-10-27 21:25:55]

reillyg@chromium.org changed reviewers:
+ jam@chromium.org

[Reilly Grant (use Gerrit), 2016-10-27 21:34:11]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 21:35:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 21:40:38]

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

[commit-bot: I haz the power, 2016-10-27 21:40:38]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Reilly Grant (use Gerrit), 2016-10-27 21:53:38]

Rebased.

[Reilly Grant (use Gerrit), 2016-10-27 21:53:41]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 21:54:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-10-27 22:45:34]

On 2016/10/27 21:25:37, Reilly Grant wrote:
> jam@ because he's likely curious about this patch and because I need a
//content
> owner for browser/BUILD.gn and test/BUILD.gn.

Great. I didn't review except for the files you mentioned.
lgtm

[commit-bot: I haz the power, 2016-10-27 23:01:33]

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

[cmumford, 2016-10-28 18:40:54]

Looks fine, but I did have two times where the Blink Layout test failed:

Regressions: Unexpected text-only failures (1)
  storage/indexeddb/idbdatabase-createObjectStore-exception-order.html [ Failure
]

This is a testharness.js-based test.
FAIL IDBDatabase.createObjectStore exception order: InvalidStateError vs.
TransactionInactiveError assert_throws: "running an upgrade transaction" check
(InvalidStateError) should precede "not active" check (TransactionInactiveError)
function "() => { db.createObjectStore('s2'); }" threw object
"TransactionInactiveError: Failed to execute 'createObjectStore' on
'IDBDatabase': The transaction has finished." that is not a DOMException
InvalidStateError: property "code" is equal to 0, expected 11
PASS IDBDatabase.createObjectStore exception order: TransactionInactiveError vs.
SyntaxError 
PASS IDBDatabase.createObjectStore exception order: SyntaxError vs.
ConstraintError 
PASS IDBDatabase.createObjectStore exception order: ConstraintError vs.
InvalidAccessError 
Harness: the test ran to completion.


It's intermittent, and I cannot get master to fail, but can get this patch to
fail about 20% of the time.

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/database_impl.h (right):

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/database_impl.h:122:
scoped_refptr<base::SingleThreadTaskRunner> idb_runner_;
DISALLOW_COPY_AND_ASSIGN(DatabaseImpl);

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_callbacks.cc:64:
::indexed_db::mojom::ReturnValuePtr ConvertReturnValue(
Do we eventually want to use Mojo's type mapping in lieu of the conversion
functions?

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:399:
context()->TransactionComplete(
Nested statement is now multi-line so wrap in braces.

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.h:77: int ipc_process_id()
{ return ipc_process_id_; }
const

[jsbell, 2016-10-28 18:51:50]

On 2016/10/28 18:40:54, cmumford wrote:
> Looks fine, but I did have two times where the Blink Layout test failed:
> 
> Regressions: Unexpected text-only failures (1)
>   storage/indexeddb/idbdatabase-createObjectStore-exception-order.html [
Failure
> ]
> 
> It's intermittent, and I cannot get master to fail, but can get this patch to
> fail about 20% of the time.
> 

That's https://bugs.chromium.org/p/chromium/issues/detail?id=657968 which is
unrelated. I'll go mark those tests flaky in TestExpectations.

[jsbell, 2016-10-28 19:02:27]

On 2016/10/28 18:51:50, jsbell wrote:
> On 2016/10/28 18:40:54, cmumford wrote:
> > Looks fine, but I did have two times where the Blink Layout test failed:
> > 
> > Regressions: Unexpected text-only failures (1)
> >   storage/indexeddb/idbdatabase-createObjectStore-exception-order.html [
> Failure
> > ]
> > 
> > It's intermittent, and I cannot get master to fail, but can get this patch
to
> > fail about 20% of the time.
> > 
> 
> That's https://bugs.chromium.org/p/chromium/issues/detail?id=657968 which is
> unrelated. I'll go mark those tests flaky in TestExpectations.

FYI, the flakiness due is a race between an asynchronous operation in the test
and an IPC message. It's harmless: there are two error checks in a row, and
depending on the race the first may fail or not, but the second definitely will.
Since it's timing related the switch to Mojo probably explains why cmumford sees
it more w/ the patch.

[jsbell, 2016-10-28 19:04:33]

Also, the win_chromium_compile_dbg_ng build failure looks legit:

e:\b\c\b\win\src\content\child\indexed_db\webidbdatabase_impl.h(31): warning
C4275: non dll-interface class 'blink::WebIDBDatabase' used as base for
dll-interface class 'content::WebIDBDatabaseImpl'
e:\b\c\b\win\src\third_party\webkit\public\platform\modules\indexeddb\webidbdatabase.h(46):
note: see declaration of 'blink::WebIDBDatabase'
e:\b\c\b\win\src\content\child\indexed_db\webidbdatabase_impl.h(31): note: see
declaration of 'content::WebIDBDatabaseImpl'

[Reilly Grant (use Gerrit), 2016-11-01 01:26:45]

Address cmumford@ and jsbell@'s comments.

[Reilly Grant (use Gerrit), 2016-11-01 01:26:55]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 01:27:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 02:57:40]

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

[commit-bot: I haz the power, 2016-11-01 02:57:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[cmumford, 2016-11-02 14:31:14]

lgtm

[dcheng, 2016-11-03 04:27:05]

Sorry for the slow review =(

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
File content/browser/indexed_db/database_impl.cc (right):

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/database_impl.cc:416: if
(!connection_->IsConnected())
I don't know if it's easily possible, but it would be kind of nice if these
calls were just autocancelled with a weak pointer of some sort (rather than
forcing every single method to do this check)

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/database_impl.cc:566: IndexedDBBlobInfo(info->uuid,
path, info->file_name, info->mime_type);
Is UUID actually non-empty in this case?

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/database_impl.cc:573: blob_info[i] =
IndexedDBBlobInfo(info->uuid, info->mime_type, info->size);
Also, is it important to validate that this UUID is valid?

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/database_impl.cc:739:
dispatcher_host_->context()->TaskRunner(), GURL(origin_.Serialize()),
origin_.GetURL()

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:73:
std::swap(mojo_value->value->bits, value->bits);
Aside: I think it's slightly surprising to destructively take state from the
original object on a conversion function, though I understand why we do this.

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:414: blob_info =
value->blob_info;
This is pretty complicated and a bit hard to follow. Is there any hope of
simplifying this in the future?

Also, from what I can tell from tracing code, this conversion can assume that
all the invariants hold, right? Since we've already validated (hopefully?) the
data by running it through pipeline here?

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:772: if
(!shareable_file.get()) {
Nit: no .get()

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:166: int64_t
value_length) {
Let's make the types match: from what I can tell, the size map holds uint64_t's.

https://codereview.chromium.org/2449953008/diff/60001/content/child/indexed_d...
File content/child/indexed_db/webidbdatabase_impl.cc (right):

https://codereview.chromium.org/2449953008/diff/60001/content/child/indexed_d...
content/child/indexed_db/webidbdatabase_impl.cc:335: .c_str())));
c_str() shouldn't be needed.

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
File content/common/indexed_db/indexed_db.mojom (right):

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:69: bool is_file;
Perhaps this could have a union to separate the blob and file specific stuff.

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:73: mojo.common.mojom.String16
file_path;
Does mojo.mojom.FilePath work?

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:75: double last_modified;
Is it possible to use mojo.common.mojom.Time for this?

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
File content/common/indexed_db/indexed_db_struct_traits.h (right):

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
content/common/indexed_db/indexed_db_struct_traits.h:26: return
data.ReadIndexKeys(&out->second);
Nit: out-of-line

[Reilly Grant (use Gerrit), 2016-11-03 21:32:22]

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/database_impl.h (right):

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/database_impl.h:122:
scoped_refptr<base::SingleThreadTaskRunner> idb_runner_;
On 2016/10/28 at 18:40:53, cmumford wrote:
> DISALLOW_COPY_AND_ASSIGN(DatabaseImpl);

Done.

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_callbacks.cc:64:
::indexed_db::mojom::ReturnValuePtr ConvertReturnValue(
On 2016/10/28 at 18:40:53, cmumford wrote:
> Do we eventually want to use Mojo's type mapping in lieu of the conversion
functions?

We can't in this particular case because part of the indexed_db::mojom::Value
sent is computed on the IO thread after the blobs are prepared.

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:399:
context()->TransactionComplete(
On 2016/10/28 at 18:40:53, cmumford wrote:
> Nested statement is now multi-line so wrap in braces.

Done.

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/2449953008/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.h:77: int ipc_process_id()
{ return ipc_process_id_; }
On 2016/10/28 at 18:40:53, cmumford wrote:
> const

Done.

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
File content/browser/indexed_db/database_impl.cc (right):

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/database_impl.cc:416: if
(!connection_->IsConnected())
On 2016/11/03 at 04:27:04, dcheng wrote:
> I don't know if it's easily possible, but it would be kind of nice if these
calls were just autocancelled with a weak pointer of some sort (rather than
forcing every single method to do this check)

Once the Database pipe is no longer associated and can be bound to the IDB
thread then I will make IndexedDBConnection implement the Database interface
directly and we will get this behavior for free. Until then I don't want to make
too many changes to the internals of IndexedDBConnection because it just makes
these patches bigger.

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/database_impl.cc:566: IndexedDBBlobInfo(info->uuid,
path, info->file_name, info->mime_type);
On 2016/11/03 at 04:27:04, dcheng wrote:
> Is UUID actually non-empty in this case?

UUID gets populated whether or not the blob is a file.

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/database_impl.cc:573: blob_info[i] =
IndexedDBBlobInfo(info->uuid, info->mime_type, info->size);
On 2016/11/03 at 04:27:04, dcheng wrote:
> Also, is it important to validate that this UUID is valid?

The UUID is looked up in DatabaseImpl::Put when we call GetBlobDataFromUUID,
which returns nullptr if it wasn't found. It would assume that the existing code
would be checking that the handles are all valid but I'm not seeing that logic
anywhere. I'll look into treating a failure in GetBlobDataFromUUID has a bad
message.

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/database_impl.cc:739:
dispatcher_host_->context()->TaskRunner(), GURL(origin_.Serialize()),
On 2016/11/03 at 04:27:04, dcheng wrote:
> origin_.GetURL()

Done.

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:73:
std::swap(mojo_value->value->bits, value->bits);
On 2016/11/03 at 04:27:04, dcheng wrote:
> Aside: I think it's slightly surprising to destructively take state from the
original object on a conversion function, though I understand why we do this.

It surprised me too. I've added a comment.

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:414: blob_info =
value->blob_info;
On 2016/11/03 at 04:27:04, dcheng wrote:
> This is pretty complicated and a bit hard to follow. Is there any hope of
simplifying this in the future?
> 
> Also, from what I can tell from tracing code, this conversion can assume that
all the invariants hold, right? Since we've already validated (hopefully?) the
data by running it through pipeline here?

I am hoping that once blobs are using Mojo this will get simpler.

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:772: if
(!shareable_file.get()) {
On 2016/11/03 at 04:27:04, dcheng wrote:
> Nit: no .get()

Done.

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:166: int64_t
value_length) {
On 2016/11/03 at 04:27:04, dcheng wrote:
> Let's make the types match: from what I can tell, the size map holds
uint64_t's.

Done.

https://codereview.chromium.org/2449953008/diff/60001/content/child/indexed_d...
File content/child/indexed_db/webidbdatabase_impl.cc (right):

https://codereview.chromium.org/2449953008/diff/60001/content/child/indexed_d...
content/child/indexed_db/webidbdatabase_impl.cc:335: .c_str())));
On 2016/11/03 at 04:27:04, dcheng wrote:
> c_str() shouldn't be needed.

Done.

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
File content/common/indexed_db/indexed_db.mojom (right):

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:69: bool is_file;
On 2016/11/03 at 04:27:04, dcheng wrote:
> Perhaps this could have a union to separate the blob and file specific stuff.

Since files are a subset of blobs I've made the file info fields an optional
struct within blob info.

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:73: mojo.common.mojom.String16
file_path;
On 2016/11/03 at 04:27:04, dcheng wrote:
> Does mojo.mojom.FilePath work?

Done.

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:75: double last_modified;
On 2016/11/03 at 04:27:04, dcheng wrote:
> Is it possible to use mojo.common.mojom.Time for this?

Done.

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
File content/common/indexed_db/indexed_db_struct_traits.h (right):

https://codereview.chromium.org/2449953008/diff/60001/content/common/indexed_...
content/common/indexed_db/indexed_db_struct_traits.h:26: return
data.ReadIndexKeys(&out->second);
On 2016/11/03 at 04:27:04, dcheng wrote:
> Nit: out-of-line

Done.

[Reilly Grant (use Gerrit), 2016-11-03 21:32:35]

Address dcheng@'s comments.

[Reilly Grant (use Gerrit), 2016-11-03 21:32:42]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 21:33:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Reilly Grant (use Gerrit), 2016-11-03 21:35:39]

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2449953008/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:166: int64_t
value_length) {
On 2016/11/03 at 21:32:22, Reilly Grant wrote:
> On 2016/11/03 at 04:27:04, dcheng wrote:
> > Let's make the types match: from what I can tell, the size map holds
uint64_t's.
> 
> Done.

I forgot to change this but since quota is reported as an int64_t I've changed
all the uint64_t's to int64_t for consistency instead of the other way around.

[commit-bot: I haz the power, 2016-11-03 23:35:46]

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

[commit-bot: I haz the power, 2016-11-03 23:35:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2016-11-04 21:37:47]

ipc lgtm... I think. It's really hard for me to be sure, since this CL is over
4000 lines.

In the future, it would be nice if we could split this into smaller chunks and
only convert a couple IPCs at a time.

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
File content/browser/indexed_db/database_impl.cc (right):

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
content/browser/indexed_db/database_impl.cc:637: key_only, task_type,
callbacks);
Nit: std::move(callbacks) here and elsewhere?

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:74:
std::swap(mojo_value->value->bits, value->bits);
Nit: standard way to write this is:

using std::swap;
swap(mojo_value->value->bits, value->bits);

just in case there's a more efficient swap() overload that would be found with
ADL.

(Same comment for the other places where this shows up)

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:245:
std::unique_ptr<DatabaseImpl> database(
Nit: auto database = base::MakeUnique<DataBaseImpl>(...);

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:561: new
IndexedDBMsg_CallbacksSuccessValue_Params());
auto params = base::MakeUnique<...>(); might make this a bit shorter.

https://codereview.chromium.org/2449953008/diff/80001/content/common/indexed_...
File content/common/indexed_db/indexed_db.mojom (right):

https://codereview.chromium.org/2449953008/diff/80001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:75: string uuid;
I'm thinking we might want to add a UUID mojo type in the future: WDYT?

(Out of scope of this CL obviously)

[Reilly Grant (use Gerrit), 2016-11-04 22:02:55]

Sorry again for the size. I promise the remaining Mojo conversion patches will
be smaller.

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
File content/browser/indexed_db/database_impl.cc (right):

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
content/browser/indexed_db/database_impl.cc:637: key_only, task_type,
callbacks);
On 2016/11/04 21:37:46, dcheng wrote:
> Nit: std::move(callbacks) here and elsewhere?

Done.

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:74:
std::swap(mojo_value->value->bits, value->bits);
On 2016/11/04 21:37:46, dcheng wrote:
> Nit: standard way to write this is:
> 
> using std::swap;
> swap(mojo_value->value->bits, value->bits);
> 
> just in case there's a more efficient swap() overload that would be found with
> ADL.
> 
> (Same comment for the other places where this shows up)

Done.

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:245:
std::unique_ptr<DatabaseImpl> database(
On 2016/11/04 21:37:46, dcheng wrote:
> Nit: auto database = base::MakeUnique<DataBaseImpl>(...);

Done.

https://codereview.chromium.org/2449953008/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:561: new
IndexedDBMsg_CallbacksSuccessValue_Params());
On 2016/11/04 21:37:46, dcheng wrote:
> auto params = base::MakeUnique<...>(); might make this a bit shorter.

Done.

https://codereview.chromium.org/2449953008/diff/80001/content/common/indexed_...
File content/common/indexed_db/indexed_db.mojom (right):

https://codereview.chromium.org/2449953008/diff/80001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:75: string uuid;
On 2016/11/04 21:37:46, dcheng wrote:
> I'm thinking we might want to add a UUID mojo type in the future: WDYT?
> 
> (Out of scope of this CL obviously)

I agree. I recall WebBluetooth also wanting one.

[Reilly Grant (use Gerrit), 2016-11-04 23:46:22]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-11-04 23:46:23]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/2449953008/#ps80001
(title: "Address dcheng@'s comments.")

[Reilly Grant (use Gerrit), 2016-11-04 23:46:43]

The CQ bit was unchecked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-11-04 23:48:24]

Address more comments from dcheng@.

[Reilly Grant (use Gerrit), 2016-11-04 23:50:10]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-11-04 23:50:11]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, cmumford@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2449953008/#ps100001
(title: "Address more comments from dcheng@.")

[commit-bot: I haz the power, 2016-11-04 23:51:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-05 01:11:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-05 01:13:27]

Description was changed from

==========
Port messages sent by WebIDBDatabaseImpl to Mojo.

This is part two of the series of patches that converts the IPC messages
sent by WebIDBFactoryImpl, WebIDBDatabaseImpl and WebIDBCursorImpl to
Mojo messages.

This is a more mechanical change because the conversion of messages sent
by WebIDBFactoryImpl added most of the necessary infrastructure.

BUG=627484
==========

to

==========
Port messages sent by WebIDBDatabaseImpl to Mojo.

This is part two of the series of patches that converts the IPC messages
sent by WebIDBFactoryImpl, WebIDBDatabaseImpl and WebIDBCursorImpl to
Mojo messages.

This is a more mechanical change because the conversion of messages sent
by WebIDBFactoryImpl added most of the necessary infrastructure.

BUG=627484

Committed: https://crrev.com/963ec6df64089bef6867ccdc608101d5fc06d5c0
Cr-Commit-Position: refs/heads/master@{#430110}
==========

[commit-bot: I haz the power, 2016-11-05 01:13:28]

Patchset 6 (id:??) landed as
https://crrev.com/963ec6df64089bef6867ccdc608101d5fc06d5c0
Cr-Commit-Position: refs/heads/master@{#430110}

[agrieve, 2016-11-08 15:14:05]

On 2016/11/05 01:13:28, commit-bot: I haz the power wrote:
> Patchset 6 (id:??) landed as
> https://crrev.com/963ec6df64089bef6867ccdc608101d5fc06d5c0
> Cr-Commit-Position: refs/heads/master@{#430110}

FYI - this change increased android .so size by 24kb. Is it the case that there
is another follow-up to remove old IPC code, or is the mojo conversion just this
costly (as tracked by
https://bugs.chromium.org/p/chromium/issues/detail?id=597125)

[Reilly Grant (use Gerrit), 2016-11-08 15:42:54]

On 2016/11/08 at 15:14:05, agrieve wrote:
> On 2016/11/05 01:13:28, commit-bot: I haz the power wrote:
> > Patchset 6 (id:??) landed as
> > https://crrev.com/963ec6df64089bef6867ccdc608101d5fc06d5c0
> > Cr-Commit-Position: refs/heads/master@{#430110}
> 
> FYI - this change increased android .so size by 24kb. Is it the case that
there is another follow-up to remove old IPC code, or is the mojo conversion
just this costly (as tracked by
https://bugs.chromium.org/p/chromium/issues/detail?id=597125)

There is a good amount of code currently duplicated between supporting the
remaining legacy IPC calls and the new Mojo messages. That will be removed in my
next patch and even more code will be removed when this conversion allows me to
remove the //content/child layer from the IndexedDB implementation completely
and use the Mojo interface from Blink directly.