Simplify IDBAny |
|||||
Issue descriptionThe 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
,
Jan 10 2018
I'm taking this!
,
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.
,
Jan 11 2018
,
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 :)
,
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
,
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
,
Jan 19 2018
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?
,
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.
,
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
,
Jan 19 2018
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.
,
Jan 19 2018
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.
,
Jan 19 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by jsb...@chromium.org
, Jan 3 2018