Port messages sent by WebIDBFactoryImpl to Mojo. (issue 2370643004 by reillyg@chromium.org)

9 views
Skip to first unread message

rei...@chromium.org

unread,
Sep 26, 2016, 6:41:54 AM9/26/16
to cmum...@chromium.org, roc...@chromium.org, dcheng+...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, j...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Reviewers: cmumford, Ken Rockot, dcheng
CL: https://codereview.chromium.org/2370643004/

Message:
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.

Description:
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

Affected files (+1304, -734 lines):
M content/browser/indexed_db/indexed_db_callbacks.h
M content/browser/indexed_db/indexed_db_callbacks.cc
M content/browser/indexed_db/indexed_db_database_callbacks.h
M content/browser/indexed_db/indexed_db_database_callbacks.cc
M content/browser/indexed_db/indexed_db_dispatcher_host.h
M content/browser/indexed_db/indexed_db_dispatcher_host.cc
M content/browser/indexed_db/indexed_db_factory_unittest.cc
M content/browser/indexed_db/mock_indexed_db_database_callbacks.cc
M content/child/BUILD.gn
A content/child/indexed_db/indexed_db_callbacks_impl.h
A content/child/indexed_db/indexed_db_callbacks_impl.cc
A content/child/indexed_db/indexed_db_database_callbacks_impl.h
A content/child/indexed_db/indexed_db_database_callbacks_impl.cc
M content/child/indexed_db/indexed_db_dispatcher.h
M content/child/indexed_db/indexed_db_dispatcher.cc
M content/child/indexed_db/indexed_db_message_filter.h
M content/child/indexed_db/indexed_db_message_filter.cc
M content/child/indexed_db/mock_webidbcallbacks.h
M content/child/indexed_db/webidbdatabase_impl.h
M content/child/indexed_db/webidbdatabase_impl.cc
M content/child/indexed_db/webidbfactory_impl.h
M content/child/indexed_db/webidbfactory_impl.cc
M content/common/BUILD.gn
M content/common/indexed_db/OWNERS
A content/common/indexed_db/indexed_db.mojom
A content/common/indexed_db/indexed_db.typemap
M content/common/indexed_db/indexed_db_messages.h
A + content/common/indexed_db/typemaps.gni
M content/renderer/renderer_blink_platform_impl.cc
M ipc/ipc_sync_message_filter.h
M ipc/ipc_sync_message_filter.cc
M mojo/public/cpp/bindings/associated_interface_ptr_info.h
M mojo/public/tools/bindings/chromium_bindings_configuration.gni
M third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp
M third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp
M third_party/WebKit/Source/modules/indexeddb/IDBDatabaseCallbacks.h
M third_party/WebKit/Source/modules/indexeddb/IDBDatabaseCallbacks.cpp
M third_party/WebKit/Source/modules/indexeddb/IDBFactory.cpp
M third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp
M third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
M third_party/WebKit/Source/modules/indexeddb/IDBOpenDBRequest.h
M third_party/WebKit/Source/modules/indexeddb/IDBOpenDBRequest.cpp
M third_party/WebKit/Source/modules/indexeddb/IDBRequest.h
M third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp
M third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.h
M third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
M third_party/WebKit/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.h
M third_party/WebKit/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.cpp
M third_party/WebKit/public/platform/modules/indexeddb/WebIDBCallbacks.h
M third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabaseCallbacks.h


rei...@chromium.org

unread,
Sep 27, 2016, 5:30:54 AM9/27/16
to cmum...@chromium.org, dcheng+...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, j...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
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/

Ken Rockot

unread,
Sep 27, 2016, 11:14:39 AM9/27/16
to Reilly Grant, Chris Mumford, dcheng+...@chromium.org, Ken Rockot, Aaron Boodman, Adam Barth, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, Darin Fisher, Darin Fisher, dglazko...@chromium.org, Kentaro Hara, John Abd-El-Malek, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
If you have any dependencies on native typemaps you can specify cpp_only = true in your mojom rule.

rei...@chromium.org

unread,
Sep 27, 2016, 10:26:50 PM9/27/16
to cmum...@chromium.org, dcheng+...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, j...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
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.

roc...@chromium.org

unread,
Sep 28, 2016, 4:06:45 PM9/28/16
to rei...@chromium.org, cmum...@chromium.org, dcheng+...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, j...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
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_db/indexed_db_database_callbacks_impl.h
File content/child/indexed_db/indexed_db_database_callbacks_impl.h
(right):

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_db/indexed_db_database_callbacks_impl.h#newcode24
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_db/webidbfactory_impl.cc
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_db/webidbfactory_impl.cc#newcode108
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_db/indexed_db.mojom
File content/common/indexed_db/indexed_db.mojom (right):

https://codereview.chromium.org/2370643004/diff/40001/content/common/indexed_db/indexed_db.mojom#newcode47
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/bindings/mojom.gni
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/2370643004/diff/40001/mojo/public/tools/bindings/mojom.gni#newcode212
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?

https://codereview.chromium.org/2370643004/

rei...@chromium.org

unread,
Sep 29, 2016, 2:44:51 AM9/29/16
to cmum...@chromium.org, dcheng+...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, j...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

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

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_db/indexed_db_database_callbacks_impl.h#newcode24
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_db/webidbfactory_impl.cc
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_db/webidbfactory_impl.cc#newcode108
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.
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.
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?

roc...@chromium.org

unread,
Sep 29, 2016, 6:37:50 PM9/29/16
to rei...@chromium.org, cmum...@chromium.org, dcheng+...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, j...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
LGTM with a little optional refactoring



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

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_db/indexed_db_database_callbacks_impl.h#newcode24
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_db/webidbfactory_impl.cc
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_db/webidbfactory_impl.cc#newcode108
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?
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.

https://codereview.chromium.org/2370643004/

rei...@chromium.org

unread,
Sep 30, 2016, 3:24:21 AM9/30/16
to cmum...@chromium.org, dcheng+...@chromium.org, roc...@chromium.org, j...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, j...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
jam@, can you give me a top-level //content review?



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

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_db/indexed_db_database_callbacks_impl.h#newcode24
content/child/indexed_db/indexed_db_database_callbacks_impl.h:24: void
Bind(indexed_db::mojom::DatabaseCallbacksAssociatedPtrInfo* ptr_info,
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_db/webidbfactory_impl.cc
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/40001/content/child/indexed_db/webidbfactory_impl.cc#newcode108
content/child/indexed_db/webidbfactory_impl.cc:108: void
WebIDBFactoryImpl::IOThreadHelper::EnsureServiceConnection() {
Oh look! Now there's a StrongAssociatedBinding.

https://codereview.chromium.org/2370643004/

j...@chromium.org

unread,
Sep 30, 2016, 12:35:25 PM9/30/16
to rei...@chromium.org, cmum...@chromium.org, dcheng+...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
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)


https://codereview.chromium.org/2370643004/

dch...@chromium.org

unread,
Oct 5, 2016, 4:00:03 AM10/5/16
to rei...@chromium.org, cmum...@chromium.org, roc...@chromium.org, j...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Sorry for the review latency =(


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

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexed_db/indexed_db_callbacks.cc#newcode43
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/indexed_db/indexed_db_callbacks.cc#newcode705
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/indexed_db/indexed_db_dispatcher_host.cc
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexed_db/indexed_db_dispatcher_host.cc#newcode50
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_filter.h
File ipc/ipc_sync_message_filter.h (right):

https://codereview.chromium.org/2370643004/diff/100001/ipc/ipc_sync_message_filter.h#newcode61
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/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/100001/third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp#newcode62
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)

https://codereview.chromium.org/2370643004/

rei...@chromium.org

unread,
Oct 5, 2016, 5:16:26 AM10/5/16
to cmum...@chromium.org, dcheng+...@chromium.org, j...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

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

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexed_db/indexed_db_callbacks.cc#newcode43
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/indexed_db/indexed_db_callbacks.cc#newcode705
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/indexed_db/indexed_db_dispatcher_host.cc
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2370643004/diff/100001/content/browser/indexed_db/indexed_db_dispatcher_host.cc#newcode50
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.
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/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/100001/third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp#newcode62
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)

Ken Rockot

unread,
Oct 5, 2016, 9:16:07 AM10/5/16
to jsbel...@chromium.org, blink-re...@chromium.org, cmum...@chromium.org, chromium...@chromium.org, j...@chromium.org, a...@chromium.org, rei...@chromium.org, viettrung...@chromium.org, da...@chromium.org, dglazko...@chromium.org, mlamouri+wa...@chromium.org, dcheng+...@chromium.org, dari...@chromium.org, aba...@chromium.org, blink-...@chromium.org, yzshen...@chromium.org, har...@chromium.org, qsr+...@chromium.org

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

I'll go ahead and get that landed.

roc...@chromium.org

unread,
Oct 5, 2016, 1:57:15 PM10/5/16
to rei...@chromium.org, cmum...@chromium.org, dcheng+...@chromium.org, j...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
FYI the SyncMessageFilter change has landed now.

https://codereview.chromium.org/2370643004/

cmum...@chromium.org

unread,
Oct 11, 2016, 2:30:54 PM10/11/16
to rei...@chromium.org, roc...@chromium.org, dcheng+...@chromium.org, j...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Looks great - all minor nit comments and a few questions.


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

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexed_db/indexed_db_callbacks.cc#newcode42
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/indexed_db/indexed_db_callbacks.cc#newcode88
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/indexed_db/indexed_db_callbacks.cc#newcode95
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/indexed_db/indexed_db_callbacks.cc#newcode137
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/indexed_db/indexed_db_callbacks.h
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexed_db/indexed_db_callbacks.h#newcode150
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/indexed_db/indexed_db_database_callbacks.cc
File content/browser/indexed_db/indexed_db_database_callbacks.cc
(right):

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

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexed_db/indexed_db_database_callbacks.cc#newcode27
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/indexed_db/indexed_db_database_callbacks.cc#newcode50
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_db/indexed_db_callbacks_impl.cc
File content/child/indexed_db/indexed_db_callbacks_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_db/indexed_db_callbacks_impl.cc#newcode89
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_db/indexed_db_callbacks_impl.cc#newcode96
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_db/indexed_db_callbacks_impl.h
File content/child/indexed_db/indexed_db_callbacks_impl.h (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_db/indexed_db_callbacks_impl.h#newcode28
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_db/indexed_db_callbacks_impl.h#newcode48
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_db/indexed_db_database_callbacks_impl.h
File content/child/indexed_db/indexed_db_database_callbacks_impl.h
(right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_db/indexed_db_database_callbacks_impl.h#newcode33
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_db/indexed_db_message_filter.cc
File content/child/indexed_db/indexed_db_message_filter.cc (left):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_db/indexed_db_message_filter.cc#oldcode38
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_db/webidbfactory_impl.cc
File content/child/indexed_db/webidbfactory_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_db/webidbfactory_impl.cc#newcode55
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_db/webidbfactory_impl.cc#newcode64
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_db/webidbfactory_impl.cc#newcode94
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/bindings/associated_interface_ptr_info.h
File mojo/public/cpp/bindings/associated_interface_ptr_info.h (right):

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindings/associated_interface_ptr_info.h#newcode23
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/bindings/strong_associated_binding.h
File mojo/public/cpp/bindings/strong_associated_binding.h (right):

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindings/strong_associated_binding.h#newcode98
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/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/120001/third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp#newcode91
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/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp#newcode180
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...

https://codereview.chromium.org/2370643004/

rei...@chromium.org

unread,
Oct 11, 2016, 7:46:13 PM10/11/16
to cmum...@chromium.org, dcheng+...@chromium.org, j...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

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

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexed_db/indexed_db_callbacks.cc#newcode42
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/indexed_db/indexed_db_callbacks.cc#newcode88
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/indexed_db/indexed_db_callbacks.cc#newcode95
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/indexed_db/indexed_db_callbacks.cc#newcode137
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.
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/indexed_db/indexed_db_database_callbacks.cc
File content/browser/indexed_db/indexed_db_database_callbacks.cc
(right):

https://codereview.chromium.org/2370643004/diff/120001/content/browser/indexed_db/indexed_db_database_callbacks.cc#newcode18
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/indexed_db/indexed_db_database_callbacks.cc#newcode27
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/indexed_db/indexed_db_database_callbacks.cc#newcode50
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_db/indexed_db_callbacks_impl.cc
File content/child/indexed_db/indexed_db_callbacks_impl.cc (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_db/indexed_db_callbacks_impl.cc#newcode89
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_db/indexed_db_callbacks_impl.cc#newcode96
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_db/indexed_db_callbacks_impl.h
File content/child/indexed_db/indexed_db_callbacks_impl.h (right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_db/indexed_db_callbacks_impl.h#newcode28
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_db/indexed_db_callbacks_impl.h#newcode48
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_db/indexed_db_database_callbacks_impl.h
File content/child/indexed_db/indexed_db_database_callbacks_impl.h
(right):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_db/indexed_db_database_callbacks_impl.h#newcode33
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_db/indexed_db_message_filter.cc
File content/child/indexed_db/indexed_db_message_filter.cc (left):

https://codereview.chromium.org/2370643004/diff/120001/content/child/indexed_db/indexed_db_message_filter.cc#oldcode38
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.
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_db/webidbfactory_impl.cc#newcode64
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_db/webidbfactory_impl.cc#newcode94
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/bindings/associated_interface_ptr_info.h
File mojo/public/cpp/bindings/associated_interface_ptr_info.h (right):

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindings/associated_interface_ptr_info.h#newcode23
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/bindings/strong_associated_binding.h
File mojo/public/cpp/bindings/strong_associated_binding.h (right):

https://codereview.chromium.org/2370643004/diff/120001/mojo/public/cpp/bindings/strong_associated_binding.h#newcode98
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/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/120001/third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp#newcode91
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.
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.

https://codereview.chromium.org/2370643004/

cmum...@chromium.org

unread,
Oct 12, 2016, 1:18:16 PM10/12/16
to rei...@chromium.org, dcheng+...@chromium.org, j...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

rei...@chromium.org

unread,
Oct 12, 2016, 1:54:07 PM10/12/16
to cmum...@chromium.org, mea...@chromium.org, j...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dche...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Asking meacer@ for an IPC security review because dcheng@ is backlogged.

https://codereview.chromium.org/2370643004/

dch...@chromium.org

unread,
Oct 12, 2016, 2:25:01 PM10/12/16
to rei...@chromium.org, cmum...@chromium.org, mea...@chromium.org, j...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
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

https://codereview.chromium.org/2370643004/

mea...@chromium.org

unread,
Oct 12, 2016, 2:45:33 PM10/12/16
to rei...@chromium.org, cmum...@chromium.org, j...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
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
:)

https://codereview.chromium.org/2370643004/

rei...@chromium.org

unread,
Oct 13, 2016, 9:28:43 PM10/13/16
to cmum...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
dcheng@ and rocket@, I've added StructTraits for IndexedDBDatabaseMetadata and
friends. Please take another look.

https://codereview.chromium.org/2370643004/

roc...@chromium.org

unread,
Oct 14, 2016, 5:50:47 PM10/14/16
to rei...@chromium.org, cmum...@chromium.org, j...@chromium.org, mea...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

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

https://codereview.chromium.org/2370643004/diff/160001/content/common/indexed_db/indexed_db_struct_traits.h#newcode128
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/bindings/array_traits_stl.h
File mojo/public/cpp/bindings/array_traits_stl.h (right):

https://codereview.chromium.org/2370643004/diff/160001/mojo/public/cpp/bindings/array_traits_stl.h#newcode92
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.

https://codereview.chromium.org/2370643004/

rei...@chromium.org

unread,
Oct 14, 2016, 8:13:42 PM10/14/16
to cmum...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

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

https://codereview.chromium.org/2370643004/diff/160001/mojo/public/cpp/bindings/array_traits_stl.h#newcode92
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.

https://codereview.chromium.org/2370643004/

dch...@chromium.org

unread,
Oct 17, 2016, 1:33:44 AM10/17/16
to rei...@chromium.org, cmum...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

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

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexed_db/indexed_db_callbacks.cc#newcode38
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/indexed_db/indexed_db_callbacks.cc#newcode45
content/browser/indexed_db/indexed_db_callbacks.cc:45:
IOThreadHelper(CallbacksAssociatedPtrInfo callbacks_info);
Nit: explicit

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexed_db/indexed_db_callbacks.cc#newcode105
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/indexed_db/indexed_db_callbacks.cc#newcode611
content/browser/indexed_db/indexed_db_callbacks.cc:611: dispatcher_host_
= NULL;
Nit: nullptr

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

https://codereview.chromium.org/2370643004/diff/180001/content/browser/indexed_db/indexed_db_callbacks.h#newcode150
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_db/indexed_db.mojom
File content/common/indexed_db/indexed_db.mojom (right):

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed_db/indexed_db.mojom#newcode63
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_db/indexed_db_struct_traits.h
File content/common/indexed_db/indexed_db_struct_traits.h (right):

https://codereview.chromium.org/2370643004/diff/180001/content/common/indexed_db/indexed_db_struct_traits.h#newcode17
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_db/indexed_db_struct_traits.h#newcode33
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_db/indexed_db_struct_traits.h#newcode84
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/bindings/associated_interface_ptr_info.h
File mojo/public/cpp/bindings/associated_interface_ptr_info.h (right):

https://codereview.chromium.org/2370643004/diff/180001/mojo/public/cpp/bindings/associated_interface_ptr_info.h#newcode23
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/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp#newcode62
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.

https://codereview.chromium.org/2370643004/

roc...@chromium.org

unread,
Oct 18, 2016, 12:05:00 PM10/18/16
to rei...@chromium.org, cmum...@chromium.org, dcheng+...@chromium.org, j...@chromium.org, mea...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
new traits changes lgtm % dcheng's comments

https://codereview.chromium.org/2370643004/

har...@chromium.org

unread,
Oct 18, 2016, 3:01:29 PM10/18/16
to rei...@chromium.org, roc...@chromium.org, cmum...@chromium.org, dcheng+...@chromium.org, j...@chromium.org, mea...@chromium.org, esp...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
+Elliott



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

https://chromiumcodereview.appspot.com/2370643004/diff/180001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp#newcode146
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/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
(right):

https://chromiumcodereview.appspot.com/2370643004/diff/180001/third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp#newcode63
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/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp#newcode91

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/WebKit/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.cpp
File
third_party/WebKit/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.cpp
(right):

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

Maybe we want to share the code with WebIDBCallbacksImpl.

https://chromiumcodereview.appspot.com/2370643004/

rei...@chromium.org

unread,
Oct 18, 2016, 8:36:52 PM10/18/16
to cmum...@chromium.org, dcheng+...@chromium.org, esp...@chromium.org, har...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
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/indexed_db/indexed_db_callbacks.cc#newcode45
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/indexed_db/indexed_db_callbacks.cc#newcode105
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/indexed_db/indexed_db_callbacks.cc#newcode611
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.
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.
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.
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_db/indexed_db_struct_traits.h#newcode33
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_db/indexed_db_struct_traits.h#newcode84
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/bindings/associated_interface_ptr_info.h
File mojo/public/cpp/bindings/associated_interface_ptr_info.h (right):

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

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.

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/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp#newcode91

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/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.cpp
File
third_party/WebKit/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.cpp
(right):

https://codereview.chromium.org/2370643004/diff/180001/third_party/WebKit/Source/modules/indexeddb/WebIDBDatabaseCallbacksImpl.cpp#newcode58
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.

https://codereview.chromium.org/2370643004/

dch...@chromium.org

unread,
Oct 18, 2016, 11:55:29 PM10/18/16
to rei...@chromium.org, cmum...@chromium.org, esp...@chromium.org, har...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
It looks like the latest snapshot didn't upload thanks to the SSL woes, but some
preliminary replies.
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.
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_db/indexed_db_struct_traits.h#newcode84
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)

https://codereview.chromium.org/2370643004/

rei...@chromium.org

unread,
Oct 19, 2016, 2:01:25 AM10/19/16
to cmum...@chromium.org, dcheng+...@chromium.org, esp...@chromium.org, har...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, har...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Successfully uploaded a new version now.
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.

har...@chromium.org

unread,
Oct 19, 2016, 6:14:30 AM10/19/16
to rei...@chromium.org, cmum...@chromium.org, dcheng+...@chromium.org, esp...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

rei...@chromium.org

unread,
Oct 19, 2016, 2:33:16 PM10/19/16
to cmum...@chromium.org, dcheng+...@chromium.org, esp...@chromium.org, har...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
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.

dch...@chromium.org

unread,
Oct 19, 2016, 7:44:03 PM10/19/16
to rei...@chromium.org, cmum...@chromium.org, esp...@chromium.org, har...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
mojo lgtm


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

https://codereview.chromium.org/2370643004/diff/220001/content/browser/indexed_db/indexed_db_database_unittest.cc#newcode116
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_db/indexed_db_callbacks_impl.h
File content/child/indexed_db/indexed_db_callbacks_impl.h (right):

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_db/indexed_db_callbacks_impl.h#newcode26
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_db/indexed_db_database_callbacks_impl.cc
File content/child/indexed_db/indexed_db_database_callbacks_impl.cc
(right):

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_db/indexed_db_database_callbacks_impl.cc#newcode43
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)

https://codereview.chromium.org/2370643004/

rei...@chromium.org

unread,
Oct 19, 2016, 9:46:18 PM10/19/16
to cmum...@chromium.org, dcheng+...@chromium.org, esp...@chromium.org, har...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Ignore comment #82.



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

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_db/indexed_db_callbacks_impl.h#newcode26
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_db/indexed_db_database_callbacks_impl.cc
File content/child/indexed_db/indexed_db_database_callbacks_impl.cc
(right):

https://codereview.chromium.org/2370643004/diff/220001/content/child/indexed_db/indexed_db_database_callbacks_impl.cc#newcode43
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)

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 19, 2016, 9:47:35 PM10/19/16
to rei...@chromium.org, cmum...@chromium.org, dcheng+...@chromium.org, esp...@chromium.org, har...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, commi...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 20, 2016, 1:13:09 AM10/20/16
to rei...@chromium.org, cmum...@chromium.org, dcheng+...@chromium.org, esp...@chromium.org, har...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, commi...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Committed patchset #13 (id:250001)

https://chromiumcodereview.appspot.com/2370643004/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,
Oct 21, 2016, 9:15:59 AM10/21/16
to rei...@chromium.org, cmum...@chromium.org, dcheng+...@chromium.org, esp...@chromium.org, har...@chromium.org, j...@chromium.org, mea...@chromium.org, roc...@chromium.org, commi...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, cmum...@chromium.org, da...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, jsbel...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Patchset 13 (id:??) landed as
https://crrev.com/627e7f73d47910f4255031de55075e2a04b222f6
Cr-Commit-Position: refs/heads/master@{#426412}

https://codereview.chromium.org/2370643004/
Reply all
Reply to author
Forward
0 new messages