Port messages sent by WebIDBCursorImpl to Mojo.

Port messages sent by WebIDBCursorImpl to Mojo.

This is the third and final patch in a series that converts the IPC
messages sent by WebIDBFactoryImpl, WebIDBDatabaseImpl and
WebIDBCursorImpl to Mojo messages.

This is a mostly mechanical change which also allows a lot of code
supporting the legacy IPC messages to be removed.

BUG=627484
Committed: https://crrev.com/39fb46622f2322243ff84f8c282f575d46c54c13
Cr-Commit-Position: refs/heads/master@{#433964}

[Reilly Grant (use Gerrit), 2016-11-15 02:39:40]

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

[Reilly Grant (use Gerrit), 2016-11-15 02:40:42]

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

[Reilly Grant (use Gerrit), 2016-11-15 02:40:43]

dcheng@, IPC security review please.
cmumford@, general review please.
yzshen@, please review the test-only function I added to associated_group.h.

[commit-bot: I haz the power, 2016-11-15 02:46:23]

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-15 05:02:41]

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

[commit-bot: I haz the power, 2016-11-15 05:02:42]

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_...)

[cmumford, 2016-11-15 15:33:35]

cmumford@chromium.org changed reviewers:
+ cmumford@chromium.org

[cmumford, 2016-11-15 15:33:35]

reillyg@ Looks like memory leaks in:

  WebIDBCursorImplTest.AdvancePrefetchTest
  WebIDBCursorImplTest.PrefetchReset

Identified by linux_chromium_asan_rel_ng.

[yzshen1, 2016-11-15 17:39:49]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2016-11-15 17:39:50]

https://codereview.chromium.org/2500263003/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/associated_group.h (right):

https://codereview.chromium.org/2500263003/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/associated_group.h:92:
AssociatedInterfaceRequest<Interface> GetProxyForTesting(
Does it make sense to:
- move this function to associated_interface_ptr.h;
- change the first param to AssociatedInterfacePtr (instead of Info)?

[Reilly Grant (use Gerrit), 2016-11-15 17:43:11]

https://codereview.chromium.org/2500263003/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/associated_group.h (right):

https://codereview.chromium.org/2500263003/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/associated_group.h:92:
AssociatedInterfaceRequest<Interface> GetProxyForTesting(
On 2016/11/15 at 17:39:50, yzshen1 wrote:
> Does it make sense to:
> - move this function to associated_interface_ptr.h;

I tried putting it in associated_interface_ptr_info.h but that created a
circular dependency with associated_group.h.

> - change the first param to AssociatedInterfacePtr (instead of Info)?

Mojo passes associated interfaces as AssociatedInterfacePtrInfo and so that is
what WebIDBCursorImpl expects as an argument. This is useful because it means
that I can pass it around for a little while before binding it to a thread. That
said, it's easy enough to make the first param an AssociatedInterfacePtr and
just PassInterface() it into WebIDBCursorImpl. However, since
CreateAssociatedInterface() also takes an AssociatedInterfacePtrInfo this seemed
more natural.

[yzshen1, 2016-11-15 17:59:35]

https://codereview.chromium.org/2500263003/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/associated_group.h (right):

https://codereview.chromium.org/2500263003/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/associated_group.h:92:
AssociatedInterfaceRequest<Interface> GetProxyForTesting(
On 2016/11/15 17:43:11, Reilly Grant wrote:
> On 2016/11/15 at 17:39:50, yzshen1 wrote:
> > Does it make sense to:
> > - move this function to associated_interface_ptr.h;
> 
> I tried putting it in associated_interface_ptr_info.h but that created a
> circular dependency with associated_group.h.

I meant associated_interface_ptr.h (instead of associated_interface_ptr_info.h).
It has GetProxy/GetDummyProxyForTesting there already. And it shouldn't have
circular dependency issue.

Maybe we should move such testing utils to a separate file?

> 
> > - change the first param to AssociatedInterfacePtr (instead of Info)?
> 
> Mojo passes associated interfaces as AssociatedInterfacePtrInfo and so that is
> what WebIDBCursorImpl expects as an argument. This is useful because it means
> that I can pass it around for a little while before binding it to a thread.
That
> said, it's easy enough to make the first param an AssociatedInterfacePtr and
> just PassInterface() it into WebIDBCursorImpl. However, since
> CreateAssociatedInterface() also takes an AssociatedInterfacePtrInfo this
seemed
> more natural.

Using AssociatedInterfacePtr* is more consistent with other
GetPorxy()/GetDummyProxyForTesting() functions. And also the word "Proxy"
usually means Ptr instead of PtrInfo.

Maybe an alternative is to name it CreateAssociatedInterfaceForTesting, and have
similar signature as AssociatedGroup::CreateAssociatedInterface?

[Reilly Grant (use Gerrit), 2016-11-15 21:19:17]

Address yzshen@'s comments and fix leak.

[Reilly Grant (use Gerrit), 2016-11-15 21:20:28]

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

[commit-bot: I haz the power, 2016-11-15 21:21:06]

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-15 21:21:17]

On 2016/11/15 at 15:33:35, cmumford wrote:
> reillyg@ Looks like memory leaks in:
> 
>   WebIDBCursorImplTest.AdvancePrefetchTest
>   WebIDBCursorImplTest.PrefetchReset
> 
> Identified by linux_chromium_asan_rel_ng.

Fixed.

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

On 2016/11/15 at 17:59:35, yzshen wrote:
>
https://codereview.chromium.org/2500263003/diff/20001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/associated_group.h (right):
> 
>
https://codereview.chromium.org/2500263003/diff/20001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/associated_group.h:92:
AssociatedInterfaceRequest<Interface> GetProxyForTesting(
> On 2016/11/15 17:43:11, Reilly Grant wrote:
> > On 2016/11/15 at 17:39:50, yzshen1 wrote:
> > > Does it make sense to:
> > > - move this function to associated_interface_ptr.h;
> > 
> > I tried putting it in associated_interface_ptr_info.h but that created a
> > circular dependency with associated_group.h.
> 
> I meant associated_interface_ptr.h (instead of
associated_interface_ptr_info.h). It has GetProxy/GetDummyProxyForTesting there
already. And it shouldn't have circular dependency issue.
> 
> Maybe we should move such testing utils to a separate file?
> 
> > 
> > > - change the first param to AssociatedInterfacePtr (instead of Info)?
> > 
> > Mojo passes associated interfaces as AssociatedInterfacePtrInfo and so that
is
> > what WebIDBCursorImpl expects as an argument. This is useful because it
means
> > that I can pass it around for a little while before binding it to a thread.
That
> > said, it's easy enough to make the first param an AssociatedInterfacePtr and
> > just PassInterface() it into WebIDBCursorImpl. However, since
> > CreateAssociatedInterface() also takes an AssociatedInterfacePtrInfo this
seemed
> > more natural.
> 
> Using AssociatedInterfacePtr* is more consistent with other
GetPorxy()/GetDummyProxyForTesting() functions. And also the word "Proxy"
usually means Ptr instead of PtrInfo.
> 
> Maybe an alternative is to name it CreateAssociatedInterfaceForTesting, and
have similar signature as AssociatedGroup::CreateAssociatedInterface?

I've moved mojo::GetProxyForTesting() into associated_interface_ptr.h and it
seems to be working without any header include issues.

[yzshen1, 2016-11-15 21:24:13]

mojo/public/cpp/bindings LGTM

[commit-bot: I haz the power, 2016-11-15 21:40:13]

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

[commit-bot: I haz the power, 2016-11-15 21:40:14]

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

[dcheng, 2016-11-17 03:46:23]

Mostly just nits. I'm handwaving a bit here, but the other bits look reasonable
to me (I'll take a more detailed look later, but I don't expect to have any more
comments really)

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/cursor_impl.cc (right):

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/cursor_impl.cc:37: dispatcher_host_(dispatcher_host),
Nit: std::move()

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/cursor_impl.cc:99: cursor_->Advance(count,
callbacks);
Nit: std::move()

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/cursor_impl.cc:110: callbacks);
Nit: std::move()

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

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:330:
std::swap(mojo_value->bits, value->bits);
No std:: to get ADL

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

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:105: return false;
Does this still need to be a BrowserMessageFilter?

https://codereview.chromium.org/2500263003/diff/40001/content/child/indexed_d...
File content/child/indexed_db/indexed_db_callbacks_impl.h (right):

https://codereview.chromium.org/2500263003/diff/40001/content/child/indexed_d...
content/child/indexed_db/indexed_db_callbacks_impl.h:32:
base::WeakPtr<WebIDBCursorImpl> cursor_,
Nit: cursor. Also const base::WeakPtr<>& (in theory, we could std::move here,
but we don't do this, so let's just be consistent with other code)

https://codereview.chromium.org/2500263003/diff/40001/content/child/indexed_d...
File content/child/indexed_db/indexed_db_dispatcher.h (right):

https://codereview.chromium.org/2500263003/diff/40001/content/child/indexed_d...
content/child/indexed_db/indexed_db_dispatcher.h:49: explicit
IndexedDBDispatcher();
Nit: no explicit

[Reilly Grant (use Gerrit), 2016-11-17 20:03:47]

Address dcheng@'s feedback.

[Reilly Grant (use Gerrit), 2016-11-17 20:04:52]

jam@, please review these top-level //content files:

content/browser/BUILD.gn
content/renderer/render_thread_impl.cc
content/renderer/renderer_blink_platform_impl.cc

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/cursor_impl.cc (right):

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/cursor_impl.cc:37: dispatcher_host_(dispatcher_host),
On 2016/11/17 at 03:46:23, dcheng wrote:
> Nit: std::move()

Done.

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/cursor_impl.cc:99: cursor_->Advance(count,
callbacks);
On 2016/11/17 at 03:46:23, dcheng wrote:
> Nit: std::move()

Done.

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/cursor_impl.cc:110: callbacks);
On 2016/11/17 at 03:46:23, dcheng wrote:
> Nit: std::move()

Done.

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/cursor_impl.cc:116: cursor_->PrefetchContinue(count,
callbacks);
Done here too. :)

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

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:330:
std::swap(mojo_value->bits, value->bits);
On 2016/11/17 at 03:46:23, dcheng wrote:
> No std:: to get ADL

Done.

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

https://codereview.chromium.org/2500263003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:105: return false;
On 2016/11/17 at 03:46:23, dcheng wrote:
> Does this still need to be a BrowserMessageFilter?

It still needs to send one IPC message. My next patch will make it no longer a
BrowserMessageFilter.

https://codereview.chromium.org/2500263003/diff/40001/content/child/indexed_d...
File content/child/indexed_db/indexed_db_callbacks_impl.h (right):

https://codereview.chromium.org/2500263003/diff/40001/content/child/indexed_d...
content/child/indexed_db/indexed_db_callbacks_impl.h:32:
base::WeakPtr<WebIDBCursorImpl> cursor_,
On 2016/11/17 at 03:46:23, dcheng wrote:
> Nit: cursor. Also const base::WeakPtr<>& (in theory, we could std::move here,
but we don't do this, so let's just be consistent with other code)

Done.

https://codereview.chromium.org/2500263003/diff/40001/content/child/indexed_d...
File content/child/indexed_db/indexed_db_dispatcher.h (right):

https://codereview.chromium.org/2500263003/diff/40001/content/child/indexed_d...
content/child/indexed_db/indexed_db_dispatcher.h:49: explicit
IndexedDBDispatcher();
On 2016/11/17 at 03:46:23, dcheng wrote:
> Nit: no explicit

Done.

[Reilly Grant (use Gerrit), 2016-11-17 20:04:58]

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

[commit-bot: I haz the power, 2016-11-17 20:06:12]

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-18 00:40:31]

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

[commit-bot: I haz the power, 2016-11-18 00:40:32]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-11-19 01:21:48]

Rebased.

[Reilly Grant (use Gerrit), 2016-11-19 01:33:56]

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

[commit-bot: I haz the power, 2016-11-19 01:34:28]

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-19 02:12:26]

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

[Reilly Grant (use Gerrit), 2016-11-19 02:12:27]

pfeldman@, please review top-level //content files:

 * content/browser/BUILD.gn
 * content/renderer/render_thread_impl.cc
 * content/renderer/renderer_blink_platform_impl.cc

[commit-bot: I haz the power, 2016-11-19 03:55:04]

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

[commit-bot: I haz the power, 2016-11-19 03:55:05]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-11-21 10:31:57]

Slightly handwavy mojo lgtm

Looking back on these CLS, I think the main thing I wish we could improve is all
the IOHelpers that are required. It feels like boilerplate that we shouldn't
need to manually write =(

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

https://codereview.chromium.org/2500263003/diff/80001/content/browser/indexed...
content/browser/indexed_db/cursor_impl.cc:60: dispatcher_host_.get(), origin_,
std::move(callbacks_info)));
Btw, I missed this, but IndexedDBCallbacks seems to take a ref to the dispatcher
host. It'd be nice to change it to a scoped_refptr param in a followup.

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

https://codereview.chromium.org/2500263003/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:294: new CursorImpl(cursor,
origin_, dispatcher_host_));
Nit: base::MakeUnique here

https://codereview.chromium.org/2500263003/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:333: }
Would this make sense to pull out into a helper?

(Maybe it's not worth it, since I think we might be able to simplify this once
blob handling is ported too?)

https://codereview.chromium.org/2500263003/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:359:
mojo_values.push_back(ConvertValue(&(*values)[i]));
It does seem like chunks like this repeat a decent amount though.

https://codereview.chromium.org/2500263003/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/associated_interface_ptr.h (right):

https://codereview.chromium.org/2500263003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/associated_interface_ptr.h:249:
CHECK(!handle.is_local());
Nit: why CHECK? If this is something that should "never happen", it seems like
it ought to be a DCHECK per
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[cmumford, 2016-11-21 16:43:40]

lgtm

[Reilly Grant (use Gerrit), 2016-11-21 19:00:34]

Address dcheng@'s comments.

[Reilly Grant (use Gerrit), 2016-11-21 19:02:22]

Thanks for the reviews.

In general I don't like all the boilerplate that I've had to write with the
IDBThreadHelpers and IOThreadHelpers but at least when it comes to the blob
management code it's nice to have a well-defined separation between what happens
on the IO thread and what doesn't.

All this stuff will go away when I can make manage blobs and make Mojo calls
directly from worker threads and the IDB thread. That's just waiting on
refactoring in the blob code.

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

https://codereview.chromium.org/2500263003/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:294: new CursorImpl(cursor,
origin_, dispatcher_host_));
On 2016/11/21 at 10:31:57, dcheng wrote:
> Nit: base::MakeUnique here

Done.

https://codereview.chromium.org/2500263003/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:333: }
On 2016/11/21 at 10:31:57, dcheng wrote:
> Would this make sense to pull out into a helper?
> 
> (Maybe it's not worth it, since I think we might be able to simplify this once
blob handling is ported too?)

Already had a helper for this actually.

https://codereview.chromium.org/2500263003/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/associated_interface_ptr.h (right):

https://codereview.chromium.org/2500263003/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/associated_interface_ptr.h:249:
CHECK(!handle.is_local());
On 2016/11/21 at 10:31:57, dcheng wrote:
> Nit: why CHECK? If this is something that should "never happen", it seems like
it ought to be a DCHECK per
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Changed to DCHECK. This is testing code anyways.

[Reilly Grant (use Gerrit), 2016-11-22 17:59:28]

pfeldman@, ping.

[Reilly Grant (use Gerrit), 2016-11-22 17:59:56]

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

[commit-bot: I haz the power, 2016-11-22 18:00:09]

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

[pfeldman, 2016-11-22 19:50:53]

lgtm

[commit-bot: I haz the power, 2016-11-22 19:54:26]

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

[commit-bot: I haz the power, 2016-11-22 19:54:27]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-11-22 20:19:55]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-11-22 20:19:55]

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

[commit-bot: I haz the power, 2016-11-22 20:20:38]

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-22 20:27:24]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479845995376350,
"parent_rev": "b20074f663cfcec1e82f8060661f6fba850f8e75", "commit_rev":
"9448c1f8fcde38c10ae2df02de3b18cc0cd20204"}

[commit-bot: I haz the power, 2016-11-22 20:27:53]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-22 20:31:45]

Description was changed from

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

This is the third and final patch in a series that converts the IPC
messages sent by WebIDBFactoryImpl, WebIDBDatabaseImpl and
WebIDBCursorImpl to Mojo messages.

This is a mostly mechanical change which also allows a lot of code
supporting the legacy IPC messages to be removed.

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

to

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

This is the third and final patch in a series that converts the IPC
messages sent by WebIDBFactoryImpl, WebIDBDatabaseImpl and
WebIDBCursorImpl to Mojo messages.

This is a mostly mechanical change which also allows a lot of code
supporting the legacy IPC messages to be removed.

BUG=627484

Committed: https://crrev.com/39fb46622f2322243ff84f8c282f575d46c54c13
Cr-Commit-Position: refs/heads/master@{#433964}
==========

[commit-bot: I haz the power, 2016-11-22 20:31:47]

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