Port messages sent by WebIDBFactoryImpl to Mojo.

Port messages sent by WebIDBFactoryImpl to Mojo.

This is the first of a series of patches that convert the IPC messages
sent by WebIDBFactoryImpl, WebIDBDatabaseImpl and WebIDBCursorImpl to
Mojo messages.

Once all of the IPC sent by IndexedDB and Blob storage are converted to
Mojo they can be moved off of channel-associated interfaces and more
agressive removal of unnecessary layers of abstraction (including
removal of all of the manual thread hopping I added) can be done.

BUG=627484
Committed: https://crrev.com/627e7f73d47910f4255031de55075e2a04b222f6
Cr-Commit-Position: refs/heads/master@{#426412}

[Reilly Grant (use Gerrit), 2016-09-26 10:40:33]

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

[commit-bot: I haz the power, 2016-09-26 10:40:46]

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-09-26 10:41:53]

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

[Reilly Grant (use Gerrit), 2016-09-26 10:41:53]

This is a new version of https://codereview.chromium.org/2320213004 which does
not attempt to do Onion Soup at the same time as Mojoification because the
thread hopping necessary to use channel-associated interfaces from workers is
not possible inside Blink.

[commit-bot: I haz the power, 2016-09-26 11:35:44]

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

[commit-bot: I haz the power, 2016-09-26 11:35:45]

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

[Reilly Grant (use Gerrit), 2016-09-27 04:29:27]

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

[commit-bot: I haz the power, 2016-09-27 04:29:49]

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-09-27 05:58:51]

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

[commit-bot: I haz the power, 2016-09-27 05:58:52]

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

[Reilly Grant (use Gerrit), 2016-09-27 09:30:54]

rockot@, can you take a look at the Android build failure? It looks like this
builder is building "all" and that means it compiles
//content/common:mojo_bindings_java which won't compile because I'm using the
mojo.common.mojom.String16 native type. We probably shouldn't generate
definitions that depend on native types when no mapping in that language exists.

[Ken Rockot(use gerrit already), 2016-09-27 15:14:40]

If you have any dependencies on native typemaps you can specify cpp_only =
true in your mojom rule.

On Tue, Sep 27, 2016 at 2:30 AM, <reillyg@chromium.org> wrote:

> rockot@, can you take a look at the Android build failure? It looks like
> this
> builder is building "all" and that means it compiles
> //content/common:mojo_bindings_java which won't compile because I'm using
> the
> mojo.common.mojom.String16 native type. We probably shouldn't generate
> definitions that depend on native types when no mapping in that language
> exists.
>
> https://codereview.chromium.org/2370643004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-09-27 15:14:53]

If you have any dependencies on native typemaps you can specify cpp_only =
true in your mojom rule.

On Tue, Sep 27, 2016 at 2:30 AM, <reillyg@chromium.org> wrote:

> rockot@, can you take a look at the Android build failure? It looks like
> this
> builder is building "all" and that means it compiles
> //content/common:mojo_bindings_java which won't compile because I'm using
> the
> mojo.common.mojom.String16 native type. We probably shouldn't generate
> definitions that depend on native types when no mapping in that language
> exists.
>
> https://codereview.chromium.org/2370643004/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Reilly Grant (use Gerrit), 2016-09-28 02:21:02]

Allow cpp_only to be set by the invoker.

[Reilly Grant (use Gerrit), 2016-09-28 02:26:46]

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

[Reilly Grant (use Gerrit), 2016-09-28 02:26:50]

On 2016/09/27 at 15:14:53, rockot wrote:
> If you have any dependencies on native typemaps you can specify cpp_only =
> true in your mojom rule.

Now you can.

[commit-bot: I haz the power, 2016-09-28 02:26:58]

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-09-28 06:48:12]

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

[commit-bot: I haz the power, 2016-09-28 06:48:13]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-09-28 20:06:45]

IPC looks good, Mojo usage looks good. I just wonder if we can't do something a
little better than the Callbacks/DatabaseCallbacks pattern for client messages.

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

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
content/child/indexed_db/indexed_db_database_callbacks_impl.h:24: void
Bind(indexed_db::mojom::DatabaseCallbacksAssociatedPtrInfo* ptr_info,
Any reason why you chose to have this take a PtrInfo* instead of a Request? I
generally discourage this because it's a less flexible interface -- you can
always create a new pipe to get a Request and Ptr/PtrInfo, but if you already
have a Request, you can't use an method that takes a Ptr*/PtrInfo*.

In fact I'd really like to delete all the Binding/AssociatedBinding methods
which take a Ptr*/PtrInfo*. But if there are legitimate reasons to keep them
which I'm overlooking (IMHO less typing does not trump having a minimal yet
robust interface), I'd like to hear about them!

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
content/child/indexed_db/webidbfactory_impl.cc:108: void
WebIDBFactoryImpl::IOThreadHelper::EnsureServiceConnection() {
nit: I think it'd just be cleaner to have an accessor which lazily initializes
service_ and returns service_.get().

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

https://codereview.chromium.org/2370643004/diff/40001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:47: interface Callbacks {
I see that this is mirroring the existing implementation, but it seems
incredibly weird. Is it really true that every Factory request can receive N >=
0 responses, and that all the messages in Callbacks (or DatabaseCallbacks where
applicable) are appropriate for all request types?

https://codereview.chromium.org/2370643004/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/2370643004/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:212: if (defined(invoker.cpp_only)) {
Oh, I thought we had already exposed this. Can you please briefly document this
at the top of the GNI along with the other variables?

[Reilly Grant (use Gerrit), 2016-09-29 06:44:50]

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

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
content/child/indexed_db/indexed_db_database_callbacks_impl.h:24: void
Bind(indexed_db::mojom::DatabaseCallbacksAssociatedPtrInfo* ptr_info,
On 2016/09/28 at 20:06:44, Ken Rockot wrote:
> Any reason why you chose to have this take a PtrInfo* instead of a Request? I
generally discourage this because it's a less flexible interface -- you can
always create a new pipe to get a Request and Ptr/PtrInfo, but if you already
have a Request, you can't use an method that takes a Ptr*/PtrInfo*.
> 
> In fact I'd really like to delete all the Binding/AssociatedBinding methods
which take a Ptr*/PtrInfo*. But if there are legitimate reasons to keep them
which I'm overlooking (IMHO less typing does not trump having a minimal yet
robust interface), I'd like to hear about them!

It looks like the Ptr -> Request flow works when you want to pass the request
(mojo::GetProxy calls CreateAssociatedInterface with WILL_PASS_REQUEST) but if
you want to pass the Ptr you actually have to pass a PtrInfo and there's no
method to convert a PtrInfo to a Request (without calling MakeAssociatedRequest
which is not typesafe because you have to pass an untyped
ScopedInterfaceEndpointHandle). AssociatedBinding::Bind calls
CreateAssociatedInterface with WILL_PASS_PTR so that's what I used.

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
content/child/indexed_db/webidbfactory_impl.cc:108: void
WebIDBFactoryImpl::IOThreadHelper::EnsureServiceConnection() {
On 2016/09/28 at 20:06:44, Ken Rockot wrote:
> nit: I think it'd just be cleaner to have an accessor which lazily initializes
service_ and returns service_.get().

I considered that but since I usually need both service_.associated_group() and
service_.get() in each method this was easier.

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

https://codereview.chromium.org/2370643004/diff/40001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:47: interface Callbacks {
On 2016/09/28 at 20:06:44, Ken Rockot wrote:
> I see that this is mirroring the existing implementation, but it seems
incredibly weird. Is it really true that every Factory request can receive N >=
0 responses, and that all the messages in Callbacks (or DatabaseCallbacks where
applicable) are appropriate for all request types?

Most messages will only generate one response but database open in particular
will first generate UpgradeNeeded, then SuccessDatabase. Right now the concept
of the Callbacks object is baked into IndexedDB but I'd like to start cleaning
it up as I start to remove layers from the implementation.

https://codereview.chromium.org/2370643004/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/2370643004/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:212: if (defined(invoker.cpp_only)) {
On 2016/09/28 at 20:06:44, Ken Rockot wrote:
> Oh, I thought we had already exposed this. Can you please briefly document
this at the top of the GNI along with the other variables?

Done.

[Ken Rockot(use gerrit already), 2016-09-29 22:37:50]

LGTM with a little optional refactoring

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

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
content/child/indexed_db/indexed_db_database_callbacks_impl.h:24: void
Bind(indexed_db::mojom::DatabaseCallbacksAssociatedPtrInfo* ptr_info,
On 2016/09/29 at 06:44:50, Reilly Grant wrote:
> On 2016/09/28 at 20:06:44, Ken Rockot wrote:
> > Any reason why you chose to have this take a PtrInfo* instead of a Request?
I generally discourage this because it's a less flexible interface -- you can
always create a new pipe to get a Request and Ptr/PtrInfo, but if you already
have a Request, you can't use an method that takes a Ptr*/PtrInfo*.
> > 
> > In fact I'd really like to delete all the Binding/AssociatedBinding methods
which take a Ptr*/PtrInfo*. But if there are legitimate reasons to keep them
which I'm overlooking (IMHO less typing does not trump having a minimal yet
robust interface), I'd like to hear about them!
> 
> It looks like the Ptr -> Request flow works when you want to pass the request
(mojo::GetProxy calls CreateAssociatedInterface with WILL_PASS_REQUEST) but if
you want to pass the Ptr you actually have to pass a PtrInfo and there's no
method to convert a PtrInfo to a Request (without calling MakeAssociatedRequest
which is not typesafe because you have to pass an untyped
ScopedInterfaceEndpointHandle). AssociatedBinding::Bind calls
CreateAssociatedInterface with WILL_PASS_PTR so that's what I used.


I see, so it's out of convenience because AssociatedBinding already essentially
does GetProxy but with WILL_PASS_PTR instead.

My point still stands, a method which takes a request is strictly more flexible
than one which forces you to use a request it generates internally, but I
understand you using this given the APIs we have available. So, not your problem
and this is fine as-is for now.

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
content/child/indexed_db/webidbfactory_impl.cc:108: void
WebIDBFactoryImpl::IOThreadHelper::EnsureServiceConnection() {
On 2016/09/29 at 06:44:50, Reilly Grant wrote:
> On 2016/09/28 at 20:06:44, Ken Rockot wrote:
> > nit: I think it'd just be cleaner to have an accessor which lazily
initializes service_ and returns service_.get().
> 
> I considered that but since I usually need both service_.associated_group()
and service_.get() in each method this was easier.


Fair enough. You could of course have the lazy getter return a non-const ref to
the Ptr itself. You could also then provide something like:

CallbacksAssociatedPtr WebIDBFactoryImpl::GetCallbacksProxy(
    std::unique_ptr<IndexedDBCallbacksImpl> callbacks) {
  CallbacksAssociatedRequest request;
  CallbacksAssociatedPtrInfo ptr_info;
  service().associated_group()->CreateAssociatedInterface(
      mojo::AssocaiatedGroup::WILL_PASS_PTR, &ptr_info, &request);

  // Passes ownership of |callbacks| to the |request| pipe.
  callbacks->release()->Bind(std::move(request));

  // side note: this would be a really nice use of StrongAssociatedBinding which
  // doesn't (yet) exist:
  //   mojo::MakeStrongAssociatedBinding(std::move(callbacks),
std::move(request));
  // and then we don't need a scary release() and callbacks doesn't need to own
  // any kind of Binding object.

  CallbacksAssociatedPtr ptr;
  ptr.Bind(std::move(ptr_info));
  return ptr;
}


You could do the same for DatabaseCallbacks.

Then you can get rid of Ensure... and all your usage sites look like:

  service()->DeleteDatabase(CreateCallbacksProxy(std::move(callbacks)), origin,
name);

Which IMHO is much easier to read.


Biased opinion, but I really like my suggestion. WDYT about these changes?

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

https://codereview.chromium.org/2370643004/diff/40001/content/common/indexed_...
content/common/indexed_db/indexed_db.mojom:47: interface Callbacks {
On 2016/09/29 at 06:44:50, Reilly Grant wrote:
> On 2016/09/28 at 20:06:44, Ken Rockot wrote:
> > I see that this is mirroring the existing implementation, but it seems
incredibly weird. Is it really true that every Factory request can receive N >=
0 responses, and that all the messages in Callbacks (or DatabaseCallbacks where
applicable) are appropriate for all request types?
> 
> Most messages will only generate one response but database open in particular
will first generate UpgradeNeeded, then SuccessDatabase. Right now the concept
of the Callbacks object is baked into IndexedDB but I'd like to start cleaning
it up as I start to remove layers from the implementation.

OK. I still think it's weird, and I'd prefer to not leave things in this state
for long. But I'll let owners decide if they're OK with it.

[Reilly Grant (use Gerrit), 2016-09-30 07:21:28]

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

[commit-bot: I haz the power, 2016-09-30 07:21:40]

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-09-30 07:24:19]

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

[Reilly Grant (use Gerrit), 2016-09-30 07:24:21]

jam@, can you give me a top-level //content review?

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

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
content/child/indexed_db/indexed_db_database_callbacks_impl.h:24: void
Bind(indexed_db::mojom::DatabaseCallbacksAssociatedPtrInfo* ptr_info,
On 2016/09/29 at 22:37:50, Ken Rockot wrote:
> On 2016/09/29 at 06:44:50, Reilly Grant wrote:
> > On 2016/09/28 at 20:06:44, Ken Rockot wrote:
> > > Any reason why you chose to have this take a PtrInfo* instead of a
Request? I generally discourage this because it's a less flexible interface --
you can always create a new pipe to get a Request and Ptr/PtrInfo, but if you
already have a Request, you can't use an method that takes a Ptr*/PtrInfo*.
> > > 
> > > In fact I'd really like to delete all the Binding/AssociatedBinding
methods which take a Ptr*/PtrInfo*. But if there are legitimate reasons to keep
them which I'm overlooking (IMHO less typing does not trump having a minimal yet
robust interface), I'd like to hear about them!
> > 
> > It looks like the Ptr -> Request flow works when you want to pass the
request (mojo::GetProxy calls CreateAssociatedInterface with WILL_PASS_REQUEST)
but if you want to pass the Ptr you actually have to pass a PtrInfo and there's
no method to convert a PtrInfo to a Request (without calling
MakeAssociatedRequest which is not typesafe because you have to pass an untyped
ScopedInterfaceEndpointHandle). AssociatedBinding::Bind calls
CreateAssociatedInterface with WILL_PASS_PTR so that's what I used.
> 
> 
> I see, so it's out of convenience because AssociatedBinding already
essentially does GetProxy but with WILL_PASS_PTR instead.
> 
> My point still stands, a method which takes a request is strictly more
flexible than one which forces you to use a request it generates internally, but
I understand you using this given the APIs we have available. So, not your
problem and this is fine as-is for now.

I agree, and liked your other suggestion so I've changed this to take a request.

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_d...
content/child/indexed_db/webidbfactory_impl.cc:108: void
WebIDBFactoryImpl::IOThreadHelper::EnsureServiceConnection() {
On 2016/09/29 at 22:37:50, Ken Rockot wrote:
> On 2016/09/29 at 06:44:50, Reilly Grant wrote:
> > On 2016/09/28 at 20:06:44, Ken Rockot wrote:
> > > nit: I think it'd just be cleaner to have an accessor which lazily
initializes service_ and returns service_.get().
> > 
> > I considered that but since I usually need both service_.associated_group()
and service_.get() in each method this was easier.
> 
> 
> Fair enough. You could of course have the lazy getter return a non-const ref
to the Ptr itself. You could also then provide something like:
> 
> CallbacksAssociatedPtr WebIDBFactoryImpl::GetCallbacksProxy(
>     std::unique_ptr<IndexedDBCallbacksImpl> callbacks) {
>   CallbacksAssociatedRequest request;
>   CallbacksAssociatedPtrInfo ptr_info;
>   service().associated_group()->CreateAssociatedInterface(
>       mojo::AssocaiatedGroup::WILL_PASS_PTR, &ptr_info, &request);
> 
>   // Passes ownership of |callbacks| to the |request| pipe.
>   callbacks->release()->Bind(std::move(request));
> 
>   // side note: this would be a really nice use of StrongAssociatedBinding
which
>   // doesn't (yet) exist:
>   //   mojo::MakeStrongAssociatedBinding(std::move(callbacks),
std::move(request));
>   // and then we don't need a scary release() and callbacks doesn't need to
own
>   // any kind of Binding object.
> 
>   CallbacksAssociatedPtr ptr;
>   ptr.Bind(std::move(ptr_info));
>   return ptr;
> }
> 
> 
> You could do the same for DatabaseCallbacks.
> 
> Then you can get rid of Ensure... and all your usage sites look like:
> 
>   service()->DeleteDatabase(CreateCallbacksProxy(std::move(callbacks)),
origin, name);
> 
> Which IMHO is much easier to read.
> 
> 
> Biased opinion, but I really like my suggestion. WDYT about these changes?

Oh look! Now there's a StrongAssociatedBinding.

[commit-bot: I haz the power, 2016-09-30 09:12:25]

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

[commit-bot: I haz the power, 2016-09-30 09:12:26]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-30 16:35:09]

On 2016/09/30 07:24:21, Reilly Grant wrote:
> jam@, can you give me a top-level //content review?

lgtm for the non indexed_db directories (I defer to Chris for that)

[Reilly Grant (use Gerrit), 2016-10-03 05:39:41]

Rebase over the Blink reformatting.

[dcheng, 2016-10-05 08:00:03]

Sorry for the review latency =(

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:43: const
content::IndexedDBDatabaseMetadata& web_metadata) {
Is it possible to use typemapping for these conversions rather than embedding it
here? Documentation at
https://www.chromium.org/developers/design-documents/mojo/type-mapping, and I'm
happy to help as well.

(Similar comments apply to the other conversion helpers)

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:705:
static_cast<::indexed_db::mojom::DataLoss>(data_loss), data_loss_message,
Similar to above, would it be possible to use EnumTraits here?

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:50: const char*
kInvalidOrigin = "Origin is invalid";
As written, this can actually be mutated:

kInvalidOrigin = "my new error message";

To prevent that, this can be written constexpr char kInvalidOrigin[] = ...;
instead.

https://codereview.chromium.org/2370643004/diff/100001/ipc/ipc_sync_message_f...
File ipc/ipc_sync_message_filter.h (right):

https://codereview.chromium.org/2370643004/diff/100001/ipc/ipc_sync_message_f...
ipc/ipc_sync_message_filter.h:61: }
Part of me feels like it'd be nice to land this part separately, even if it
won't be used until the rest of this CL lands.

I don't feel strongly about this though.

https://codereview.chromium.org/2370643004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:62: typedef
std::vector<std::unique_ptr<WebIDBCallbacksImpl>> CallbacksList;
Nit: Prefer type aliases (using NewType = Definition) to typedefs in new code
(here and elsewhere)

[Reilly Grant (use Gerrit), 2016-10-05 09:16:26]

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:43: const
content::IndexedDBDatabaseMetadata& web_metadata) {
On 2016/10/05 at 08:00:03, dcheng wrote:
> Is it possible to use typemapping for these conversions rather than embedding
it here? Documentation at
https://www.chromium.org/developers/design-documents/mojo/type-mapping, and I'm
happy to help as well.
> 
> (Similar comments apply to the other conversion helpers)

This particular case is tricky because I need to map to
content::IndexedDBDatabaseMetadata in the browser process and
blink::WebIDBMetadata in the render process. I don't think I can do that with a
typemap because they're both using the same variant.

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:705:
static_cast<::indexed_db::mojom::DataLoss>(data_loss), data_loss_message,
On 2016/10/05 at 08:00:03, dcheng wrote:
> Similar to above, would it be possible to use EnumTraits here?

Done. Wasn't sure this worked for enums. Glad it does.

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:50: const char*
kInvalidOrigin = "Origin is invalid";
On 2016/10/05 at 08:00:03, dcheng wrote:
> As written, this can actually be mutated:
> 
> kInvalidOrigin = "my new error message";
> 
> To prevent that, this can be written constexpr char kInvalidOrigin[] = ...;
instead.

Done.

https://codereview.chromium.org/2370643004/diff/100001/ipc/ipc_sync_message_f...
File ipc/ipc_sync_message_filter.h (right):

https://codereview.chromium.org/2370643004/diff/100001/ipc/ipc_sync_message_f...
ipc/ipc_sync_message_filter.h:61: }
On 2016/10/05 at 08:00:03, dcheng wrote:
> Part of me feels like it'd be nice to land this part separately, even if it
won't be used until the rest of this CL lands.
> 
> I don't feel strongly about this though.

This actually comes from Ken's patch: https://codereview.chromium.org/2344333002

It looks like he closed it without landing it.

https://codereview.chromium.org/2370643004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:62: typedef
std::vector<std::unique_ptr<WebIDBCallbacksImpl>> CallbacksList;
On 2016/10/05 at 08:00:03, dcheng wrote:
> Nit: Prefer type aliases (using NewType = Definition) to typedefs in new code
(here and elsewhere)

Done.

[Reilly Grant (use Gerrit), 2016-10-05 09:17:02]

Addressed most of dcheng@'s feedback.

[Reilly Grant (use Gerrit), 2016-10-05 09:17:37]

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

[commit-bot: I haz the power, 2016-10-05 09:17:56]

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-05 11:09:34]

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

[commit-bot: I haz the power, 2016-10-05 11:09:35]

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

[Ken Rockot(use gerrit already), 2016-10-05 13:16:08]

On Oct 5, 2016 2:16 AM, <reillyg@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
> File content/browser/indexed_db/indexed_db_callbacks.cc (right):
>
>
https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
> content/browser/indexed_db/indexed_db_callbacks.cc:43: const
> content::IndexedDBDatabaseMetadata& web_metadata) {
> On 2016/10/05 at 08:00:03, dcheng wrote:
> > Is it possible to use typemapping for these conversions rather than
> embedding it here? Documentation at
> https://www.chromium.org/developers/design-documents/mojo/type-mapping,
> and I'm happy to help as well.
> >
> > (Similar comments apply to the other conversion helpers)
>
> This particular case is tricky because I need to map to
> content::IndexedDBDatabaseMetadata in the browser process and
> blink::WebIDBMetadata in the render process. I don't think I can do that
> with a typemap because they're both using the same variant.
>
>
>
https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
> content/browser/indexed_db/indexed_db_callbacks.cc:705:
> static_cast<::indexed_db::mojom::DataLoss>(data_loss),
> data_loss_message,
> On 2016/10/05 at 08:00:03, dcheng wrote:
> > Similar to above, would it be possible to use EnumTraits here?
>
> Done. Wasn't sure this worked for enums. Glad it does.
>
>
>
https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
> File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):
>
>
https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
> content/browser/indexed_db/indexed_db_dispatcher_host.cc:50: const char*
> kInvalidOrigin = "Origin is invalid";
> On 2016/10/05 at 08:00:03, dcheng wrote:
> > As written, this can actually be mutated:
> >
> > kInvalidOrigin = "my new error message";
> >
> > To prevent that, this can be written constexpr char kInvalidOrigin[] =
> ...; instead.
>
> Done.
>
>
>
https://codereview.chromium.org/2370643004/diff/100001/ipc/ipc_sync_message_f...
> File ipc/ipc_sync_message_filter.h (right):
>
>
https://codereview.chromium.org/2370643004/diff/100001/ipc/ipc_sync_message_f...
> ipc/ipc_sync_message_filter.h:61: }
> On 2016/10/05 at 08:00:03, dcheng wrote:
> > Part of me feels like it'd be nice to land this part separately, even
> if it won't be used until the rest of this CL lands.
> >
> > I don't feel strongly about this though.
>
> This actually comes from Ken's patch:
> https://codereview.chromium.org/2344333002
>
> It looks like he closed it without landing it.
>
>

I only closed it because i assumed it would land with this CL :)

I'll go ahead and get that landed.

>
https://codereview.chromium.org/2370643004/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
> (right):
>
>
https://codereview.chromium.org/2370643004/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:62:
> typedef std::vector<std::unique_ptr<WebIDBCallbacksImpl>> CallbacksList;
> On 2016/10/05 at 08:00:03, dcheng wrote:
> > Nit: Prefer type aliases (using NewType = Definition) to typedefs in
> new code (here and elsewhere)
>
> Done.
>
> https://codereview.chromium.org/2370643004/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-10-05 13:16:47]

On Oct 5, 2016 2:16 AM, <reillyg@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
> File content/browser/indexed_db/indexed_db_callbacks.cc (right):
>
>
https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
> content/browser/indexed_db/indexed_db_callbacks.cc:43: const
> content::IndexedDBDatabaseMetadata& web_metadata) {
> On 2016/10/05 at 08:00:03, dcheng wrote:
> > Is it possible to use typemapping for these conversions rather than
> embedding it here? Documentation at
> https://www.chromium.org/developers/design-documents/mojo/type-mapping,
> and I'm happy to help as well.
> >
> > (Similar comments apply to the other conversion helpers)
>
> This particular case is tricky because I need to map to
> content::IndexedDBDatabaseMetadata in the browser process and
> blink::WebIDBMetadata in the render process. I don't think I can do that
> with a typemap because they're both using the same variant.
>
>
>
https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
> content/browser/indexed_db/indexed_db_callbacks.cc:705:
> static_cast<::indexed_db::mojom::DataLoss>(data_loss),
> data_loss_message,
> On 2016/10/05 at 08:00:03, dcheng wrote:
> > Similar to above, would it be possible to use EnumTraits here?
>
> Done. Wasn't sure this worked for enums. Glad it does.
>
>
>
https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
> File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):
>
>
https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexe...
> content/browser/indexed_db/indexed_db_dispatcher_host.cc:50: const char*
> kInvalidOrigin = "Origin is invalid";
> On 2016/10/05 at 08:00:03, dcheng wrote:
> > As written, this can actually be mutated:
> >
> > kInvalidOrigin = "my new error message";
> >
> > To prevent that, this can be written constexpr char kInvalidOrigin[] =
> ...; instead.
>
> Done.
>
>
>
https://codereview.chromium.org/2370643004/diff/100001/ipc/ipc_sync_message_f...
> File ipc/ipc_sync_message_filter.h (right):
>
>
https://codereview.chromium.org/2370643004/diff/100001/ipc/ipc_sync_message_f...
> ipc/ipc_sync_message_filter.h:61: }
> On 2016/10/05 at 08:00:03, dcheng wrote:
> > Part of me feels like it'd be nice to land this part separately, even
> if it won't be used until the rest of this CL lands.
> >
> > I don't feel strongly about this though.
>
> This actually comes from Ken's patch:
> https://codereview.chromium.org/2344333002
>
> It looks like he closed it without landing it.
>
>

I only closed it because i assumed it would land with this CL :)

I'll go ahead and get that landed.

>
https://codereview.chromium.org/2370643004/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
> (right):
>
>
https://codereview.chromium.org/2370643004/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:62:
> typedef std::vector<std::unique_ptr<WebIDBCallbacksImpl>> CallbacksList;
> On 2016/10/05 at 08:00:03, dcheng wrote:
> > Nit: Prefer type aliases (using NewType = Definition) to typedefs in
> new code (here and elsewhere)
>
> Done.
>
> https://codereview.chromium.org/2370643004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-10-05 17:56:54]

FYI the SyncMessageFilter change has landed now.

[cmumford, 2016-10-11 18:30:55]

Looks great - all minor nit comments and a few questions.

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:42:
::indexed_db::mojom::DatabaseMetadataPtr ConvertMetadata(
What about splitting this into three functions: 1) ConvertDatabaseMetadata, 2)
ConvertObjectStoreMetadata, and 3) ConvertIndexMetadata? I believe you may need
those in upcoming CL's.

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:88: void
SendUpgradeNeeded(int32_t database_id,
nit: Moving SendUpgradeNeeded to be below SendSuccessDatabase would keep the
database methods together which I think will be nice once the other interfaces
arrive in later CL's.

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:95:
::indexed_db::mojom::CallbacksAssociatedPtr callbacks_;
DISALLOW_COPY_AND_ASSIGN(IOThreadHelper);

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:137: if
(callbacks_info.is_valid())
Might not be a bad idea to add:

DCHECK_CURRENTLY_ON(BrowserThread::IO);

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.h:150: IOThreadHelper* helper_;
nit: io_helper_? Just something to indicate what the helper is helping with?

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database_callbacks.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_callbacks.cc:18:
IOThreadHelper(DatabaseCallbacksAssociatedPtrInfo callbacks_info);
explicit

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_callbacks.cc:27:
::indexed_db::mojom::DatabaseCallbacksAssociatedPtr callbacks_;
DISALLOW_COPY_AND_ASSIGN(IOThreadHelper);

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_callbacks.cc:50:
BrowserThread::PostTask(
Do you want to:

  DCHECK(helper_);

before all PostTasks?

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
File content/child/indexed_db/indexed_db_callbacks_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.cc:89:
scoped_refptr<ThreadSafeSender> thread_safe_sender_;
DISALLOW_COPY_AND_ASSIGN(InternalState);

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.cc:96: internal_state_ = new
InternalState(callbacks, std::move(thread_safe_sender));
Initialize |internal_state_| in initializer section.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
File content/child/indexed_db/indexed_db_callbacks_impl.h (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.h:28: //
indexed_db::mojom::Callbacks implementation
nit: end comment with ':'

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.h:48:
scoped_refptr<base::SingleThreadTaskRunner> callback_runner_;
DISALLOW_COPY_AND_ASSIGN(IndexedDBCallbacksImpl);

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
File content/child/indexed_db/indexed_db_database_callbacks_impl.h (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_database_callbacks_impl.h:33:
blink::WebIDBDatabaseCallbacks* callbacks_;
DISALLOW_COPY_AND_ASSIGN(IndexedDBDatabaseCallbacksImpl);

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
File content/child/indexed_db/indexed_db_message_filter.cc (left):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_message_filter.cc:38: void
IndexedDBMessageFilter::OnStaleMessageReceived(const IPC::Message& msg) {
Can also no longer include "indexed_db_constants.h"

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/webidbfactory_impl.cc:55: FactoryAssociatedPtr
service_;
DISALLOW_COPY_AND_ASSIGN(IOThreadHelper);

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/webidbfactory_impl.cc:64: helper_ = new
IOThreadHelper(std::move(sync_message_filter));
set |helper_| in class initialization section.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/webidbfactory_impl.cc:94: WorkerThread::GetCurrentId(),
base::string16(name), version,
no need to explicitly typecase to base::string16.

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/associated_interface_ptr_info.h (right):

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/associated_interface_ptr_info.h:23:
AssociatedInterfacePtrInfo(decltype(nullptr)) : version_(0u) {}
Should use std::nullptr_t here.

However, I believe this constructor is best avoided entirely as nullptr_t is
really used to resolve ambiguous overloaded functions accepting pointers - and
there are no other constructors taking pointers.

Instead you could pass:

  ::indexed_db::mojom::DatabaseCallbacksAssociatedPtrInfo()

as the third parameter to the IndexedDBDatabaseCallbacks constructor to
explicitly create that object.

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/strong_associated_binding.h (right):

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/strong_associated_binding.h:98: else if
(!connection_error_with_reason_handler_.is_null())
I'm assuming that only one type of error handler will ever be set?

https://codereview.chromium.org/2370643004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:91:
std::find_if(callbacks.begin(), callbacks.end(),
Why can't you just do?

auto it = std::find(callbacks.begin(), callbacks.end(), this);
if (it != callbacks.end())
  ...

https://codereview.chromium.org/2370643004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:180: if
(m_request) {
Nit: I think the style guide prefers:

  if (!m_request)
    return
  ...

but I could be wrong...

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

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

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

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:42:
::indexed_db::mojom::DatabaseMetadataPtr ConvertMetadata(
On 2016/10/11 at 18:30:54, cmumford wrote:
> What about splitting this into three functions: 1) ConvertDatabaseMetadata, 2)
ConvertObjectStoreMetadata, and 3) ConvertIndexMetadata? I believe you may need
those in upcoming CL's.

Done. I don't think I'll be using it anywhere else but separating this into
multiple functions greatly increases readability.

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:88: void
SendUpgradeNeeded(int32_t database_id,
On 2016/10/11 at 18:30:54, cmumford wrote:
> nit: Moving SendUpgradeNeeded to be below SendSuccessDatabase would keep the
database methods together which I think will be nice once the other interfaces
arrive in later CL's.

I'll re-order these to match indexed_db_callbacks.h and add matching comments
documenting which methods generate which callbacks.

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:95:
::indexed_db::mojom::CallbacksAssociatedPtr callbacks_;
On 2016/10/11 at 18:30:54, cmumford wrote:
> DISALLOW_COPY_AND_ASSIGN(IOThreadHelper);

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:137: if
(callbacks_info.is_valid())
On 2016/10/11 at 18:30:53, cmumford wrote:
> Might not be a bad idea to add:
> 
> DCHECK_CURRENTLY_ON(BrowserThread::IO);

I added it down in the IOThreadHelper constructor since that is where the Bind
happens and it is important that we be on the IO thread for that.

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.h:150: IOThreadHelper* helper_;
On 2016/10/11 at 18:30:54, cmumford wrote:
> nit: io_helper_? Just something to indicate what the helper is helping with?

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database_callbacks.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_callbacks.cc:18:
IOThreadHelper(DatabaseCallbacksAssociatedPtrInfo callbacks_info);
On 2016/10/11 at 18:30:54, cmumford wrote:
> explicit

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_callbacks.cc:27:
::indexed_db::mojom::DatabaseCallbacksAssociatedPtr callbacks_;
On 2016/10/11 at 18:30:54, cmumford wrote:
> DISALLOW_COPY_AND_ASSIGN(IOThreadHelper);

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_callbacks.cc:50:
BrowserThread::PostTask(
On 2016/10/11 at 18:30:54, cmumford wrote:
> Do you want to:
> 
>   DCHECK(helper_);
> 
> before all PostTasks?

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
File content/child/indexed_db/indexed_db_callbacks_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.cc:89:
scoped_refptr<ThreadSafeSender> thread_safe_sender_;
On 2016/10/11 at 18:30:54, cmumford wrote:
> DISALLOW_COPY_AND_ASSIGN(InternalState);

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.cc:96: internal_state_ = new
InternalState(callbacks, std::move(thread_safe_sender));
On 2016/10/11 at 18:30:54, cmumford wrote:
> Initialize |internal_state_| in initializer section.

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
File content/child/indexed_db/indexed_db_callbacks_impl.h (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.h:28: //
indexed_db::mojom::Callbacks implementation
On 2016/10/11 at 18:30:54, cmumford wrote:
> nit: end comment with ':'

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.h:48:
scoped_refptr<base::SingleThreadTaskRunner> callback_runner_;
On 2016/10/11 at 18:30:54, cmumford wrote:
> DISALLOW_COPY_AND_ASSIGN(IndexedDBCallbacksImpl);

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
File content/child/indexed_db/indexed_db_database_callbacks_impl.h (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_database_callbacks_impl.h:33:
blink::WebIDBDatabaseCallbacks* callbacks_;
On 2016/10/11 at 18:30:54, cmumford wrote:
> DISALLOW_COPY_AND_ASSIGN(IndexedDBDatabaseCallbacksImpl);

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
File content/child/indexed_db/indexed_db_message_filter.cc (left):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/indexed_db_message_filter.cc:38: void
IndexedDBMessageFilter::OnStaleMessageReceived(const IPC::Message& msg) {
On 2016/10/11 at 18:30:54, cmumford wrote:
> Can also no longer include "indexed_db_constants.h"

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/webidbfactory_impl.cc:55: FactoryAssociatedPtr
service_;
On 2016/10/11 at 18:30:54, cmumford wrote:
> DISALLOW_COPY_AND_ASSIGN(IOThreadHelper);

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/webidbfactory_impl.cc:64: helper_ = new
IOThreadHelper(std::move(sync_message_filter));
On 2016/10/11 at 18:30:54, cmumford wrote:
> set |helper_| in class initialization section.

Done.

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_...
content/child/indexed_db/webidbfactory_impl.cc:94: WorkerThread::GetCurrentId(),
base::string16(name), version,
On 2016/10/11 at 18:30:54, cmumford wrote:
> no need to explicitly typecase to base::string16.

I do because I want the conversion and copy to happen here instead of when the
callback is run on the IO thread. WebString is not thread-safe.

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/associated_interface_ptr_info.h (right):

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/associated_interface_ptr_info.h:23:
AssociatedInterfacePtrInfo(decltype(nullptr)) : version_(0u) {}
On 2016/10/11 at 18:30:54, cmumford wrote:
> Should use std::nullptr_t here.
> 
> However, I believe this constructor is best avoided entirely as nullptr_t is
really used to resolve ambiguous overloaded functions accepting pointers - and
there are no other constructors taking pointers.
> 
> Instead you could pass:
> 
>   ::indexed_db::mojom::DatabaseCallbacksAssociatedPtrInfo()
> 
> as the third parameter to the IndexedDBDatabaseCallbacks constructor to
explicitly create that object.

The goal is to make AssociatedPtrInfo behave identically to InterfacePtr in that
"nullptr" means an invalid one. The alternative seems very verbose.

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/strong_associated_binding.h (right):

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/strong_associated_binding.h:98: else if
(!connection_error_with_reason_handler_.is_null())
On 2016/10/11 at 18:30:54, cmumford wrote:
> I'm assuming that only one type of error handler will ever be set?

Yes.

https://codereview.chromium.org/2370643004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:91:
std::find_if(callbacks.begin(), callbacks.end(),
On 2016/10/11 at 18:30:54, cmumford wrote:
> Why can't you just do?
> 
> auto it = std::find(callbacks.begin(), callbacks.end(), this);
> if (it != callbacks.end())
>   ...

std::unique_ptr<T> is not comparable to T so I need the lambda to unwrap it.

https://codereview.chromium.org/2370643004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:180: if
(m_request) {
On 2016/10/11 at 18:30:54, cmumford wrote:
> Nit: I think the style guide prefers:
> 
>   if (!m_request)
>     return
>   ...
> 
> but I could be wrong...

I don't see anything in the style guide about this but I agree your suggestion
is a bit cleaner.

[commit-bot: I haz the power, 2016-10-11 23:47:04]

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-12 01:56:34]

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

[commit-bot: I haz the power, 2016-10-12 01:56:35]

Dry run: This issue passed the CQ dry run.

[cmumford, 2016-10-12 17:18:15]

lgtm

[Reilly Grant (use Gerrit), 2016-10-12 17:54:05]

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

[Reilly Grant (use Gerrit), 2016-10-12 17:54:06]

Asking meacer@ for an IPC security review because dcheng@ is backlogged.

[dcheng, 2016-10-12 18:25:00]

On 2016/10/12 17:54:06, Reilly Grant wrote:
> Asking meacer@ for an IPC security review because dcheng@ is backlogged.

I'm actually already reviewing this =P

[meacer, 2016-10-12 18:45:33]

On 2016/10/12 18:25:00, dcheng wrote:
> On 2016/10/12 17:54:06, Reilly Grant wrote:
> > Asking meacer@ for an IPC security review because dcheng@ is backlogged.
> 
> I'm actually already reviewing this =P

Heh okay. Looked fine to me, but would be nice to get a second opinion from you
:)

[Reilly Grant (use Gerrit), 2016-10-14 01:27:56]

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

[commit-bot: I haz the power, 2016-10-14 01:28:19]

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-10-14 01:28:43]

dcheng@ and rocket@, I've added StructTraits for IndexedDBDatabaseMetadata and
friends. Please take another look.

[commit-bot: I haz the power, 2016-10-14 03:08:34]

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

[commit-bot: I haz the power, 2016-10-14 03:08:36]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-10-14 21:50:47]

https://codereview.chromium.org/2370643004/diff/160001/content/common/indexed...
File content/common/indexed_db/indexed_db_struct_traits.h (right):

https://codereview.chromium.org/2370643004/diff/160001/content/common/indexed...
content/common/indexed_db/indexed_db_struct_traits.h:128: if
(!StructTraits<indexed_db::mojom::ObjectStoreMetadataDataView,
(no action required)

This is a little verbose. Maybe we should improve this.

https://codereview.chromium.org/2370643004/diff/160001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/array_traits_stl.h (right):

https://codereview.chromium.org/2370643004/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/array_traits_stl.h:92: struct ArrayTraits<std::map<K,
V>> {
I think this is probably a bit confusing to do. I would create a cheap
intermediate type like mojo::MapValueArrayView<T>, and then create the
ArrayTraits over that. Then your StructTraits have to explicitly return a
MapValueArrayView. Otherwise it's easy to accidentally flatten a map into an
array.

[Reilly Grant (use Gerrit), 2016-10-15 00:13:42]

https://codereview.chromium.org/2370643004/diff/160001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/array_traits_stl.h (right):

https://codereview.chromium.org/2370643004/diff/160001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/array_traits_stl.h:92: struct ArrayTraits<std::map<K,
V>> {
On 2016/10/14 at 21:50:47, Ken Rockot wrote:
> I think this is probably a bit confusing to do. I would create a cheap
intermediate type like mojo::MapValueArrayView<T>, and then create the
ArrayTraits over that. Then your StructTraits have to explicitly return a
MapValueArrayView. Otherwise it's easy to accidentally flatten a map into an
array.

Done. Let me know if there's a way to make usage of this wrapper even less
verbose.

[Reilly Grant (use Gerrit), 2016-10-15 00:14:01]

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

[commit-bot: I haz the power, 2016-10-15 00:14:17]

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-15 01:11:15]

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

[commit-bot: I haz the power, 2016-10-15 01:11:16]

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

[dcheng, 2016-10-17 05:33:42]

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

[dcheng, 2016-10-17 05:33:45]

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:38: 
Nit: be consistent with newlines surrounding the contents of the namespace block

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:45:
IOThreadHelper(CallbacksAssociatedPtrInfo callbacks_info);
Nit: explicit

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:105: if
(callbacks_info.is_valid())
What are the circumstances where this wouldn't be set? I would expect this ctor
to be used only when handling mojo calls, and it seems like we should always be
using IOHelper in that case?

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:611: dispatcher_host_ = NULL;
Nit: nullptr

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.h:150: IOThreadHelper*
io_helper_;
Can this be std::unique_ptr<IOThreadHelper, BrowserThread::DeleteOnIOThread>?

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
File content/common/indexed_db/indexed_db.mojom (right):

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
content/common/indexed_db/indexed_db.mojom:63: interface DatabaseCallbacks {
Mind documenting the Callbacks and DatabaseCallbacks interfaces a bit? Callbacks
and DatabaseCallbacks are pretty generic names. I'm not sure if |Callbacks| and
|DatabaseCallbacks| correspond to something in the IndexedDB spec, but if they
do, it would be nice to have a pointer here as well.

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
File content/common/indexed_db/indexed_db_struct_traits.h (right):

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
content/common/indexed_db/indexed_db_struct_traits.h:17: return metadata.id;
Does it make sense to DCHECK(metadata.id >= 0) here and below?

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
content/common/indexed_db/indexed_db_struct_traits.h:33:
content::IndexedDBIndexMetadata* out) {
Nit: please out-of-line non-trivial methods.

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
content/common/indexed_db/indexed_db_struct_traits.h:84: indexes.GetDataView(i,
&index);
Should this check that 0 <= index.id() < indexes.size() (similar question below)

https://codereview.chromium.org/2370643004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/associated_interface_ptr_info.h (right):

https://codereview.chromium.org/2370643004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/associated_interface_ptr_info.h:23:
AssociatedInterfacePtrInfo(decltype(nullptr)) : version_(0u) {}
Nit: nullptr_t

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:62: using
CallbacksList = std::vector<std::unique_ptr<WebIDBCallbacksImpl>>;
Inside Blink, we should probably be using WTF::Vector.

[Ken Rockot(use gerrit already), 2016-10-18 16:04:59]

new traits changes lgtm % dcheng's comments

[haraken, 2016-10-18 19:01:27]

haraken@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org

[haraken, 2016-10-18 19:01:29]

+Elliott

https://chromiumcodereview.appspot.com/2370643004/diff/180001/third_party/Web...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right):

https://chromiumcodereview.appspot.com/2370643004/diff/180001/third_party/Web...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:146: m_webCallbacks =
nullptr;

Don't we need to call m_webCallbacks->detach()?

Just to confirm: Why do we need to call webCallbacksDestroyed() when the context
gets detached? I just want to confirm that contextDestroyed() is a right timing
to call webCallbacksDestroyed().

https://chromiumcodereview.appspot.com/2370643004/diff/180001/third_party/Web...
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://chromiumcodereview.appspot.com/2370643004/diff/180001/third_party/Web...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:63: static
ThreadSpecific<CallbacksList>& outstandingCallbacks() {

It's not really nice to use ThreadSpecific to keep track of per-worker data
structures... Would there be any per-worker class that can be used to track the
callback vector?

If there's no alternative, I'm okay with ThreadSpecific though.

https://chromiumcodereview.appspot.com/2370643004/diff/180001/third_party/Web...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:91:
std::find_if(callbacks.begin(), callbacks.end(),

To avoid the O(N) search, would it be better to use HashSet?

Nit: I'd prefer not using an advanced C++ feature like a lambda function.

https://chromiumcodereview.appspot.com/2370643004/diff/180001/third_party/Web...
File third_party/WebKit/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.cpp
(right):

https://chromiumcodereview.appspot.com/2370643004/diff/180001/third_party/Web...
third_party/WebKit/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.cpp:58:
WebIDBDatabaseCallbacksImpl::~WebIDBDatabaseCallbacksImpl() {

Maybe we want to share the code with WebIDBCallbacksImpl.

[Reilly Grant (use Gerrit), 2016-10-19 00:36:51]

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:38: 
On 2016/10/17 at 05:33:44, dcheng wrote:
> Nit: be consistent with newlines surrounding the contents of the namespace
block

clang-format keeps eating my newlines. I need to figure out why.

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:45:
IOThreadHelper(CallbacksAssociatedPtrInfo callbacks_info);
On 2016/10/17 at 05:33:44, dcheng wrote:
> Nit: explicit

Done.

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:105: if
(callbacks_info.is_valid())
On 2016/10/17 at 05:33:44, dcheng wrote:
> What are the circumstances where this wouldn't be set? I would expect this
ctor to be used only when handling mojo calls, and it seems like we should
always be using IOHelper in that case?

|callbacks_info| is invalid in tests, where this constructor is called from the
MockIndexedDBCallbacks constructor.

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:611: dispatcher_host_ = NULL;
On 2016/10/17 at 05:33:44, dcheng wrote:
> Nit: nullptr

Done.

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.h:150: IOThreadHelper*
io_helper_;
On 2016/10/17 at 05:33:44, dcheng wrote:
> Can this be std::unique_ptr<IOThreadHelper, BrowserThread::DeleteOnIOThread>?

No, because tests initialize this class and don't always have an IO thread. By
only calling BrowserThread::PostTask(IO) when there's actually an |io_helper_|
to destroy I avoid requiring an IO thread in tests.

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
File content/common/indexed_db/indexed_db.mojom (right):

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
content/common/indexed_db/indexed_db.mojom:63: interface DatabaseCallbacks {
On 2016/10/17 at 05:33:44, dcheng wrote:
> Mind documenting the Callbacks and DatabaseCallbacks interfaces a bit?
Callbacks and DatabaseCallbacks are pretty generic names. I'm not sure if
|Callbacks| and |DatabaseCallbacks| correspond to something in the IndexedDB
spec, but if they do, it would be nice to have a pointer here as well.

The fact that these are just two large interfaces is more of a quirk of how
IndexedDB was implemented on top of legacy IPC. I've added top-level comments to
the interfaces and a TODO to try to convert as many messages as possible to use
Mojo replies instead of taking a generic Callbacks interface.

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
File content/common/indexed_db/indexed_db_struct_traits.h (right):

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
content/common/indexed_db/indexed_db_struct_traits.h:17: return metadata.id;
On 2016/10/17 at 05:33:44, dcheng wrote:
> Does it make sense to DCHECK(metadata.id >= 0) here and below?

These are dictionary keys. I don't know if negative values are valid.

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
content/common/indexed_db/indexed_db_struct_traits.h:33:
content::IndexedDBIndexMetadata* out) {
On 2016/10/17 at 05:33:44, dcheng wrote:
> Nit: please out-of-line non-trivial methods.

Done.

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
content/common/indexed_db/indexed_db_struct_traits.h:84: indexes.GetDataView(i,
&index);
On 2016/10/17 at 05:33:44, dcheng wrote:
> Should this check that 0 <= index.id() < indexes.size() (similar question
below)

|indexes| is a map so index.id() is allowed to have any value. It might be worth
DCHECKing that index.id() is not already one of the values in the map.

https://codereview.chromium.org/2370643004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/associated_interface_ptr_info.h (right):

https://codereview.chromium.org/2370643004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/associated_interface_ptr_info.h:23:
AssociatedInterfacePtrInfo(decltype(nullptr)) : version_(0u) {}
On 2016/10/17 at 05:33:44, dcheng wrote:
> Nit: nullptr_t

Done.

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:146: m_webCallbacks =
nullptr;
On 2016/10/18 at 19:01:29, haraken wrote:
> Don't we need to call m_webCallbacks->detach()?

m_webCallbacks->detach() just clears the callback object's pointer back to this
object. Since we know the callback object is being destroyed here there's no
reason to tell it to clear a pointer that's about to be cleared anyways.

> Just to confirm: Why do we need to call webCallbacksDestroyed() when the
context gets detached? I just want to confirm that contextDestroyed() is a right
timing to call webCallbacksDestroyed().

We call IDBRequest::webCallbacksDestroyed() so that IDBRequest doesn't call
WebIDBCallbacks::detach() after the WebIDBCallbacks has been destroyed. The
detach is the important part because it prevents IDBRequest from getting calls
after the execution context been destroyed because existing code in IDBRequest
assumes that getExecutionContext() will always return non-NULL. The call to
webCallbacksDestroyed() exists to prevent a use after free in the other
direction since these two objects can be destroyed in either order.

Note that all of this mess can go away when IDBRequest owns the
mojo::Binding<Callbacks> that WebIDBCallbacksImpl currently does and calls
Binding::reset() in both contextDestroyed() and its pre-finalizer.

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:63: static
ThreadSpecific<CallbacksList>& outstandingCallbacks() {
On 2016/10/18 at 19:01:29, haraken wrote:
> It's not really nice to use ThreadSpecific to keep track of per-worker data
structures... Would there be any per-worker class that can be used to track the
callback vector?
> 
> If there's no alternative, I'm okay with ThreadSpecific though.

The only per-worker object I know of that could hold these is
IndexedDBDispatcher but that isn't accessible from Blink and the point of this
patch series is to remove IndexedDBDispatcher. That said, I think I could make
that work if that is what you prefer.

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:91:
std::find_if(callbacks.begin(), callbacks.end(),
On 2016/10/18 at 19:01:29, haraken wrote:
> To avoid the O(N) search, would it be better to use HashSet?

The number of outstanding requests is expected to be small so I wanted to avoid
the overhead of the more complex data structure.

> Nit: I'd prefer not using an advanced C++ feature like a lambda function.

This usage of lambda functions is permitted by the style guide.

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.cpp:58:
WebIDBDatabaseCallbacksImpl::~WebIDBDatabaseCallbacksImpl() {
On 2016/10/18 at 19:01:29, haraken wrote:
> Maybe we want to share the code with WebIDBCallbacksImpl.

I've moved this logic into IndexedDBDispatcher where it is a little less
verbose.

[dcheng, 2016-10-19 03:55:29]

It looks like the latest snapshot didn't upload thanks to the SSL woes, but some
preliminary replies.

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.h:150: IOThreadHelper*
io_helper_;
On 2016/10/19 00:36:51, Reilly Grant wrote:
> On 2016/10/17 at 05:33:44, dcheng wrote:
> > Can this be std::unique_ptr<IOThreadHelper,
BrowserThread::DeleteOnIOThread>?
> 
> No, because tests initialize this class and don't always have an IO thread. By
> only calling BrowserThread::PostTask(IO) when there's actually an |io_helper_|
> to destroy I avoid requiring an IO thread in tests.

Can we use TestBrowserThreadBundle in the tests? In general, I'd prefer the
complexity be in the tests rather than in the impl, since that makes it harder
to understand what the invariants are in release code.

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
File content/common/indexed_db/indexed_db_struct_traits.h (right):

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
content/common/indexed_db/indexed_db_struct_traits.h:17: return metadata.id;
On 2016/10/19 00:36:51, Reilly Grant wrote:
> On 2016/10/17 at 05:33:44, dcheng wrote:
> > Does it make sense to DCHECK(metadata.id >= 0) here and below?
> 
> These are dictionary keys. I don't know if negative values are valid.

Ah, I saw kInvalidIndex = -1 in IndexedDBIndexMetadata's declaration, and for
some reason, I thought this would be sequential. Sorry!

This is fine: it's no different than what the current IPC serialization is
doing. Though (as a followup) it might be interesting to consider what happens
if we plumb through -1 as the id through the rest of the system =)

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed...
content/common/indexed_db/indexed_db_struct_traits.h:84: indexes.GetDataView(i,
&index);
On 2016/10/19 00:36:51, Reilly Grant wrote:
> On 2016/10/17 at 05:33:44, dcheng wrote:
> > Should this check that 0 <= index.id() < indexes.size() (similar question
> below)
> 
> |indexes| is a map so index.id() is allowed to have any value. It might be
worth
> DCHECKing that index.id() is not already one of the values in the map.

I agree the current code is fine (for some reason, I was thinking that the
values would be contiguous, which is obviously not required to be true)

[Reilly Grant (use Gerrit), 2016-10-19 06:01:24]

Successfully uploaded a new version now.

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.h:150: IOThreadHelper*
io_helper_;
On 2016/10/19 at 03:55:29, dcheng wrote:
> On 2016/10/19 00:36:51, Reilly Grant wrote:
> > On 2016/10/17 at 05:33:44, dcheng wrote:
> > > Can this be std::unique_ptr<IOThreadHelper,
BrowserThread::DeleteOnIOThread>?
> > 
> > No, because tests initialize this class and don't always have an IO thread.
By
> > only calling BrowserThread::PostTask(IO) when there's actually an
|io_helper_|
> > to destroy I avoid requiring an IO thread in tests.
> 
> Can we use TestBrowserThreadBundle in the tests? In general, I'd prefer the
complexity be in the tests rather than in the impl, since that makes it harder
to understand what the invariants are in release code.

I'll have that a try.

[Reilly Grant (use Gerrit), 2016-10-19 06:01:35]

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

[commit-bot: I haz the power, 2016-10-19 06:02:57]

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-19 10:08:32]

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

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

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

[haraken, 2016-10-19 10:14:29]

WebKit LGTM

[Reilly Grant (use Gerrit), 2016-10-19 18:32:57]

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

[Reilly Grant (use Gerrit), 2016-10-19 18:33:14]

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.h:150: IOThreadHelper*
io_helper_;
On 2016/10/19 at 06:01:24, Reilly Grant wrote:
> On 2016/10/19 at 03:55:29, dcheng wrote:
> > On 2016/10/19 00:36:51, Reilly Grant wrote:
> > > On 2016/10/17 at 05:33:44, dcheng wrote:
> > > > Can this be std::unique_ptr<IOThreadHelper,
BrowserThread::DeleteOnIOThread>?
> > > 
> > > No, because tests initialize this class and don't always have an IO
thread. By
> > > only calling BrowserThread::PostTask(IO) when there's actually an
|io_helper_|
> > > to destroy I avoid requiring an IO thread in tests.
> > 
> > Can we use TestBrowserThreadBundle in the tests? In general, I'd prefer the
complexity be in the tests rather than in the impl, since that makes it harder
to understand what the invariants are in release code.
> 
> I'll have that a try.

Done.

[commit-bot: I haz the power, 2016-10-19 18:33:38]

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-19 20:42:26]

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

[commit-bot: I haz the power, 2016-10-19 20:42:28]

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

[dcheng, 2016-10-19 23:44:03]

mojo lgtm

https://codereview.chromium.org/2370643004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/2370643004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_unittest.cc:116:
content::TestBrowserThreadBundle thread_bundle;
Not necessary in this CL, but having a test fixture might reduce the amount of
duplication needed.

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_...
File content/child/indexed_db/indexed_db_callbacks_impl.h (right):

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.h:26:
InternalState(blink::WebIDBCallbacks* callbacks,
Nit: make this a std::unique_ptr if it's not too viral

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_...
File content/child/indexed_db/indexed_db_database_callbacks_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_...
content/child/indexed_db/indexed_db_database_callbacks_impl.cc:43:
base::Bind(&DeleteDatabaseCallbacks, callbacks_, thread_safe_sender_));
Nit: If thread_safe_sender is wrapped with base::RetainedRef, then
DeleteDatabaseCallbacks can just take a raw pointer (and save a ref / unref)

[Reilly Grant (use Gerrit), 2016-10-20 01:21:49]

Don't create IOThreadHelper in tests because it leaks.

[Reilly Grant (use Gerrit), 2016-10-20 01:26:51]

Patchset #13 (id:230001) has been deleted

[Reilly Grant (use Gerrit), 2016-10-20 01:46:18]

Ignore comment #82.

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_...
File content/child/indexed_db/indexed_db_callbacks_impl.h (right):

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.h:26:
InternalState(blink::WebIDBCallbacks* callbacks,
On 2016/10/19 at 23:44:02, dcheng wrote:
> Nit: make this a std::unique_ptr if it's not too viral

Done.

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_...
File content/child/indexed_db/indexed_db_database_callbacks_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_...
content/child/indexed_db/indexed_db_database_callbacks_impl.cc:43:
base::Bind(&DeleteDatabaseCallbacks, callbacks_, thread_safe_sender_));
On 2016/10/19 at 23:44:02, dcheng wrote:
> Nit: If thread_safe_sender is wrapped with base::RetainedRef, then
DeleteDatabaseCallbacks can just take a raw pointer (and save a ref / unref)

Done.

[Reilly Grant (use Gerrit), 2016-10-20 01:46:32]

Address last nits and fix leaks in unit tests.

[Reilly Grant (use Gerrit), 2016-10-20 01:46:51]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-10-20 01:46:52]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
cmumford@chromium.org, rockot@chromium.org, haraken@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2370643004/#ps250001
(title: "Address last nits and fix leaks in unit tests.")

[commit-bot: I haz the power, 2016-10-20 01:47:34]

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-20 05:13:08]

Committed patchset #13 (id:250001)

[commit-bot: I haz the power, 2016-10-21 13:15:56]

Description was changed from

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

This is the first of a series of patches that convert the IPC messages
sent by WebIDBFactoryImpl, WebIDBDatabaseImpl and WebIDBCursorImpl to
Mojo messages.

Once all of the IPC sent by IndexedDB and Blob storage are converted to
Mojo they can be moved off of channel-associated interfaces and more
agressive removal of unnecessary layers of abstraction (including
removal of all of the manual thread hopping I added) can be done.

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

to

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

This is the first of a series of patches that convert the IPC messages
sent by WebIDBFactoryImpl, WebIDBDatabaseImpl and WebIDBCursorImpl to
Mojo messages.

Once all of the IPC sent by IndexedDB and Blob storage are converted to
Mojo they can be moved off of channel-associated interfaces and more
agressive removal of unnecessary layers of abstraction (including
removal of all of the manual thread hopping I added) can be done.

BUG=627484

Committed: https://crrev.com/627e7f73d47910f4255031de55075e2a04b222f6
Cr-Commit-Position: refs/heads/master@{#426412}
==========

[commit-bot: I haz the power, 2016-10-21 13:15:58]

Patchset 13 (id:??) landed as
https://crrev.com/627e7f73d47910f4255031de55075e2a04b222f6
Cr-Commit-Position: refs/heads/master@{#426412}