Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2125213002:
[IndexedDB] Propogating changes to observers : Renderer (Closed)
|
Created:
4 years, 5 months ago by palakj1 Modified:
4 years, 5 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, jsbell+idb_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lifetime Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[IndexedDB] Propogating changes to observers
* Changes received from browser are multiplexed and passed on to each observer.
* Each observer extracts the required subset of observations from bulk observations of the connnection using its array of indices.
* The callback is fired with a map of object store to array of observations.
Current Implementation and Future Scope: https://goo.gl/Y2dobn
Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md
BUG=609934
Committed: https://crrev.com/2751bda57a56b4d2bc02899a8a39e3026fccc599
Cr-Commit-Position: refs/heads/master@{#406722}
Patch Set 1 #Patch Set 2 : Filtering Changes to Observer : Browser End #
Total comments: 19
Patch Set 3 : Post cmumford review #
Total comments: 16
Patch Set 4 : Post dmurph review #
Total comments: 15
Patch Set 5 : Propogating Changes : Renderer #Patch Set 6 : Propogating Changes to Renderer #Patch Set 7 : Propogating Changes to Renderer #
Total comments: 37
Patch Set 8 : IDBDatabase weakptr introduced on IDBObserver #
Total comments: 24
Patch Set 9 : Implements IDBObserverChanges IDL records map #
Total comments: 7
Patch Set 10 : Implements Map in IDBObserverChanges idl #
Total comments: 11
Patch Set 11 : Minor bugs fixed #
Total comments: 55
Patch Set 12 : Operation type support added #Patch Set 13 : Observer test added #Patch Set 14 : Test added #
Total comments: 10
Patch Set 15 : Tests added #Patch Set 16 : Tests added #
Total comments: 10
Patch Set 17 : Options constrcutor modified #
Total comments: 17
Patch Set 18 : Renderer Changes #Patch Set 19 : Observer changes at renderer #Patch Set 20 : Renderer changes #Patch Set 21 : Expected tests changed #Patch Set 22 : Renderer changes #Patch Set 23 : Expected test results changed #Patch Set 24 : Renderer changes #
Total comments: 38
Patch Set 25 : Post cmumford review #Patch Set 26 : Minor fix #
Total comments: 15
Patch Set 27 : Post haraken review #
Total comments: 1
Patch Set 28 : Post haraken review #
Total comments: 6
Patch Set 29 : Post wfh, chrishtr review #
Total comments: 1
Patch Set 30 : Minor fix #Patch Set 31 : Minor typecasting #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 131 (58 generated)
Description was changed from ========== [IndexedDB] Propogating changes to observers BUG= ========== to ========== [IndexedDB] Propogating changes to observers * Each transaction has a connection-changes map. * Changes contain a set of observations and map of observer_id to list of indices from observations that the observer is listening to. Current Implementation and Future Scope: https://docs.google.com/a/google.com/document/d/1viweV7zje2vK9IC-_G0PYlWgGrBh... Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 ==========
palakj@google.com changed reviewers: + cmumford@chromium.org, dmurph@chromium.org, jsbell@chromium.org, mek@chromium.org, palmer@chromium.org
Please have a look.
Just a very quick (and incomplete) first pass by me. BTW the IDB unit tests are crashing with this change. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_callbacks.cc (right): https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_callbacks.cc:209: connection_ptr->setId(ipc_object_id); IndexedDBDispatcherHost::Add() will delete connection if IndexedDBDispatcherHost::database_dispatcher_host_ is NULL - and then connection_ptr will be a dangling pointer. Same comment as the prior CL. It would be great if either Add() set the ID, and even better if that could somehow be supplied to the IndexedDBConnection constructor. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_connection.h:38: void setId(int32_t id) { id_ = id; } Nit: setId/id are a pair, so I'd eliminate whitespace in between. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_connection.h:53: int32_t id_; id_ got put in between the comment and database_, so something needs to move. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:573: if (!connection) How would a NULL connection wind up in connections_? https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:578: for (const auto& observer : observers) { Just use: for (const auto& observer : connection->active_observers()) { ... } https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:601: for (const auto& it : connections_) { Nit: no need for braces for a single line. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:1547: // FilterObservation(transaction, object_store_id, Why is this commented out? https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_observer.h:30: return operation_type_[type_index]; When is operation_type_ set? https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_observer.h:41: bool operation_type_[4] = {false}; using a int32_t (above in IsRecordingType) and [4] is not type safe, and fragile. Consider something like: enum OperationType { ADD = 0, PUT, DELETE, CLEAR, NUM_TYPES, // must be last enumeration }; and then: bool operation_type_[IndexedDBObservation::OperationType::NUM_TYPES] = {false}; Then you can have: bool IsRecordingType(IndexedDBObservation::OperationType type) const { DCHECK_NE(type, IndexedDBObservation::OperationType::NUM_TYPES); return operation_type_[type]; } https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_observer_changes.h:24: struct CONTENT_EXPORT IndexedDBObserverChanges { IndexedDBObserverChanges has methods and not just a passive object. Make a class as per https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
This is a wip. Changes are filtered and recorded at the backend and sent through ipc. Please have a look at the design. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_connection.h:38: void setId(int32_t id) { id_ = id; } On 2016/07/07 at 21:14:21, cmumford wrote: > Nit: setId/id are a pair, so I'd eliminate whitespace in between. Done. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_connection.h:53: int32_t id_; On 2016/07/07 at 21:14:21, cmumford wrote: > id_ got put in between the comment and database_, so something needs to move. Sorry for that. Changed. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:573: if (!connection) On 2016/07/07 at 21:14:21, cmumford wrote: > How would a NULL connection wind up in connections_? Right, that's redundant. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:578: for (const auto& observer : observers) { On 2016/07/07 at 21:14:21, cmumford wrote: > Just use: > > for (const auto& observer : connection->active_observers()) { > ... > } Done. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:601: for (const auto& it : connections_) { On 2016/07/07 at 21:14:21, cmumford wrote: > Nit: no need for braces for a single line. Done. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:1547: // FilterObservation(transaction, object_store_id, On 2016/07/07 at 21:14:21, cmumford wrote: > Why is this commented out? Done. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_observer.h:30: return operation_type_[type_index]; On 2016/07/07 at 21:14:21, cmumford wrote: > When is operation_type_ set? Sorry, this is still a wip. I should have mentioned that. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_observer.h:41: bool operation_type_[4] = {false}; On 2016/07/07 at 21:14:21, cmumford wrote: > using a int32_t (above in IsRecordingType) and [4] is not type safe, and fragile. Consider something like: > > enum OperationType { > ADD = 0, > PUT, > DELETE, > CLEAR, > NUM_TYPES, // must be last enumeration > }; > > and then: > > bool operation_type_[IndexedDBObservation::OperationType::NUM_TYPES] = {false}; > > Then you can have: > > bool IsRecordingType(IndexedDBObservation::OperationType type) const { > DCHECK_NE(type, IndexedDBObservation::OperationType::NUM_TYPES); > return operation_type_[type]; > } Thanks. Done. https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_observer_changes.h:24: struct CONTENT_EXPORT IndexedDBObserverChanges { On 2016/07/07 at 21:14:21, cmumford wrote: > IndexedDBObserverChanges has methods and not just a passive object. Make a class as per https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes Done.
Hey! major comment here is to merge the message struct and the indexed_db_observer_changes, see the two classes I linked. You will have to put the class in the content/common/indexed_db/ directory, instead of browser/. https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_callbacks.cc (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_callbacks.cc:166: Remove whitespace changes in this file. https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database_callbacks.cc (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_database_callbacks.cc:72: IndexedDBDispatcherHost::ConvertObserverChanges(changes.release()))); make this function accept a unique ptr. Then move the map, as you know you have the only reference. https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.cc:327: idb_changes.observation_index = changes->observation_index(); Yeah, do a move. https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.cc:329: ::IndexedDBObservation idb_observation = Don't you need to store these somewhere? https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_observer_changes.h:24: class CONTENT_EXPORT IndexedDBObserverChanges { Ok, so, each set of changes is on a per-connection basis, correct? So let's say that here. Also, it's very confusion what AddObservationIndex does by looking at the header file. You need to document this, and possibly name it better. Maybe "RecordObserverForLastObservation". https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_observer_changes.h:24: class CONTENT_EXPORT IndexedDBObserverChanges { Also, instead of having a duplicate changes in the messages, let's just use this as the message struct. See https://cs.chromium.org/chromium/src/content/common/fileapi/webblob_messages.... https://cs.chromium.org/chromium/src/storage/common/blob_storage/blob_item_by... https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:361: database_->SendObservations(std::move(connection_changes_map_)); Activate pending observers after this. https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.h:80: void AddObservationIndex(int32_t observer_id, int32_t connection_id); This is confusing to me. Maybe change the name like above, to specify you're recording for the last observation added. https://codereview.chromium.org/2125213002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h (right): https://codereview.chromium.org/2125213002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h:79: WebIDBOperationTypeCount = 4, Remove the " = 4" here, enums auto increment and that value will always be the last one + 1.
Please have a look. Regarding sending message to renderer, is my current method of copying IndexedDBObserverChanges to an equivalent struct fine or should I rather move IndexedDBObserverChanges to content/common as suggested by +dmurph. Note that I need to send IndexedDBValue resting in content/browser as a part of changes which content/common cannot access. https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_callbacks.cc (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_callbacks.cc:166: On 2016/07/08 at 18:40:44, dmurph wrote: > Remove whitespace changes in this file. Done. https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database_callbacks.cc (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_database_callbacks.cc:72: IndexedDBDispatcherHost::ConvertObserverChanges(changes.release()))); On 2016/07/08 at 18:40:44, dmurph wrote: > make this function accept a unique ptr. Then move the map, as you know you have the only reference. Done. https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.cc:329: ::IndexedDBObservation idb_observation = On 2016/07/08 at 18:40:44, dmurph wrote: > Don't you need to store these somewhere? oops! changed. https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_observer_changes.h:24: class CONTENT_EXPORT IndexedDBObserverChanges { On 2016/07/08 at 18:40:44, dmurph wrote: > Ok, so, each set of changes is on a per-connection basis, correct? So let's say that here. Also, it's very confusion what AddObservationIndex does by looking at the header file. You need to document this, and possibly name it better. Maybe "RecordObserverForLastObservation". Renamed. Deferring messages change to next patch https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:361: database_->SendObservations(std::move(connection_changes_map_)); On 2016/07/08 at 18:40:44, dmurph wrote: > Activate pending observers after this. Sorry. Moving the orders. https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2125213002/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.h:80: void AddObservationIndex(int32_t observer_id, int32_t connection_id); On 2016/07/08 at 18:40:44, dmurph wrote: > This is confusing to me. Maybe change the name like above, to specify you're recording for the last observation added. Done. https://codereview.chromium.org/2125213002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h (right): https://codereview.chromium.org/2125213002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h:79: WebIDBOperationTypeCount = 4, On 2016/07/08 at 18:40:44, dmurph wrote: > Remove the " = 4" here, enums auto increment and that value will always be the last one + 1. Done.
On 2016/07/08 22:17:54, palakj1 wrote: > Please have a look. > > Regarding sending message to renderer, is my current method of copying > IndexedDBObserverChanges to an equivalent struct fine or should I rather move > IndexedDBObserverChanges to content/common as suggested by +dmurph. > Note that I need to send IndexedDBValue resting in content/browser as a part of > changes which content/common cannot access. I don't see any reason to move it. Note that the move to Mojo will completely eliminate indexed_db_messages.h (and your IndexedDBObservation struct). But that's not landing anytime soon. See what jsbell@ thinks. Was dmurph@ just looking to eliminate redundant structures?
On 2016/07/08 22:50:04, cmumford wrote: > On 2016/07/08 22:17:54, palakj1 wrote: > > Please have a look. > > > > Regarding sending message to renderer, is my current method of copying > > IndexedDBObserverChanges to an equivalent struct fine or should I rather move > > IndexedDBObserverChanges to content/common as suggested by +dmurph. > > Note that I need to send IndexedDBValue resting in content/browser as a part > of > > changes which content/common cannot access. > > I don't see any reason to move it. Note that the move to Mojo will completely > eliminate indexed_db_messages.h (and your IndexedDBObservation struct). But > that's not landing anytime soon. See what jsbell@ thinks. Was dmurph@ just > looking to eliminate redundant structures? Yes, I was just trying to eliminate redundant strucutures, as having the Convert* methods is a little crufty. I chatted with Palak, and it looks like the main issue here is that IDBValue isn't available (and we have the message-only struct of IndexedDBMsg_Value). It would be nice to have all the IPC stuff be in that spot, but if it'll just make the mojo transition harder (and not matter anyways), then I'm fine with having the conversion as it is. I would probably just suggest changing the names so they aren't exactly the same, maybe like IndexedDBMsg_Observations.
On 2016/07/08 at 22:53:35, dmurph wrote: > On 2016/07/08 22:50:04, cmumford wrote: > > On 2016/07/08 22:17:54, palakj1 wrote: > > > Please have a look. > > > > > > Regarding sending message to renderer, is my current method of copying > > > IndexedDBObserverChanges to an equivalent struct fine or should I rather move > > > IndexedDBObserverChanges to content/common as suggested by +dmurph. > > > Note that I need to send IndexedDBValue resting in content/browser as a part > > of > > > changes which content/common cannot access. > > > > I don't see any reason to move it. Note that the move to Mojo will completely > > eliminate indexed_db_messages.h (and your IndexedDBObservation struct). But > > that's not landing anytime soon. See what jsbell@ thinks. Was dmurph@ just > > looking to eliminate redundant structures? > > Yes, I was just trying to eliminate redundant strucutures, as having the Convert* methods is a little crufty. I chatted with Palak, and it looks like the main issue here is that IDBValue isn't available (and we have the message-only struct of IndexedDBMsg_Value). It would be nice to have all the IPC stuff be in that spot, but if it'll just make the mojo transition harder (and not matter anyways), then I'm fine with having the conversion as it is. I would probably just suggest changing the names so they aren't exactly the same, maybe like IndexedDBMsg_Observations. Alright then. I'll just rename it for now.
https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.cc:327: idb_changes.observation_index = changes->observation_index(); Since you grab a unique_ptr, you can safely do an move here (or a swap). https://codereview.chromium.org/2125213002/diff/60001/content/child/indexed_d... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/60001/content/child/indexed_d... content/child/indexed_db/indexed_db_dispatcher.cc:805: // for(auto& obs: changes_map.observation_index) { Ah, so here we'll probably want to decompose to each WebIDBObserver like you did, and then I guess you'll have to convert the data.... You can probably put the multiplexing logic here, especially because you'll be translating into types that IDBObserver can understand/see.
some minor comments https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_observation.cc (right): https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_observation.cc:10: DCHECK(type_ == blink::WebIDBClear); DCHECK_EQ(type_, blink::WebIDBClear) https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_observation.cc:16: DCHECK(type_ == blink::WebIDBAdd || type_ == blink::WebIDBPut); Shouldn't this also allow WebIDBDelete? https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_observation.cc:23: DCHECK(type_ != blink::WebIDBDelete); DCHECK_NE(type_, blink::WebIDBDelete) https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:358: database_->SendObservations(std::move(connection_changes_map_)); I don't think the c++ standard technically guarantees that moving from a std::map (or a std::vector below) clears the moved from container. So to be perfectly safe you should probably have clear() calls both her on connection_changes_map_ and below on pending_observers_, after you move from them. https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.h:38: // using Changes = IndexedDBDatabase::Changes; don't add commented out code? https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.h:159: typedef std::map<int32_t, std::unique_ptr<IndexedDBObserverChanges>> using instead of typedef
Please have a look. * Note that we require IDBDatabase object as a part of IDBObserverChanges. However there seems no way of accessing IDBDatbaase through it's corresponding WebIDBDatabase object, we have access to. What can we do here? * The test fails to execute due to a unique_ptr error. I shall resolve it with the next patch. https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.cc:327: idb_changes.observation_index = changes->observation_index(); On 2016/07/08 at 23:51:40, dmurph wrote: > Since you grab a unique_ptr, you can safely do an move here (or a swap). Done. https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_observation.cc (right): https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_observation.cc:16: DCHECK(type_ == blink::WebIDBAdd || type_ == blink::WebIDBPut); On 2016/07/08 at 23:57:57, Marijn Kruisselbrink wrote: > Shouldn't this also allow WebIDBDelete? Done https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_observation.cc:23: DCHECK(type_ != blink::WebIDBDelete); On 2016/07/08 at 23:57:57, Marijn Kruisselbrink wrote: > DCHECK_NE(type_, blink::WebIDBDelete) Changed. https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:358: database_->SendObservations(std::move(connection_changes_map_)); On 2016/07/08 at 23:57:57, Marijn Kruisselbrink wrote: > I don't think the c++ standard technically guarantees that moving from a std::map (or a std::vector below) clears the moved from container. So to be perfectly safe you should probably have clear() calls both her on connection_changes_map_ and below on pending_observers_, after you move from them. Done. Should i be clearing every time I'm moving map/vector? https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.h:38: // using Changes = IndexedDBDatabase::Changes; On 2016/07/08 at 23:57:57, Marijn Kruisselbrink wrote: > don't add commented out code? Done. https://codereview.chromium.org/2125213002/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.h:159: typedef std::map<int32_t, std::unique_ptr<IndexedDBObserverChanges>> On 2016/07/08 at 23:57:57, Marijn Kruisselbrink wrote: > using instead of typedef done. https://codereview.chromium.org/2125213002/diff/60001/content/child/indexed_d... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/60001/content/child/indexed_d... content/child/indexed_db/indexed_db_dispatcher.cc:805: // for(auto& obs: changes_map.observation_index) { On 2016/07/08 at 23:51:40, dmurph wrote: > Ah, so here we'll probably want to decompose to each WebIDBObserver like you did, and then I guess you'll have to convert the data.... You can probably put the multiplexing logic here, especially because you'll be translating into types that IDBObserver can understand/see. Done.
Exciting! Overall this is looking like it's on track. Review is just initial notes; I'll re-review after the onDatabaseChange hookup is done https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:15: : id_(kInvalidId), This can have the default initialization in the header file. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:38: void setId(int32_t id); Per style guide, it's OK (preferred?) to name trivial setter as: set_id() https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:575: observer->IsRecordingObjectStore(object_store_id)) { It might be more readable to invert this test and `continue;` rather than indenting further. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:323: ::IndexedDBMsg_ObserverChanges IndexedDBDispatcherHost::ConvertObserverChanges( Is the leading :: needed here? Its only used in ConvertMetadata to distinguish ::IndexedDBDatabaseMetadata (an IPC-specific struct) from content::IndexedDBDatabaseMetadata (the struct used in the non-IPC code) https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:324: std::unique_ptr<content::IndexedDBObserverChanges> changes) { Similarly, I don't think you need content:: here https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:329: ConvertObservation(std::move(observation))); It's subtle that this is freeing the observations during iteration. Perhaps add a comment, and (as noted below) make the ConvertObservation() take a const ptr and do the removal explicitly here. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:335: std::unique_ptr<content::IndexedDBObservation> observation) { Since this is not transferring ownership, can this be a const raw ptr rather than a unique_ptr ? https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.cc:17: observation_index_[observer_id].push_back(observations_.size() - 1); Use observations_.back() https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.h:44: // map observer_id to corresponding set of indices in observations. Nit: Make the comment a sentence (i.e. start with uppercase) https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:157: using ConnectionChangeMap = Since this type is used in only one place, I wouldn't bother naming it. https://codereview.chromium.org/2125213002/diff/120001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:826: // TODO(palakj): Get IDBDatabase object from ipc_database_id. Communication from the back end to an IDBDatabase is done via WebIDBDatabaseCallbacks. Starting in IndexedDBDatabaseCallbacks::OnDatabaseChange you can pass the ipc_database_callbacks_id_ through in the message instead of ipc_database_id. That lets you look up a WebIDBDatabaseCallbacks instance in the IndexedDBDispatcher; you'd add onChange() there and in WebIDBDatabaseCallbacksImpl and in IDBDatabaseCallbacks which has an IDBDatabase weakptr. Look at how OnAbort is propagated back for an example. (Converting to Mojo will let us eliminate these indirections!) https://codereview.chromium.org/2125213002/diff/120001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2125213002/diff/120001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:371: // IPC_MESSAGE macros fail on the std::map, when expanding. We need to define Interesting; I thought std::map just wasn't supported by IPC! (I think we manually convert maps to vectors-of-pairs elsewhere...) Is there a bug about issue? https://codereview.chromium.org/2125213002/diff/120001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:373: // map observer_id to corresponding set of indices in observations. Nit: "Map ..." https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp (right): https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:32: Nit: remove this blank line https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp (right): https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp:175: auto it = m_metadata.objectStores.find(objectStoreId); Should this have ASSERT_WITH_SECURITY_IMPLICATION(it != m_metadata.objectStores.end()); (similar to indexCreated()) ? Otherwise, just do this all one one line, and use objectStores[objectStoreId] ? https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:45: // bool m_operationType[WebIDBOperationTypeCount] = {true}; Remove or change into a TODO before landing https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBKeyRange.h (right): https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBKeyRange.h:40: WebIDBKeyRange() {} Nit: leave a blank line. https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBObservation.h (right): https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBObservation.h:15: nit: remove blank line
Josh's comment about putting a little more logic WebIDBDatabaseCallbacksImpl is interesting, does that make things simpler? You'll have a weak pointer there to the database. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.cc:17: observation_index_[observer_id].push_back(observations_.size() - 1); On 2016/07/11 18:25:33, jsbell wrote: > Use observations_.back() She needs the index, not the object.
We have 2 alternatives to fetching IDBDatabase for our callback as discussed in the comments. Please suggest which one is more preferrable. https://codereview.chromium.org/2125213002/diff/120001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:826: // TODO(palakj): Get IDBDatabase object from ipc_database_id. On 2016/07/11 at 18:25:33, jsbell wrote: > Communication from the back end to an IDBDatabase is done via WebIDBDatabaseCallbacks. > > Starting in IndexedDBDatabaseCallbacks::OnDatabaseChange you can pass the ipc_database_callbacks_id_ through in the message instead of ipc_database_id. That lets you look up a WebIDBDatabaseCallbacks instance in the IndexedDBDispatcher; you'd add onChange() there and in WebIDBDatabaseCallbacksImpl and in IDBDatabaseCallbacks which has an IDBDatabase weakptr. > > Look at how OnAbort is propagated back for an example. > > (Converting to Mojo will let us eliminate these indirections!) Thanks for the insight. This seems a good option. Our other alternative is to have IDBObserver hold a map of observer_id to IDBDatabase weakptr corresponding to each observe call 'observer.observe(db)' (we already have a set of observer_id here, need to turn it to a map). A benefit with this apporach is that the unobserve logic gets cleaner. Currently, for observer.unobserve(db) , IDBObserver needs to check with WebIDBDatabase if the latter holds the observer_id. Having a map of observer_id to IDBDatabase, we can complete the entire logic within IDBObserver. Moving a bit of logic from webidb to blink. The downside is creating a new weak ptr. What would you prefer?
https://codereview.chromium.org/2125213002/diff/120001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:826: // TODO(palakj): Get IDBDatabase object from ipc_database_id. On 2016/07/11 19:37:54, palakj1 wrote: > On 2016/07/11 at 18:25:33, jsbell wrote: > > Communication from the back end to an IDBDatabase is done via > WebIDBDatabaseCallbacks. > > > > Starting in IndexedDBDatabaseCallbacks::OnDatabaseChange you can pass the > ipc_database_callbacks_id_ through in the message instead of ipc_database_id. > That lets you look up a WebIDBDatabaseCallbacks instance in the > IndexedDBDispatcher; you'd add onChange() there and in > WebIDBDatabaseCallbacksImpl and in IDBDatabaseCallbacks which has an IDBDatabase > weakptr. > > > > Look at how OnAbort is propagated back for an example. > > > > (Converting to Mojo will let us eliminate these indirections!) > > Thanks for the insight. This seems a good option. > Our other alternative is to have IDBObserver hold a map of observer_id to > IDBDatabase weakptr corresponding to each observe call 'observer.observe(db)' > (we already have a set of observer_id here, need to turn it to a map). > A benefit with this apporach is that the unobserve logic gets cleaner. > Currently, for observer.unobserve(db) , IDBObserver needs to check with > WebIDBDatabase if the latter holds the observer_id. > Having a map of observer_id to IDBDatabase, we can complete the entire logic > within IDBObserver. Moving a bit of logic from webidb to blink. > The downside is creating a new weak ptr. > > What would you prefer? It sounds like the weakptr in IDBObserver would be cleaner, so starting with that approach sgtm.
Please have a look. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:15: : id_(kInvalidId), On 2016/07/11 at 18:25:33, jsbell wrote: > This can have the default initialization in the header file. Done. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:38: void setId(int32_t id); On 2016/07/11 at 18:25:33, jsbell wrote: > Per style guide, it's OK (preferred?) to name trivial setter as: set_id() Done. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:575: observer->IsRecordingObjectStore(object_store_id)) { On 2016/07/11 at 18:25:33, jsbell wrote: > It might be more readable to invert this test and `continue;` rather than indenting further. Done. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:323: ::IndexedDBMsg_ObserverChanges IndexedDBDispatcherHost::ConvertObserverChanges( On 2016/07/11 at 18:25:33, jsbell wrote: > Is the leading :: needed here? Its only used in ConvertMetadata to distinguish ::IndexedDBDatabaseMetadata (an IPC-specific struct) from content::IndexedDBDatabaseMetadata (the struct used in the non-IPC code) Done. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:324: std::unique_ptr<content::IndexedDBObserverChanges> changes) { On 2016/07/11 at 18:25:33, jsbell wrote: > Similarly, I don't think you need content:: here Done. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:329: ConvertObservation(std::move(observation))); On 2016/07/11 at 18:25:33, jsbell wrote: > It's subtle that this is freeing the observations during iteration. Perhaps add a comment, and (as noted below) make the ConvertObservation() take a const ptr and do the removal explicitly here. done. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:335: std::unique_ptr<content::IndexedDBObservation> observation) { On 2016/07/11 at 18:25:33, jsbell wrote: > Since this is not transferring ownership, can this be a const raw ptr rather than a unique_ptr ? Done. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.h:44: // map observer_id to corresponding set of indices in observations. On 2016/07/11 at 18:25:33, jsbell wrote: > Nit: Make the comment a sentence (i.e. start with uppercase) Done. https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2125213002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:157: using ConnectionChangeMap = On 2016/07/11 at 18:25:33, jsbell wrote: > Since this type is used in only one place, I wouldn't bother naming it. Done. https://codereview.chromium.org/2125213002/diff/120001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2125213002/diff/120001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:371: // IPC_MESSAGE macros fail on the std::map, when expanding. We need to define On 2016/07/11 at 18:25:33, jsbell wrote: > Interesting; I thought std::map just wasn't supported by IPC! (I think we manually convert maps to vectors-of-pairs elsewhere...) > > Is there a bug about issue? None that I could find. https://codereview.chromium.org/2125213002/diff/120001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:373: // map observer_id to corresponding set of indices in observations. On 2016/07/11 at 18:25:33, jsbell wrote: > Nit: "Map ..." Done. https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp (right): https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:32: On 2016/07/11 at 18:25:33, jsbell wrote: > Nit: remove this blank line Done https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp (right): https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp:175: auto it = m_metadata.objectStores.find(objectStoreId); On 2016/07/11 at 18:25:33, jsbell wrote: > Should this have ASSERT_WITH_SECURITY_IMPLICATION(it != m_metadata.objectStores.end()); (similar to indexCreated()) ? > > Otherwise, just do this all one one line, and use objectStores[objectStoreId] ? changed to using [] https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:45: // bool m_operationType[WebIDBOperationTypeCount] = {true}; On 2016/07/11 at 18:25:33, jsbell wrote: > Remove or change into a TODO before landing Done. https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBKeyRange.h (right): https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBKeyRange.h:40: WebIDBKeyRange() {} On 2016/07/11 at 18:25:33, jsbell wrote: > Nit: leave a blank line. Done. https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBObservation.h (right): https://codereview.chromium.org/2125213002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBObservation.h:15: On 2016/07/11 at 18:25:33, jsbell wrote: > nit: remove blank line Done.
https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:599: std::move(change_map[it->id()]); Rather than using operator[] to look up the connection in the change_map and then checking if the returned pointer is null, I think it would be cleaner to use find() to find the iterator, and check if the returned iterator != end(). That way you avoid inserting unnecesary null pointers in change_map (it doesn't matter too much since change_map gets thrown away anyway, but using [] to check if an entry exists in a map is kind of an anti-pattern as it inserts a new entry). https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:329: std::move(changes->observations())); No need to move the vector out of changes->observations() and iterating on that rather then just iterating on changes->observations() directly. https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:332: ConvertObservation(observation.release())); You probably meant .get() rather than .release() here, to avoid leaking all these instances? https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.h:35: std::map<int32_t, std::vector<int32_t>>& observation_index() { Would it make sense to instead of having methods that return non-const references have methods that either return by const reference (if that is needed anywhere) and a different method that move-returns by value? The move-returning method can then have a more explicit name that makes it clear that IndexedDBObserverChanges state is being changed (something like release_observations()) https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.h:44: std::map<int32_t, std::vector<int32_t>> observation_index_; Is observation_index really the best name for this concept? It seems that at least it should be observation_indices_, as it stores a list of indices for each observer... https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:500: connection_changes_map_[connection_id]->RecordObserverForLastObservation( Maybe add a DCHECK here to make sure that connection_id actually exists in connection_changes_map_? Although I suppose unique_ptr will probably assert anyway if somehow it ends up being called with an invalid connection_id. https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:79: void RecordObserverForLastObservation(int32_t observer_id, For consistency with AddObservation it might make sense to swap the order of the arguments? So have connection_id be the first argument in both cases. https://codereview.chromium.org/2125213002/diff/140001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2125213002/diff/140001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:13: #include "base/memory/ptr_util.h" I don't believe you're actually using anything from this file in this file? https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:31: // API methods I'm not sure what the "API methods" and "API Attributes" comments are supposed to say? Generally classes implementing IDL interfaces have some block of methods to implement that IDL interface, with some comment saying so. But these "API foo" comments don't seem to refer to that, as most of these methods don't correspond to anything in IDL. https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:51: std::map<int32_t, WeakMember<IDBDatabase>> m_observerIds; You should not use stl containers to hold references to garbage collected object (for one the WeakMember won't be properly zeroed out without tracing). You probably want to use a HeapHashMap<int32_t, WeakMember<IDBDatabase>> here, and trace it in your trace method. Also, should this really be a WeakMember? I'm afraid I'm not entirely familiar with the exact lifetime guarantees of IDBDatabase instances and IDBObserver instances (does anything observable happens if an IDBDatabase instance is garbage collected?) https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h (right): https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h:27: // TODO(palakj): Add IDBDatabase* to create. You seemed to have resolved this TODO already? https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right): https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:17: [CallWith=ScriptState] readonly attribute sequence<IDBObservation> records; (maybe not relevant if you're somehow changing this to a map anyway, but:) This should definitely not be sequence<IDBObservation>. sequence should never be used for interface attributes in IDL (see the note at https://heycam.github.io/webidl/#idl-sequence). If you want to return an array FrozenArray might be what you want instead.
palakj@google.com changed reviewers: + haraken@chromium.org, ikilpatrick@chromium.org
Implemented IDBObserverChanges IDL map for records. After discussion with +ikilpatrick and +dehrenberg, I inferred the following ways to create a v8 object for representing javascript map. i. use maplike as in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webmid... ii. use v8::Map object https://cs.chromium.org/chromium/src/v8/include/v8.h?sq=package:chromium&dr=C... iii. Current Implementation: HeapVector<std::pair<String, HeapVector<ScriptWrappable>> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... iv. V8ObjectBuilder https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... As suggested by +ikilpatrick, I went with iii. option and hence added a toV8 definition to meet my requirements. However, iv. might also be a good way to do it. Please advice if this is alright. https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:599: std::move(change_map[it->id()]); On 2016/07/11 at 23:10:39, Marijn Kruisselbrink wrote: > Rather than using operator[] to look up the connection in the change_map and then checking if the returned pointer is null, I think it would be cleaner to use find() to find the iterator, and check if the returned iterator != end(). That way you avoid inserting unnecesary null pointers in change_map (it doesn't matter too much since change_map gets thrown away anyway, but using [] to check if an entry exists in a map is kind of an anti-pattern as it inserts a new entry). I see your point. Done. https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:329: std::move(changes->observations())); On 2016/07/11 at 23:10:39, Marijn Kruisselbrink wrote: > No need to move the vector out of changes->observations() and iterating on that rather then just iterating on changes->observations() directly. Done. https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:332: ConvertObservation(observation.release())); On 2016/07/11 at 23:10:39, Marijn Kruisselbrink wrote: > You probably meant .get() rather than .release() here, to avoid leaking all these instances? Done. https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.h:35: std::map<int32_t, std::vector<int32_t>>& observation_index() { On 2016/07/11 at 23:10:39, Marijn Kruisselbrink wrote: > Would it make sense to instead of having methods that return non-const references have methods that either return by const reference (if that is needed anywhere) and a different method that move-returns by value? The move-returning method can then have a more explicit name that makes it clear that IndexedDBObserverChanges state is being changed (something like release_observations()) Done. https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.h:44: std::map<int32_t, std::vector<int32_t>> observation_index_; On 2016/07/11 at 23:10:39, Marijn Kruisselbrink wrote: > Is observation_index really the best name for this concept? It seems that at least it should be observation_indices_, as it stores a list of indices for each observer... Agreed. Changed to observation_indices_map. Any better suggestions? https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:500: connection_changes_map_[connection_id]->RecordObserverForLastObservation( On 2016/07/11 at 23:10:39, Marijn Kruisselbrink wrote: > Maybe add a DCHECK here to make sure that connection_id actually exists in connection_changes_map_? Although I suppose unique_ptr will probably assert anyway if somehow it ends up being called with an invalid connection_id. Done. Thanks for pointing that out. https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2125213002/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:79: void RecordObserverForLastObservation(int32_t observer_id, On 2016/07/11 at 23:10:40, Marijn Kruisselbrink wrote: > For consistency with AddObservation it might make sense to swap the order of the arguments? So have connection_id be the first argument in both cases. done. https://codereview.chromium.org/2125213002/diff/140001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2125213002/diff/140001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:13: #include "base/memory/ptr_util.h" On 2016/07/11 at 23:10:40, Marijn Kruisselbrink wrote: > I don't believe you're actually using anything from this file in this file? removed. https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:31: // API methods On 2016/07/11 at 23:10:40, Marijn Kruisselbrink wrote: > I'm not sure what the "API methods" and "API Attributes" comments are supposed to say? Generally classes implementing IDL interfaces have some block of methods to implement that IDL interface, with some comment saying so. But these "API foo" comments don't seem to refer to that, as most of these methods don't correspond to anything in IDL. Removed. Not aware of the right comments to put here. https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:51: std::map<int32_t, WeakMember<IDBDatabase>> m_observerIds; On 2016/07/11 at 23:10:40, Marijn Kruisselbrink wrote: > You should not use stl containers to hold references to garbage collected object (for one the WeakMember won't be properly zeroed out without tracing). You probably want to use a HeapHashMap<int32_t, WeakMember<IDBDatabase>> here, and trace it in your trace method. > > Also, should this really be a WeakMember? I'm afraid I'm not entirely familiar with the exact lifetime guarantees of IDBDatabase instances and IDBObserver instances (does anything observable happens if an IDBDatabase instance is garbage collected?) Changed stl container to heaphashmap. As discussed with you, we ensure that if IDBDatabase is getting deleted, our reference is removed prior to it being garbage collected. So, keeping a weakptr reference to IDBDatabase. https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h (right): https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h:27: // TODO(palakj): Add IDBDatabase* to create. On 2016/07/11 at 23:10:40, Marijn Kruisselbrink wrote: > You seemed to have resolved this TODO already? removed. https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right): https://codereview.chromium.org/2125213002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:17: [CallWith=ScriptState] readonly attribute sequence<IDBObservation> records; On 2016/07/11 at 23:10:40, Marijn Kruisselbrink wrote: > (maybe not relevant if you're somehow changing this to a map anyway, but:) This should definitely not be sequence<IDBObservation>. sequence should never be used for interface attributes in IDL (see the note at https://heycam.github.io/webidl/#idl-sequence). If you want to return an array FrozenArray might be what you want instead. Changed this to 'any'.
On 2016/07/13 00:43:51, palakj1 wrote: > Implemented IDBObserverChanges IDL map for records. > > After discussion with +ikilpatrick and +dehrenberg, I inferred the following > ways to create a v8 object for representing javascript map. > i. use maplike as in > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webmid... > > ii. use v8::Map object > https://cs.chromium.org/chromium/src/v8/include/v8.h?sq=package:chromium&dr=C... > > iii. Current Implementation: HeapVector<std::pair<String, > HeapVector<ScriptWrappable>> > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > > iv. V8ObjectBuilder > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > > As suggested by +ikilpatrick, I went with iii. option and hence added a toV8 > definition to meet my requirements. However, iv. might also be a good way to do > it. > > Please advice if this is alright. The option iii looks reasonable to me.
https://codereview.chromium.org/2125213002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2125213002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:70: observerIdsToRemove.push_back(it.key); typo : the if and else condition must be swapped. Changing with next patch.
On 2016/07/13 at 01:55:19, haraken wrote: > On 2016/07/13 00:43:51, palakj1 wrote: > > Implemented IDBObserverChanges IDL map for records. > > > > After discussion with +ikilpatrick and +dehrenberg, I inferred the following > > ways to create a v8 object for representing javascript map. > > i. use maplike as in > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webmid... > > > > ii. use v8::Map object > > https://cs.chromium.org/chromium/src/v8/include/v8.h?sq=package:chromium&dr=C... > > > > iii. Current Implementation: HeapVector<std::pair<String, > > HeapVector<ScriptWrappable>> > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > > > > iv. V8ObjectBuilder > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > > > > As suggested by +ikilpatrick, I went with iii. option and hence added a toV8 > > definition to meet my requirements. However, iv. might also be a good way to do > > it. > > > > Please advice if this is alright. > > The option iii looks reasonable to me. I think eventually we'll end up with something like i. but for now keeping things simple and returning observations as an object/dictionary rather than an actual Map is probably good enough. https://codereview.chromium.org/2125213002/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:52: bool operation_type_[blink::WebIDBOperationTypeCount] = {true}; Any particular reason why the default value for operation_type_ is {true, false, false, false}? Is that just a placeholder until a future CL will initialize this with the actual types the observer wants to observe? https://codereview.chromium.org/2125213002/diff/160001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (left): https://codereview.chromium.org/2125213002/diff/160001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:443: // Indexed DB messages sent from the renderer to the browser. Why did you remove this comment? https://codereview.chromium.org/2125213002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right): https://codereview.chromium.org/2125213002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:14: // This is the equivalent to javascript { String : sequence<IDBObservation> } dictionary, I think it's fine to make this change for now and not make things more complicated by trying to figure out how to return a map, but maybe add a TODO somewhere that the spec actually does require a Map rather than just an object/dictionary.
* IDBObserverChanges idl records changed to v8::Map to support javascript map object. +haraken Does this seem fine to you? * Added observer test for changes. https://codereview.chromium.org/2125213002/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:52: bool operation_type_[blink::WebIDBOperationTypeCount] = {true}; On 2016/07/13 at 17:58:44, Marijn Kruisselbrink wrote: > Any particular reason why the default value for operation_type_ is {true, false, false, false}? Is that just a placeholder until a future CL will initialize this with the actual types the observer wants to observe? Yes. We will be initializing the values in the constructor. Sorry for the confusion. This was just for temporary testing. Removing it. https://codereview.chromium.org/2125213002/diff/160001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (left): https://codereview.chromium.org/2125213002/diff/160001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:443: // Indexed DB messages sent from the renderer to the browser. On 2016/07/13 at 17:58:45, Marijn Kruisselbrink wrote: > Why did you remove this comment? Inserted it back. https://codereview.chromium.org/2125213002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right): https://codereview.chromium.org/2125213002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:14: // This is the equivalent to javascript { String : sequence<IDBObservation> } dictionary, On 2016/07/13 at 17:58:45, Marijn Kruisselbrink wrote: > I think it's fine to make this change for now and not make things more complicated by trying to figure out how to return a map, but maybe add a TODO somewhere that the spec actually does require a Map rather than just an object/dictionary. Changed to map
This looks really great! few nits, and one copy perf improvement. https://codereview.chromium.org/2125213002/diff/180001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/180001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:832: // TODO(palakj): Get IDBDatabase object from ipc_database_id. Does this TODO still apply? https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h (right): https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h:24: void handleEvent(IDBObserverChanges&, IDBObserver&) override; nit: since we're technically not an event, can you rename this method something like handleChanges? https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right): https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:43: void IDBObserverChanges::createMap(const std::vector<WebIDBObservation>& observations, const std::vector<int32_t>& observationIndex) maybe extractChanges is a better name? I'm expecting createMap to give me back a map. https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:47: HeapVector<Member<IDBObservation>> result; Do you always have to get/set here? It's doing a lot of copying. Try grabbing a reference instead. So something like: if (!m_records.contains(key) m_records.set(key, HeapVector<Member<IDBObservation>>()); HeapVector<Member<IDBObservation>>* changes = m_records.find(key).get(); changes->append(IDBObservation::create(observations[idx])); https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h (right): https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h:24: IDBObserverChanges(IDBDatabase*, const std::vector<WebIDBObservation>&, const std::vector<int32_t>& observationIndex); Can you move this constructor back under private?
https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right): https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:47: HeapVector<Member<IDBObservation>> result; On 2016/07/13 at 23:29:13, dmurph wrote: > Do you always have to get/set here? It's doing a lot of copying. Try grabbing a reference instead. > > So something like: > > if (!m_records.contains(key) > m_records.set(key, HeapVector<Member<IDBObservation>>()); > > HeapVector<Member<IDBObservation>>* changes = m_records.find(key).get(); > changes->append(IDBObservation::create(observations[idx])); Or just something like (since add returns the existing entry rather than adding a new one if an entry already existed): m_records.add(key, HeapVector<Member<IDBObservation>>()).storedValue->value.append(IDBObseravtion::create(observations[idx]));
palakj@google.com changed reviewers: - ikilpatrick@chromium.org
Please have a look Test cases to be added. https://codereview.chromium.org/2125213002/diff/180001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/180001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:832: // TODO(palakj): Get IDBDatabase object from ipc_database_id. On 2016/07/13 at 23:29:13, dmurph wrote: > Does this TODO still apply? removed. https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h (right): https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h:24: void handleEvent(IDBObserverChanges&, IDBObserver&) override; On 2016/07/13 at 23:29:13, dmurph wrote: > nit: since we're technically not an event, can you rename this method something like handleChanges? Thanks for pointing that out. Changed https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right): https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:43: void IDBObserverChanges::createMap(const std::vector<WebIDBObservation>& observations, const std::vector<int32_t>& observationIndex) On 2016/07/13 at 23:29:13, dmurph wrote: > maybe extractChanges is a better name? I'm expecting createMap to give me back a map. True. changed. https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:47: HeapVector<Member<IDBObservation>> result; On 2016/07/13 at 23:34:39, Marijn Kruisselbrink wrote: > On 2016/07/13 at 23:29:13, dmurph wrote: > > Do you always have to get/set here? It's doing a lot of copying. Try grabbing a reference instead. > > > > So something like: > > > > if (!m_records.contains(key) > > m_records.set(key, HeapVector<Member<IDBObservation>>()); > > > > HeapVector<Member<IDBObservation>>* changes = m_records.find(key).get(); > > changes->append(IDBObservation::create(observations[idx])); > > Or just something like (since add returns the existing entry rather than adding a new one if an entry already existed): > m_records.add(key, HeapVector<Member<IDBObservation>>()).storedValue->value.append(IDBObseravtion::create(observations[idx])); Wow! I was really looking for some suggestions here. But never thought it could be a one liner. Thanks a lot. https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h (right): https://codereview.chromium.org/2125213002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h:24: IDBObserverChanges(IDBDatabase*, const std::vector<WebIDBObservation>&, const std::vector<int32_t>& observationIndex); On 2016/07/13 at 23:29:13, dmurph wrote: > Can you move this constructor back under private? done.
Awesome awesome! Last thing: for the layout tests, can you separate each test case out into it's own test? That helps us debug errors, as they fail independently instead of one large test that fails. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:78: var description = 'observer changes test'; don't need this, right?
https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:53: int32_t id_ = kInvalidId; /* ipc_database_id */ Match the surrounding comment style (// on preceding lines) Also, 'ipc_database_id' is poorly named inside IndexedDBDispatcherHost::Add - a better description would be idb_connection_id. (It's just that a connection is called 'IDBDatabase' on the blink side, to match script/spec) https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:548: bool includeTransaction, Would it make sense to bundle these up into an IndexedDBObserver::Options struct (passed by value) to avoid 3 args everywhere ? Not critical, I could just imagine more options in the future making this more verbose. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observation.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observation.h:15: Can you add a comment about the purpose of this class? https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observation.h:16: class CONTENT_EXPORT IndexedDBObservation { Is CONTENT_EXPORT needed here? https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.cc:19: for (int i = 0; i < 4; i++) No magic number: use blink::WebIDBOperationTypeCount (or, ideally, std::bitset::set()) https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:52: bool operation_type_[blink::WebIDBOperationTypeCount]; How about std::bitset rather than an array of bools? https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.cc:17: observation_indices_map_[observer_id].push_back(observations_.size() - 1); Add DCHECK(!observations_.empty()) ? https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.h:5: // This class holds all observed operations of a transaction corresponding to a Move this comment down to just above class {} https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:353: // SendObservations must happen before Oncomplete is fired to align with the nit: OnComplete Also, maybe: "happen" -> "be called", and "fired" -> "called" (methods are called, events are fired; I know we're not consistent in our existing comments, though) https://codereview.chromium.org/2125213002/diff/200001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:832: // An observer might have been removed from the render, but still exist in nit: "renderer" Also, can you word-wrap this comment as a paragraph rather than sentence-by-sentence? https://codereview.chromium.org/2125213002/diff/200001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:836: // Is there a case, when unobserve call has been made, but the observer has This looks like a question. Can you make it a TODO ? https://codereview.chromium.org/2125213002/diff/200001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:478: // Indexed DB messages sent from the render to the browser. This should be "renderer" https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:1: /* This test checks the following Please use // comments rather than /* */, like the other tests in these directories. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:15: (function(){ nit: space between () and { https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:28: if(actual.type == 'clear') { nit: space between if and ( https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:37: if(actual.type == 'kDelete') { ditto https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:41: assert_equals(actual.value, null , 'observation value'); Add a TODO that this will need to be updated? https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:45: assert_equals(changes.database.name, dbname, 'database'); Can you make the assertion messages clearer? Ideally they correspond to claims about how the results match the spec, e.g. "The change record's database should be the same as the database being acted on" https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:98: for(i = 1; i <= 4; i++) nit: space between for and ( https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:127: }, 'observer changes test'); Can you describe more about what is being tested? https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp:25: return ScriptValue::from(scriptState, IDBAny::createUndefined()); I'd lean towards: return ScriptValue(scriptState, v8::Undefined(scriptState->isolate())); ... but not strongly. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp:33: if (!m_value) { Not this CL but: don't need {} if every branch of if/else is single line. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp:38: ScriptValue scriptValue = ScriptValue::from(scriptState, value); Not this CL but: why is there a temp variable here rather than just returning? https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:14: // This is the javascript Map object from String to list of IDBObservation, nit: extra space, and either say "script" or "JavaScript" nit: should list be Array ? Also, maybe "... String (object store name) to ..." https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:11: #include <vector> Not allowed... https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:31: void onChange(const std::vector<WebIDBObservation>&, std::vector<int32_t> observationIndex); Can't use std::vector here - use WebVector (which will should wrap/convert a std::vector automagically) https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h:23: virtual void onChange(const std::vector<WebIDBObservation>&, std::vector<int32_t> observationIndex) = 0; These need to be WebVector
https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:549: bool noRecords, +1 to jsbell's suggestion. Also naming convention for Chromium code is unix_style: so no_records, etc. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:26: bool includeTransaction, unix_style. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.h:36: std::map<int32_t, std::vector<int32_t>> release_observation_indices_map() { Are observation_indices_map_ and observations_ linked? If one is released and the other is not are we going to get into trouble? https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:469: void IndexedDBTransaction::AddPendingObserver(int32_t observer_id, Nothing wrong here, but you need to fix all calls to this function, specifically content/browser/indexed_db/indexed_db_transaction_unittest.cc
After chatting with Josh, we decided that the more expansive test suite that we discussed can be in a followup patch, as we're getting pretty close to the wire here with your internship. lgtm w/ Josh's and Chris's comments addresssed.
Tests not complete yet. Please have a look. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:53: int32_t id_ = kInvalidId; /* ipc_database_id */ On 2016/07/14 at 20:08:13, jsbell wrote: > Match the surrounding comment style (// on preceding lines) > > Also, 'ipc_database_id' is poorly named inside IndexedDBDispatcherHost::Add - a better description would be idb_connection_id. (It's just that a connection is called 'IDBDatabase' on the blink side, to match script/spec) Do you mean we should rename all 'ipc_database_id' to idb_connection_id or just within IndexedDBDispatcherHost::Add? https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:548: bool includeTransaction, On 2016/07/14 at 20:08:13, jsbell wrote: > Would it make sense to bundle these up into an IndexedDBObserver::Options struct (passed by value) to avoid 3 args everywhere ? Not critical, I could just imagine more options in the future making this more verbose. True. Done. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observation.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observation.h:15: On 2016/07/14 at 20:08:13, jsbell wrote: > Can you add a comment about the purpose of this class? Done. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observation.h:16: class CONTENT_EXPORT IndexedDBObservation { On 2016/07/14 at 20:08:13, jsbell wrote: > Is CONTENT_EXPORT needed here? Removed. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.cc:19: for (int i = 0; i < 4; i++) On 2016/07/14 at 20:08:13, jsbell wrote: > No magic number: use blink::WebIDBOperationTypeCount > > (or, ideally, std::bitset::set()) Done. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:26: bool includeTransaction, On 2016/07/14 at 21:11:35, cmumford wrote: > unix_style. changed. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:52: bool operation_type_[blink::WebIDBOperationTypeCount]; On 2016/07/14 at 20:08:13, jsbell wrote: > How about std::bitset rather than an array of bools? Never knew about it. Changing. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.cc:17: observation_indices_map_[observer_id].push_back(observations_.size() - 1); On 2016/07/14 at 20:08:13, jsbell wrote: > Add DCHECK(!observations_.empty()) ? done. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer_changes.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.h:5: // This class holds all observed operations of a transaction corresponding to a On 2016/07/14 at 20:08:13, jsbell wrote: > Move this comment down to just above class {} Done. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer_changes.h:36: std::map<int32_t, std::vector<int32_t>> release_observation_indices_map() { On 2016/07/14 at 21:11:35, cmumford wrote: > Are observation_indices_map_ and observations_ linked? If one is released and the other is not are we going to get into trouble? We use observation_indices_map to retrieve an index idx and look into observations_[idx] . They do not hold any reference to each other. The release happens only when we are sending an ipc callback message in IndexedDBDispatcherHost::ConvertObserverChanges. observation_indices_map_ is released first and then observations_. Would that be an issue? https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:353: // SendObservations must happen before Oncomplete is fired to align with the On 2016/07/14 at 20:08:13, jsbell wrote: > nit: OnComplete > > Also, maybe: "happen" -> "be called", and "fired" -> "called" (methods are called, events are fired; I know we're not consistent in our existing comments, though) My bad. Corrected. https://codereview.chromium.org/2125213002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:469: void IndexedDBTransaction::AddPendingObserver(int32_t observer_id, On 2016/07/14 at 21:11:35, cmumford wrote: > Nothing wrong here, but you need to fix all calls to this function, specifically content/browser/indexed_db/indexed_db_transaction_unittest.cc Done. thanks for pointing that out. https://codereview.chromium.org/2125213002/diff/200001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/200001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:832: // An observer might have been removed from the render, but still exist in On 2016/07/14 at 20:08:13, jsbell wrote: > nit: "renderer" > > Also, can you word-wrap this comment as a paragraph rather than sentence-by-sentence? Done. https://codereview.chromium.org/2125213002/diff/200001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:836: // Is there a case, when unobserve call has been made, but the observer has On 2016/07/14 at 20:08:13, jsbell wrote: > This looks like a question. Can you make it a TODO ? Done. https://codereview.chromium.org/2125213002/diff/200001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2125213002/diff/200001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:478: // Indexed DB messages sent from the render to the browser. On 2016/07/14 at 20:08:13, jsbell wrote: > This should be "renderer" Done. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:1: /* This test checks the following On 2016/07/14 at 20:08:14, jsbell wrote: > Please use // comments rather than /* */, like the other tests in these directories. Done. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:1: /* This test checks the following On 2016/07/14 at 20:08:14, jsbell wrote: > Please use // comments rather than /* */, like the other tests in these directories. Done. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp:25: return ScriptValue::from(scriptState, IDBAny::createUndefined()); On 2016/07/14 at 20:08:14, jsbell wrote: > I'd lean towards: > > return ScriptValue(scriptState, v8::Undefined(scriptState->isolate())); > > ... but not strongly. Was looking for something like that. Thanks for pointing it out. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp:33: if (!m_value) { On 2016/07/14 at 20:08:14, jsbell wrote: > Not this CL but: don't need {} if every branch of if/else is single line. Done. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:14: // This is the javascript Map object from String to list of IDBObservation, On 2016/07/14 at 20:08:14, jsbell wrote: > nit: extra space, and either say "script" or "JavaScript" > nit: should list be Array ? > > Also, maybe "... String (object store name) to ..." Done. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:11: #include <vector> On 2016/07/14 at 20:08:14, jsbell wrote: > Not allowed... Okay. Had used it in my previous cl as well. Changing that. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:31: void onChange(const std::vector<WebIDBObservation>&, std::vector<int32_t> observationIndex); On 2016/07/14 at 20:08:14, jsbell wrote: > Can't use std::vector here - use WebVector (which will should wrap/convert a std::vector automagically) Done. https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h (right): https://codereview.chromium.org/2125213002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h:23: virtual void onChange(const std::vector<WebIDBObservation>&, std::vector<int32_t> observationIndex) = 0; On 2016/07/14 at 20:08:14, jsbell wrote: > These need to be WebVector changed.
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Test added. Please have a look.
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Thanks for making things a little more extendable on the testing front. We can expand that in the next patch easily. lgtm still
https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:54: int32_t id_ = kInvalidId; Style nit (ignore me if other people suggested to do it this way): I think we normally set default values for fields in the constructor(s), rather than doing it here. I don't think it matters too much though. https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:567: // TODO(palakj): Augment the function with IDBValue later. It's always a good idea to refer to a crbug.com link in a TODO. https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:598: for (const auto& conn : connections_) { Nit: Up above (line 572) you said for (const auto* connection : connections_) and here you use a reference (& instead of *) and call it |conn|. I think it's best to be consistent; the general Chrome pattern is for const things to be const references, and to use complete words for identifiers. Thus I suggest using for (const auto& connection : connections_) on line 572, and then doing the same here. Note that references use the . operator, rather than the -> operator, so you'll have to change that in |FilterObservation| and below on line 599. https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:336: // TODO(palakj): Modify function for indexed_db_value. Always mention a crbug.com link in a TODO. https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observation.h (right): https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... content/browser/indexed_db/indexed_db_observation.h:22: IndexedDBObservation(int32_t object_store_id, OperationType); Give these arguments names as well as types. (Use the same names as you use in the .cc file.)
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The patch is almost complete w/ tests and a minor fix in IDBObservation (replacing 'kDelete' to 'delete'). Please have a look. https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:54: int32_t id_ = kInvalidId; On 2016/07/16 at 00:03:14, palmer wrote: > Style nit (ignore me if other people suggested to do it this way): I think we normally set default values for fields in the constructor(s), rather than doing it here. > > I don't think it matters too much though. It was suggested by jsbell that I do it here, since it's constant always. https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:567: // TODO(palakj): Augment the function with IDBValue later. On 2016/07/16 at 00:03:14, palmer wrote: > It's always a good idea to refer to a crbug.com link in a TODO. Done https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:598: for (const auto& conn : connections_) { On 2016/07/16 at 00:03:14, palmer wrote: > Nit: Up above (line 572) you said > > for (const auto* connection : connections_) > > and here you use a reference (& instead of *) and call it |conn|. > > I think it's best to be consistent; the general Chrome pattern is for const things to be const references, and to use complete words for identifiers. > > Thus I suggest using > > for (const auto& connection : connections_) > > on line 572, and then doing the same here. > > Note that references use the . operator, rather than the -> operator, so you'll have to change that in |FilterObservation| and below on line 599. Done https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:336: // TODO(palakj): Modify function for indexed_db_value. On 2016/07/16 at 00:03:14, palmer wrote: > Always mention a crbug.com link in a TODO. Done. https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observation.h (right): https://codereview.chromium.org/2125213002/diff/260001/content/browser/indexe... content/browser/indexed_db/indexed_db_observation.h:22: IndexedDBObservation(int32_t object_store_id, OperationType); On 2016/07/16 at 00:03:14, palmer wrote: > Give these arguments names as well as types. (Use the same names as you use in the .cc file.) Done.
lgtm https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (left): https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:51: nit: don't delete this blank line https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:328: for (auto& observation : changes->release_observations()) { nit: no need for {} for single-line single-statement for loops https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:35: Options(Options&); This is an odd constructor? Copy constructors generally take their arguments by const reference. https://codereview.chromium.org/2125213002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp (right): https://codereview.chromium.org/2125213002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:53: if (changesHandle.IsEmpty()) { nit: no {} for single-line single-statement ifs https://codereview.chromium.org/2125213002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverInit.idl (right): https://codereview.chromium.org/2125213002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverInit.idl:13: IDBObservationType[] operationTypes = []; this should be sequence<IDBObservationType>
Please have a look. Tests to be added with another cl. https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (left): https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:51: On 2016/07/18 at 22:21:11, Marijn Kruisselbrink wrote: > nit: don't delete this blank line Done https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:328: for (auto& observation : changes->release_observations()) { On 2016/07/18 at 22:21:11, Marijn Kruisselbrink wrote: > nit: no need for {} for single-line single-statement for loops Done https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/300001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:35: Options(Options&); On 2016/07/18 at 22:21:11, Marijn Kruisselbrink wrote: > This is an odd constructor? Copy constructors generally take their arguments by const reference. Modified https://codereview.chromium.org/2125213002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp (right): https://codereview.chromium.org/2125213002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:53: if (changesHandle.IsEmpty()) { On 2016/07/18 at 22:21:11, Marijn Kruisselbrink wrote: > nit: no {} for single-line single-statement ifs Done https://codereview.chromium.org/2125213002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverInit.idl (right): https://codereview.chromium.org/2125213002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverInit.idl:13: IDBObservationType[] operationTypes = []; On 2016/07/18 at 22:21:11, Marijn Kruisselbrink wrote: > this should be sequence<IDBObservationType> Done
lgtm with a few last nits. I did not look at the updated tests in detail, though. https://codereview.chromium.org/2125213002/diff/320001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/320001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:27: // Operation types and corresponding bits are add(0), put(1), delete(2), Change the comment to reference WebIDBOperationType rather than duplicating the names/values https://codereview.chromium.org/2125213002/diff/320001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:44: const std::set<int64_t>& GetObjectStoreIds() const { Since it's a simple accessor, could be named: object_store_ids() https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObservation.idl (right): https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObservation.idl:10: // FIXME: Operation type being returned as kDelete instead of delete. TODO(palakj) not FIXME. https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:33: m_operationTypes[0] = options.operationTypes().contains("add"); #include "modules/IndexedDBNames.h" and then this should be: IndexedDBNames::add (etc) https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:33: m_operationTypes[0] = options.operationTypes().contains("add"); [0] should be [WebIDBAdd] https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:35: m_operationTypes[2] = options.operationTypes().contains("delete"); This ends up being IndexedDBNames::kDelete I guess? https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:38: std::bitset<WebIDBOperationTypeCount> operationTypes() const Return a reference, rather than a copy. https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:40: return m_operationTypes; You can put this all on one line. https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:56: // Operation types and corresponding bits are add(0), put(1), delete(2), clear(3). Can you reference WebIDBOperationType in the comment, rather than duplicating the names/values?
Please have a look. https://codereview.chromium.org/2125213002/diff/320001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2125213002/diff/320001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:27: // Operation types and corresponding bits are add(0), put(1), delete(2), On 2016/07/18 at 23:38:57, jsbell wrote: > Change the comment to reference WebIDBOperationType rather than duplicating the names/values Done. https://codereview.chromium.org/2125213002/diff/320001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:44: const std::set<int64_t>& GetObjectStoreIds() const { On 2016/07/18 at 23:38:57, jsbell wrote: > Since it's a simple accessor, could be named: object_store_ids() Done https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObservation.idl (right): https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObservation.idl:10: // FIXME: Operation type being returned as kDelete instead of delete. On 2016/07/18 at 23:38:57, jsbell wrote: > TODO(palakj) not FIXME. Sorry. Misinterpreted the usage of FIXME https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:33: m_operationTypes[0] = options.operationTypes().contains("add"); On 2016/07/18 at 23:38:57, jsbell wrote: > #include "modules/IndexedDBNames.h" and then this should be: > > IndexedDBNames::add > > (etc) Done https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:35: m_operationTypes[2] = options.operationTypes().contains("delete"); On 2016/07/18 at 23:38:57, jsbell wrote: > This ends up being IndexedDBNames::kDelete I guess? done https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:38: std::bitset<WebIDBOperationTypeCount> operationTypes() const On 2016/07/18 at 23:38:57, jsbell wrote: > Return a reference, rather than a copy. Done https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:40: return m_operationTypes; On 2016/07/18 at 23:38:57, jsbell wrote: > You can put this all on one line. Done https://codereview.chromium.org/2125213002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:56: // Operation types and corresponding bits are add(0), put(1), delete(2), clear(3). On 2016/07/18 at 23:38:57, jsbell wrote: > Can you reference WebIDBOperationType in the comment, rather than duplicating the names/values? Done
Description was changed from ========== [IndexedDB] Propogating changes to observers * Each transaction has a connection-changes map. * Changes contain a set of observations and map of observer_id to list of indices from observations that the observer is listening to. Current Implementation and Future Scope: https://docs.google.com/a/google.com/document/d/1viweV7zje2vK9IC-_G0PYlWgGrBh... Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 ========== to ========== [IndexedDB] Propogating changes to observers * Changes received from browser are multiplexed and passed on to each observer. * Each observer extracts the required subset of observations from bulk observations of the connnection using its array of indices. * The callback is fired with a map of object store to array of observations. Current Implementation and Future Scope: https://docs.google.com/a/google.com/document/d/1viweV7zje2vK9IC-_G0PYlWgGrBh... Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 ==========
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/19 05:43:21, palakj1 wrote: You can move the IDBObserverChangesRecord.idl changes out of this CL too (just delete them in the 1st CL, remove the build entries, and then rebase).
On 2016/07/19 at 05:50:11, dmurph wrote: > On 2016/07/19 05:43:21, palakj1 wrote: > > You can move the IDBObserverChangesRecord.idl changes out of this CL too (just delete them in the 1st CL, remove the build entries, and then rebase). Thanks for pointing that out. Didn't notice they were here. Removing them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please have a look.
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
most (or all) are nits. https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:137: web_observation.keyRange = Are you missing "value" - or maybe a TODO here? https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:175: IPC_MESSAGE_HANDLER(IndexedDBMsg_DatabaseCallbacksChanges, OnDatabaseChange) Should this be OnDatabaseChanges (plural) for symmetry? https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:197: static_cast<unsigned short>(observer->operationTypes().to_ulong()); Instead of using "unsigned short" we should be using int16_t https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:832: for (auto& obs : changes.observation_index) { Nit: When iterating a map we usually use "it" for iterator - even for range-based for-loop's. https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:837: // from fix line breaks. https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:10: var actual_obsv = actual.records.get(key); Nit: "obsv" is a nonstandard abbreviation and doesn't save much. https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:13: for (i in expected_obsv) Put "var" before "i" to avoid putting in window scope. Also, elsewhere in this file. https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:38: assert_equals(actual.value, null, 'Put/Add operation value does not match'); Nit: Message is _slightly_ confusing. Maybe something like "Put/Add should be nil"??? The value is ignored here right? https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:41: function countCallbacks(actual, expected){ Nit: space before brace. https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:18: var tx2 = db.transaction('store', 'readwrite'); Should probably also have: tx1.onerror = t.unreached_func('transaction should not fail'); tx2.onerror = t.unreached_func('transaction should not fail'); Otherwise the test timeout which will slow down the bots. https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:20: tx2.objectStore('store').put(1,1); Nit: space before second param. https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:45: tx2.objectStore('store').put(1,1); space. https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:134: }, 'Abort transaction observer is recording'); Nit: slightly odd title. https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:33: m_operationTypes[0] = options.operationTypes().contains("add"); Use WebIDBAdd, etc. instead of hard-coded constants. https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:36: m_operationTypes[3] = options.operationTypes().contains("clear"); Also: DCHECK(m_operationTypes.size(), WebIDBOperationTypeCount); Just in case we add a new operation and forget to update this function. https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h:19: virtual void handleChanges(IDBObserverChanges&, IDBObserver&) = 0; first param => const https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:14: // This is the Javascript Map object from String (object store name) to Array of IDBObservation. Nit: delete "This is the Javascript". https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:27: bool transaction() { return m_observer->transaction(); } make all functions const.
Please have a look https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:137: web_observation.keyRange = On 2016/07/20 at 00:25:12, cmumford wrote: > Are you missing "value" - or maybe a TODO here? added a todo https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:175: IPC_MESSAGE_HANDLER(IndexedDBMsg_DatabaseCallbacksChanges, OnDatabaseChange) On 2016/07/20 at 00:25:12, cmumford wrote: > Should this be OnDatabaseChanges (plural) for symmetry? Changed https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:197: static_cast<unsigned short>(observer->operationTypes().to_ulong()); On 2016/07/20 at 00:25:12, cmumford wrote: > Instead of using "unsigned short" we should be using int16_t Changed https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:832: for (auto& obs : changes.observation_index) { On 2016/07/20 at 00:25:12, cmumford wrote: > Nit: When iterating a map we usually use "it" for iterator - even for range-based for-loop's. Done https://codereview.chromium.org/2125213002/diff/460001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:837: // from On 2016/07/20 at 00:25:12, cmumford wrote: > fix line breaks. done https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:10: var actual_obsv = actual.records.get(key); On 2016/07/20 at 00:25:12, cmumford wrote: > Nit: "obsv" is a nonstandard abbreviation and doesn't save much. https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Changed to observation https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:13: for (i in expected_obsv) On 2016/07/20 at 00:25:12, cmumford wrote: > Put "var" before "i" to avoid putting in window scope. Also, elsewhere in this file. done https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:38: assert_equals(actual.value, null, 'Put/Add operation value does not match'); On 2016/07/20 at 00:25:12, cmumford wrote: > Nit: Message is _slightly_ confusing. Maybe something like "Put/Add should be nil"??? The value is ignored here right? done https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:41: function countCallbacks(actual, expected){ On 2016/07/20 at 00:25:12, cmumford wrote: > Nit: space before brace. done https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:18: var tx2 = db.transaction('store', 'readwrite'); On 2016/07/20 at 00:25:12, cmumford wrote: > Should probably also have: > > tx1.onerror = t.unreached_func('transaction should not fail'); > tx2.onerror = t.unreached_func('transaction should not fail'); > > Otherwise the test timeout which will slow down the bots. thanks for pointing this out. https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:20: tx2.objectStore('store').put(1,1); On 2016/07/20 at 00:25:12, cmumford wrote: > Nit: space before second param. done https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:45: tx2.objectStore('store').put(1,1); On 2016/07/20 at 00:25:12, cmumford wrote: > space. done https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:134: }, 'Abort transaction observer is recording'); On 2016/07/20 at 00:25:12, cmumford wrote: > Nit: slightly odd title. changed https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:33: m_operationTypes[0] = options.operationTypes().contains("add"); On 2016/07/20 at 00:25:13, cmumford wrote: > Use WebIDBAdd, etc. instead of hard-coded constants. changed https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:36: m_operationTypes[3] = options.operationTypes().contains("clear"); On 2016/07/20 at 00:25:13, cmumford wrote: > Also: > > DCHECK(m_operationTypes.size(), WebIDBOperationTypeCount); > > Just in case we add a new operation and forget to update this function. Done https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h:19: virtual void handleChanges(IDBObserverChanges&, IDBObserver&) = 0; On 2016/07/20 at 00:25:13, cmumford wrote: > first param => const done https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:14: // This is the Javascript Map object from String (object store name) to Array of IDBObservation. On 2016/07/20 at 00:25:13, cmumford wrote: > Nit: delete "This is the Javascript". done https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverInit.idl (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverInit.idl:13: sequence<IDBObservationType> operationTypes; Need to initialize this sequence<IDBObservationType> operationTypes = [] https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:27: bool transaction() { return m_observer->transaction(); } On 2016/07/20 at 00:25:13, cmumford wrote: > make all functions const. done.
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h (right): https://codereview.chromium.org/2125213002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h:19: virtual void handleChanges(IDBObserverChanges&, IDBObserver&) = 0; On 2016/07/20 at 01:33:36, palakj1 wrote: > On 2016/07/20 at 00:25:13, cmumford wrote: > > first param => const > > done I later realized that IDBObserverChanges is being later passed toV8 by reference. Does it makes sense to make it constant. Also, content unittests failed due to this change. Reverting back to non const. Let me know if this sounds okay.
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm
Sorry about the review delay. Mostly looks good! https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:59: exceptionCatcher.SetVerbose(true); Would you help me understand why you need the TryCatch block? If you need the TryCatch block, don't you need to rethrow the exception caught? https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp:176: SECURITY_DCHECK(it != m_metadata.objectStores.end()); SECURITY_DCHECK => DCHECK I don't think this is security-sensitive. https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp:25: return ScriptValue::from(scriptState, v8::Undefined(scriptState->isolate())); Haven't you made this change in https://codereview.chromium.org/2160163002/? https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:80: m_observerIds.swap(observersToRetain); Can we use m_observerIds.removeAll() instead of building up another hash map? https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:26: v8CallOrCrash(map->Set(context, key, value)); Use CreateDataProperty instead of Set. If the setter of Map is overridden, map->Set() may execute the overridden setter and cause unexpected side-effects.
https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp:176: SECURITY_DCHECK(it != m_metadata.objectStores.end()); On 2016/07/20 16:22:11, haraken wrote: > > SECURITY_DCHECK => DCHECK > > I don't think this is security-sensitive. The objectStoreId in this case was sent browser->renderer; this could fail if the process or IPC is compromised. Based on past guidance, we use ASSERT_WITH_SECURITY_IMPLICATIONS (the old name for the macro) elsewhere for similar cases. But since we dereference `it` immediately (which will crash) I think it's OK to change to a DCHECK.
https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:59: exceptionCatcher.SetVerbose(true); On 2016/07/20 at 16:22:11, haraken wrote: > Would you help me understand why you need the TryCatch block? If you need the TryCatch block, don't you need to rethrow the exception caught? I had referenced this from here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... This usage seems to align with the other instances of v8::TryCatch. However, I am not entirely sure as to why dont need a rethrow. I shall try to get back to you on this soon. https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBDatabase.cpp:176: SECURITY_DCHECK(it != m_metadata.objectStores.end()); On 2016/07/20 at 16:22:11, haraken wrote: > SECURITY_DCHECK => DCHECK > > I don't think this is security-sensitive. As re-confimred by jsbell, changing back to DCHECK. https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObservation.cpp:25: return ScriptValue::from(scriptState, v8::Undefined(scriptState->isolate())); On 2016/07/20 at 16:22:12, haraken wrote: > Haven't you made this change in https://codereview.chromium.org/2160163002/? No, I had later modified IDBObservation in 2160163002 to have only the renaming changes. Since, we were using WebIDBObsevation which is a part of this cl. Sorry, for the confusion. https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:80: m_observerIds.swap(observersToRetain); On 2016/07/20 at 16:22:12, haraken wrote: > Can we use m_observerIds.removeAll() instead of building up another hash map? I believed swapping would be faster, since, removeAll needs to iterate over all the elements to be removed. But it is definitely cleaner. I can switch to that. https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:26: v8CallOrCrash(map->Set(context, key, value)); On 2016/07/20 at 16:22:12, haraken wrote: > Use CreateDataProperty instead of Set. If the setter of Map is overridden, map->Set() may execute the overridden setter and cause unexpected side-effects. Is using CreateDataProperty on map identical to Set. Doing something like map->CreateDataProperty(context, key, value) in IDBObserverChanges.cpp and in javascript : function callback(changes) changes.records.get(key) returns NaN with CreateDataProperty while it works fine with Set. Could you elaborate the usage
https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:26: v8CallOrCrash(map->Set(context, key, value)); On 2016/07/20 18:22:11, palakj1 wrote: > On 2016/07/20 at 16:22:12, haraken wrote: > > Use CreateDataProperty instead of Set. If the setter of Map is overridden, > map->Set() may execute the overridden setter and cause unexpected side-effects. > > Is using CreateDataProperty on map identical to Set. > Doing something like > map->CreateDataProperty(context, key, value) in IDBObserverChanges.cpp > > and in javascript : > function callback(changes) > changes.records.get(key) > > returns NaN with CreateDataProperty while it works fine with Set. > > Could you elaborate the usage CreateDataProperty is definitely not correct here; that's for e.g. x = {}; x['k'] = v. In this case, map is a Map and and desire is to populate it as if calling m = new Map; m.set('k', v); - but yes, we want to run the "initial" version of set(), not the current value of Map.prototype.set I can't follow the v8 code enough to know for certain if v8::Map::Set is overridden by a change to Map.prototype.set; the impl (src/api.cc) looks like it calls into the registered 'map_set' which maps to MapSet (src/js/collection.js); this does not appear to go through properties on the instance itself, but the indirection in src/contexts.h is not clear to me. palakj: you could test by running on a page which does e.g. Map.prototype.set = function(k, v) { console.log('overridden!', k, v); } and then ensure that observation maps are populated (i.e. the overridden code did not execute)
On 2016/07/20 at 18:41:48, jsbell wrote: > https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right): > > https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:26: v8CallOrCrash(map->Set(context, key, value)); > On 2016/07/20 18:22:11, palakj1 wrote: > > On 2016/07/20 at 16:22:12, haraken wrote: > > > Use CreateDataProperty instead of Set. If the setter of Map is overridden, > > map->Set() may execute the overridden setter and cause unexpected side-effects. > > > > Is using CreateDataProperty on map identical to Set. > > Doing something like > > map->CreateDataProperty(context, key, value) in IDBObserverChanges.cpp > > > > and in javascript : > > function callback(changes) > > changes.records.get(key) > > > > returns NaN with CreateDataProperty while it works fine with Set. > > > > Could you elaborate the usage > > CreateDataProperty is definitely not correct here; that's for e.g. x = {}; x['k'] = v. In this case, map is a Map and and desire is to populate it as if calling m = new Map; m.set('k', v); - but yes, we want to run the "initial" version of set(), not the current value of Map.prototype.set > > I can't follow the v8 code enough to know for certain if v8::Map::Set is overridden by a change to Map.prototype.set; the impl (src/api.cc) looks like it calls into the registered 'map_set' which maps to MapSet (src/js/collection.js); this does not appear to go through properties on the instance itself, but the indirection in src/contexts.h is not clear to me. > > palakj: you could test by running on a page which does e.g. Map.prototype.set = function(k, v) { console.log('overridden!', k, v); } and then ensure that observation maps are populated (i.e. the overridden code did not execute) I haven't tested it yet. But, +adamk pointed out that the user cannot override the Set function. I will still try the tests suggested by jsbell.
palakj@google.com changed reviewers: + cmumford@chromium.org, dmurph@chromium.org, haraken@chromium.org, jsbell@chromium.org, mek@chromium.org
https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:59: exceptionCatcher.SetVerbose(true); On 2016/07/20 18:22:11, palakj1 wrote: > On 2016/07/20 at 16:22:11, haraken wrote: > > Would you help me understand why you need the TryCatch block? If you need the > TryCatch block, don't you need to rethrow the exception caught? > > I had referenced this from here > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > This usage seems to align with the other instances of v8::TryCatch. However, I > am not entirely sure as to why dont need a rethrow. I shall try to get back to > you on this soon. Honestly speaking, it's hard to say if you need the TryCatch block or not without having tests. If you don't have any specific reason, it might be better to just remove the TryCatch block and add it back when you or daniel add tests for it. https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:26: v8CallOrCrash(map->Set(context, key, value)); On 2016/07/20 18:41:48, jsbell wrote: > On 2016/07/20 18:22:11, palakj1 wrote: > > On 2016/07/20 at 16:22:12, haraken wrote: > > > Use CreateDataProperty instead of Set. If the setter of Map is overridden, > > map->Set() may execute the overridden setter and cause unexpected > side-effects. > > > > Is using CreateDataProperty on map identical to Set. > > Doing something like > > map->CreateDataProperty(context, key, value) in IDBObserverChanges.cpp > > > > and in javascript : > > function callback(changes) > > changes.records.get(key) > > > > returns NaN with CreateDataProperty while it works fine with Set. > > > > Could you elaborate the usage > > CreateDataProperty is definitely not correct here; that's for e.g. x = {}; > x['k'] = v. In this case, map is a Map and and desire is to populate it as if > calling m = new Map; m.set('k', v); - but yes, we want to run the "initial" > version of set(), not the current value of Map.prototype.set > > I can't follow the v8 code enough to know for certain if v8::Map::Set is > overridden by a change to Map.prototype.set; the impl (src/api.cc) looks like it > calls into the registered 'map_set' which maps to MapSet (src/js/collection.js); > this does not appear to go through properties on the instance itself, but the > indirection in src/contexts.h is not clear to me. > > palakj: you could test by running on a page which does e.g. Map.prototype.set = > function(k, v) { console.log('overridden!', k, v); } and then ensure that > observation maps are populated (i.e. the overridden code did not execute) Thanks for the clarification! Yeah, I realized that CreateDataProperty is definitely not correct.
adamk@chromium.org changed reviewers: + adamk@chromium.org
https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right): https://codereview.chromium.org/2125213002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:26: v8CallOrCrash(map->Set(context, key, value)); On 2016/07/20 18:41:48, jsbell wrote: > On 2016/07/20 18:22:11, palakj1 wrote: > > On 2016/07/20 at 16:22:12, haraken wrote: > > > Use CreateDataProperty instead of Set. If the setter of Map is overridden, > > map->Set() may execute the overridden setter and cause unexpected > side-effects. > > > > Is using CreateDataProperty on map identical to Set. > > Doing something like > > map->CreateDataProperty(context, key, value) in IDBObserverChanges.cpp > > > > and in javascript : > > function callback(changes) > > changes.records.get(key) > > > > returns NaN with CreateDataProperty while it works fine with Set. > > > > Could you elaborate the usage > > CreateDataProperty is definitely not correct here; that's for e.g. x = {}; > x['k'] = v. In this case, map is a Map and and desire is to populate it as if > calling m = new Map; m.set('k', v); - but yes, we want to run the "initial" > version of set(), not the current value of Map.prototype.set > > I can't follow the v8 code enough to know for certain if v8::Map::Set is > overridden by a change to Map.prototype.set; the impl (src/api.cc) looks like it > calls into the registered 'map_set' which maps to MapSet (src/js/collection.js); > this does not appear to go through properties on the instance itself, but the > indirection in src/contexts.h is not clear to me. > > palakj: you could test by running on a page which does e.g. Map.prototype.set = > function(k, v) { console.log('overridden!', k, v); } and then ensure that > observation maps are populated (i.e. the overridden code did not execute) jsbell's reading of the code is correct: v8::Map::Set() always calls the original Map.prototype.set function, no matter what the author code may have done to it since the context was created. So calling Map::Set() is the right way to fill a Map. Apologies for the fact that Map::Set and Object::Set have the same name, I didn't know what else to call the former.
BTW, you need to get an LG from a public API owner to land this CL. It would be better to cc one of the API owners now.
LGTM https://codereview.chromium.org/2125213002/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp (right): https://codereview.chromium.org/2125213002/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:60: V8ScriptRunner::callFunction(m_callback.newLocal(m_scriptState->isolate()), m_scriptState->getExecutionContext(), thisObject, 1, argv, m_scriptState->isolate()); 1 => WTF_ARRAY_LENGTH
palakj@google.com changed reviewers: + chrishtr@chromium.org - adamk@chromium.org
Please have a look.
palakj@google.com changed reviewers: + wfh@chromium.org
palakj@google.com changed reviewers: + wfh@chromium.org
Please have a look.
Please have a look.
https://codereview.chromium.org/2125213002/diff/540001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h (right): https://codereview.chromium.org/2125213002/diff/540001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h:26: virtual void onChange(const WebVector<WebIDBObservation>&, WebVector<int32_t> observationIndex) = 0; Should the second argument be a const reference?
https://codereview.chromium.org/2125213002/diff/540001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/540001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:176: IPC_MESSAGE_HANDLER(IndexedDBMsg_DatabaseCallbacksChanges, It's unusual to add an IPC implemention without the corresponding IPC message being defined - it appears this was defined in crrev.com/2160163002 (and content/browser/indexed_db/indexed_db_database_callbacks.c is actually sending these messages?) please keep the definition of both ends of a new IPC in the same CL if possible. Thanks! https://codereview.chromium.org/2125213002/diff/540001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:199: static_cast<uint16_t>(observer->operationTypes().to_ulong()); I'd like to see some kind of static_assert that WebIDBOperationTypeCount is less than 16 and will fit in a uint16_t
Please have a look. https://codereview.chromium.org/2125213002/diff/540001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/540001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:176: IPC_MESSAGE_HANDLER(IndexedDBMsg_DatabaseCallbacksChanges, On 2016/07/20 at 19:53:28, Will Harris wrote: > It's unusual to add an IPC implemention without the corresponding IPC message being defined - it appears this was defined in crrev.com/2160163002 (and content/browser/indexed_db/indexed_db_database_callbacks.c is actually sending these messages?) please keep the definition of both ends of a new IPC in the same CL if possible. Thanks! Sorry for that. It was originally all a part of the same cl. But the cl had to be split, since it got a bit large. Splitting browser and renderer tasks was the best way I could think of. But I realize how this makes ipc message incomprehensive. It is indexed_db_database_callbacks.cc that is sending the message. https://codereview.chromium.org/2125213002/diff/540001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:199: static_cast<uint16_t>(observer->operationTypes().to_ulong()); On 2016/07/20 at 19:53:28, Will Harris wrote: > I'd like to see some kind of static_assert that WebIDBOperationTypeCount is less than 16 and will fit in a uint16_t Thanks for pointing that out. Added. https://codereview.chromium.org/2125213002/diff/540001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h (right): https://codereview.chromium.org/2125213002/diff/540001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h:26: virtual void onChange(const WebVector<WebIDBObservation>&, WebVector<int32_t> observationIndex) = 0; On 2016/07/20 at 19:51:05, chrishtr wrote: > Should the second argument be a const reference? sorry. changed
Description was changed from ========== [IndexedDB] Propogating changes to observers * Changes received from browser are multiplexed and passed on to each observer. * Each observer extracts the required subset of observations from bulk observations of the connnection using its array of indices. * The callback is fired with a map of object store to array of observations. Current Implementation and Future Scope: https://docs.google.com/a/google.com/document/d/1viweV7zje2vK9IC-_G0PYlWgGrBh... Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 ========== to ========== [IndexedDB] Propogating changes to observers * Changes received from browser are multiplexed and passed on to each observer. * Each observer extracts the required subset of observations from bulk observations of the connnection using its array of indices. * The callback is fired with a map of object store to array of observations. Current Implementation and Future Scope: https://docs.google.com/a/google.com/document/d/1th7YySjg1GYdGDPXQ56NL2a9kCbL... Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 ==========
https://codereview.chromium.org/2125213002/diff/560001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2125213002/diff/560001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:197: blink::WebIDBOperationTypeCount <= std::numeric_limits<uint16_t>::max(), should this be < sizeof(uint16_t) not max()?
lgtm LGTM for WebKit/public/*
static_assert for WebIDBOperationTypeCount corrected.
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== [IndexedDB] Propogating changes to observers * Changes received from browser are multiplexed and passed on to each observer. * Each observer extracts the required subset of observations from bulk observations of the connnection using its array of indices. * The callback is fired with a map of object store to array of observations. Current Implementation and Future Scope: https://docs.google.com/a/google.com/document/d/1th7YySjg1GYdGDPXQ56NL2a9kCbL... Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 ========== to ========== [IndexedDB] Propogating changes to observers * Changes received from browser are multiplexed and passed on to each observer. * Each observer extracts the required subset of observations from bulk observations of the connnection using its array of indices. * The callback is fired with a map of object store to array of observations. Current Implementation and Future Scope: http://goo.gl/r0eUpe Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 ==========
The CQ bit was checked by palakj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by palakj@google.com
The CQ bit was checked by palakj@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org, mek@chromium.org, jsbell@chromium.org, cmumford@chromium.org, haraken@chromium.org, chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/2125213002/#ps600001 (title: "Minor typecasting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #31 (id:600001)
Message was sent while issue was closed.
Description was changed from ========== [IndexedDB] Propogating changes to observers * Changes received from browser are multiplexed and passed on to each observer. * Each observer extracts the required subset of observations from bulk observations of the connnection using its array of indices. * The callback is fired with a map of object store to array of observations. Current Implementation and Future Scope: http://goo.gl/r0eUpe Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 ========== to ========== [IndexedDB] Propogating changes to observers * Changes received from browser are multiplexed and passed on to each observer. * Each observer extracts the required subset of observations from bulk observations of the connnection using its array of indices. * The callback is fired with a map of object store to array of observations. Current Implementation and Future Scope: http://goo.gl/r0eUpe Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 Committed: https://crrev.com/2751bda57a56b4d2bc02899a8a39e3026fccc599 Cr-Commit-Position: refs/heads/master@{#406722} ==========
Message was sent while issue was closed.
Patchset 31 (id:??) landed as https://crrev.com/2751bda57a56b4d2bc02899a8a39e3026fccc599 Cr-Commit-Position: refs/heads/master@{#406722}
Message was sent while issue was closed.
On 2016/07/21 00:22:15, commit-bot: I haz the power wrote: > Patchset 31 (id:??) landed as > https://crrev.com/2751bda57a56b4d2bc02899a8a39e3026fccc599 > Cr-Commit-Position: refs/heads/master@{#406722} Congrats!
Message was sent while issue was closed.
Description was changed from ========== [IndexedDB] Propogating changes to observers * Changes received from browser are multiplexed and passed on to each observer. * Each observer extracts the required subset of observations from bulk observations of the connnection using its array of indices. * The callback is fired with a map of object store to array of observations. Current Implementation and Future Scope: http://goo.gl/r0eUpe Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 Committed: https://crrev.com/2751bda57a56b4d2bc02899a8a39e3026fccc599 Cr-Commit-Position: refs/heads/master@{#406722} ========== to ========== [IndexedDB] Propogating changes to observers * Changes received from browser are multiplexed and passed on to each observer. * Each observer extracts the required subset of observations from bulk observations of the connnection using its array of indices. * The callback is fired with a map of object store to array of observations. Current Implementation and Future Scope: https://goo.gl/Y2dobn Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md BUG=609934 Committed: https://crrev.com/2751bda57a56b4d2bc02899a8a39e3026fccc599 Cr-Commit-Position: refs/heads/master@{#406722} ========== |
