New issue
Advanced search Search tips

Issue 798819 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Simplify IDBAny

Project Member Reported by jsb...@chromium.org, Jan 3 2018

Issue description

The IDBAny type is used to represent several concepts:

* The `result` of an IDBRequest
* The `value` of an IDBCursor
* The `source` of an IDBRequest or IDBCursor

The former two correspond to an `any` IDL type.

The latter type is represented in the spec IDL by a union, although it's still an `any` in the blink IDL.

We could simplify the IDBAny code by using an IDL-generated union type instead, better aligned with the spec. This would remove the idb_index_ and idb_object_store_ members from IDBAny, with corresponding accessors/setters/type-enums.

EDIT: We need to keep idb_database_ as that is used by IDBOpenRequest


 
Components: Blink>Storage>IndexedDB
I'm taking this!

Comment 3 by jsb...@chromium.org, Jan 10 2018

Thanks!

Please ping any of us (pwnall@, dmurph@, or jsbell@) for any assistance. 

* It will be easiest to start with IDBCursor's `source` property. I don't think that will actually simplify IDBAny but will reduce the uses of it, and get you familiar with the IDL/C++/bindings changes.
* Changing IDBRequest's `source` will be similar, although slightly more complicated, and that will actually allow simplifying IDBAny.


Comment 4 Deleted

Comment 5 by pwnall@chromium.org, Jan 11 2018

Status: Started (was: Available)

Comment 6 by pwnall@chromium.org, Jan 12 2018

I suggest that changing IDBRequest's source be done as a separate CL from the IDBAny simplification. Think small, manageable steps towards victory :)
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b9868b89317a30208b3d8e1e6da0792fa863fe4c

commit b9868b89317a30208b3d8e1e6da0792fa863fe4c
Author: Xunran Ding <xunran.ding@samsung.com>
Date: Fri Jan 12 17:45:58 2018

IndexedDB: Reduce IDBAny usage in IDBCursor

Use IDL-generated union type 'IDBObjectStoreOrIDBIndex' instead
of IDBAny. This aligns better with the spec.

Bug:  798819 
Change-Id: I2296cf1cb99dc4ed954e3d79dafa8f70d4796851
Reviewed-on: https://chromium-review.googlesource.com/861682
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528988}
[modify] https://crrev.com/b9868b89317a30208b3d8e1e6da0792fa863fe4c/AUTHORS
[modify] https://crrev.com/b9868b89317a30208b3d8e1e6da0792fa863fe4c/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[modify] https://crrev.com/b9868b89317a30208b3d8e1e6da0792fa863fe4c/third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp
[modify] https://crrev.com/b9868b89317a30208b3d8e1e6da0792fa863fe4c/third_party/WebKit/Source/modules/indexeddb/IDBCursor.h
[modify] https://crrev.com/b9868b89317a30208b3d8e1e6da0792fa863fe4c/third_party/WebKit/Source/modules/indexeddb/IDBCursor.idl
[modify] https://crrev.com/b9868b89317a30208b3d8e1e6da0792fa863fe4c/third_party/WebKit/Source/modules/indexeddb/IDBCursorWithValue.cpp
[modify] https://crrev.com/b9868b89317a30208b3d8e1e6da0792fa863fe4c/third_party/WebKit/Source/modules/indexeddb/IDBCursorWithValue.h
[modify] https://crrev.com/b9868b89317a30208b3d8e1e6da0792fa863fe4c/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d5bddb57d9b9451153f239a62b874327662996f

commit 4d5bddb57d9b9451153f239a62b874327662996f
Author: Xunran Ding <xunran.ding@samsung.com>
Date: Thu Jan 18 11:42:39 2018

IndexedDB: Reduce IDBAny usage in IDBRequest

Use IDL-generated union type 'IDBObjectStoreOrIDBIndexOrIDBCursor' instead
of IDBAny. This aligns better with the spec.

Bug:  798819 
Change-Id: I6cecab16acd042637ca82d7aa48fac6fde24fc78
Reviewed-on: https://chromium-review.googlesource.com/866652
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530122}
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBFactory.cpp
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBOpenDBRequest.cpp
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBRequest.h
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBRequest.idl
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp
[modify] https://crrev.com/4d5bddb57d9b9451153f239a62b874327662996f/third_party/WebKit/Source/modules/indexeddb/IDBTransactionTest.cpp

It seems that idb_index_ and idb_object_store_ can be esaly removed from IDBAny. But idb_database_ can not be removed.

@pwnal, would you pls confirm this?

Comment 10 by costan@google.com, Jan 19 2018

#9: I think that's true. Let's remove idb_index_ and idb_object_store_, and I'll take a quick look to see why idb_database_ still needs to be there.
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ae76e5f5365edd2b7b2359511a1eb5c93158931

commit 8ae76e5f5365edd2b7b2359511a1eb5c93158931
Author: Xunran Ding <xunran.ding@samsung.com>
Date: Fri Jan 19 12:43:36 2018

IndexedDB: Simplify IDBAny type

The IDBAny type is no longer used as 'source' of an IDBRequest or IDBCursor.
Remove the idb_index_ and idb_object_store_ members from IDBAny.

Bug:  798819 
Change-Id: If5d6d87601e17638acba97db9e6ab87c53941b01
Reviewed-on: https://chromium-review.googlesource.com/875517
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530492}
[modify] https://crrev.com/8ae76e5f5365edd2b7b2359511a1eb5c93158931/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
[modify] https://crrev.com/8ae76e5f5365edd2b7b2359511a1eb5c93158931/third_party/WebKit/Source/modules/indexeddb/IDBAny.cpp
[modify] https://crrev.com/8ae76e5f5365edd2b7b2359511a1eb5c93158931/third_party/WebKit/Source/modules/indexeddb/IDBAny.h

Comment 12 by costan@google.com, Jan 19 2018

Cc: jsb...@chromium.org
xunran.ding: Thank you very much for your work!

jsbell: Please let me know if you agree with what I wrote below. If so, I'll update the bug description and mark it Fixed.

I'm reasonably sure that idb_database_ is only used for the result of IDBOpenRequest. It seems to me that IDBOpenRequest has its own code for handling results from the backing store, in IDBOpenRequest::DispatchEventInternal and IDBOpenRequest::EnqueueResponse.

If that is the case, it should be possible to refactor IDBOpenRequest to hold onto an IDBDatabase result, and to override IDBRequest::result and the related methods. At the same time, I'm not sure this would actually make things simpler.

Benefits:
* IDBAny gets smaller
* Fewer cases to consider in special bindings code for IDBAny

Downsides:
* IDBOpenRequest is-a IDBRequest becomes false, as calling ResultAsIdbAny() should/would crash.
* IDBAny would not match the "any" used in the spec's IDL (for IDBRequest.result) -- https://w3c.github.io/IndexedDB/#request-api
* IDBRequest::result() becomes virtual
* More complexity around IDBRequest::SetResult() and perhaps others

In conclusion, I'm not sure if it's a good idea to remove idb_database_ from IDBAny. I'm fairly sure that the tradeoffs involved mean the work doesn't meet GoodFirstBug criteria, so it should at the very least be moved to a separate bug. Assuming jsbell@ agrees, I plan to update this bug and mark it as fixed.
Status: Fixed (was: Started)
Agreed with keeping idb_database_ - apologies for not considering the IDBOpenRequest in the original description. Let's just keep it in IDBAny.

Let's call this fixed!


Thank you very much xunran.ding@ ! Please let us know if you'd like pointers to other GoodFirstBug possibilities.
Description: Show this description

Sign in to add a comment