Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2062203004:
IDBObserver: Lifetime Management: Adding Observer (Closed)
|
Created:
4 years, 6 months ago by palakj1 Modified:
4 years, 5 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-bindings_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, falken, haraken, horo+watch_chromium.org, jam, jsbell+idb_chromium.org, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, kinuko+serviceworker, michaeln, nhiroki, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[IDBObserver] Lifetime Management: Adding and Removing Observer
* Corresponding to each observe call at renderer side, an Observer object is created in the backend.
* The observer begins observing after the completion of transaction it was created with.
* The observer is removed when we have an unobserve call on observer or database connection closes.
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/05f35eab096fe52eab3af2c6db191c1285dc5a68
Cr-Commit-Position: refs/heads/master@{#404283}
Patch Set 1 #Patch Set 2 : Adding Observer #
Total comments: 27
Patch Set 3 : Unobserve functionality #
Total comments: 9
Patch Set 4 : Post dmurph review #
Total comments: 9
Patch Set 5 : Unobserve functionality: Wip #Patch Set 6 : Observer moved to connection #
Total comments: 51
Patch Set 7 : Post dmurph review #Patch Set 8 : post dmuprh review #
Total comments: 34
Patch Set 9 : Post dmurph review #
Total comments: 20
Patch Set 10 : Observer addition removal #Patch Set 11 : Observer addition and removal #
Total comments: 12
Patch Set 12 : Post dmuprh review #Patch Set 13 : Post dmurph review #
Total comments: 57
Patch Set 14 : Units tests added #
Total comments: 12
Patch Set 15 : Unit tests added #
Total comments: 4
Patch Set 16 : Expected test results changed #
Total comments: 39
Patch Set 17 : Post cmumford review #Patch Set 18 : Post cmumford review #
Total comments: 21
Patch Set 19 : Initialize id for WebIDBObserver #
Total comments: 2
Patch Set 20 : Final architechture #Dependent Patchsets: Messages
Total messages: 81 (20 generated)
Description was changed from ========== observer added observer added observer added observe added to database observer running observe database observe function Correction to V8IDBObserverCallback.cpp todo added idl observer changes interface idl observer changes interface Obsever changes changesrecord interfaces created format format idl changes , worker test redundant headers removed format worker test Removed redundant headers changes after mek review wiring expected files expected files for interface changes changes private property change made private property change made IndexedDB Observers with webidl files, initial constructor for creating observer and observe function. BUG= patch from issue 2031113002 at patchset 100001 (http://crrev.com/2031113002#ps100001) ========== to ========== Adds observer to database * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * As per the spec, the observer begins observing after the completion of transaction it was created with. BUG=609934 patch from issue 2031113002 at patchset 100001 (http://crrev.com/2031113002#ps100001) Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md ==========
palakj@google.com changed reviewers: + dmurph@chromium.org, mek@chromium.org
Patch adds observer to the database. Here's the outline of the working: * Each observer call at the renderer creates a new observer entry. * Only observer id passed to browser, observer callback is stored on the dispatcher. * The host observer object added to a pending_observer list in transaction (passed as argument in observe call) * After transaction finishes, observer added to database's active_observer list. Please review the design
Did a first pass. We need to make sure we keep track of the ids in the webidbdatabase_impl, and store stuff in a map on the backend. Not sure what the id serialization functions were doing. If we need more ids to specify the renderer, then we can just add them to the hash function. https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.h:54: std::vector<std::unique_ptr<IndexedDBObserver>> active_observers_; we'll want a map here of unique id -> observer so we can easily remove as well. Generate the unique ID as outlined below. https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.h:115: int64_t HostObserverId(int64_t observer_id); Why do we need these? Can we just store the ipc_thread_id and observe_id in the observer? It looks like you're trying to combine the ids for a map? Let's do something like int64_t GenerateObserveId(int64_t ipc_thread_id, int64_t observe_id) { // Basic djb2 hash. int64_t hash = 5381; hash = ((hash << 5) + hash) + ipc_thread_id; hash = ((hash << 5) + hash) + observe_id; return hash; } https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.h:129: void AddActiveObserver(); what about 'MaybeActivatePendingObservers()' https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... content/child/indexed_db/indexed_db_dispatcher.cc:174: int64_t observer_id = maxObserverId++; If you change to IDMap, we get this id from the 'insert' call into the map. https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... content/child/indexed_db/indexed_db_dispatcher.h:77: Also add RemoveIDBObservers, which accepts a vector of IDs, and is called by webidbdatabase_impl in close() https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... content/child/indexed_db/indexed_db_dispatcher.h:78: void AddIDBObserver(int32_t ipc_database_id, Have this return the observe ID so you can store it in the webdatabase_impl https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... content/child/indexed_db/indexed_db_dispatcher.h:274: int maxObserverId = 0; remove this since IDMap will handle id allocation. https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... content/child/indexed_db/indexed_db_dispatcher.h:290: std::map<int64_t, blink::IDBObserver*> observers_; Use IDMap like above, but we need to make sure the lifetime story works. This means we'll be using a RefPtr. So something like: IDMap<RefPtr<blink::IDBObserver>, IDMapOwnPointer>> The only quirk here is that we'll be allocating the RefPtr on the heap and not the stack, as the map expects to be given a pointer. https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... File content/child/indexed_db/webidbdatabase_impl.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... content/child/indexed_db/webidbdatabase_impl.h:48: You'll also want to store a collection (vector is fine) of observe ids, which we then give to the dispatcher when 'close' is called. https://codereview.chromium.org/2062203004/diff/20001/content/common/indexed_... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/common/indexed_... content/common/indexed_db/indexed_db_messages.h:504: IndexedDBHostMsg_DatabaseObserve_Params) Probably don't need the params struct as we just have 4 items to send. https://codereview.chromium.org/2062203004/diff/20001/content/common/indexed_... content/common/indexed_db/indexed_db_messages.h:505: Also add a message to remove the observer, which has the thread_id, database_id, and observer_id
just some random comments https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.cc:200: // observer_id are guaranteed to be unique within that renderer. There is no such guarantee. There is a separate IndexedDBDispatcher for each thread in the renderer (so documents and workers don't share the same dispatcher instance). And at least as currently implemented there are no guarantees that those two separate dispatchers don't create the same IDs. Separately I'm not quite sure what these IDs are used for/why we need this complicated ID scheme. It all seems rather fragile/complicated... https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:429: pending_observers_.emplace_back(std::move(observer)); why emplace_back? You probably just want push_back. https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:434: database_->active_observers_.emplace_back(std::move(pending_observers_[i])); Same here, this should probably just be push_back https://codereview.chromium.org/2062203004/diff/20001/content/common/indexed_... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/common/indexed_... content/common/indexed_db/indexed_db_messages.h:121: IPC_STRUCT_MEMBER(int64_t, observer_id) if we actually require the observer_id to be 32 bit it seems a bit weird to send it over as a 64 bit integer. Although I must say I don't quite understand all the ID magic yet.
Unobserve: Work in progress. Please have a look. https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.h:54: std::vector<std::unique_ptr<IndexedDBObserver>> active_observers_; On 2016/06/15 at 12:49:43, dmurph wrote: > we'll want a map here of unique id -> observer so we can easily remove as well. > > Generate the unique ID as outlined below. Done. https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.cc:200: // observer_id are guaranteed to be unique within that renderer. On 2016/06/15 at 13:14:57, Marijn Kruisselbrink wrote: > There is no such guarantee. There is a separate IndexedDBDispatcher for each thread in the renderer (so documents and workers don't share the same dispatcher instance). And at least as currently implemented there are no guarantees that those two separate dispatchers don't create the same IDs. > Separately I'm not quite sure what these IDs are used for/why we need this complicated ID scheme. It all seems rather fragile/complicated... Oh, I was trying to imitate the way host transaction id are generated. Changed now to djb2 hash suggested by dmurph. https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.h:115: int64_t HostObserverId(int64_t observer_id); On 2016/06/15 at 12:49:43, dmurph wrote: > Why do we need these? Can we just store the ipc_thread_id and observe_id in the observer? > > It looks like you're trying to combine the ids for a map? Let's do something like > > int64_t GenerateObserveId(int64_t ipc_thread_id, int64_t observe_id) { > // Basic djb2 hash. > int64_t hash = 5381; > hash = ((hash << 5) + hash) + ipc_thread_id; > hash = ((hash << 5) + hash) + observe_id; > return hash; > } I see your point. Keeping connection_id and observer_id separate would help in cleaning up when connection is closed or unobserve is called. But why doesn't a simple concatenation (ipc_thread_id <<32 + observer_id) work? https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:429: pending_observers_.emplace_back(std::move(observer)); On 2016/06/15 at 13:14:57, Marijn Kruisselbrink wrote: > why emplace_back? You probably just want push_back. Changed. Never knew the difference between emplace_back and push_back uptil now. https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.cc:434: database_->active_observers_.emplace_back(std::move(pending_observers_[i])); On 2016/06/15 at 13:14:57, Marijn Kruisselbrink wrote: > Same here, this should probably just be push_back Done https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_transaction.h:129: void AddActiveObserver(); On 2016/06/15 at 12:49:43, dmurph wrote: > what about 'MaybeActivatePendingObservers()' yaa, that's a lot more intutive. changed. https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... content/child/indexed_db/indexed_db_dispatcher.h:78: void AddIDBObserver(int32_t ipc_database_id, On 2016/06/15 at 12:49:43, dmurph wrote: > Have this return the observe ID so you can store it in the webdatabase_impl Done https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... content/child/indexed_db/indexed_db_dispatcher.h:290: std::map<int64_t, blink::IDBObserver*> observers_; On 2016/06/15 at 12:49:44, dmurph wrote: > Use IDMap like above, but we need to make sure the lifetime story works. This means we'll be using a RefPtr. So something like: > IDMap<RefPtr<blink::IDBObserver>, IDMapOwnPointer>> > > The only quirk here is that we'll be allocating the RefPtr on the heap and not the stack, as the map expects to be given a pointer. Do we use IDMap cause it provides some performance benefits (auto generating id) over std::map? Or can we not use std::map in this context at all? Also, I can't make IDMap work with IDBObserver, unable to include it in header (relative path trouble) and IDMap has a concrete destructor, so forward declaration doesn't work. Deferring it to another patch. https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... File content/child/indexed_db/webidbdatabase_impl.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/child/indexed_d... content/child/indexed_db/webidbdatabase_impl.h:48: On 2016/06/15 at 12:49:44, dmurph wrote: > You'll also want to store a collection (vector is fine) of observe ids, which we then give to the dispatcher when 'close' is called. Is it okay to introduce a vector and do something concrete in WebIDB layer? The layer seems to only channel IDBdatabase calls to dispatcher and nothing more. Just a question of our convention. https://codereview.chromium.org/2062203004/diff/20001/content/common/indexed_... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/common/indexed_... content/common/indexed_db/indexed_db_messages.h:121: IPC_STRUCT_MEMBER(int64_t, observer_id) On 2016/06/15 at 13:14:57, Marijn Kruisselbrink wrote: > if we actually require the observer_id to be 32 bit it seems a bit weird to send it over as a 64 bit integer. Although I must say I don't quite understand all the ID magic yet. Changed https://codereview.chromium.org/2062203004/diff/20001/content/common/indexed_... content/common/indexed_db/indexed_db_messages.h:504: IndexedDBHostMsg_DatabaseObserve_Params) On 2016/06/15 at 12:49:44, dmurph wrote: > Probably don't need the params struct as we just have 4 items to send. Done
It looks like your dependency patchset (upstream) is patch 2. This is confusing to review, can you make the upstream branch back to this patch: https://codereview.chromium.org/2031113002/? I made some initial comments. Regarding IDMap: what problems were you having with relative paths? I believe you should be able to include that file. You probably don't need it in the header (as you'll be storing the RefPtr. The only weird thing is that you have to allocate the refptr on the heap (new RefPtr). Let me know what problems you have here. https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.h (right): https://codereview.chromium.org/2062203004/diff/20001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.h:115: int64_t HostObserverId(int64_t observer_id); On 2016/06/16 at 07:05:40, palakj1 wrote: > On 2016/06/15 at 12:49:43, dmurph wrote: > > Why do we need these? Can we just store the ipc_thread_id and observe_id in the observer? > > > > It looks like you're trying to combine the ids for a map? Let's do something like > > > > int64_t GenerateObserveId(int64_t ipc_thread_id, int64_t observe_id) { > > // Basic djb2 hash. > > int64_t hash = 5381; > > hash = ((hash << 5) + hash) + ipc_thread_id; > > hash = ((hash << 5) + hash) + observe_id; > > return hash; > > } > > I see your point. Keeping connection_id and observer_id separate would help in cleaning up when connection is closed or unobserve is called. > But why doesn't a simple concatenation (ipc_thread_id <<32 + observer_id) work? Ah, so the thread id is always int32_t, huh. Sure, that would work just fine. just make sure you cast it to a uint64_t before you bit shift. https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:559: active_observers_.erase(obs); I believe you can just do active_observers_.erase(observersToRemove[i]) without the existence check here. https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.h (right): https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.h:220: int64_t GenerateObserverId(int32_t observer_id, int32_t ipc_thread_id); How about calling the result ObserverAndThreadId? So GenerateObserverAndThreadId? That would avoid confusion about which id we have. https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.h:221: std::vector<int64_t> RemoveObservers(std::vector<int32_t> remove_observers, We still need the ipc_thread_id here as well. https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.h:223: Make sure we store a vector<std::pair<int32_t, int64_t>> in_use_database_observers, which we use on destruction to remove all of them from the databases. Populate this in OnObserve
Dependency patch set correct. https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:559: active_observers_.erase(obs); On 2016/06/16 at 07:30:33, dmurph wrote: > I believe you can just do active_observers_.erase(observersToRemove[i]) without the existence check here. Done https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:559: active_observers_.erase(obs); On 2016/06/16 at 07:30:33, dmurph wrote: > I believe you can just do active_observers_.erase(observersToRemove[i]) without the existence check here. Done https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.h (right): https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.h:220: int64_t GenerateObserverId(int32_t observer_id, int32_t ipc_thread_id); On 2016/06/16 at 07:30:33, dmurph wrote: > How about calling the result ObserverAndThreadId? So GenerateObserverAndThreadId? That would avoid confusion about which id we have. Done https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.h:221: std::vector<int64_t> RemoveObservers(std::vector<int32_t> remove_observers, On 2016/06/16 at 07:30:33, dmurph wrote: > We still need the ipc_thread_id here as well. Done https://codereview.chromium.org/2062203004/diff/40001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.h:223: On 2016/06/16 at 07:30:33, dmurph wrote: > Make sure we store a vector<std::pair<int32_t, int64_t>> in_use_database_observers, which we use on destruction to remove all of them from the databases. Populate this in OnObserve done
Leaving some architecture comments, we chatted earlier about how to get IDBObserver successfully into webidbdatabase_impl, which will require that wrapper. https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:557: Iterator obs = active_observers_.find(observersToRemove[i]); replace these with active_observers_.erase(observersToRemove[i]); https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database.h (right): https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.h:54: std::map<int64_t, IndexedDBObserver*> active_observers_; wrap IndexedDBObserver with std::unique_ptr https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.cc:644: int32_t observer_id) { can your rename the int32_t version of observer_id to local_observer_id? That makes it a little more clear that it's not a global identifier (or if you have a better name go for it). https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.cc:653: observers_.push_back(std::make_pair(observer_id, observer_thread_id)); You need to figure out how to store these so that you can use this list in the destructor to remove the observers from the database. I would suggest maybe storing map<database_id, vector<observer_and_thread_id>>, so you can lookup each database once and then remove all observers for that database. https://codereview.chromium.org/2062203004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h (right): https://codereview.chromium.org/2062203004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h:65: virtual void observe(IDBObserver*, long long) = 0; please put the argument name for 'long long', as we don't know what it is.
This is a wip. https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.cc:557: Iterator obs = active_observers_.find(observersToRemove[i]); On 2016/06/17 at 09:05:06, dmurph wrote: > replace these with active_observers_.erase(observersToRemove[i]); Done https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_database.h (right): https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_database.h:54: std::map<int64_t, IndexedDBObserver*> active_observers_; On 2016/06/17 at 09:05:06, dmurph wrote: > wrap IndexedDBObserver with std::unique_ptr Done https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2062203004/diff/60001/content/browser/indexed... content/browser/indexed_db/indexed_db_dispatcher_host.cc:644: int32_t observer_id) { On 2016/06/17 at 09:05:06, dmurph wrote: > can your rename the int32_t version of observer_id to local_observer_id? That makes it a little more clear that it's not a global identifier (or if you have a better name go for it). It's only local to the renderer thread right, not to the host? Should I directly send <ipc_thread_id.front_end_observer_id> in the message instead? Also our id's rest on database backend, not the dispatcher_host. Would ipc_thread_id.front_end_observer_id be unique for the database? Do I need to inject the dispatcher_host id as well? https://codereview.chromium.org/2062203004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h (right): https://codereview.chromium.org/2062203004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h:65: virtual void observe(IDBObserver*, long long) = 0; On 2016/06/17 at 09:05:07, dmurph wrote: > please put the argument name for 'long long', as we don't know what it is. Done
Please have a look.
quick comments https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_class_factory.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_class_factory.cc:40: IndexedDBConnection* connection, base::WeakPtr<IndexedDBConnection> https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_class_factory.cc:46: return new IndexedDBTransaction(id, connection, callbacks, scope, mode, db, std::move(connection) https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_class_factory.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_class_factory.h:52: IndexedDBConnection* connection, base::WeakPtr<IndexedDBConnection> here and below. You create it by using the ptr_factory_ in IndexedDBConnection, and then you can move the weak pointer by std::move https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:25: // Is this needed? Yes, this makes sense here. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:41: // TODO(palakj):Is this needed? Yes, looks good. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:62: observersToRemove.erase(observersToRemove.begin() + i); ? Dont need this line, it breaks your algorithm. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:66: database_->RemovePendingObservers(this, observersToRemove); Hm, I see what you're doing here. Let's create a new vector for this list (we're changing the input to a const ref), and have a 'bool found = false' that we set to true if line 60 is true. If it's still false, add it to that new list, which we call here. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:31: void Unobserve(std::vector<int32_t> observersToRemove); const ref, and please use underscore_naming https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:563: // Do I move this logic to transaction. Sure, it might make a little more sense. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:566: std::find(observersToRemove.begin(), observersToRemove.end(), why not just observersToRemove.find? Also, remove camelCase. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.h:130: std::vector<int32_t> observersToRemove); const ref, or a pointer if we're doing the 'erase' part of your algorithm. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:523: IPC_MESSAGE_HANDLER(IndexedDBHostMsg_DatabaseObserve, OnObserve) Also add OnUnobserve https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.h:181: std::vector<int32_t> observersToRemove); const ref. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:437: connection_->active_observers_.push_back(std::move(pending_observers_[i])); Maybe make this a method, so we're not calling into private variables https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:438: ; remove extra semicolon https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:77: IndexedDBConnection* getConnection() const { return connection_; } connection_.get(); https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:97: // TODO(palakj): Eliminate callbacks and db. I'm not sure what you mean by 'eliminate callbacks and db' https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:98: IndexedDBConnection* connection, base::WeakPtr<IndexedDBConnection> connection https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:144: IndexedDBConnection* connection_; Please use a base::WeakPtr<IndexedDBConnection> https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:174: // TODO(palakj): Do I wrap WebIDBObserver with unique_ptr as in unique_ptr https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:186: std::vector<int32_t> observersToRemove; no camelCase https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:200: void IndexedDBDispatcher::ClearIDBObserver( I don't think this method needs to unobserve on the backend, as the close logic should handle that, right? Also, ClearIDBObservers (add s). https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:80: blink::WebIDBObserver* observer); please use std::unique_ptr as an argument, and then std::move it. https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:84: std::vector<int32_t> database_observers_id); const ref https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:85: void ClearIDBObserver(std::vector<int32_t> observersToRemove); const ref, no camelCase https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:96: // TODO(palakj): Can pass observers_id along with RequestIDBDatabaseClose. Yes, you could do that also. https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:99: observers_id_.clear(); // Do I need to do this? sure. https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:123: std::vector<int32_t> observersToRemove = no camelCase. https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:127: std::vector<int>::iterator position = std::find( I think using observer_ids_.find would be simpler. https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.h:122: std::vector<int32_t> observers_id_; ids_ https://codereview.chromium.org/2062203004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h (right): https://codereview.chromium.org/2062203004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:34: #include "wtf/PassOwnPtr.h" Please double check these headers. https://codereview.chromium.org/2062203004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:41: class WebIDBObserverImpl final : public WebIDBObserver { Can your IDBObserver just extend WeBIDBOBserver instead? I don't think we need this class.
palakj@google.com changed reviewers: + jsbell@chromium.org
https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_class_factory.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_class_factory.cc:40: IndexedDBConnection* connection, On 2016/06/22 at 01:09:48, dmurph wrote: > base::WeakPtr<IndexedDBConnection> Done https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_class_factory.cc:46: return new IndexedDBTransaction(id, connection, callbacks, scope, mode, db, On 2016/06/22 at 01:09:48, dmurph wrote: > std::move(connection) Done https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_class_factory.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_class_factory.h:52: IndexedDBConnection* connection, On 2016/06/22 at 01:09:48, dmurph wrote: > base::WeakPtr<IndexedDBConnection> > > here and below. You create it by using the ptr_factory_ in IndexedDBConnection, and then you can move the weak pointer by std::move Done https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:66: database_->RemovePendingObservers(this, observersToRemove); On 2016/06/22 at 01:09:48, dmurph wrote: > Hm, I see what you're doing here. Let's create a new vector for this list (we're changing the input to a const ref), and have a 'bool found = false' that we set to true if line 60 is true. If it's still false, add it to that new list, which we call here. I get your point. And the observersToRemove should have been within the if loop.But I am changing that part anyway. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:31: void Unobserve(std::vector<int32_t> observersToRemove); On 2016/06/22 at 01:09:49, dmurph wrote: > const ref, and please use underscore_naming changed to const ref. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:563: // Do I move this logic to transaction. On 2016/06/22 at 01:09:49, dmurph wrote: > Sure, it might make a little more sense. Done https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:566: std::find(observersToRemove.begin(), observersToRemove.end(), On 2016/06/22 at 01:09:49, dmurph wrote: > why not just observersToRemove.find? Also, remove camelCase. Does a vector have find function? https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.h:130: std::vector<int32_t> observersToRemove); On 2016/06/22 at 01:09:49, dmurph wrote: > const ref, or a pointer if we're doing the 'erase' part of your algorithm. Changed it to const ref https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:523: IPC_MESSAGE_HANDLER(IndexedDBHostMsg_DatabaseObserve, OnObserve) On 2016/06/22 at 01:09:49, dmurph wrote: > Also add OnUnobserve Oops! added. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.h:181: std::vector<int32_t> observersToRemove); On 2016/06/22 at 01:09:49, dmurph wrote: > const ref. Done https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:438: ; On 2016/06/22 at 01:09:49, dmurph wrote: > remove extra semicolon Done https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:77: IndexedDBConnection* getConnection() const { return connection_; } On 2016/06/22 at 01:09:49, dmurph wrote: > connection_.get(); Done. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:97: // TODO(palakj): Eliminate callbacks and db. On 2016/06/22 at 01:09:49, dmurph wrote: > I'm not sure what you mean by 'eliminate callbacks and db' the connection object includes a reference to both callbacks and db. Storing the connection, we don't really need the other two. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:98: IndexedDBConnection* connection, On 2016/06/22 at 01:09:49, dmurph wrote: > base::WeakPtr<IndexedDBConnection> connection Dome. https://codereview.chromium.org/2062203004/diff/100001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:144: IndexedDBConnection* connection_; On 2016/06/22 at 01:09:49, dmurph wrote: > Please use a base::WeakPtr<IndexedDBConnection> Done. https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.cc (right): https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:96: // TODO(palakj): Can pass observers_id along with RequestIDBDatabaseClose. On 2016/06/22 at 01:09:50, dmurph wrote: > Yes, you could do that also. Would you prefer that over the current implementation? I feel this is a little cleaner, but requires an extra call. https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:123: std::vector<int32_t> observersToRemove = On 2016/06/22 at 01:09:50, dmurph wrote: > no camelCase. Changed. https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.h (right): https://codereview.chromium.org/2062203004/diff/100001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.h:122: std::vector<int32_t> observers_id_; On 2016/06/22 at 01:09:50, dmurph wrote: > ids_ Done. https://codereview.chromium.org/2062203004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h (right): https://codereview.chromium.org/2062203004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:41: class WebIDBObserverImpl final : public WebIDBObserver { On 2016/06/22 at 01:09:50, dmurph wrote: > Can your IDBObserver just extend WeBIDBOBserver instead? I don't think we need this class. Done.
Looking close! Nice job with the WebIDBConnection, I know that was super confusing. https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:57: active_observers_.emplace_back(std::move(pending_observers->at(i))); I believe we just want push_back here. emplace_back is the same, except it also takes a list of construction arguments for the item we're inserting. We want to avoid accidentally doing this, so let's just use push_back. https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:70: active_observers_.erase(active_observers_.begin() + j); In order for us to be able to erase while iterating, we should either use the iterator interface (which lets us call remove or something), or iterate backwards through the list. This currently won't work (we would skip elements). https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:555: // TODO(palakj): Better Implementation needed. Remove this todo. This is fine. https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:348: if (!pending_observers_.empty()) Do a check that the connection is around as well. if (!pending_observers_.empty() && connection_) https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:468: pending_observers_.erase(pending_observers_.begin() + j); Same issue here. You have to either use iterator interface, or iterate backwards. https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:97: // TODO(palakj): Eliminate callbacks and db. Why? It looks like we hold refptrs to those things, do we need to make sure that they stick around while our transaction is alive? I would say leave this alone for now. https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:184: const std::vector<int32_t>& database_observers_id) { nit: database_observer_ids https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:78: int32_t AddIDBObserver(int32_t ipc_database_id, How about AddObserver, IDB is redundant. https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:81: std::vector<int32_t> RemoveIDBObservers( How about 'RemoveObserversForDatabase' https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:84: const std::vector<int32_t>& database_observers_id); database_observer_ids https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:85: void ClearIDBObserver(const std::vector<int32_t>& observer_ids_to_remove); Then just 'RemoveObservers' here, and comment that this doesn't send any IPC messages and just clears them from our map. https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.cc (right): https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:95: // TODO(palakj): Is it better to pass observers_id along with Since we're closing the whole connection, the backend can handle the observer cleanup. https://codereview.chromium.org/2062203004/diff/140001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2062203004/diff/140001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:488: // WebIDBDatabase::deleteObjectStore() message. Remove this comment. https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:6: #define IDBObserver_h Please use the new copyright header https://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserver.cpp (right): https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserver.cpp:29: */ Please use the new copyright header https://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserver.cpp:44: m_observer.reset(); This happens automatically, you can remove this. https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h:25: Please use the new copyright header https://www.chromium.org/developers/coding-style#TOC-File-headers
Post dmurph review. https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:57: active_observers_.emplace_back(std::move(pending_observers->at(i))); On 2016/06/23 at 21:53:04, dmurph wrote: > I believe we just want push_back here. emplace_back is the same, except it also takes a list of construction arguments for the item we're inserting. We want to avoid accidentally doing this, so let's just use push_back. Changed. https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:70: active_observers_.erase(active_observers_.begin() + j); On 2016/06/23 at 21:53:04, dmurph wrote: > In order for us to be able to erase while iterating, we should either use the iterator interface (which lets us call remove or something), or iterate backwards through the list. This currently won't work (we would skip elements). After each erase, we break. So the state of iterator doesn't really matter. https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:555: // TODO(palakj): Better Implementation needed. On 2016/06/23 at 21:53:04, dmurph wrote: > Remove this todo. This is fine. Done https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:348: if (!pending_observers_.empty()) On 2016/06/23 at 21:53:04, dmurph wrote: > Do a check that the connection is around as well. > if (!pending_observers_.empty() && connection_) Done. https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:468: pending_observers_.erase(pending_observers_.begin() + j); On 2016/06/23 at 21:53:04, dmurph wrote: > Same issue here. You have to either use iterator interface, or iterate backwards. Same reason here. https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/140001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:97: // TODO(palakj): Eliminate callbacks and db. On 2016/06/23 at 21:53:04, dmurph wrote: > Why? It looks like we hold refptrs to those things, do we need to make sure that they stick around while our transaction is alive? I would say leave this alone for now. Yaa, I didn't remove them due to refptrs. But I suppose it'll introduce some redundancy. Will let this be as is. https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:184: const std::vector<int32_t>& database_observers_id) { On 2016/06/23 at 21:53:04, dmurph wrote: > nit: database_observer_ids Done. https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:78: int32_t AddIDBObserver(int32_t ipc_database_id, On 2016/06/23 at 21:53:04, dmurph wrote: > How about AddObserver, IDB is redundant. There's another Observer associated with WorkerThread, wanted to make this clearer. https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:81: std::vector<int32_t> RemoveIDBObservers( On 2016/06/23 at 21:53:04, dmurph wrote: > How about 'RemoveObserversForDatabase' Done. RemoveIDBObserversFromDatabse (IDB due to the other WorkerThread::Observer). https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:84: const std::vector<int32_t>& database_observers_id); On 2016/06/23 at 21:53:04, dmurph wrote: > database_observer_ids Done. https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:85: void ClearIDBObserver(const std::vector<int32_t>& observer_ids_to_remove); On 2016/06/23 at 21:53:04, dmurph wrote: > Then just 'RemoveObservers' here, and comment that this doesn't send any IPC messages and just clears them from our map. Done. https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.cc (right): https://codereview.chromium.org/2062203004/diff/140001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:95: // TODO(palakj): Is it better to pass observers_id along with On 2016/06/23 at 21:53:04, dmurph wrote: > Since we're closing the whole connection, the backend can handle the observer cleanup. No need for observer_ids_.clear()? We still need to clear observers from dispatcher map, right? https://codereview.chromium.org/2062203004/diff/140001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2062203004/diff/140001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:488: // WebIDBDatabase::deleteObjectStore() message. On 2016/06/23 at 21:53:05, dmurph wrote: > Remove this comment. Changed. https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:6: #define IDBObserver_h On 2016/06/23 at 21:53:05, dmurph wrote: > Please use the new copyright header > https://www.chromium.org/developers/coding-style#TOC-File-headers Isn't it the same header? https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserver.cpp (right): https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserver.cpp:29: */ On 2016/06/23 at 21:53:05, dmurph wrote: > Please use the new copyright header > https://www.chromium.org/developers/coding-style#TOC-File-headers Done. https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserver.cpp:44: m_observer.reset(); On 2016/06/23 at 21:53:05, dmurph wrote: > This happens automatically, you can remove this. Without explicitly stating that, WebPrivatePtr fails to destruct. (Check failed: !m_storage) https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h:25: On 2016/06/23 at 21:53:05, dmurph wrote: > Please use the new copyright header > https://www.chromium.org/developers/coding-style#TOC-File-headers Done.
https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:59: active_observers_.push_back(std::move(pending_observers->at(i))); I think it's a bit of a weird API that ActivatePendingObservers changes |pending_observers| to be a vector full of nullptrs. It might at least make senes to have a pending_observers->clear() at the end of this method? Or this method could take pending_observers by value rather than by pointer, with the caller using std::move to cheaply move the vector. https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:68: for (size_t i = 0; i < observer_ids_to_remove.size(); i++) { nit: You're not really using the index |i|, so you could also write this as for (int32_t id_to_remove : observer_ids_to_remove) { ... } https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:70: for (size_t j = 0; j < active_observers_.size(); j++) { You could rewrite the body of the outer loop as (untested): auto it = std::find_if(active_observers_.begin(), active_oberservers_.end(), [id_to_remove](const std::unique_ptr<IndexedDBObserver>& observer) { return observer->id() == id_to_remove; }); if (it != active_observers_.end()) { active_observers_.erase(it); } else { pending_observer_ids.push_back(id_to_remove); } Although that isn't really any shorter, and arguably not any clearer than what you have now either, so probably fine/better to just leave what you have now. https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:37: const std::vector<std::unique_ptr<IndexedDBObserver>>& activeObservers() the "activeObservers" name does not match our naming conventions (https://google.github.io/styleguide/cppguide.html#Function_Names). Either call it active_observers or GetActiveObservers. (I know it's confusing that code in blink has a different code style with different naming conventions than code everywhere else) https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:41: base::WeakPtr<IndexedDBConnection> weakPtr() { similarly this should be called GetWeakPtr() https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.cc (right): https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.cc:10: #include "base/bind.h" You're not using anything from bind.h (or from most of the other headers you include for that matter). https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. This and many of the other new files should all also use the correct copyright header: no (c), and 2016 https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:21: int32_t id() { return observer_id_; } Shouldn't this return int64_t? https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:77: IndexedDBConnection* getConnection() const { return connection_.get(); } getConnection should be GetConnection (or connection). But either way having two different connection getters is rather confusing... I don't really know this code, but maybe renaming the pre-existing connection() method to something else makes sense? https://codereview.chromium.org/2062203004/diff/160001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2062203004/diff/160001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:187: if (observers_.Lookup(database_observer_ids[i]) == observer.get()) { I'm confused about this check. observers_ owns WebIDBObserver instances, so it should never be possible to have a unique_ptr that points to a WebIDBObserver instance that is also in observers_ (if you would have such a pointer the observer would end up being deleted twice, as it has two owners). In other words, this check will always return false, or cause crashes. https://codereview.chromium.org/2062203004/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h (right): https://codereview.chromium.org/2062203004/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h:66: virtual void observe(std::unique_ptr<WebIDBObserver>, long long transactionId) = 0; I think it's this API that doesn't make sense to me. WebIDBObserver is kind of already it's own smart pointer (being nothing more than a wrapper around a pointer to an IDBObserver). As such passing them as std::unique_ptr to observer/unobserve doesn't make sense, you should probably just pass them by const WebIDBObserver& and add copy and/or move constructors and assignment operators to WebIDBObserver.
Please have a look at the modified design. https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:59: active_observers_.push_back(std::move(pending_observers->at(i))); On 2016/06/24 at 00:48:08, Marijn Kruisselbrink wrote: > I think it's a bit of a weird API that ActivatePendingObservers changes |pending_observers| to be a vector full of nullptrs. It might at least make senes to have a pending_observers->clear() at the end of this method? > > Or this method could take pending_observers by value rather than by pointer, with the caller using std::move to cheaply move the vector. For the second option, doesn't the chromium convention say, that we pass an argument either as a pointer or const ref. Am I allowed to pass a value that's not const? https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:68: for (size_t i = 0; i < observer_ids_to_remove.size(); i++) { On 2016/06/24 at 00:48:08, Marijn Kruisselbrink wrote: > nit: You're not really using the index |i|, so you could also write this as > for (int32_t id_to_remove : observer_ids_to_remove) { ... } Done. https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:70: for (size_t j = 0; j < active_observers_.size(); j++) { On 2016/06/24 at 00:48:08, Marijn Kruisselbrink wrote: > You could rewrite the body of the outer loop as (untested): > > auto it = std::find_if(active_observers_.begin(), active_oberservers_.end(), > [id_to_remove](const std::unique_ptr<IndexedDBObserver>& observer) { > return observer->id() == id_to_remove; > }); > if (it != active_observers_.end()) { > active_observers_.erase(it); > } else { > pending_observer_ids.push_back(id_to_remove); > } > > Although that isn't really any shorter, and arguably not any clearer than what you have now either, so probably fine/better to just leave what you have now. this is nice. But, I'm not really comfortable with lambda functions. So, leaving it as is, unless it offers a performance advantage. https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:37: const std::vector<std::unique_ptr<IndexedDBObserver>>& activeObservers() On 2016/06/24 at 00:48:08, Marijn Kruisselbrink wrote: > the "activeObservers" name does not match our naming conventions (https://google.github.io/styleguide/cppguide.html#Function_Names). Either call it active_observers or GetActiveObservers. > > (I know it's confusing that code in blink has a different code style with different naming conventions than code everywhere else) Done. https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:41: base::WeakPtr<IndexedDBConnection> weakPtr() { On 2016/06/24 at 00:48:08, Marijn Kruisselbrink wrote: > similarly this should be called GetWeakPtr() Changed. I should read the style guide carefully. https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.cc (right): https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.cc:10: #include "base/bind.h" On 2016/06/24 at 00:48:09, Marijn Kruisselbrink wrote: > You're not using anything from bind.h (or from most of the other headers you include for that matter). Cleaned it up. https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2016/06/24 at 00:48:09, Marijn Kruisselbrink wrote: > This and many of the other new files should all also use the correct copyright header: no (c), and 2016 Done. https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:21: int32_t id() { return observer_id_; } On 2016/06/24 at 00:48:09, Marijn Kruisselbrink wrote: > Shouldn't this return int64_t? Oops! the id is 32 bit. https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/160001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:77: IndexedDBConnection* getConnection() const { return connection_.get(); } On 2016/06/24 at 00:48:09, Marijn Kruisselbrink wrote: > getConnection should be GetConnection (or connection). But either way having two different connection getters is rather confusing... I don't really know this code, but maybe renaming the pre-existing connection() method to something else makes sense? Changed.
https://codereview.chromium.org/2062203004/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:62: // pending_observers->clear(); Probably safe to clear here. https://codereview.chromium.org/2062203004/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:74: active_observers_.erase(active_observers_.begin() + j); Again, this won't work. You'll remove the item at 'i', which shifts the array back to that the item at 'i' is different, then you increment and skip that item. Ok, so let's do the temp vector route. If the ids don't match, add it to the temporary vector, and then swap them at the end. Then, you can send that active list to the database. https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:81: void RemoveIDBObserversFromDatabase( Please comment what this does a bit more. Something like: "The |observer_ids_to_remove| are assumed to be observing the |ipc_database_id|. We remove our references to these observer object, and send an IPC to remove the observers in the backend." https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.cc (right): https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:122: void WebIDBDatabaseImpl::removeObservers( Don't we also need to remove the observers from our observer_ids_ set? https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.h (right): https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.h:50: // returns true if the element has been removed from the set. Please capitalize the sentence (all comments should be complete sentences). https://codereview.chromium.org/2062203004/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h (right): https://codereview.chromium.org/2062203004/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h:69: virtual bool removeObserverId(int32_t id) = 0; I don't like how we expect these methods to be called first and then second, where the first one removes and the second one forwards the call. Instead, how about we have: bool containsObserver(id); instead of removeObserverId. Then, in removeObservers, that will do the set removal as well.
https://codereview.chromium.org/2062203004/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:74: active_observers_.erase(active_observers_.begin() + j); On 2016/06/27 at 21:20:51, dmurph wrote: > Again, this won't work. You'll remove the item at 'i', which shifts the array back to that the item at 'i' is different, then you increment and skip that item. > > Ok, so let's do the temp vector route. If the ids don't match, add it to the temporary vector, and then swap them at the end. Then, you can send that active list to the database. Nevermind, this works.
Description was changed from ========== Adds observer to database * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * As per the spec, the observer begins observing after the completion of transaction it was created with. BUG=609934 patch from issue 2031113002 at patchset 100001 (http://crrev.com/2031113002#ps100001) Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md ========== to ========== Adds observer to database * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * As per the spec, the observer begins observing after the completion of transaction it was created with. BUG=609934 patch from issue 2031113002 at patchset 100001 (http://crrev.com/2031113002#ps100001) Current Implementation and Future Scope: https://docs.google.com/document/d/1viweV7zje2vK9IC-_G0PYlWgGrBhcVYlIMFsogsoH... Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md ==========
Please have a look. The design doc's link has been updated. It's still a wip. https://codereview.chromium.org/2062203004/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:62: // pending_observers->clear(); On 2016/06/27 at 21:20:51, dmurph wrote: > Probably safe to clear here. Done. Is it okay to pass the argument by value? https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:81: void RemoveIDBObserversFromDatabase( On 2016/06/27 at 21:20:51, dmurph wrote: > Please comment what this does a bit more. Something like: "The |observer_ids_to_remove| are assumed to be observing the |ipc_database_id|. We remove our references to these observer object, and send an IPC to remove the observers in the backend." Done. https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.cc (right): https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:122: void WebIDBDatabaseImpl::removeObservers( On 2016/06/27 at 21:20:51, dmurph wrote: > Don't we also need to remove the observers from our observer_ids_ set? That is done while checking whether an observer id belongs to the database in the removeObserverId function. https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.h (right): https://codereview.chromium.org/2062203004/diff/200001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.h:50: // returns true if the element has been removed from the set. On 2016/06/27 at 21:20:51, dmurph wrote: > Please capitalize the sentence (all comments should be complete sentences). Done. https://codereview.chromium.org/2062203004/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h (right): https://codereview.chromium.org/2062203004/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h:69: virtual bool removeObserverId(int32_t id) = 0; On 2016/06/27 at 21:20:51, dmurph wrote: > I don't like how we expect these methods to be called first and then second, where the first one removes and the second one forwards the call. Instead, how about we have: > > bool containsObserver(id); > > instead of removeObserverId. Then, in removeObservers, that will do the set removal as well. Yes, we can do that. It's a bit redundant since, we find and erase each element separately. But that's cleaner.
Description was changed from ========== Adds observer to database * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * As per the spec, the observer begins observing after the completion of transaction it was created with. BUG=609934 patch from issue 2031113002 at patchset 100001 (http://crrev.com/2031113002#ps100001) Current Implementation and Future Scope: https://docs.google.com/document/d/1viweV7zje2vK9IC-_G0PYlWgGrBhcVYlIMFsogsoH... Reference: https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md ========== to ========== Adds observer to database * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * As per the spec, the observer begins observing after the completion of transaction it was created with. BUG=609934 patch from issue 2031113002 at patchset 100001 (http://crrev.com/2031113002#ps100001) 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 ==========
Please have a look.
palakj@google.com changed reviewers: + cmumford@chromium.org, palmer@chromium.org - jsbell@chromium.org
Please have a look +cmumford for indexed_db/ +palmer for indexed_db_messages
Architecture looks good, last thing to do is to add some unittests to indexed_db_transaction_unittests.cc. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:90: void RemovePendingObservers(const std::vector<int32_t>& pending_observer_ids); nit: MaybeRemovePendingObservers https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:174: std::unique_ptr<WebIDBObserver> observer(observer_ptr); This is weird, just pass the observer_ptr to the observers_. nit: A slightly cleaner way to do this is have AddIDBObserver accept a unique_ptr (and have it always be a unique ptr, like from create()), and the release it in the .Add call. https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:82: /* The |observer_ids_to_remove| are observing the |ipc_database_id|. Can you change this to all single-line comments to match the rest of the file? https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:21: class MODULES_EXPORT IDBObserver final : public GarbageCollectedFinalized<IDBObserver>, public ScriptWrappable { I think we can just be GarbageCollected, right? https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h (right): https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h:70: // rename this. nit: remove this comment.
https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:31: void ActivatePendingObservers( I think some comments here could be helpful too. For example why is this called ActivatePendingObservers rather than just AddObservers? https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.h:127: void AddPendingObserver(int64_t transaction_id, int64_t observer_id); Should observer_id here be int32_t rather than int64_t? https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.h (left): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.h:302: nit: why did you remove this blank line? https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (left): https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:261: nit: don't remove empty line https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:275: nit: don't remove empty line https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.cc (right): https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:119: return observer_ids_.find(id) != observer_ids_.end(); nit: you could write this as ContainsKey(observer_ids_, id) (from stl_util.h). Although not actually sure why we have that wrapper as the find() != end() code isn't that hard to read either... so fine with me to leave as is as well. https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h (right): https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:8: // TODO(palakj): header to include for persistent? that would be platform/heap/Persistent.h? https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:9: #include "public/platform/WebPrivatePtr.h" I don't think you're using this header? https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:11: #include <memory> I don't think you're using this header either? https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h (right): https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h:68: virtual int32_t addObserver(WebIDBObserver*, long long transactionId) = 0; You seem to be transferring ownership of the WebIDBObserver instance. So why not just pass it as std::unique_ptr<WebIDBObserver> rather than unwrapping and rewrapping the pointer in a unique_ptr everywhere? https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h:70: // rename this. Remove the comment and/or rename whatever you want to rename? https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h:13: virtual void removeObserver(int32_t id) = 0; I'm a bit confused about this API. First of all you should add some comments describing what this class is used for, what it's ownership and thread safety guarantees etc are. But then specifically this removeObserver method I don't understand. As far as I can tell this method is called at most once on any WebIDBObserver instance, and that one time is right before the instance is destroyed. If that is the case, couldn't you just store the observer id in the WebIDBObserverImpl instance, and use the destructor being called to do what you currently have removeObserver for? Essentially it seems WebIDBObserver represents a particular observer() call, and as such already is associated with an id, so no need to pass the id to removeObserver. And on top of that the content layer will already destroy the WebIDBObserver instance when it calls removeObserver. So you could just use the destructor of WebIDBObserver to do what removeObserver currently does? Although I guess you wouldn't actually have an id until after you've transferred ownership of the WebIDBObserver instance to the content layer, so setting the ID would be a bit tricky. If you decide to go this way you could either add a setId() method to WebIDBObserver, or have IDBObserver hold on to a raw pointer to the WebIDBObserverImpl after passing it to the content layer and use that to store the returned id in the WebIDBObserverImpl.
On 2016/06/28 18:32:19, palakj1 wrote: > Please have a look > > +cmumford for indexed_db/ > +palmer for indexed_db_messages Your commit message needs to be as follows: <summary line> <blank line> <supplemental description> This is so that Git can extract a nice one-line summary.
On 2016/06/28 19:01:16, cmumford wrote: > On 2016/06/28 18:32:19, palakj1 wrote: > > Please have a look > > > > +cmumford for indexed_db/ > > +palmer for indexed_db_messages > > Your commit message needs to be as follows: > > <summary line> > <blank line> > <supplemental description> > > This is so that Git can extract a nice one-line summary. Are also need to make modifications to some tests. Specifically: content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.(cc|h) You can build them all with: ninja -C out/Debug env_chromium_unittests browser_tests webkit_unit_tests unit_tests content_unittests content_browsertests
https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:59: for (uint32_t i = 0; i < pending_observers.size(); i++) { Note that std::vector::size returns size_t, not uint32_t. But, you can just declare |i| as auto, anyway. :) https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:67: // TODO(palakj): Change from vector to set or create tmp vector instead of You might also consider http://www.cplusplus.com/reference/algorithm/set_difference/. https://codereview.chromium.org/2062203004/diff/230001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:492: int32_t) /* observer_id */ Note that in a few places you use int64_t as the observer ID type. Standardize on 1 (probably int32_t is fine).
I'll do another review after the tests are fixed, but here's my first quick pass. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_class_factory.h (left): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_class_factory.h:14: #include "content/browser/indexed_db/indexed_db_backing_store.h" No need to include indexed_db_connection.h. Just forward declare IndexedDBConnection. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:65: void IndexedDBConnection::RemoveObservers( Would this function be simpler if active_observers_ was a map of id -> observer? https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:37: const std::vector<std::unique_ptr<IndexedDBObserver>>& GetActiveObservers() This is a "getter" so you can name it active_observers(). https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:41: base::WeakPtr<IndexedDBConnection> GetWeakPtr() { I think this is the "old" way. Instead consider deriving IndexedDBConnection from base::SupportsWeakPtr<IndexedDBConnection>, and then you can delete GetWeakPtr() and use AsWeakPtr() to mint a new WeakPtr. BTW, are you sure you're obeying the threading rules in order to use a WeakPtr to this class? Read comment in weak_ptr.h. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:1688: transaction_id, connection->GetWeakPtr(), connection->callbacks(), Nit: If CreateIndexedDBTransaction has the connection then it can call callbacks() if needed. Seems a little redundant now. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:21: // TODO(palakj): Add other fields. What other fields do you intend to add? https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.h:50: bool containsObserverId(int32_t id) override; make method const.
https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:41: base::WeakPtr<IndexedDBConnection> GetWeakPtr() { On 2016/06/28 at 20:26:22, cmumford wrote: > I think this is the "old" way. Instead consider deriving IndexedDBConnection from base::SupportsWeakPtr<IndexedDBConnection>, and then you can delete GetWeakPtr() and use AsWeakPtr() to mint a new WeakPtr. The "old" way? I think in general a WeakPtrFactory is preferred over inheriting from base::SupportsWeakPtr. See the comment for SupportsWeakPtr "since SupportsWeakPtr's destructor won't invalidate weak pointers to the class until after the derived class' members have been destroyed, its use can lead to subtle use-after-destroy issues.". There even was some discussion back in 2013 to completely kill SupportsWeakPtr in favor of WeakPtrFactory (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2p...)
IPC security LGTM.
Added unit tests for observer and modified IndexedDBTransaction constructor. As suggested by +cmumford, I have eliminated callbacks and also database from transaction constructor. However, I am not sure if I can remove the corresponding members from transaction as those are ref counted. Please guide me with this. More tests to be added in the IDB layer. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_class_factory.h (left): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_class_factory.h:14: #include "content/browser/indexed_db/indexed_db_backing_store.h" On 2016/06/28 at 20:26:22, cmumford wrote: > No need to include indexed_db_connection.h. Just forward declare IndexedDBConnection. Done. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:59: for (uint32_t i = 0; i < pending_observers.size(); i++) { On 2016/06/28 at 19:34:30, palmer wrote: > Note that std::vector::size returns size_t, not uint32_t. But, you can just declare |i| as auto, anyway. :) Done. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:65: void IndexedDBConnection::RemoveObservers( On 2016/06/28 at 20:26:22, cmumford wrote: > Would this function be simpler if active_observers_ was a map of id -> observer? Yes. But the more dominant (much more used) use case would involve iterating over active_observers_ for determining it the observer is listening for a change or not (part of next cl) like this: observers = connection->GetActiveObservers() for(const auto& observer : observers ) { if(observer->IsRecording(some property)){ add observation } I believe a vector would be faster for this case than map. But if they are almost equivalent performance wise, I can sure switch to map. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:67: // TODO(palakj): Change from vector to set or create tmp vector instead of On 2016/06/28 at 19:34:30, palmer wrote: > You might also consider http://www.cplusplus.com/reference/algorithm/set_difference/. Wouldn't that require sorted vectors? Is it preferred to sort the vectors and apply set_difference than iterating over both of them. Also, I can populate the pending_observer_id vector this way, but I'd still need to erase the elements one by one right? https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:31: void ActivatePendingObservers( On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > I think some comments here could be helpful too. For example why is this called ActivatePendingObservers rather than just AddObservers? Thanks. This was really important. Added some comments, let me know if they throw sufficient light. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:37: const std::vector<std::unique_ptr<IndexedDBObserver>>& GetActiveObservers() On 2016/06/28 at 20:26:22, cmumford wrote: > This is a "getter" so you can name it active_observers(). Is the current GetActiveObservers usage wrong? Changing it all the same. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.cc:1688: transaction_id, connection->GetWeakPtr(), connection->callbacks(), On 2016/06/28 at 20:26:22, cmumford wrote: > Nit: If CreateIndexedDBTransaction has the connection then it can call callbacks() if needed. Seems a little redundant now. I can eliminate both callbacks and database. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.h:127: void AddPendingObserver(int64_t transaction_id, int64_t observer_id); On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > Should observer_id here be int32_t rather than int64_t? Sorry. Corrected. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.h (left): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.h:302: On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > nit: why did you remove this blank line? Done https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:21: // TODO(palakj): Add other fields. On 2016/06/28 at 20:26:23, cmumford wrote: > What other fields do you intend to add? None, for this cl. I'll remove the todo for now. The fields to be added later act as filters for the observer: map<objectstores, list<indexeddb_key_ranges>>; bool value, noRecord, transaction; https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:90: void RemovePendingObservers(const std::vector<int32_t>& pending_observer_ids); On 2016/06/28 at 18:53:50, dmurph wrote: > nit: MaybeRemovePendingObservers would adding comment about the functionality be sufficient? This is going to make the function name even bigger without adding complete clarity of what's happening. https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:174: std::unique_ptr<WebIDBObserver> observer(observer_ptr); On 2016/06/28 at 18:53:50, dmurph wrote: > This is weird, just pass the observer_ptr to the observers_. > > nit: A slightly cleaner way to do this is have AddIDBObserver accept a unique_ptr (and have it always be a unique ptr, like from create()), and the release it in the .Add call. I had been imitating the previous styles. But, you are right. Change made. https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (left): https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:261: On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > nit: don't remove empty line Done. https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:275: On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > nit: don't remove empty line Done. https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:82: /* The |observer_ids_to_remove| are observing the |ipc_database_id|. On 2016/06/28 at 18:53:50, dmurph wrote: > Can you change this to all single-line comments to match the rest of the file? Done. https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.cc (right): https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:119: return observer_ids_.find(id) != observer_ids_.end(); On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > nit: you could write this as ContainsKey(observer_ids_, id) (from stl_util.h). Although not actually sure why we have that wrapper as the find() != end() code isn't that hard to read either... so fine with me to leave as is as well. Done. https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.h:50: bool containsObserverId(int32_t id) override; On 2016/06/28 at 20:26:23, cmumford wrote: > make method const. Done. https://codereview.chromium.org/2062203004/diff/230001/content/common/indexed... File content/common/indexed_db/indexed_db_messages.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/common/indexed... content/common/indexed_db/indexed_db_messages.h:492: int32_t) /* observer_id */ On 2016/06/28 at 19:34:30, palmer wrote: > Note that in a few places you use int64_t as the observer ID type. Standardize on 1 (probably int32_t is fine). Done. https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:21: class MODULES_EXPORT IDBObserver final : public GarbageCollectedFinalized<IDBObserver>, public ScriptWrappable { On 2016/06/28 at 18:53:50, dmurph wrote: > I think we can just be GarbageCollected, right? std::set<int32_t> m_observerIds requires finalization. https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h (right): https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:8: // TODO(palakj): header to include for persistent? On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > that would be platform/heap/Persistent.h? Done. https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:9: #include "public/platform/WebPrivatePtr.h" On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > I don't think you're using this header? Removed. https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:11: #include <memory> On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > I don't think you're using this header either? Removed. https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h (right): https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h:68: virtual int32_t addObserver(WebIDBObserver*, long long transactionId) = 0; On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > You seem to be transferring ownership of the WebIDBObserver instance. So why not just pass it as std::unique_ptr<WebIDBObserver> rather than unwrapping and rewrapping the pointer in a unique_ptr everywhere? Had been imitating the existing code, but changes made now. https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabase.h:70: // rename this. On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > Remove the comment and/or rename whatever you want to rename? Done. https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/230001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/indexeddb/WebIDBObserver.h:13: virtual void removeObserver(int32_t id) = 0; On 2016/06/28 at 18:58:31, Marijn Kruisselbrink wrote: > I'm a bit confused about this API. First of all you should add some comments describing what this class is used for, what it's ownership and thread safety guarantees etc are. > > But then specifically this removeObserver method I don't understand. As far as I can tell this method is called at most once on any WebIDBObserver instance, and that one time is right before the instance is destroyed. If that is the case, couldn't you just store the observer id in the WebIDBObserverImpl instance, and use the destructor being called to do what you currently have removeObserver for? > > Essentially it seems WebIDBObserver represents a particular observer() call, and as such already is associated with an id, so no need to pass the id to removeObserver. And on top of that the content layer will already destroy the WebIDBObserver instance when it calls removeObserver. So you could just use the destructor of WebIDBObserver to do what removeObserver currently does? > > Although I guess you wouldn't actually have an id until after you've transferred ownership of the WebIDBObserver instance to the content layer, so setting the ID would be a bit tricky. If you decide to go this way you could either add a setId() method to WebIDBObserver, or have IDBObserver hold on to a raw pointer to the WebIDBObserverImpl after passing it to the content layer and use that to store the returned id in the WebIDBObserverImpl. Not really sure about this. We hold reference to WebIDBObserver objects each of which in turn holds IDBObserver. Isn't that a little weird? Maybe I can discuss this with you.
https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:71: IndexedDBConnection* connection, Please change back to WeakPtr here, based on our convo. https://codereview.chromium.org/2062203004/diff/250001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2062203004/diff/250001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:176: observer->setId(observer_id); Move this to IDBObserver pls.
https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database_unittest.cc (right): https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... content/browser/indexed_db/indexed_db_database_unittest.cc:262: IndexedDBConnection* connection_; this should probably be a unique_ptr<IndexedDBConnection> to make sure the connection actually gets cleaned up at the end of a test? https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction_unittest.cc (right): https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction_unittest.cc:91: id, new IndexedDBConnection(db_, new MockIndexedDBDatabaseCallbacks()), here too I believe you're leaking IndexedDBConnection instances. You need to make sure that every IndexedDBConnection you create is also actually destroyed at some point. https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction_unittest.cc:352: IndexedDBConnection* connection = probably make this a std::unique_ptr<IndexedDBConnection> to ensure it doesn't leak. https://codereview.chromium.org/2062203004/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/MockWebIDBDatabase.h (right): https://codereview.chromium.org/2062203004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/MockWebIDBDatabase.h:34: // Gmock does not support movable type, so cannot use MOCK_METHOD for addObserver. You could mention https://github.com/google/googletest/issues/395 in this comment, as that's the gmock issue you're running into.
Please have a look. https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database_unittest.cc (right): https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... content/browser/indexed_db/indexed_db_database_unittest.cc:262: IndexedDBConnection* connection_; On 2016/06/30 at 00:17:25, Marijn Kruisselbrink wrote: > this should probably be a unique_ptr<IndexedDBConnection> to make sure the connection actually gets cleaned up at the end of a test? Done. https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:71: IndexedDBConnection* connection, On 2016/06/30 at 00:14:06, dmurph wrote: > Please change back to WeakPtr here, based on our convo. Done. https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction_unittest.cc (right): https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction_unittest.cc:91: id, new IndexedDBConnection(db_, new MockIndexedDBDatabaseCallbacks()), On 2016/06/30 at 00:17:25, Marijn Kruisselbrink wrote: > here too I believe you're leaking IndexedDBConnection instances. You need to make sure that every IndexedDBConnection you create is also actually destroyed at some point. Done. https://codereview.chromium.org/2062203004/diff/250001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction_unittest.cc:352: IndexedDBConnection* connection = On 2016/06/30 at 00:17:25, Marijn Kruisselbrink wrote: > probably make this a std::unique_ptr<IndexedDBConnection> to ensure it doesn't leak. Done. https://codereview.chromium.org/2062203004/diff/250001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.cc (right): https://codereview.chromium.org/2062203004/diff/250001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.cc:176: observer->setId(observer_id); On 2016/06/30 at 00:14:06, dmurph wrote: > Move this to IDBObserver pls. Done. https://codereview.chromium.org/2062203004/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/MockWebIDBDatabase.h (right): https://codereview.chromium.org/2062203004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/MockWebIDBDatabase.h:34: // Gmock does not support movable type, so cannot use MOCK_METHOD for addObserver. On 2016/06/30 at 00:17:25, Marijn Kruisselbrink wrote: > You could mention https://github.com/google/googletest/issues/395 in this comment, as that's the gmock issue you're running into. Thanks. I'll do that.
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_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
lgtm with nits: Please edit your summary to follow what Chris said: <summary line> <blank line> <supplemental description> This is so that Git can extract a nice one-line summary. https://codereview.chromium.org/2062203004/diff/270001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/270001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:141: // TODO(palakj): Eliminate callbacks_ and database_ as we already have nit: remove this todo, as we still want to have refptrs to increment the refcount. This probably won't effect the database, but definitely the callbacks. https://codereview.chromium.org/2062203004/diff/270001/content/browser/indexe... File content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.cc (right): https://codereview.chromium.org/2062203004/diff/270001/content/browser/indexe... content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.cc:268: IndexedDBConnection* connection, nit: update this to weakptr?
Description was changed from ========== Adds observer to database * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * As per the spec, the observer begins observing after the completion of transaction it was created with. BUG=609934 patch from issue 2031113002 at patchset 100001 (http://crrev.com/2031113002#ps100001) 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 ========== to ========== [IDBObserver] Lifetime Management: Adding and Removing Observer * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * The observer begins observing after the completion of transaction it was created with. * The observer is removed when we have an unobserve call on observer or database connection closes. 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 ==========
Changed some expected test results for observer interface. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:41: base::WeakPtr<IndexedDBConnection> GetWeakPtr() { On 2016/06/28 at 20:26:22, cmumford wrote: > I think this is the "old" way. Instead consider deriving IndexedDBConnection from base::SupportsWeakPtr<IndexedDBConnection>, and then you can delete GetWeakPtr() and use AsWeakPtr() to mint a new WeakPtr. > > BTW, are you sure you're obeying the threading rules in order to use a WeakPtr to this class? Read comment in weak_ptr.h. Not really sure about thread safety. The transaction that holds weakptr to connection, is supposed to die before the connection does. Not certain if the weakptr is always " dereferenced and invalidated on the same SequencedTaskRunner" as mentioned in weak_ptr.h Do you feel there's a problem here?
lgtm
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2062203004/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2062203004/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:53: observerPtr->setId(observerId); Would you help me understand why you need to introduce an ID system? Can we probably just make WebIDBObserverImpl GarbageCollected and make Backend::m_observers a hash set of WeakMembers?
Not yet finished, but wanted to send out these comments/questions. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:65: void IndexedDBConnection::RemoveObservers( On 2016/06/29 23:02:40, palakj1 wrote: > On 2016/06/28 at 20:26:22, cmumford wrote: > > Would this function be simpler if active_observers_ was a map of id -> > observer? > > > Yes. But the more dominant (much more used) use case would involve iterating > over active_observers_ for determining it the observer is listening for a change > or not (part of next cl) like this: > > observers = connection->GetActiveObservers() > for(const auto& observer : observers ) { > if(observer->IsRecording(some property)){ > add observation > } > > I believe a vector would be faster for this case than map. But if they are > almost equivalent performance wise, I can sure switch to map. I think these should be called in order, so that might make using a map wrong. Probably best to stick with a vector. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:41: base::WeakPtr<IndexedDBConnection> GetWeakPtr() { On 2016/06/28 20:37:30, Marijn Kruisselbrink wrote: > On 2016/06/28 at 20:26:22, cmumford wrote: > > I think this is the "old" way. Instead consider deriving IndexedDBConnection > from base::SupportsWeakPtr<IndexedDBConnection>, and then you can delete > GetWeakPtr() and use AsWeakPtr() to mint a new WeakPtr. > The "old" way? I think in general a WeakPtrFactory is preferred over inheriting > from base::SupportsWeakPtr. See the comment for SupportsWeakPtr "since > SupportsWeakPtr's destructor won't invalidate weak pointers to the class until > after the derived class' members have been destroyed, its use can lead to subtle > use-after-destroy issues.". There even was some discussion back in 2013 to > completely kill SupportsWeakPtr in favor of WeakPtrFactory > (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2p...) Wow! Thanks for pointing that out Marijn. https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2062203004/diff/230001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:21: // TODO(palakj): Add other fields. On 2016/06/29 23:02:40, palakj1 wrote: > On 2016/06/28 at 20:26:23, cmumford wrote: > > What other fields do you intend to add? > > None, for this cl. I'll remove the todo for now. > > The fields to be added later act as filters for the observer: > map<objectstores, list<indexeddb_key_ranges>>; > bool value, noRecord, transaction; I was less concerned with the TODO comment, just trying to see what this class was going to eventually look like. thx. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:67: // TODO(palakj): Change from vector to set or create tmp vector instead of This seems like a TODO that can be done now - any reason why not? I only ask because TODO's have a way of staying in the code for years (or forever), so best to avoid. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:71: bool removed = false; Your implementation is fine, but using a lambda might be cleaner - that is, if you find lambda's clean. const auto& it = std::find_if( active_observers_.begin(), active_observers_.end(), [&id_to_remove](const std::unique_ptr<IndexedDBObserver>& o) { return o->id() == id_to_remove; }); if (it != active_observers_.end()) active_observers_.erase(it); else pending_observer_ids.push_back(id_to_remove); But passing in a std::set<> would be most efficient. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:81: if (!removed) { Nit: Curly-braces not required for single-line statements: https://google.github.io/styleguide/cppguide.html#Conditionals https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (left): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:15: class IndexedDBCallbacks; This is unrelated to your change, but can you delete the unused forward declaration of IndexedDBConnection? https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:32: // The observers begin listening to changes only once they are activated. Nit: I think this comment is better placed above active_observers_. https://google.github.io/styleguide/cppguide.html#Function_Comments "Declaration comments describe use of the function; comments at the definition of a function describe operation." https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:33: virtual void ActivatePendingObservers( Should pending_observers be a non-const reference - this makes a copy of that vector right? https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:35: virtual void RemoveObservers(const std::vector<int32_t>& remove_observer_ids); Maybe add a comment above RemoveObservers stating that it will remove either a pending or an active observer. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.h (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.h:129: void RemovePendingObservers(IndexedDBConnection*, Nit: Blink style says to "Leave meaningless variable names out of function declarations", but in chromium we always have them, so add "connection". https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database_unittest.cc (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_database_unittest.cc:241: std::unique_ptr<IndexedDBConnection> connection_( connection_ is a local variable (not a class member), so shouldn't have a trailing underscore. Also, once this method, Setup(), goes out of scope transaction_ will have a null WeakPtr to a connection. Do you want to make connection a member of this class? https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:650: DCHECK(parent_->context()->TaskRunner()->RunsTasksOnCurrentThread()); Nit: My preference would be to DCHECK that observer_ids_to_remove is not empty. The render process should never be sending a message with an empty list. Also, remove the empty check below. If RemoveObservers does get called with an empty list (which shouldn't happen) then it's no big deal. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:18: IndexedDBObserver(int32_t); Add parameter name. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:21: int32_t id() { return observer_id_; } make method const. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:197: connection_ = nullptr; What about also calling: pending_observers_.clear(); https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:462: for (size_t i = 0; i < pending_observer_ids.size(); i++) { Nit: I don't believe we have a rule on using iterators vs indexes - I don't believe we do. I generally avoid using "break" when possible, and iterators make it easier to switch from one container type to another without modifying the code - but may be less pleasing to the eye. auto it = pending_observers_.begin(); while (it != pending_observers_.end()) { if (std::find(pending_observer_ids.begin(), pending_observer_ids.end(), (*it)->id()) != pending_observer_ids.end()) it = pending_observers_.erase(it); else ++it; } Just something to consider - only change if you think this is cleaner. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (left): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:72: IndexedDBDatabaseCallbacks* connection() const { return callbacks_.get(); } Thanks for fixing this incorrect name. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:69: // Removes |pending_observer_ids| from |pending_observers_| if it is present. Nit: I think the comment is a little odd because pending_observers_ isn't a collection of the same type, and it really deletes the observer too. Maybe a less specific comment like: // Delete pending observers with ID's listed in |pending_observer_ids|. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:141: // TODO(palakj): Eliminate callbacks_ and database_ as we already have Same on this TODO - it'd be great to consolidate these now before ever landing in the codebase. I generally try to only add a TODO when I see something that already exists that needs to be changed. However, if it's new code I try to do it "right" the first time. https://google.github.io/styleguide/cppguide.html#TODO_Comments https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:146: // These observers do not listen to changes until they are activated. Nit: delete "They" and "they are". https://codereview.chromium.org/2062203004/diff/290001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/290001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:84: // to clean up Nit: comment wrap. https://codereview.chromium.org/2062203004/diff/290001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.cc (right): https://codereview.chromium.org/2062203004/diff/290001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:34: using blink::WebIDBObserver; Either forward declare (if possible), or #include WebIDBObserve.h, but not both.
https://codereview.chromium.org/2062203004/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2062203004/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:53: observerPtr->setId(observerId); On 2016/07/01 at 01:55:02, haraken wrote: > Would you help me understand why you need to introduce an ID system? > > Can we probably just make WebIDBObserverImpl GarbageCollected and make Backend::m_observers a hash set of WeakMembers? Do you mean in the browser process? It's because everything is across IPC, so we need an ID for everything: connections, transactions, requests, callbacks, etc.
Please have a look. https://codereview.chromium.org/2062203004/diff/270001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/270001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:141: // TODO(palakj): Eliminate callbacks_ and database_ as we already have On 2016/06/30 at 18:06:07, dmurph wrote: > nit: remove this todo, as we still want to have refptrs to increment the refcount. This probably won't effect the database, but definitely the callbacks. Done. https://codereview.chromium.org/2062203004/diff/270001/content/browser/indexe... File content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.cc (right): https://codereview.chromium.org/2062203004/diff/270001/content/browser/indexe... content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.cc:268: IndexedDBConnection* connection, On 2016/06/30 at 18:06:07, dmurph wrote: > nit: update this to weakptr? Done. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.cc:81: if (!removed) { On 2016/07/01 at 18:35:00, cmumford wrote: > Nit: Curly-braces not required for single-line statements: > > https://google.github.io/styleguide/cppguide.html#Conditionals Done. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (left): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:15: class IndexedDBCallbacks; On 2016/07/01 at 18:35:00, cmumford wrote: > This is unrelated to your change, but can you delete the unused forward declaration of IndexedDBConnection? Done. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_connection.h (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:32: // The observers begin listening to changes only once they are activated. On 2016/07/01 at 18:35:00, cmumford wrote: > Nit: I think this comment is better placed above active_observers_. > > https://google.github.io/styleguide/cppguide.html#Function_Comments > > "Declaration comments describe use of the function; comments at the definition of a function describe operation." Thanks for that nit. Change made. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:33: virtual void ActivatePendingObservers( On 2016/07/01 at 18:35:00, cmumford wrote: > Should pending_observers be a non-const reference - this makes a copy of that vector right? I am actually moving the argument vector not copying it connection_->ActivatePendingObservers(std::move(pending_observers_)); (indexed_db_transaction.cc:348) Since, we need to move the elements of the vector, can't pass it as a const ref. I can make it a pointer however, if that's preferred to moving. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_connection.h:35: virtual void RemoveObservers(const std::vector<int32_t>& remove_observer_ids); On 2016/07/01 at 18:35:00, cmumford wrote: > Maybe add a comment above RemoveObservers stating that it will remove either a pending or an active observer. Done. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database.h (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_database.h:129: void RemovePendingObservers(IndexedDBConnection*, On 2016/07/01 at 18:35:00, cmumford wrote: > Nit: Blink style says to "Leave meaningless variable names out of function declarations", but in chromium we always have them, so add "connection". Done. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_database_unittest.cc (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_database_unittest.cc:241: std::unique_ptr<IndexedDBConnection> connection_( On 2016/07/01 at 18:35:00, cmumford wrote: > connection_ is a local variable (not a class member), so shouldn't have a trailing underscore. > > Also, once this method, Setup(), goes out of scope transaction_ will have a null WeakPtr to a connection. Do you want to make connection a member of this class? Thanks for pointing that out. Turned connection to a member. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:650: DCHECK(parent_->context()->TaskRunner()->RunsTasksOnCurrentThread()); On 2016/07/01 at 18:35:00, cmumford wrote: > Nit: My preference would be to DCHECK that observer_ids_to_remove is not empty. The render process should never be sending a message with an empty list. Also, remove the empty check below. If RemoveObservers does get called with an empty list (which shouldn't happen) then it's no big deal. That makes a lot more sense. Thanks. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:18: IndexedDBObserver(int32_t); On 2016/07/01 at 18:35:01, cmumford wrote: > Add parameter name. Done. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:21: int32_t id() { return observer_id_; } On 2016/07/01 at 18:35:01, cmumford wrote: > make method const. Done. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:197: connection_ = nullptr; On 2016/07/01 at 18:35:01, cmumford wrote: > What about also calling: > > pending_observers_.clear(); Done. Also adding a check in transaction destructor DCHECK(pending_observers_.empty()) https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:462: for (size_t i = 0; i < pending_observer_ids.size(); i++) { On 2016/07/01 at 18:35:01, cmumford wrote: > Nit: I don't believe we have a rule on using iterators vs indexes - I don't believe we do. I generally avoid using "break" when possible, and iterators make it easier to switch from one container type to another without modifying the code - but may be less pleasing to the eye. > > auto it = pending_observers_.begin(); > while (it != pending_observers_.end()) { > if (std::find(pending_observer_ids.begin(), pending_observer_ids.end(), > (*it)->id()) != pending_observer_ids.end()) > it = pending_observers_.erase(it); > else > ++it; > } > > Just something to consider - only change if you think this is cleaner. Changed. Used remove_if function instead to eliminate the loop. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.h (right): https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:69: // Removes |pending_observer_ids| from |pending_observers_| if it is present. On 2016/07/01 at 18:35:01, cmumford wrote: > Nit: I think the comment is a little odd because pending_observers_ isn't a collection of the same type, and it really deletes the observer too. Maybe a less specific comment like: > > // Delete pending observers with ID's listed in |pending_observer_ids|. Done. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:141: // TODO(palakj): Eliminate callbacks_ and database_ as we already have On 2016/07/01 at 18:35:01, cmumford wrote: > Same on this TODO - it'd be great to consolidate these now before ever landing in the codebase. I generally try to only add a TODO when I see something that already exists that needs to be changed. However, if it's new code I try to do it "right" the first time. > > https://google.github.io/styleguide/cppguide.html#TODO_Comments I am not sure if I can eliminate these. callbacks_ and database_ are scoped_refptr. I have it as a TODO cause I'd need some guidance here. https://codereview.chromium.org/2062203004/diff/290001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.h:146: // These observers do not listen to changes until they are activated. On 2016/07/01 at 18:35:01, cmumford wrote: > Nit: delete "They" and "they are". Done. https://codereview.chromium.org/2062203004/diff/290001/content/child/indexed_... File content/child/indexed_db/indexed_db_dispatcher.h (right): https://codereview.chromium.org/2062203004/diff/290001/content/child/indexed_... content/child/indexed_db/indexed_db_dispatcher.h:84: // to clean up On 2016/07/01 at 18:35:01, cmumford wrote: > Nit: comment wrap. done. https://codereview.chromium.org/2062203004/diff/290001/content/child/indexed_... File content/child/indexed_db/webidbdatabase_impl.cc (right): https://codereview.chromium.org/2062203004/diff/290001/content/child/indexed_... content/child/indexed_db/webidbdatabase_impl.cc:34: using blink::WebIDBObserver; On 2016/07/01 at 18:35:01, cmumford wrote: > Either forward declare (if possible), or #include WebIDBObserve.h, but not both. 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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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...
Please have a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
a few nits and one bug. Still lgtm w/ the bug fix. https://codereview.chromium.org/2062203004/diff/330001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2062203004/diff/330001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:24: DISALLOW_COPY_AND_ASSIGN(IndexedDBObserver); nit: we usually put these statements at the end of the class, after the class members. So: private: int32_t observer_id_; DISALLOW_COPY_AND_ASSIGN(IndexedDBObserver); }; https://codereview.chromium.org/2062203004/diff/330001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/330001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:470: if (it != pending_observers_.end()) This code currently removes more than it should, I don't believe we need this if statement + erase. the remove_if should handle the removal just fine.
https://codereview.chromium.org/2062203004/diff/330001/content/browser/indexe... File content/browser/indexed_db/indexed_db_observer.h (right): https://codereview.chromium.org/2062203004/diff/330001/content/browser/indexe... content/browser/indexed_db/indexed_db_observer.h:24: DISALLOW_COPY_AND_ASSIGN(IndexedDBObserver); On 2016/07/06 at 18:25:10, dmurph wrote: > nit: we usually put these statements at the end of the class, after the class members. So: > > private: > int32_t observer_id_; > > DISALLOW_COPY_AND_ASSIGN(IndexedDBObserver); > }; Oops! changing this with the next patch. https://codereview.chromium.org/2062203004/diff/330001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/330001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:470: if (it != pending_observers_.end()) On 2016/07/06 at 18:25:10, dmurph wrote: > This code currently removes more than it should, I don't believe we need this if statement + erase. the remove_if should handle the removal just fine. remove_if doesn't really erase anything, it simply moves elements not satisfied by the lambda function in the front and returns an iterator following the last element not removed. http://stackoverflow.com/questions/9053883/eraseremove-if-anomaly
https://codereview.chromium.org/2062203004/diff/330001/content/browser/indexe... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2062203004/diff/330001/content/browser/indexe... content/browser/indexed_db/indexed_db_transaction.cc:470: if (it != pending_observers_.end()) On 2016/07/06 at 18:58:44, palakj1 wrote: > On 2016/07/06 at 18:25:10, dmurph wrote: > > This code currently removes more than it should, I don't believe we need this if statement + erase. the remove_if should handle the removal just fine. > > remove_if doesn't really erase anything, it simply moves elements not satisfied by the lambda function in the front and returns an iterator following the last element not removed. > http://stackoverflow.com/questions/9053883/eraseremove-if-anomaly Makes sense, I didn't fully read the spec. lgtm
https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:50: WebIDBObserverImpl* observerPtr = observer.get(); Can you remove observerPtr and instead have WebIDBDatabaseImpl::addObserver() set the observer ID? Technically after addObserver() is called the database now owns that object, and it's not guaranteed to even still exist, or have the same address right? https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:62: std::set<int32_t>::iterator it = m_observerIds.begin(); Nit: You could make this "auto it = ..." if you want, but is OK as is. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:18: class IDBDatabase; Put forward declarations in alphabetical order. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:42: std::set<int32_t> m_observerIds; So both IDBObserver and WebIDBDatabaseImpl have a set of observer ID's, both are in the same thread, and both are identical (ignoring short time during add and erase) right? Is there any reason why we can't just use the one std::set() owned by WebIDBDatabaseImpl? https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.cpp (right): https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.cpp:19: : m_observer(observer) m_id is not initialized in the constructor. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.cpp:26: m_observer->removeObserver(m_id); You should create some constant to represent the uninitialized value for m_id, and only call removeObserver if it has been set via setId(). https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h (right): https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:23: void setId(int32_t id) { m_id = id; } Nit: Not required, but when you add a constant for initializing m_id, you could add a DCHECK to assert that m_id is only initialized once.
https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:42: std::set<int32_t> m_observerIds; On 2016/07/06 22:57:57, cmumford wrote: > So both IDBObserver and WebIDBDatabaseImpl have a set of observer ID's, both are > in the same thread, and both are identical (ignoring short time during add and > erase) right? Is there any reason why we can't just use the one std::set() owned > by WebIDBDatabaseImpl? They're not identical. Here's the overview: 1. Every observer can have multiple 'observe call' ids. This can belong to multiple databases, as we can observe multiple databases. There is one id per call to 'observe'. 2. Every database has a set of 'observe call' ids that all of the observervations registered for that database. These can be from different observers. operations: 1. observer.unobserve(db) stops observation on the given database. This means we need to remove all of the ids we have in that observer that apply to the given database, as well as remove those from the database, and then tell the backend this set of ids. 2. db.close() needs to make all observers that are observing it remove any 'observe call' ids that apply to that database. We do this through the WebIDBObserver destructor (which we have one registered per observe call). Does that make more sense?
Please have a look. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right): https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:50: WebIDBObserverImpl* observerPtr = observer.get(); On 2016/07/06 at 22:57:56, cmumford wrote: > Can you remove observerPtr and instead have WebIDBDatabaseImpl::addObserver() set the observer ID? Technically after addObserver() is called the database now owns that object, and it's not guaranteed to even still exist, or have the same address right? That's true, technically speaking. If we want it to be completely correct, it is the dispatcher and not the WebIDBDatabaseImpl that owns the observer object. I can do the adding id part there. We just wanted to have less overhead in content::indexed_db_dispatcher.cc. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:62: std::set<int32_t>::iterator it = m_observerIds.begin(); On 2016/07/06 at 22:57:56, cmumford wrote: > Nit: You could make this "auto it = ..." if you want, but is OK as is. Done. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:18: class IDBDatabase; On 2016/07/06 at 22:57:56, cmumford wrote: > Put forward declarations in alphabetical order. Done. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:42: std::set<int32_t> m_observerIds; On 2016/07/06 at 22:57:57, cmumford wrote: > So both IDBObserver and WebIDBDatabaseImpl have a set of observer ID's, both are in the same thread, and both are identical (ignoring short time during add and erase) right? Is there any reason why we can't just use the one std::set() owned by WebIDBDatabaseImpl? They are in the same thread, but not the same. Consider, obs1 = new IDBObserver(); obs2 = new IDBObserver(); obs1.observe(db1); // id 1 obs1.observe(db2); // id 2 obs2.observe(db1); // id 3 obs1.observer_id = { 1, 2 } obs2.observer_id = { 3 } db1.observer_id = { 1, 3 } db2.observer_id = { 2 } obs1.unobserve(db2) // removes id 2 db1.close() // removes id 1, 3 We need both the lists because there are 2 ways to remove the observe calls i. obs.unobserve(db) : This removes all observe calls associated with the observer object 'obs' on the database 'db'. ii. connection close : This removes all observe calls of all the observers on the database 'db'. Thus, we need to bookkeep observe calls on an intersection of observer and database to deal with unobserve functionality. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.cpp (right): https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.cpp:19: : m_observer(observer) On 2016/07/06 at 22:57:57, cmumford wrote: > m_id is not initialized in the constructor. Done. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.cpp:26: m_observer->removeObserver(m_id); On 2016/07/06 at 22:57:57, cmumford wrote: > You should create some constant to represent the uninitialized value for m_id, and only call removeObserver if it has been set via setId(). Done. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h (right): https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.h:23: void setId(int32_t id) { m_id = id; } On 2016/07/06 at 22:57:57, cmumford wrote: > Nit: Not required, but when you add a constant for initializing m_id, you could add a DCHECK to assert that m_id is only initialized once. Done.
LGTM - with one minor comment. https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right): https://codereview.chromium.org/2062203004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:42: std::set<int32_t> m_observerIds; On 2016/07/06 23:34:24, dmurph wrote: > On 2016/07/06 22:57:57, cmumford wrote: > > So both IDBObserver and WebIDBDatabaseImpl have a set of observer ID's, both > are > > in the same thread, and both are identical (ignoring short time during add and > > erase) right? Is there any reason why we can't just use the one std::set() > owned > > by WebIDBDatabaseImpl? > > They're not identical. Here's the overview: > 1. Every observer can have multiple 'observe call' ids. This can belong to > multiple databases, as we can observe multiple databases. There is one id per > call to 'observe'. > > 2. Every database has a set of 'observe call' ids that all of the > observervations registered for that database. These can be from different > observers. > > operations: > 1. observer.unobserve(db) stops observation on the given database. This means we > need to remove all of the ids we have in that observer that apply to the given > database, as well as remove those from the database, and then tell the backend > this set of ids. > 2. db.close() needs to make all observers that are observing it remove any > 'observe call' ids that apply to that database. We do this through the > WebIDBObserver destructor (which we have one registered per observe call). > > Does that make more sense? yep - thx. https://codereview.chromium.org/2062203004/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.cpp (right): https://codereview.chromium.org/2062203004/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.cpp:27: DCHECK_NE(kInvalidObserverId, m_id); I think this is OK to have a runtime check - not a DCHECK. Just to enable this class to be created and destroyed w/o calling setId.
The CQ bit was checked by palakj@google.com to run a CQ dry run
https://codereview.chromium.org/2062203004/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.cpp (right): https://codereview.chromium.org/2062203004/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBObserverImpl.cpp:27: DCHECK_NE(kInvalidObserverId, m_id); On 2016/07/07 at 19:48:51, cmumford wrote: > I think this is OK to have a runtime check - not a DCHECK. Just to enable this class to be created and destroyed w/o calling setId. replacing this with a if check if (m_id != kInvalidObserverId) m_observer->removeObserver(m_id);
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_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by palakj@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, mek@chromium.org, dmurph@chromium.org, cmumford@chromium.org Link to the patchset: https://codereview.chromium.org/2062203004/#ps370001 (title: "Final architechture")
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 #20 (id:370001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [IDBObserver] Lifetime Management: Adding and Removing Observer * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * The observer begins observing after the completion of transaction it was created with. * The observer is removed when we have an unobserve call on observer or database connection closes. 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 ========== [IDBObserver] Lifetime Management: Adding and Removing Observer * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * The observer begins observing after the completion of transaction it was created with. * The observer is removed when we have an unobserve call on observer or database connection closes. 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 Committed: https://crrev.com/05f35eab096fe52eab3af2c6db191c1285dc5a68 Cr-Commit-Position: refs/heads/master@{#404283} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/05f35eab096fe52eab3af2c6db191c1285dc5a68 Cr-Commit-Position: refs/heads/master@{#404283}
Message was sent while issue was closed.
Description was changed from ========== [IDBObserver] Lifetime Management: Adding and Removing Observer * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * The observer begins observing after the completion of transaction it was created with. * The observer is removed when we have an unobserve call on observer or database connection closes. 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 Committed: https://crrev.com/05f35eab096fe52eab3af2c6db191c1285dc5a68 Cr-Commit-Position: refs/heads/master@{#404283} ========== to ========== [IDBObserver] Lifetime Management: Adding and Removing Observer * Corresponding to each observe call at renderer side, an Observer object is created in the backend. * The observer begins observing after the completion of transaction it was created with. * The observer is removed when we have an unobserve call on observer or database connection closes. 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/05f35eab096fe52eab3af2c6db191c1285dc5a68 Cr-Commit-Position: refs/heads/master@{#404283} ========== |
