IndexedDB Observers

IndexedDB Observers with webidl files, initial constructor for creating observer and observe function.

BUG=609934
Reference:  https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md

Current Implementation and Future Scope: https://goo.gl/Y2dobn

Committed: https://crrev.com/dd97a20cea3444559290d55e8fae06bd580cd5ba
Cr-Commit-Position: refs/heads/master@{#400796}

[palakj1, 2016-06-02 22:21:45]

palakj@google.com changed reviewers:
+ dmurph@chromium.org, jsbell@chromium.org, palakj@chromium.org

[palakj1, 2016-06-02 22:31:13]

palakj@google.com changed reviewers:
- palakj@chromium.org

[palakj1, 2016-06-02 22:31:14]

IndexedDB Observers  Preliminary design with webidl files, initial constructor
for creating observer and observe function.

Please note that the observer is not being stored as yet.

[dmurph, 2016-06-02 22:41:16]

This is all looking really really good!

Do you mind moving non-Blink (non-third_part/WebKit) changes into a different CL
as well? Let's only put the IDL and accompanying lifetime/parsing changes in
here so it can be reviewed quicker. This would also move out the
WebIDBDatabase.h change, and the IDBObserver.cpp:56 change.

[palakj1, 2016-06-02 23:18:16]

Please have a look.

[dmurph, 2016-06-02 23:30:39]

dmurph@chromium.org changed reviewers:
+ mek@chromium.org

[dmurph, 2016-06-02 23:30:40]

+Marijn

[dmurph, 2016-06-02 23:48:38]

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h
(right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h:27: static
void setWeakCallback(const v8::WeakCallbackInfo<V8IDBObserverCallback>&);
Do we need this?

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
(right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:20:
V8ThrowException::throwException(createMinimumArityTypeErrorForMethod(info.GetIsolate(),
"createIDBObserver", "IDB", 1, info.Length()), info.GetIsolate());
s/"IDB"/"IDBObserver"/ here an below.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:48:
visitor->template registerWeakMembers<IDBObserver,
&IDBObserver::clearWeakMembers>(this);
I did not even know that this syntax was a thing haha.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:28: long long id;
Don't worry about an ID for now.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:29: WebIDBDatabase*
backendDB() const;
Remove this too.

[dmurph, 2016-06-02 23:48:38]

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h
(right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h:27: static
void setWeakCallback(const v8::WeakCallbackInfo<V8IDBObserverCallback>&);
Do we need this?

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
(right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:20:
V8ThrowException::throwException(createMinimumArityTypeErrorForMethod(info.GetIsolate(),
"createIDBObserver", "IDB", 1, info.Length()), info.GetIsolate());
s/"IDB"/"IDBObserver"/ here an below.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:48:
visitor->template registerWeakMembers<IDBObserver,
&IDBObserver::clearWeakMembers>(this);
I did not even know that this syntax was a thing haha.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:28: long long id;
Don't worry about an ID for now.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:29: WebIDBDatabase*
backendDB() const;
Remove this too.

[Marijn Kruisselbrink, 2016-06-02 23:51:25]

Some initial comments (and I realize at least a bunch of my comments are because
unfortunately the code you copied this from is incorrect...)

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h
(right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h:20:
CORE_EXPORT V8IDBObserverCallback(v8::Local<v8::Function>,
v8::Local<v8::Object>, ScriptState*);
Code in modules/ should use MODULES_EXPORT rather than CORE_EXPORT. I'm not sure
you need either of them here though, but it's also generally more common to
export entire types rather than just exporting the constructor.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
(right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:20:
V8ThrowException::throwException(createMinimumArityTypeErrorForMethod(info.GetIsolate(),
"createIDBObserver", "IDB", 1, info.Length()), info.GetIsolate());
I think this should be createMinimumArityTypeErrorForConstructor rather than
createMinimumArityTypeErrorForMethod?

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:27:
V8ThrowException::throwTypeError(info.GetIsolate(),
ExceptionMessages::failedToExecute("createIDBObserver", "IDB", "The callback
provided as parameter 1 is not a function."));
All these other failedToExecute methods also seem like they should be
failedToConstruct("IDBObserver", ... rather than
failedToExecute("createIDBObserver", "IDB", ...? After all these are the custom
bindings of a constructor for an IDBObserver type, not for a createIDBObserver
method in the IDB interface.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:37:
ExceptionState exceptionState(ExceptionState::ConstructionContext, "IDB",
info.Holder(), info.GetIsolate());
Here also "IDB" should be "IDBObserver" to get correct error messages.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:42: void
IDBObserver::observe(IDBDatabase* db, IDBTransaction* tx, ExceptionState&
exceptionState)
Blink coding style doesn't like using abbreviations (point 2 in
https://www.chromium.org/blink/coding-style#TOC-Names). So call these
"database", "transaction" etc, rather than db and tx.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:9: #include
"bindings/core/v8/SerializedScriptValue.h"
I don't see where this include is used in this file?

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:28: long long id;
Not sure what this |id| is?

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:31: void
observe(IDBDatabase* db, IDBTransaction* tx, ExceptionState& exceptionState);
Blink styleguide doesn't want redundant parameter names (see point 9 in
https://www.chromium.org/blink/coding-style#TOC-Names). The names of these
parameters don't add anything, so leave them out.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:36: explicit
IDBObserver(IDBObserverCallback&, bool transaction, bool values, bool
noRecords);
Having bool parameters (and especially having multiple bool parameters) tends to
make code less readable. Why not just pass the const IDBObserverInit& to the
constructor?

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:39: bool
m_transaction, m_values, m_noRecords;
We generally prefer having every variable or field declaration be its own
statement. So break this up in three lines/declarations.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:1: [
Add a copyright header to this file.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl
(right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl:1: enum
IDBObserverChangesRecordType {
Add a copyright header to this file.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverInit.idl (right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserverInit.idl:1: [
Add a copyright header to this file.

[palakj1, 2016-06-03 01:21:53]

changes post dmuprh, mek reviews

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h
(right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h:20:
CORE_EXPORT V8IDBObserverCallback(v8::Local<v8::Function>,
v8::Local<v8::Object>, ScriptState*);
On 2016/06/02 at 23:51:24, Marijn Kruisselbrink wrote:
> Code in modules/ should use MODULES_EXPORT rather than CORE_EXPORT. I'm not
sure you need either of them here though, but it's also generally more common to
export entire types rather than just exporting the constructor.

Removed CORE_EXPORT

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.h:27: static
void setWeakCallback(const v8::WeakCallbackInfo<V8IDBObserverCallback>&);
On 2016/06/02 at 23:48:37, dmurph wrote:
> Do we need this?

I had copied this peice of code from Intersection observer blindly. But
interestingly enough, post my checkout of the code, this segment seems to have
been removed from the original codebase. 
I am not certain if it's safe to remove it from our code as well. It's used here
V8IDBObserverCallback.cpp:22   m_callback.setWeak(this, &setWeakCallback);

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
(right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:20:
V8ThrowException::throwException(createMinimumArityTypeErrorForMethod(info.GetIsolate(),
"createIDBObserver", "IDB", 1, info.Length()), info.GetIsolate());
On 2016/06/02 at 23:48:37, dmurph wrote:
> s/"IDB"/"IDBObserver"/ here an below.

Done

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:27:
V8ThrowException::throwTypeError(info.GetIsolate(),
ExceptionMessages::failedToExecute("createIDBObserver", "IDB", "The callback
provided as parameter 1 is not a function."));
On 2016/06/02 at 23:51:24, Marijn Kruisselbrink wrote:
> All these other failedToExecute methods also seem like they should be
failedToConstruct("IDBObserver", ... rather than
failedToExecute("createIDBObserver", "IDB", ...? After all these are the custom
bindings of a constructor for an IDBObserver type, not for a createIDBObserver
method in the IDB interface.

Changes made.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp:37:
ExceptionState exceptionState(ExceptionState::ConstructionContext, "IDB",
info.Holder(), info.GetIsolate());
On 2016/06/02 at 23:51:24, Marijn Kruisselbrink wrote:
> Here also "IDB" should be "IDBObserver" to get correct error messages.

Done

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:42: void
IDBObserver::observe(IDBDatabase* db, IDBTransaction* tx, ExceptionState&
exceptionState)
On 2016/06/02 at 23:51:24, Marijn Kruisselbrink wrote:
> Blink coding style doesn't like using abbreviations (point 2 in
https://www.chromium.org/blink/coding-style#TOC-Names). So call these
"database", "transaction" etc, rather than db and tx.

Done. Thanks for the tip.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:48:
visitor->template registerWeakMembers<IDBObserver,
&IDBObserver::clearWeakMembers>(this);
On 2016/06/02 at 23:48:37, dmurph wrote:
> I did not even know that this syntax was a thing haha.

I don't think we'd need this. Removing it.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right):

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:9: #include
"bindings/core/v8/SerializedScriptValue.h"
On 2016/06/02 at 23:51:24, Marijn Kruisselbrink wrote:
> I don't see where this include is used in this file?

Removed.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:28: long long id;
On 2016/06/02 at 23:48:38, dmurph wrote:
> Don't worry about an ID for now.

id removed.

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:29: WebIDBDatabase*
backendDB() const;
On 2016/06/02 at 23:48:37, dmurph wrote:
> Remove this too.

Oops! forgot to remove this. Done

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:31: void
observe(IDBDatabase* db, IDBTransaction* tx, ExceptionState& exceptionState);
On 2016/06/02 at 23:51:24, Marijn Kruisselbrink wrote:
> Blink styleguide doesn't want redundant parameter names (see point 9 in
https://www.chromium.org/blink/coding-style#TOC-Names). The names of these
parameters don't add anything, so leave them out.

Done

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:36: explicit
IDBObserver(IDBObserverCallback&, bool transaction, bool values, bool
noRecords);
On 2016/06/02 at 23:51:24, Marijn Kruisselbrink wrote:
> Having bool parameters (and especially having multiple bool parameters) tends
to make code less readable. Why not just pass the const IDBObserverInit& to the
constructor?

Done

https://codereview.chromium.org/2031113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:39: bool
m_transaction, m_values, m_noRecords;
On 2016/06/02 at 23:51:24, Marijn Kruisselbrink wrote:
> We generally prefer having every variable or field declaration be its own
statement. So break this up in three lines/declarations.

Done

[dmurph, 2016-06-03 23:35:01]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 23:36:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/100001

[commit-bot: I haz the power, 2016-06-03 23:57:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 23:57:12]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmurph, 2016-06-04 00:56:18]

You'll also need to edit V8PrivateValue so it compiles. Other than that this
looks good to me, just waiting until the next patch w/ compiling.

https://codereview.chromium.org/2031113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp
(right):

https://codereview.chromium.org/2031113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:21:
V8HiddenValue::setHiddenValue(scriptState, owner,
V8HiddenValue::callback(scriptState->isolate()), callback);
As discussed, please change to match. You'll have to modify V8PrivateValue as
well.

https://codereview.chromium.org/2031113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right):

https://codereview.chromium.org/2031113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:39: // to be
implemented
nit: remove this, or put:
"TODO(palakj): Finish implementation."

[palakj1, 2016-06-06 19:49:30]

https://codereview.chromium.org/2031113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp
(right):

https://codereview.chromium.org/2031113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:21:
V8HiddenValue::setHiddenValue(scriptState, owner,
V8HiddenValue::callback(scriptState->isolate()), callback);
On 2016/06/04 at 00:56:18, dmurph wrote:
> As discussed, please change to match. You'll have to modify V8PrivateValue as
well.

Done

https://codereview.chromium.org/2031113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right):

https://codereview.chromium.org/2031113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:39: // to be
implemented
On 2016/06/04 at 00:56:18, dmurph wrote:
> nit: remove this, or put:
> "TODO(palakj): Finish implementation."

Done

[palakj1, 2016-06-06 19:49:45]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-06 19:50:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/140001

[commit-bot: I haz the power, 2016-06-06 21:14:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-06 21:14:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[palakj1, 2016-06-06 22:01:08]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-06 22:01:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/180001

[dmurph, 2016-06-06 22:35:50]

lgtm

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right):

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:39: // TODO(palakj):
Finish implementation
nit: period at the end. We try to have all comments incomplete sentences, sorry
I didn't mention this more explicitly earlier.

https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G...

[Marijn Kruisselbrink, 2016-06-06 23:09:11]

Also link to whatever spec this is implementing in the CL description (as well
as adding a bug number to the BUG= line).

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/observer.html (right):

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:2:
<title>IndexedDB: UpgradeNeeded Tests</title>
This seems wrong?

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:9:
console.log('callit');
Logging to the console almost never seems like the right thing to do in a test.
Probably you just want to pass an empty callback to the IDBObserver constructor?

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:12: (function ()
{
Wrapping the async_test inside a (function() { ... }()); doesn't really gain you
anything. The entire test is contained within the function passed to async_test
anyway.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:32: 
Maybe also run the same tests from a worker context? Could be as simple as
moving the actual tests to a resources/observer-tests.js file and having one
html file include that js script directly, and another html file calling
fetch_tests_from_worker(new Worker('resources/observer-tests.js'));

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.idl (right):

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.idl:5: callback
IDBObserverCallback = void (IDBObserverChanges changes);
Can you add a comment to every type you define in all these IDL files with a
link to the (section of) the spec that defines that type? See all the other IDL
files here and in other parts of blink.

In particular there seem to be a number of discrepancies between these idl
definitions and the closest thing to a spec I could find
(https://github.com/WICG/indexed-db-observers/blob/gh-pages/IDBObservers.webidl),
so I wonder if I'm looking at the wrong thing.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:7: ]
dictionary IDBObserverChanges {
I wonder if this (and IDBOvserverChangesRecord) shouldn't be an interface rather
than a dictionary? But that's probably more a spec question, so feel free to
ignore for now. Generally dictionaries are used to pass things from javascript
to APIs, while interfaces are used to pass data from the platform back to
javascript.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:13: any
records;
And this is really getting into more questioning the spec rather than this
implementation of it; but would it make sense to have IDBObserverChanges be a
"maplike<DOMString, FrozenArray<IDBObserverChangesRecord>> (thereby behaving
like an ecmascript Map itself) rather than just containing a map?

[palakj1, 2016-06-07 00:08:35]

Description was changed from

==========
IndexedDB Observers with webidl files, initial constructor for creating observer
and observe function.

BUG=
==========

to

==========
IndexedDB Observers with webidl files, initial constructor for creating observer
and observe function.

BUG= 609934
Reference: 
https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md
==========

[palakj1, 2016-06-07 00:11:16]

Some changes in idl files which involve spec issues would be dealt in the next
patch.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/observer.html (right):

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:2:
<title>IndexedDB: UpgradeNeeded Tests</title>
On 2016/06/06 at 23:09:10, Marijn Kruisselbrink wrote:
> This seems wrong?

Sorry. updated.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:9:
console.log('callit');
On 2016/06/06 at 23:09:10, Marijn Kruisselbrink wrote:
> Logging to the console almost never seems like the right thing to do in a
test. Probably you just want to pass an empty callback to the IDBObserver
constructor?

Done

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:12: (function ()
{
On 2016/06/06 at 23:09:10, Marijn Kruisselbrink wrote:
> Wrapping the async_test inside a (function() { ... }()); doesn't really gain
you anything. The entire test is contained within the function passed to
async_test anyway.

Got your point. Done.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:32: 
On 2016/06/06 at 23:09:10, Marijn Kruisselbrink wrote:
> Maybe also run the same tests from a worker context? Could be as simple as
moving the actual tests to a resources/observer-tests.js file and having one
html file include that js script directly, and another html file calling
fetch_tests_from_worker(new Worker('resources/observer-tests.js'));

I'll work on this.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp (right):

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp:39: // TODO(palakj):
Finish implementation
On 2016/06/06 at 22:35:50, dmurph wrote:
> nit: period at the end. We try to have all comments incomplete sentences,
sorry I didn't mention this more explicitly earlier.
> 
>
https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G...

forgot about this again. Thanks.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.idl (right):

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.idl:5: callback
IDBObserverCallback = void (IDBObserverChanges changes);
On 2016/06/06 at 23:09:10, Marijn Kruisselbrink wrote:
> Can you add a comment to every type you define in all these IDL files with a
link to the (section of) the spec that defines that type? See all the other IDL
files here and in other parts of blink.
> 
> In particular there seem to be a number of discrepancies between these idl
definitions and the closest thing to a spec I could find
(https://github.com/WICG/indexed-db-observers/blob/gh-pages/IDBObservers.webidl),
so I wonder if I'm looking at the wrong thing.

The specs have been changed a bit. I've added the references. Will keep this in
mind for future work.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:7: ]
dictionary IDBObserverChanges {
On 2016/06/06 at 23:09:10, Marijn Kruisselbrink wrote:
> I wonder if this (and IDBOvserverChangesRecord) shouldn't be an interface
rather than a dictionary? But that's probably more a spec question, so feel free
to ignore for now. Generally dictionaries are used to pass things from
javascript to APIs, while interfaces are used to pass data from the platform
back to javascript.

Thanks for this. Didn't know about it. I'll make the changes after discussion
with @dmuprh.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:13: any
records;
On 2016/06/06 at 23:09:10, Marijn Kruisselbrink wrote:
> And this is really getting into more questioning the spec rather than this
implementation of it; but would it make sense to have IDBObserverChanges be a
"maplike<DOMString, FrozenArray<IDBObserverChangesRecord>> (thereby behaving
like an ecmascript Map itself) rather than just containing a map?

I don't have enough knowledge about it. It would be great if we could discuss
about this.

[dmurph, 2016-06-07 00:15:23]

regarding the spec you found, see this issue for our idl structure:
https://github.com/WICG/indexed-db-observers/issues/24

I'm going to be changing the IDL on GitHub soon.

[dmurph, 2016-06-07 00:19:20]

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.idl (right):

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.idl:5: callback
IDBObserverCallback = void (IDBObserverChanges changes);
On 2016/06/07 at 00:11:16, palakj1 wrote:
> On 2016/06/06 at 23:09:10, Marijn Kruisselbrink wrote:
> > Can you add a comment to every type you define in all these IDL files with a
link to the (section of) the spec that defines that type? See all the other IDL
files here and in other parts of blink.
> > 
> > In particular there seem to be a number of discrepancies between these idl
definitions and the closest thing to a spec I could find
(https://github.com/WICG/indexed-db-observers/blob/gh-pages/IDBObservers.webidl),
so I wonder if I'm looking at the wrong thing.
> 
> The specs have been changed a bit. I've added the references. Will keep this
in mind for future work.

regarding the spec you found, see this issue for our idl structure:
https://github.com/WICG/indexed-db-observers/issues/24

I'm going to be changing the IDL on GitHub soon.

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:7: ]
dictionary IDBObserverChanges {
On 2016/06/06 at 23:09:10, Marijn Kruisselbrink wrote:
> I wonder if this (and IDBOvserverChangesRecord) shouldn't be an interface
rather than a dictionary? But that's probably more a spec question, so feel free
to ignore for now. Generally dictionaries are used to pass things from
javascript to APIs, while interfaces are used to pass data from the platform
back to javascript.

Ah, does it make a big difference? We just want simple a data storage "pojo".

It looks like the intersection observer uses interface and not dictionary
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Inter...),
so maybe that's better?

https://codereview.chromium.org/2031113002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:13: any
records;
On 2016/06/06 at 23:09:10, Marijn Kruisselbrink wrote:
> And this is really getting into more questioning the spec rather than this
implementation of it; but would it make sense to have IDBObserverChanges be a
"maplike<DOMString, FrozenArray<IDBObserverChangesRecord>> (thereby behaving
like an ecmascript Map itself) rather than just containing a map?

That's really cool, I didn't know we had those keywords available.

[haraken, 2016-06-07 00:21:40]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-06-07 00:21:42]

https://codereview.chromium.org/2031113002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right):

https://codereview.chromium.org/2031113002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:14: #include
"wtf/RefPtr.h"

Remove unused headers. The same comment for other files in this CL.

https://codereview.chromium.org/2031113002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:15: any
records;

+bashi: Is this an expected usage? i.e., I want to make sure that this is the
best way to express map<string, sequence<>> in IDL dictionary.

https://codereview.chromium.org/2031113002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl
(right):

https://codereview.chromium.org/2031113002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl:17: any
value;

+bashi: Should this be 'any' or 'object'?

[haraken, 2016-06-07 00:21:52]

haraken@chromium.org changed reviewers:
+ bashi@chromium.org

[dmurph, 2016-06-07 18:42:16]

https://codereview.chromium.org/2031113002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:15: any
records;
On 2016/06/07 at 00:21:41, haraken wrote:
> +bashi: Is this an expected usage? i.e., I want to make sure that this is the
best way to express map<string, sequence<>> in IDL dictionary.

Unfortunately maplike only applies to the whole interface. See here:
https://heycam.github.io/webidl/#idl-maplike

So we have to use 'any', or I guess create a whole new interface?

https://codereview.chromium.org/2031113002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl
(right):

https://codereview.chromium.org/2031113002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl:17: any
value;
On 2016/06/07 at 00:21:42, haraken wrote:
> +bashi: Should this be 'any' or 'object'?

This needs to be 'any', as we're anything that's structure cloneable.

[palakj1, 2016-06-08 20:21:00]

Added worker tests, removed redundant header.
IDBObserverChanges.idl needs to be changed from dictionary to interface.Not done
yet.

[dmurph, 2016-06-08 20:22:53]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-08 20:23:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/220001

[commit-bot: I haz the power, 2016-06-08 20:27:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-08 20:27:36]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Marijn Kruisselbrink, 2016-06-08 20:44:41]

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js
(right):

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:1: if
(self.importScripts) {
It doesn't really make sense to have this file in the fetch tests directory. You
should just keep everything together in LayoutTests/storage/indexeddb. Probably
something like LayoutTests/storage/indexeddb/resources/observer-tests.js

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:2:
importScripts('../resources/fetch-test-helpers.js');
After moving the file you'll have to change this to be the correct path to
testharness.js rather than fetch-test-helpers.js

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:7:
test(function(t) {
This should still be an async_test, like the one you have in observer.html

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:12:
openRequest.onupgradeneeded= function() {
nit: space before =

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:19:
obs1.observe(db,tx);
nit: space after ,

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:20:
done();
this should be t.done()

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:25: //
done();
And you'll need to uncomment this (to signal "all the tests have been added to
the test framework")

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/fetch/workers/observer.html
(right):

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/workers/observer.html:1:
<!doctype html>
Here too move this file to something like
LayoutTests/storage/indexeddb/observer-worker.html

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/workers/observer.html:2: <head>
nit: You don't really need the head (or the body), just like the other
observer.html this entire file can be pretty much just:

<!DOCTYPE html>
<title>IndexedDB: Observer Tests in Workers</title>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script>
fetch_tests_from_worker(
  new Worker('resources/observer-tests.js'));
</script>

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/observer.html (right):

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:5: <script>
Rather than duplicating the test, you can just <script
src="resources/observer-tests.js"></script> (assuming that's where you ended up
putting your js file) and share the tests between worker and document contexts.
The whole reason for having the "if (self.importScripts)" in the js file is to
make it possible to also include it from a document.

[palakj1, 2016-06-09 18:13:03]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[palakj1, 2016-06-09 18:13:39]

Worker test updated. Please have a look.

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js
(right):

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:2:
importScripts('../resources/fetch-test-helpers.js');
On 2016/06/08 at 20:44:40, Marijn Kruisselbrink wrote:
> After moving the file you'll have to change this to be the correct path to
testharness.js rather than fetch-test-helpers.js

Done

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:7:
test(function(t) {
On 2016/06/08 at 20:44:40, Marijn Kruisselbrink wrote:
> This should still be an async_test, like the one you have in observer.html

done

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:12:
openRequest.onupgradeneeded= function() {
On 2016/06/08 at 20:44:41, Marijn Kruisselbrink wrote:
> nit: space before =

Done

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:19:
obs1.observe(db,tx);
On 2016/06/08 at 20:44:40, Marijn Kruisselbrink wrote:
> nit: space after ,

Done

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:20:
done();
On 2016/06/08 at 20:44:40, Marijn Kruisselbrink wrote:
> this should be t.done()

Done

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/observer.js:25: //
done();
On 2016/06/08 at 20:44:40, Marijn Kruisselbrink wrote:
> And you'll need to uncomment this (to signal "all the tests have been added to
the test framework")

Done

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/fetch/workers/observer.html
(right):

https://codereview.chromium.org/2031113002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/workers/observer.html:2: <head>
On 2016/06/08 at 20:44:41, Marijn Kruisselbrink wrote:
> nit: You don't really need the head (or the body), just like the other
observer.html this entire file can be pretty much just:
> 
> <!DOCTYPE html>
> <title>IndexedDB: Observer Tests in Workers</title>
> <script src="../../resources/testharness.js"></script>
> <script src="../../resources/testharnessreport.js"></script>
> <script>
> fetch_tests_from_worker(
>   new Worker('resources/observer-tests.js'));
> </script>

Done

[commit-bot: I haz the power, 2016-06-09 18:13:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/260001

[dmurph, 2016-06-09 18:18:51]

Other than changing the dictionaries to interfaces, this LGTM

[commit-bot: I haz the power, 2016-06-09 22:37:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-09 22:37:56]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-06-10 00:26:57]

LGTM

bashi@: Please check if the IDL changes make sense. Also see #34.

https://codereview.chromium.org/2031113002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserver.h (right):

https://codereview.chromium.org/2031113002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserver.h:30: explicit
IDBObserver(IDBObserverCallback&, const IDBObserverInit&);

Drop explicit.

[bashi, 2016-06-10 00:36:45]

Sorry I missed this.

https://codereview.chromium.org/2031113002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:16: any
records;
I'd prefer to define a new interface for this map but I'm fine with this if you
are going to change IDBObserverChanges to an interface.

[palakj1, 2016-06-11 00:35:12]

IDBObserverChanges, IDBObserverChangesRecord interface added. Please have a
look.

[bashi, 2016-06-13 01:35:51]

https://codereview.chromium.org/2031113002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:15:
[CallWith=ScriptState] readonly attribute any records;
Can you elaborate why you don't use maplike<DOMString,
sequence<IDBObserverChangesRecord>> ?

[dmurph, 2016-06-13 17:23:52]

https://codereview.chromium.org/2031113002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:15:
[CallWith=ScriptState] readonly attribute any records;
On 2016/06/13 at 01:35:51, bashi1 wrote:
> Can you elaborate why you don't use maplike<DOMString,
sequence<IDBObserverChangesRecord>> ?

Unless the bindings generator we have is different than the spec, you cannot
declare that as a variable type. It's only meant for describing the type that
we're inside. So palak can make a new (dictionary? interface?) type, something
like this:

interface IDBObserverChangesMap {
  maplike<DOMString, sequence<IDBObserverChangesRecord>>;
}

and then use that type as the 'records' attribute type. But the spec doesn't let
you use maplike as a type.

But maybe our system lets you do that? Palak, what happens when you put that
there as the type (not the whole new class, just what bashi suggested).

[palakj1, 2016-06-13 21:30:12]

https://codereview.chromium.org/2031113002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:15:
[CallWith=ScriptState] readonly attribute any records;
On 2016/06/13 at 17:23:52, dmurph wrote:
> On 2016/06/13 at 01:35:51, bashi1 wrote:
> > Can you elaborate why you don't use maplike<DOMString,
sequence<IDBObserverChangesRecord>> ?
> 
> Unless the bindings generator we have is different than the spec, you cannot
declare that as a variable type. It's only meant for describing the type that
we're inside. So palak can make a new (dictionary? interface?) type, something
like this:
> 
> interface IDBObserverChangesMap {
>   maplike<DOMString, sequence<IDBObserverChangesRecord>>;
> }
> 
> and then use that type as the 'records' attribute type. But the spec doesn't
let you use maplike as a type.
> 
> But maybe our system lets you do that? Palak, what happens when you put that
there as the type (not the whole new class, just what bashi suggested).

Indeed, as dmuprh pointed out, I cannot declare maplike<..> as a varialbe type.
I'd need to create a new interface. 

Since IDBObserverChanges won't ever be created by the user, we can ensure what
the type of 'records' is without making it binding in the idl. 
So, would it be fine if I let 'records' be as is for the time being and change
its type to maplike as a part of another patch.

[palakj1, 2016-06-13 22:29:48]

Added a todo.

[bashi, 2016-06-13 23:47:20]

On 2016/06/13 22:29:48, palakj1 wrote:
> Added a todo.

Thanks for explanations. bindings/ lgtm

[palakj1, 2016-06-14 01:56:21]

On 2016/06/13 at 23:47:20, bashi wrote:
> On 2016/06/13 22:29:48, palakj1 wrote:
> > Added a todo.
> 
> Thanks for explanations. bindings/ lgtm

Sorry, didn't get you. Could you expand upon 'bindings/' ?

[bashi, 2016-06-14 02:14:11]

On 2016/06/14 01:56:21, palakj1 wrote:
> On 2016/06/13 at 23:47:20, bashi wrote:
> > On 2016/06/13 22:29:48, palakj1 wrote:
> > > Added a todo.
> > 
> > Thanks for explanations. bindings/ lgtm
> 
> Sorry, didn't get you. Could you expand upon 'bindings/' ?

I meant that IDBObserverChanges looks good if you are planning to replace "any
records" to maplike<DOMString, sequence<IDBObserverChangesRecord>>. Sorry for
the confusion. (I use bindings/ l-g-t-m for IDL changes)

[blink-reviews, 2016-06-14 02:32:52]

He means that he gives his approval for files in the "bindings/" directory
:)

On Mon, Jun 13, 2016, 6:56 PM <palakj@google.com> wrote:

> On 2016/06/13 at 23:47:20, bashi wrote:
> > On 2016/06/13 22:29:48, palakj1 wrote:
> > > Added a todo.
> >
> > Thanks for explanations. bindings/ lgtm
>
> Sorry, didn't get you. Could you expand upon 'bindings/' ?
>
> https://codereview.chromium.org/2031113002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-06-14 02:32:53]

He means that he gives his approval for files in the "bindings/" directory
:)

On Mon, Jun 13, 2016, 6:56 PM <palakj@google.com> wrote:

> On 2016/06/13 at 23:47:20, bashi wrote:
> > On 2016/06/13 22:29:48, palakj1 wrote:
> > > Added a todo.
> >
> > Thanks for explanations. bindings/ lgtm
>
> Sorry, didn't get you. Could you expand upon 'bindings/' ?
>
> https://codereview.chromium.org/2031113002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[palakj1, 2016-06-14 02:44:21]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-14 02:44:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/340001

[commit-bot: I haz the power, 2016-06-14 03:02:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-14 03:02:02]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[palakj1, 2016-06-14 03:08:59]

The CQ bit was unchecked by palakj@google.com

[palakj1, 2016-06-14 03:09:28]

https://codereview.chromium.org/2031113002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp
(right):

https://codereview.chromium.org/2031113002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp:19:
V8PrivateProperty::getIntersectionObserverCallback(scriptState->isolate()).set(scriptState->context(),
owner, callback);
getIDBObserver

[palakj1, 2016-06-14 18:43:30]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-14 18:43:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/360001

[commit-bot: I haz the power, 2016-06-14 19:03:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-14 19:03:05]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[palakj1, 2016-06-14 19:46:45]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-14 19:47:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/380001

[palakj1, 2016-06-14 19:56:16]

The CQ bit was unchecked by palakj@google.com

[palakj1, 2016-06-14 20:15:17]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-14 20:15:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/400001

[commit-bot: I haz the power, 2016-06-14 20:27:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-14 20:27:37]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[palakj1, 2016-06-15 00:42:53]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 00:43:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/460001

[commit-bot: I haz the power, 2016-06-15 00:58:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 00:58:54]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Marijn Kruisselbrink, 2016-06-15 13:02:45]

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h:17: class
IDBDatabase;
no need to forward declare Database and Transaction if you're already including
their header files above

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h:25: static
IDBObserverChanges* create(IDBDatabase*, IDBTransaction*, IDBAny*);
IDBAny doesn't make it clear what the argument is for, so add an argument name
for it

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h:36:
IDBObserverChanges(IDBDatabase*, IDBTransaction*, IDBAny*);
here also give the IDBAny parameter a name

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:9: ]
interface IDBObserverChanges {
this should probably have Exposed=(Window, Worker) like IDBObserver
(and then you'll need to also update the expectations again)

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.cpp
(right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.cpp:62:
case WebIDBDelete:
is this missing a return IndexedDBNames::del?

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.h
(right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.h:25:
static IDBObserverChangesRecord* create(IDBKey*, IDBValue*,
WebIDBOperationType);
use PassRefPtr<IDBValue> instead of raw pointers. (raw pointers are okay for
garbage collected types but not for refcounted types)

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl
(right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl:13: ]
this also should have Exposed=(Window, Worker)

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBValue.h (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBValue.h:27: virtual ~IDBValue();
Why this change?

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h:79: };
nit: blank line between this enum and the next

[dmurph, 2016-06-15 13:10:49]

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:12: #include
<v8.h>
Don't include v8

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:9: ]
interface IDBObserverChanges {
On 2016/06/15 at 13:02:44, Marijn Kruisselbrink wrote:
> this should probably have Exposed=(Window, Worker) like IDBObserver
> (and then you'll need to also update the expectations again)

Does that make the object 'constructable' there? We don't want anyone
instantiating this object, we'll be passing it back to the user. But I don't
know how the Exposed property works.

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl
(right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl:8:
"add", "put", "del", "clear"
change back to "delete"

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBValue.h (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBValue.h:27: virtual ~IDBValue();
On 2016/06/15 at 13:02:45, Marijn Kruisselbrink wrote:
> Why this change?

It was causing problems where BlobDataHandle and IDBKey needed to be included in
header files because the destructor here needs the destructor defined if we
don't virtualize it. So in our observer class it was complaining because
BlobDataHandle wasn't included for that compilation unit, even though we never
use blob data handles.

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IndexedDBNames.in (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IndexedDBNames.in:27: del
"delete ImplementedAs=kDelete" (from the emails :)

[Marijn Kruisselbrink, 2016-06-15 13:22:37]

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:9: ]
interface IDBObserverChanges {
On 2016/06/15 at 13:10:48, dmurph wrote:
> On 2016/06/15 at 13:02:44, Marijn Kruisselbrink wrote:
> > this should probably have Exposed=(Window, Worker) like IDBObserver
> > (and then you'll need to also update the expectations again)
> 
> Does that make the object 'constructable' there? We don't want anyone
instantiating this object, we'll be passing it back to the user. But I don't
know how the Exposed property works.

Exposed just means that it is visible and you can test for its type. It doesn't
make it constructable. For that you'd need to also specify a constructor. Not
sure why you wouldn't want these to be constructable though?

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBValue.h (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBValue.h:27: virtual ~IDBValue();
On 2016/06/15 at 13:10:48, dmurph wrote:
> On 2016/06/15 at 13:02:45, Marijn Kruisselbrink wrote:
> > Why this change?
> 
> It was causing problems where BlobDataHandle and IDBKey needed to be included
in header files because the destructor here needs the destructor defined if we
don't virtualize it. So in our observer class it was complaining because
BlobDataHandle wasn't included for that compilation unit, even though we never
use blob data handles.

Sorry, I can't quite parse that sentence... What was complaining about what in
which situation?

[palakj1, 2016-06-15 19:16:31]

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:12: #include
<v8.h>
On 2016/06/15 at 13:10:48, dmurph wrote:
> Don't include v8

done

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h:17: class
IDBDatabase;
On 2016/06/15 at 13:02:44, Marijn Kruisselbrink wrote:
> no need to forward declare Database and Transaction if you're already
including their header files above

done

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h:25: static
IDBObserverChanges* create(IDBDatabase*, IDBTransaction*, IDBAny*);
On 2016/06/15 at 13:02:44, Marijn Kruisselbrink wrote:
> IDBAny doesn't make it clear what the argument is for, so add an argument name
for it

done

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.h:36:
IDBObserverChanges(IDBDatabase*, IDBTransaction*, IDBAny*);
On 2016/06/15 at 13:02:44, Marijn Kruisselbrink wrote:
> here also give the IDBAny parameter a name

done

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:9: ]
interface IDBObserverChanges {
On 2016/06/15 at 13:22:37, Marijn Kruisselbrink wrote:
> On 2016/06/15 at 13:10:48, dmurph wrote:
> > On 2016/06/15 at 13:02:44, Marijn Kruisselbrink wrote:
> > > this should probably have Exposed=(Window, Worker) like IDBObserver
> > > (and then you'll need to also update the expectations again)
> > 
> > Does that make the object 'constructable' there? We don't want anyone
instantiating this object, we'll be passing it back to the user. But I don't
know how the Exposed property works.
> 
> Exposed just means that it is visible and you can test for its type. It
doesn't make it constructable. For that you'd need to also specify a
constructor. Not sure why you wouldn't want these to be constructable though?

IDBObserverChanges just needs to be passed back with the callback. The user
shouldn't be allowed to construct the object. Should we still make it
'constructable' ?

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.cpp
(right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.cpp:62:
case WebIDBDelete:
On 2016/06/15 at 13:02:44, Marijn Kruisselbrink wrote:
> is this missing a return IndexedDBNames::del?

oops! corrected.

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.h
(right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.h:25:
static IDBObserverChangesRecord* create(IDBKey*, IDBValue*,
WebIDBOperationType);
On 2016/06/15 at 13:02:44, Marijn Kruisselbrink wrote:
> use PassRefPtr<IDBValue> instead of raw pointers. (raw pointers are okay for
garbage collected types but not for refcounted types)

Made the change. But IDBCursor also seems to use RefPtr
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/indexe...)
Is it a different use case?

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl
(right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl:8:
"add", "put", "del", "clear"
On 2016/06/15 at 13:10:48, dmurph wrote:
> change back to "delete"

Done

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.idl:13: ]
On 2016/06/15 at 13:02:45, Marijn Kruisselbrink wrote:
> this also should have Exposed=(Window, Worker)

Done.

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBValue.h (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBValue.h:27: virtual ~IDBValue();
On 2016/06/15 at 13:22:37, Marijn Kruisselbrink wrote:
> On 2016/06/15 at 13:10:48, dmurph wrote:
> > On 2016/06/15 at 13:02:45, Marijn Kruisselbrink wrote:
> > > Why this change?
> > 
> > It was causing problems where BlobDataHandle and IDBKey needed to be
included in header files because the destructor here needs the destructor
defined if we don't virtualize it. So in our observer class it was complaining
because BlobDataHandle wasn't included for that compilation unit, even though we
never use blob data handles.
> 
> Sorry, I can't quite parse that sentence... What was complaining about what in
which situation?

Our destructor calls the IDBValue destructor. Since, IDBValue destructor isn't
virtualized, it looked for BlobDataHandle (IDBValue holds a pointer to
BlobDataHandle) which isn't included in our compilation unit. This throws an
'access into incomplete type' error.

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IndexedDBNames.in (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IndexedDBNames.in:27: del
On 2016/06/15 at 13:10:48, dmurph wrote:
> "delete ImplementedAs=kDelete" (from the emails :)

forgot about that. Done

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h:79: };
On 2016/06/15 at 13:02:45, Marijn Kruisselbrink wrote:
> nit: blank line between this enum and the next

Done

[dmurph, 2016-06-15 19:35:15]

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.idl:9: ]
interface IDBObserverChanges {
On 2016/06/15 at 19:16:30, palakj1 wrote:
> On 2016/06/15 at 13:22:37, Marijn Kruisselbrink wrote:
> > On 2016/06/15 at 13:10:48, dmurph wrote:
> > > On 2016/06/15 at 13:02:44, Marijn Kruisselbrink wrote:
> > > > this should probably have Exposed=(Window, Worker) like IDBObserver
> > > > (and then you'll need to also update the expectations again)
> > > 
> > > Does that make the object 'constructable' there? We don't want anyone
instantiating this object, we'll be passing it back to the user. But I don't
know how the Exposed property works.
> > 
> > Exposed just means that it is visible and you can test for its type. It
doesn't make it constructable. For that you'd need to also specify a
constructor. Not sure why you wouldn't want these to be constructable though?
> 
> IDBObserverChanges just needs to be passed back with the callback. The user
shouldn't be allowed to construct the object. Should we still make it
'constructable' ?

I don't know what's normal here, I'll defer to Marijn. It looks like there's a
default constructor, so I think all we need to do is mark it as exposed (which
is what the new patch has).

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h (right):

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h:8: #include
"platform/heap/Handle.h"
I guess Oilpan uses this to include everything? Seems weird, but I see it
everywhere.

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IndexedDBNames.in (right):

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IndexedDBNames.in:27: kDelete
Huh, I thought you could put the ImplementedAs part here, and not in the idl. If
that works, I'd prefer it that way.

[palakj1, 2016-06-15 21:24:49]

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h (right):

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h:8: #include
"platform/heap/Handle.h"
On 2016/06/15 at 19:35:15, dmurph wrote:
> I guess Oilpan uses this to include everything? Seems weird, but I see it
everywhere.

I can remove this header from this and all other files. Since, it's anyway
included through IDBDatabase (transitive dependecies). 
Would you like that better? Or should I still include particular headers used
for tracing instead of Handle.h?

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IndexedDBNames.in (right):

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IndexedDBNames.in:27: kDelete
On 2016/06/15 at 19:35:15, dmurph wrote:
> Huh, I thought you could put the ImplementedAs part here, and not in the idl.
If that works, I'd prefer it that way.

I see what you mean. Changed.

[dmurph, 2016-06-15 21:28:35]

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h (right):

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h:8: #include
"platform/heap/Handle.h"
On 2016/06/15 at 21:24:49, palakj1 wrote:
> On 2016/06/15 at 19:35:15, dmurph wrote:
> > I guess Oilpan uses this to include everything? Seems weird, but I see it
everywhere.
> 
> I can remove this header from this and all other files. Since, it's anyway
included through IDBDatabase (transitive dependecies). 
> Would you like that better? Or should I still include particular headers used
for tracing instead of Handle.h?

No, it looks like other users of Oilpan (who use GarbageCollectedFinalized, etc)
in modules also have this 'batch' include to include the oilpan macros & basic
stuff. This is fine.

Please don't rely on transitive dependencies, unless they come from the header
file for your class :)

[haraken, 2016-06-16 00:22:29]

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h (right):

https://codereview.chromium.org/2031113002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverCallback.h:8: #include
"platform/heap/Handle.h"
On 2016/06/15 21:28:35, dmurph wrote:
> On 2016/06/15 at 21:24:49, palakj1 wrote:
> > On 2016/06/15 at 19:35:15, dmurph wrote:
> > > I guess Oilpan uses this to include everything? Seems weird, but I see it
> everywhere.
> > 
> > I can remove this header from this and all other files. Since, it's anyway
> included through IDBDatabase (transitive dependecies). 
> > Would you like that better? Or should I still include particular headers
used
> for tracing instead of Handle.h?
> 
> No, it looks like other users of Oilpan (who use GarbageCollectedFinalized,
etc)
> in modules also have this 'batch' include to include the oilpan macros & basic
> stuff. This is fine.
> 
> Please don't rely on transitive dependencies, unless they come from the header
> file for your class :)

We normally just include Handle.h to use Oilpan stuff, just like we normally
include v8.h to use V8 stuff. This is not recommended but is a commonly used
pattern.

[dmurph, 2016-06-16 06:57:10]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 06:57:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/500001

[dmurph, 2016-06-16 06:59:57]

This still lgtm. Do you need any more approvals?

[Marijn Kruisselbrink, 2016-06-16 07:09:30]

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBValue.h (right):

https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBValue.h:27: virtual ~IDBValue();
On 2016/06/15 at 19:16:31, palakj1 wrote:
> On 2016/06/15 at 13:22:37, Marijn Kruisselbrink wrote:
> > On 2016/06/15 at 13:10:48, dmurph wrote:
> > > On 2016/06/15 at 13:02:45, Marijn Kruisselbrink wrote:
> > > > Why this change?
> > > 
> > > It was causing problems where BlobDataHandle and IDBKey needed to be
included in header files because the destructor here needs the destructor
defined if we don't virtualize it. So in our observer class it was complaining
because BlobDataHandle wasn't included for that compilation unit, even though we
never use blob data handles.
> > 
> > Sorry, I can't quite parse that sentence... What was complaining about what
in which situation?
> 
> Our destructor calls the IDBValue destructor. Since, IDBValue destructor isn't
virtualized, it looked for BlobDataHandle (IDBValue holds a pointer to
BlobDataHandle) which isn't included in our compilation unit. This throws an
'access into incomplete type' error.

That explains why you need an explicit destructor, with an implementation in the
.cpp file. It does not explain why suddenly that destructor needs to be virtual?

https://codereview.chromium.org/2031113002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.h
(right):

https://codereview.chromium.org/2031113002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.h:38:
PassRefPtr<IDBValue> m_value;
No, this isn't what I mean. You should of course store m_value as a RefPtr. It's
the raw IDBValue* pointers in the constructor and create method that should be
PassRefPtr<IDBValue> instead.

[commit-bot: I haz the power, 2016-06-16 08:51:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-16 08:51:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[palakj1, 2016-06-16 17:01:51]

On 2016/06/16 at 07:09:30, mek wrote:
>
https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/indexeddb/IDBValue.h (right):
> 
>
https://codereview.chromium.org/2031113002/diff/460001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/indexeddb/IDBValue.h:27: virtual
~IDBValue();
> On 2016/06/15 at 19:16:31, palakj1 wrote:
> > On 2016/06/15 at 13:22:37, Marijn Kruisselbrink wrote:
> > > On 2016/06/15 at 13:10:48, dmurph wrote:
> > > > On 2016/06/15 at 13:02:45, Marijn Kruisselbrink wrote:
> > > > > Why this change?
> > > > 
> > > > It was causing problems where BlobDataHandle and IDBKey needed to be
included in header files because the destructor here needs the destructor
defined if we don't virtualize it. So in our observer class it was complaining
because BlobDataHandle wasn't included for that compilation unit, even though we
never use blob data handles.
> > > 
> > > Sorry, I can't quite parse that sentence... What was complaining about
what in which situation?
> > 
> > Our destructor calls the IDBValue destructor. Since, IDBValue destructor
isn't virtualized, it looked for BlobDataHandle (IDBValue holds a pointer to
BlobDataHandle) which isn't included in our compilation unit. This throws an
'access into incomplete type' error.
> 
> That explains why you need an explicit destructor, with an implementation in
the .cpp file. It does not explain why suddenly that destructor needs to be
virtual?

Destructors no longer virtual. 

>
https://codereview.chromium.org/2031113002/diff/500001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.h
(right):
> 
>
https://codereview.chromium.org/2031113002/diff/500001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.h:38:
PassRefPtr<IDBValue> m_value;
> No, this isn't what I mean. You should of course store m_value as a RefPtr.
It's the raw IDBValue* pointers in the constructor and create method that should
be PassRefPtr<IDBValue> instead.

[palakj1, 2016-06-16 17:05:24]

https://codereview.chromium.org/2031113002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.h
(right):

https://codereview.chromium.org/2031113002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChangesRecord.h:38:
PassRefPtr<IDBValue> m_value;
On 2016/06/16 at 07:09:30, Marijn Kruisselbrink wrote:
> No, this isn't what I mean. You should of course store m_value as a RefPtr.
It's the raw IDBValue* pointers in the constructor and create method that should
be PassRefPtr<IDBValue> instead.

Oh okay. Changed

[palakj1, 2016-06-16 17:07:34]

On 2016/06/16 at 06:59:57, dmurph wrote:
> This still lgtm. Do you need any more approvals?

I got one from you bashi and haraken. Probably, I need it from mek.

[Marijn Kruisselbrink, 2016-06-17 07:26:20]

lgtm (but it doesn't look like you updated the test expectations after adding
the Exposed=(Window,Worker) to a few more interfaces).

[dmurph, 2016-06-17 08:47:10]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-17 08:47:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/520001

[commit-bot: I haz the power, 2016-06-17 10:15:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-17 10:16:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[palakj1, 2016-06-17 17:26:56]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-17 17:27:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/540001

[commit-bot: I haz the power, 2016-06-17 18:57:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-17 18:57:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[palakj1, 2016-06-18 02:35:07]

The CQ bit was checked by palakj@google.com

[palakj1, 2016-06-18 02:35:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, bashi@chromium.org, dmurph@chromium.org, mek@chromium.org
Link to the patchset: https://codereview.chromium.org/2031113002/#ps560001
(title: "expected files updated")

[commit-bot: I haz the power, 2016-06-18 02:35:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/560001

[commit-bot: I haz the power, 2016-06-18 02:42:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-18 02:42:38]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jsbell, 2016-06-20 17:40:46]

third_party/WebKit/public/platform/modules/indexeddb lgtm

[palakj1, 2016-06-20 19:54:32]

The CQ bit was checked by palakj@google.com

[commit-bot: I haz the power, 2016-06-20 19:55:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031113002/560001

[commit-bot: I haz the power, 2016-06-20 21:45:44]

Committed patchset #29 (id:560001)

[commit-bot: I haz the power, 2016-06-20 21:51:04]

Description was changed from

==========
IndexedDB Observers with webidl files, initial constructor for creating observer
and observe function.

BUG= 609934
Reference: 
https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md
==========

to

==========
IndexedDB Observers with webidl files, initial constructor for creating observer
and observe function.

BUG= 609934
Reference: 
https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md

Committed: https://crrev.com/dd97a20cea3444559290d55e8fae06bd580cd5ba
Cr-Commit-Position: refs/heads/master@{#400796}
==========

[commit-bot: I haz the power, 2016-06-20 21:51:07]

Patchset 29 (id:??) landed as
https://crrev.com/dd97a20cea3444559290d55e8fae06bd580cd5ba
Cr-Commit-Position: refs/heads/master@{#400796}

[palakj1, 2016-07-21 18:43:06]

Description was changed from

==========
IndexedDB Observers with webidl files, initial constructor for creating observer
and observe function.

BUG= 609934
Reference: 
https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md

Committed: https://crrev.com/dd97a20cea3444559290d55e8fae06bd580cd5ba
Cr-Commit-Position: refs/heads/master@{#400796}
==========

to

==========
IndexedDB Observers with webidl files, initial constructor for creating observer
and observe function.

BUG= 609934
Reference: 
https://github.com/dmurph/indexed-db-observers/blob/gh-pages/EXPLAINER.md

Current Implementation and Future Scope: https://goo.gl/Y2dobn

Committed: https://crrev.com/dd97a20cea3444559290d55e8fae06bd580cd5ba
Cr-Commit-Position: refs/heads/master@{#400796}
==========