[IndexedDB] [DevTools] Right-click 'Refresh' on database now updates object store view, also fixed …

Right-click 'Refresh' on database now updates object store view, also fixed IndexedDB view crash.

The crash was being caused by a null pointer exception in the view constructor since _database in Resources.IDBDatabaseTreeElement was not loaded/stored before the view was constructed. Adding a null check before constructing the view stops the exception.

BUG=727908, 728450
Review-Url: https://codereview.chromium.org/2914293002
Cr-Commit-Position: refs/heads/master@{#478073}
Committed: https://chromium.googlesource.com/chromium/src/+/525c53f0d081a4533b6c55d9b2b991ecfc9d2bec

[kristipark, 2017-06-01 21:29:15]

Description was changed from

==========
[IndexedDB] [DevTools] Right-click 'Refresh' on database now updates object
store view, also fixed IndexedDB view crash

BUG=727908, 728450
==========

to

==========
Right-click 'Refresh' on database now updates object store view, also fixed
IndexedDB view crash.

The crash was being caused by a null pointer exception in the view constructor
since _database in Resources.IDBDatabaseTreeElement was not loaded/stored before
the view was constructed. Adding a null check before constructing the view stops
the exception.

BUG=727908, 728450
==========

[kristipark, 2017-06-01 21:29:16]

kristipark@chromium.org changed reviewers:
+ dgozman@chromium.org, dmurph@chromium.org

[dmurph, 2017-06-01 22:50:31]

https://codereview.chromium.org/2914293002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html
(right):

https://codereview.chromium.org/2914293002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:186:
await InspectorTest.evaluateInPageAsync("addIDBValue('" + databaseName + "', '"
+ objectStoreName2 + "', 'testKey2', 'testValue2')");
Can you use this:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...

or create a different utility wrapper that uses promises?

https://codereview.chromium.org/2914293002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:195:
objectStoreTreeElement.onselect(false);
Can you comment what this does? This selects the tree element, and opens that
view?

[kristipark, 2017-06-02 00:16:56]

Moved null handling into IDBDatabaseView, otherwise user would be presented with
a blank view until they navigate away.

https://codereview.chromium.org/2914293002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html
(right):

https://codereview.chromium.org/2914293002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:186:
await InspectorTest.evaluateInPageAsync("addIDBValue('" + databaseName + "', '"
+ objectStoreName2 + "', 'testKey2', 'testValue2')");
On 2017/06/01 22:50:31, dmurph wrote:
> Can you use this:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
> 
> or create a different utility wrapper that uses promises?

Since those function uses callbacks, I made the above promise based utility
functions instead (using InspectorTest.evaluateInPageAsync). Moved those
functions to indexeddb-test.js.

https://codereview.chromium.org/2914293002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:195:
objectStoreTreeElement.onselect(false);
On 2017/06/01 22:50:31, dmurph wrote:
> Can you comment what this does? This selects the tree element, and opens that
> view?

Ah, you're right, it doesn't do anything. Removed!

[dmurph, 2017-06-02 19:22:06]

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html
(right):

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:26:
function waitRefreshDatabaseRightClick(callback) {
You use a lot of these callback in promises. Can you just have them return
promises? (unless you specifically need the callback version).

Also, please use variables instead of declaring as function. So:


var waitRefreshDatabaseRightClick = function() {
  return new Promise((resolve, reject) => {
      InspectorTest.addSniffer(Resources.IDBDataView.prototype,
"_updatedDataForTests", resolve, false);
      idbDatabaseTreeElement._refreshIndexedDB();
    });
}

You don't technically need the reject callback there for this example.

Then, instead of always creating a new promise to wrap this callback, you can
just say:

await waitRefreshDatabaseRightClick();

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/indexeddb-test.js
(right):

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/indexeddb-test.js:318:
var promise = new Promise((fulfill) => callback = fulfill);
Can you make this in the form:

return new Promise((resolve, reject) => {
  var request = indexedDB.open(databaseName);
  ....


});

The constructor will execute right away, and the resolve, reject callbacks will
trigger resolution of the promise.  If you don't want to use the reject callback
and instead use the onIndexedDBError, that's fine.

Example from a web platform test:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...

[dgozman, 2017-06-02 21:48:42]

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/indexeddb-test.js
(right):

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/indexeddb-test.js:318:
var promise = new Promise((fulfill) => callback = fulfill);
On 2017/06/02 19:22:05, dmurph wrote:
> Can you make this in the form:
> 
> return new Promise((resolve, reject) => {
>   var request = indexedDB.open(databaseName);
>   ....
> 
> 
> });
> 
> The constructor will execute right away, and the resolve, reject callbacks
will
> trigger resolution of the promise.  If you don't want to use the reject
callback
> and instead use the onIndexedDBError, that's fine.
> 
> Example from a web platform test:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...

This style (as in the patch) is what we usually do in DevTools. It makes code
more readable once you know the pattern and avoids unnecessary indentation.

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js
(right):

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:1089:
indexedDBModel.refreshDatabaseNames();  // Adds IDBDatabaseTreeElements.
I think we should not rely on this face. After all, we are calling into model,
which does not guarantee to add any elements.

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js
(right):

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js:44: var
databaseName = 'Loading...';  // If database was not loaded yet when view was
created
We wrap any user-readable strings with Common.UIString call:
Common.UIString('Loading\u2026')

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js:81: if
(database) {
The database is marked as non-null in line 78. How come it could be null?

[kristipark, 2017-06-03 01:42:44]

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html
(right):

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:26:
function waitRefreshDatabaseRightClick(callback) {
On 2017/06/02 19:22:05, dmurph wrote:
> You use a lot of these callback in promises. Can you just have them return
> promises? (unless you specifically need the callback version).
> 
> Also, please use variables instead of declaring as function. So:
> 
> 
> var waitRefreshDatabaseRightClick = function() {
>   return new Promise((resolve, reject) => {
>       InspectorTest.addSniffer(Resources.IDBDataView.prototype,
> "_updatedDataForTests", resolve, false);
>       idbDatabaseTreeElement._refreshIndexedDB();
>     });
> }
> 
> You don't technically need the reject callback there for this example.
> 
> Then, instead of always creating a new promise to wrap this callback, you can
> just say:
> 
> await waitRefreshDatabaseRightClick();

Good point! Changed all but the event listeners to promises (need to keep the
callback to remove the listeners after resolving)

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js
(right):

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:1089:
indexedDBModel.refreshDatabaseNames();  // Adds IDBDatabaseTreeElements.
On 2017/06/02 21:48:41, dgozman wrote:
> I think we should not rely on this face. After all, we are calling into model,
> which does not guarantee to add any elements.

Unfortunately unless refreshDatabaseNames is called, the event listener
IndexedDBModel.Events.DatabaseAdded won't be triggered. That seems to be the
only way in the model to see if databases were added or not.

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js
(right):

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js:44: var
databaseName = 'Loading...';  // If database was not loaded yet when view was
created
On 2017/06/02 21:48:41, dgozman wrote:
> We wrap any user-readable strings with Common.UIString call:
> Common.UIString('Loading\u2026')

Done.

https://codereview.chromium.org/2914293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js:81: if
(database) {
On 2017/06/02 21:48:41, dgozman wrote:
> The database is marked as non-null in line 78. How come it could be null?

The constructor calls update with a null database (line 65), so I added the null
check here. Went ahead and moved it to the constructor.

[dmurph, 2017-06-05 21:47:39]

non-owners lgtm :)

[dgozman, 2017-06-06 21:32:26]

https://codereview.chromium.org/2914293002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js
(right):

https://codereview.chromium.org/2914293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:1091:
this._idbDatabaseTreeElements[i]._refreshIndexedDB();
Instead of calling this directly, tree elements should listen to some event
which is dispatched from model once database names are updated.

https://codereview.chromium.org/2914293002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js
(right):

https://codereview.chromium.org/2914293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js:44: var
databaseName = Common.UIString('Loading\u2026');  // If database was not loaded
yet when view was created
- we usually avoid comments like this which repeat the code next to comment;
- var databaseName = database ? database.databaseId.name : Common.UIString(...)

[kristipark, 2017-06-06 22:45:48]

Thanks for the review!

https://codereview.chromium.org/2914293002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js
(right):

https://codereview.chromium.org/2914293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:1091:
this._idbDatabaseTreeElements[i]._refreshIndexedDB();
On 2017/06/06 21:32:26, dgozman wrote:
> Instead of calling this directly, tree elements should listen to some event
> which is dispatched from model once database names are updated.

Done.

https://codereview.chromium.org/2914293002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js
(right):

https://codereview.chromium.org/2914293002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js:44: var
databaseName = Common.UIString('Loading\u2026');  // If database was not loaded
yet when view was created
On 2017/06/06 21:32:26, dgozman wrote:
> - we usually avoid comments like this which repeat the code next to comment;
> - var databaseName = database ? database.databaseId.name :
Common.UIString(...)

Done.

[dgozman, 2017-06-07 21:22:05]

https://codereview.chromium.org/2914293002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js
(right):

https://codereview.chromium.org/2914293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:1089:
indexedDBModel.refreshDatabaseNames();  // Adds IDBDatabaseTreeElements.
Remove the comment.

https://codereview.chromium.org/2914293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:1179:
SDK.targetManager.addModelListener(
model.addEventListener(...)

https://codereview.chromium.org/2914293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js
(right):

https://codereview.chromium.org/2914293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:184:
this.dispatchEventToListeners(Resources.IndexedDBModel.Events.DatabaseNamesRefreshed);
This should be issued after all _loadDatabaseNames finish their async work.

[kristipark, 2017-06-08 00:22:53]

Patchset #5 (id:80001) has been deleted

[kristipark, 2017-06-08 00:32:21]

Patchset #5 (id:100001) has been deleted

[kristipark, 2017-06-08 00:43:33]

Patchset #5 (id:120001) has been deleted

[kristipark, 2017-06-08 01:04:10]

Rebased for another CL. Letting you know that it looks like the model has been
changed to use promises.

https://codereview.chromium.org/2914293002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js
(right):

https://codereview.chromium.org/2914293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:1089:
indexedDBModel.refreshDatabaseNames();  // Adds IDBDatabaseTreeElements.
On 2017/06/07 21:22:05, dgozman wrote:
> Remove the comment.

Done.

https://codereview.chromium.org/2914293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebar.js:1179:
SDK.targetManager.addModelListener(
On 2017/06/07 21:22:05, dgozman wrote:
> model.addEventListener(...)

Done.

https://codereview.chromium.org/2914293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js
(right):

https://codereview.chromium.org/2914293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:184:
this.dispatchEventToListeners(Resources.IndexedDBModel.Events.DatabaseNamesRefreshed);
On 2017/06/07 21:22:05, dgozman wrote:
> This should be issued after all _loadDatabaseNames finish their async work.

Now calling _loadDatabaseNames in parallel with Promise.all(). Let me know if
this is a good approach!

[kristipark, 2017-06-08 01:24:21]

Actually, the promises I used were incorrect, switched to awaits instead. Sorry
about that!

[dgozman, 2017-06-08 18:15:22]

lgtm, thanks!

[kristipark, 2017-06-08 18:19:39]

The CQ bit was checked by kristipark@chromium.org

[kristipark, 2017-06-08 18:19:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org
Link to the patchset: https://codereview.chromium.org/2914293002/#ps160001
(title: "Using await instead")

[commit-bot: I haz the power, 2017-06-08 18:20:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 20:37:10]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496945979888810,
"parent_rev": "6e088010a03470db917e004be9e64cc04c69e9a0", "commit_rev":
"525c53f0d081a4533b6c55d9b2b991ecfc9d2bec"}

[commit-bot: I haz the power, 2017-06-08 20:37:24]

Description was changed from

==========
Right-click 'Refresh' on database now updates object store view, also fixed
IndexedDB view crash.

The crash was being caused by a null pointer exception in the view constructor
since _database in Resources.IDBDatabaseTreeElement was not loaded/stored before
the view was constructed. Adding a null check before constructing the view stops
the exception.

BUG=727908, 728450
==========

to

==========
Right-click 'Refresh' on database now updates object store view, also fixed
IndexedDB view crash.

The crash was being caused by a null pointer exception in the view constructor
since _database in Resources.IDBDatabaseTreeElement was not loaded/stored before
the view was constructed. Adding a null check before constructing the view stops
the exception.

BUG=727908, 728450

Review-Url: https://codereview.chromium.org/2914293002
Cr-Commit-Position: refs/heads/master@{#478073}
Committed:
https://chromium.googlesource.com/chromium/src/+/525c53f0d081a4533b6c55d9b2b9...
==========

[commit-bot: I haz the power, 2017-06-08 20:37:25]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/525c53f0d081a4533b6c55d9b2b9...