Bug 167473 - Uncaught Exception: Can't make a ContentView for an unknown representedObject of type: IndexedDatabase
Summary: Uncaught Exception: Can't make a ContentView for an unknown representedObject...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-26 16:32 PST by John Wilander
Modified: 2017-05-03 15:13 PDT (History)
8 users (show)

See Also:


Attachments
[Animated GIF] Missing content view (71.97 KB, image/gif)
2017-04-28 16:53 PDT, Nikita Vasilyev
no flags Details
[Image] Chrome DevTools IndexedDB content view (63.48 KB, image/png)
2017-04-28 17:03 PDT, Nikita Vasilyev
no flags Details
Patch (12.04 KB, patch)
2017-04-28 18:43 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Image] With patch applied (42.77 KB, image/png)
2017-04-28 18:44 PDT, Nikita Vasilyev
no flags Details
[Image] Details sidebar (106.96 KB, image/png)
2017-04-28 20:11 PDT, Nikita Vasilyev
no flags Details
Patch (13.43 KB, patch)
2017-05-01 16:51 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Image] Other UI ideas (152.74 KB, image/png)
2017-05-01 18:44 PDT, Matt Baker
no flags Details
Patch (14.65 KB, patch)
2017-05-02 17:07 PDT, Nikita Vasilyev
mattbaker: review+
Details | Formatted Diff | Diff
[Image] With patch applied (89.47 KB, image/png)
2017-05-02 17:08 PDT, Nikita Vasilyev
no flags Details
Patch (14.33 KB, patch)
2017-05-03 14:31 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2017-01-26 16:32:13 PST
-------
Inspected URL:        Local page
Loading completed:    true
Frontend User Agent:  Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/604.1.5+ (KHTML, like Gecko)

Uncaught Exceptions:
 - Can't make a ContentView for an unknown representedObject of type: IndexedDatabase (at ContentView.js:152:24)
    createFromRepresentedObject @ ContentView.js:152:24
    contentViewForRepresentedObject @ ContentView.js:173:82
    showContentViewForRepresentedObject @ ContentBrowser.js:134:63
    showDefaultContentViewForTreeElement @ NavigationSidebarPanel.js:223:82
    _checkElementsForPendingViewStateCookie @ NavigationSidebarPanel.js:781:79
    _treeElementAddedOrChanged @ NavigationSidebarPanel.js:638:57
    dispatch @ Object.js:170:30
    dispatchEventToListeners @ Object.js:177:17
    appendChild @ TreeOutline.js:177:54
    _addIndexedDatabase @ StorageSidebarPanel.js:261:47
    _indexedDatabaseWasAdded @ StorageSidebarPanel.js:246:33
    dispatch @ Object.js:170:30
    dispatchEventToListeners @ Object.js:177:17
    processDatabase @ StorageManager.js:275:42
    processDatabase @ [native code]
    _dispatchResponseToCallback @ Connection.js:146:27
    _dispatchResponse @ Connection.js:116:45
    dispatch @ Connection.js:69:35
    dispatch @ InspectorBackend.js:151:49
    dispatchNextQueuedMessageFromBackend @ MessageDispatcher.js:42:34

Additional Details:
    cause --> An uncaught exception was thrown while dispatching response callback for command IndexedDB.requestDatabase.
-------

* STEPS TO REPRODUCE
1. The data records for the domain had been cleared, including IndexedDB.
2. The IndexedDB instance still showed in Web Inspector | Storage. I clicked it and got the crash.
Comment 1 Brian Burg 2017-01-28 14:13:09 PST
Thanks John, we are probably just missing a case for this.
Comment 2 Radar WebKit Bug Importer 2017-01-28 14:13:22 PST
<rdar://problem/30249715>
Comment 3 Nikita Vasilyev 2017-04-28 16:45:22 PDT
Steps:
1. Open http://www.girliemac.com/stickies/
2. Click on "New note...", type "foo", press Enter
3. In Web Inspector Storage tab, expand Indexed Databases
4. Click "stickieDB"
5. Close Web Inspector
6. Open Web Inspector
Comment 4 Nikita Vasilyev 2017-04-28 16:53:58 PDT
Created attachment 308617 [details]
[Animated GIF] Missing content view

There's no content view for:
1. Indexed Databases folder.
2. Indexed Database root item.

Currently, we only have a content view for an IndexedDB table.
Comment 5 Nikita Vasilyev 2017-04-28 17:03:22 PDT
Created attachment 308621 [details]
[Image] Chrome DevTools IndexedDB content view

Chrome DevTools show a DB name, security origin, and a version. There's also a button to delete a DB. We should do something like this, too.
Comment 6 Joseph Pecoraro 2017-04-28 18:12:02 PDT
> Chrome DevTools show a DB name, security origin, and a version. There's also
> a button to delete a DB. We should do something like this, too.

We should be showing must of this information in a context sensitive sidebar.
Comment 7 Nikita Vasilyev 2017-04-28 18:21:14 PDT
(In reply to Joseph Pecoraro from comment #6)
> > Chrome DevTools show a DB name, security origin, and a version. There's also
> > a button to delete a DB. We should do something like this, too.
> 
> We should be showing must of this information in a context sensitive sidebar.

Yes, we do show all of it in the sidebar.

I think it's strange to switch from, say, Cookies to an indexed database and not update the content view. It could be misleading.
Comment 8 Nikita Vasilyev 2017-04-28 18:43:38 PDT
Created attachment 308642 [details]
Patch
Comment 9 Nikita Vasilyev 2017-04-28 18:44:21 PDT
Created attachment 308643 [details]
[Image] With patch applied
Comment 10 Joseph Pecoraro 2017-04-28 19:43:05 PDT
What right sidebars are available with this patch? Is the IndexedDB sidebar available?
Comment 11 Nikita Vasilyev 2017-04-28 20:11:49 PDT
Created attachment 308656 [details]
[Image] Details sidebar

(In reply to Joseph Pecoraro from comment #10)
> What right sidebars are available with this patch? Is the IndexedDB sidebar
> available?

Maybe we shouldn't show a sidebar here at all.
Comment 12 Nikita Vasilyev 2017-04-28 20:17:40 PDT
(In reply to Nikita Vasilyev from comment #11)
> Created attachment 308656 [details]
> [Image] Details sidebar
> 
> (In reply to Joseph Pecoraro from comment #10)
> > What right sidebars are available with this patch? Is the IndexedDB sidebar
> > available?
> 
> Maybe we shouldn't show a sidebar here at all.

I'm not sure how to do this. When I remove WebInspector.IndexedDatabase from WebInspector.StorageTabContentView#canShowRepresentedObject clicking on the indexed database item in the sidebar ("stickieDB" on the screenshot) doesn't change the content view any longer :/
Comment 13 Nikita Vasilyev 2017-04-28 20:20:08 PDT
(In reply to Nikita Vasilyev from comment #12)
> (In reply to Nikita Vasilyev from comment #11)
> > Created attachment 308656 [details]
> > [Image] Details sidebar
> > 
> > (In reply to Joseph Pecoraro from comment #10)
> > > What right sidebars are available with this patch? Is the IndexedDB sidebar
> > > available?
> > 
> > Maybe we shouldn't show a sidebar here at all.
> 
> I'm not sure how to do this. When I remove WebInspector.IndexedDatabase from
> WebInspector.StorageTabContentView#canShowRepresentedObject clicking on the
> indexed database item in the sidebar ("stickieDB" on the screenshot) doesn't
> change the content view any longer :/

Sorry, ignore this comment, it doesn't make a lot of sense :)
Comment 14 Matt Baker 2017-05-01 16:39:01 PDT
(In reply to Nikita Vasilyev from comment #13)
> (In reply to Nikita Vasilyev from comment #12)
> > (In reply to Nikita Vasilyev from comment #11)
> > > Created attachment 308656 [details]
> > > [Image] Details sidebar
> > > 
> > > (In reply to Joseph Pecoraro from comment #10)
> > > > What right sidebars are available with this patch? Is the IndexedDB sidebar
> > > > available?
> > > 
> > > Maybe we shouldn't show a sidebar here at all.
> > 
> > I'm not sure how to do this. When I remove WebInspector.IndexedDatabase from
> > WebInspector.StorageTabContentView#canShowRepresentedObject clicking on the
> > indexed database item in the sidebar ("stickieDB" on the screenshot) doesn't
> > change the content view any longer :/
> 
> Sorry, ignore this comment, it doesn't make a lot of sense :)

The confusion arises because we continue to show the view for current represented object when selecting a tree item without a valid represented object. I think your patch helps address this.

Will review more thoroughly.
Comment 15 Nikita Vasilyev 2017-05-01 16:51:20 PDT
Created attachment 308787 [details]
Patch

Same patch, except the details sidebar for IndexedDB top level item is always hidden since it has the same content as the content view.
Comment 16 Matt Baker 2017-05-01 18:44:06 PDT
Created attachment 308796 [details]
[Image] Other UI ideas

I like the IndexedDatabaseContentView added by this patch. In the future we may want to have it serve as a summary/dashboard for the whole database (see attached mockup).

There is some redundant information already contained in the breadcrumbs/sidebar, but might still be useful to think about for a future patch.
Comment 17 Matt Baker 2017-05-01 18:49:27 PDT
Comment on attachment 308787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308787&action=review

> Source/WebInspectorUI/ChangeLog:9
> +        When an indexed database is selected in the Network navigation sidebar, show its host, security origin, and version.

I think you mean Storage navigation sidebar.

> Source/WebInspectorUI/ChangeLog:11
> +        Previously, selecting an indexed database didn't change the content view. It could led to a misleading state

typo: lead

> Source/WebInspectorUI/ChangeLog:12
> +        when an indexed database is selected in the sidebar, but the content view showed previosly selected item such as

typo: previously

> Source/WebInspectorUI/UserInterface/Main.html:513
> +    <script src="Views/CloseButtonView.js"></script>

Looks like it was included by mistake.
Comment 18 Matt Baker 2017-05-02 12:42:54 PDT
You could do this with a DetailsSection directly in the ContentView. It would be easy then to add groups for object stores, etc, in the future.

To get the UI as you have it now, you'll want to hide the DetailsSection's header in CSS, and add some styles to ContentView.css to make it prettier:

.content-view .details-section {
    font-size: 11px;
    background-color: initial;
}

.content-view .details-section > .header {
    font-size: 13px;
    font-weight: 600;
    background-color: initial;
}

.content-view .details-section > .content > .group > .row.simple > .label {
    width: 120px;
    font-weight: 600;
}

.content-view .details-section > .content > .group > .row.simple > .value {
    color: var(--text-color-gray-medium);
}

.content-view .details-section:last-child {
    border-bottom: none;
}
Comment 19 Matt Baker 2017-05-02 12:43:59 PDT
And I think you'll need to adjust the spacing around the section itself.
Comment 20 Nikita Vasilyev 2017-05-02 14:24:20 PDT
(In reply to Matt Baker from comment #16)
> Created attachment 308796 [details]
> [Image] Other UI ideas
> 
> I like the IndexedDatabaseContentView added by this patch. In the future we
> may want to have it serve as a summary/dashboard for the whole database (see
> attached mockup).
> 
> There is some redundant information already contained in the
> breadcrumbs/sidebar, but might still be useful to think about for a future
> patch.

I can't find larger versions of Database.png and DatabaseTable.png to use in the content view.

Also, these icons are pre iOS 7 style, e.g. they aren't flat. We should update them at some point.
Comment 21 Matt Baker 2017-05-02 14:32:23 PDT
(In reply to Nikita Vasilyev from comment #20)
> (In reply to Matt Baker from comment #16)
> > Created attachment 308796 [details]
> > [Image] Other UI ideas
> > 
> > I like the IndexedDatabaseContentView added by this patch. In the future we
> > may want to have it serve as a summary/dashboard for the whole database (see
> > attached mockup).
> > 
> > There is some redundant information already contained in the
> > breadcrumbs/sidebar, but might still be useful to think about for a future
> > patch.
> 
> I can't find larger versions of Database.png and DatabaseTable.png to use in
> the content view.

I think your original UI, using a DetailsSection rather than a custom table, is fine for this patch.

> Also, these icons are pre iOS 7 style, e.g. they aren't flat. We should
> update them at some point.

Agree. I'd like something vector and monochrome (or at least flat colors).
Comment 22 Nikita Vasilyev 2017-05-02 17:07:39 PDT
Created attachment 308868 [details]
Patch
Comment 23 Nikita Vasilyev 2017-05-02 17:08:26 PDT
Created attachment 308869 [details]
[Image] With patch applied
Comment 24 Matt Baker 2017-05-03 13:21:33 PDT
Comment on attachment 308868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308868&action=review

Looks good! r=me, with comments.

> Source/WebInspectorUI/UserInterface/Views/ContentView.css:45
> +}

This rule should go in IndexedDatabaseContentView.css. Headers will be nice to have when we have multiple sections in a ContentView.

> Source/WebInspectorUI/UserInterface/Views/ContentView.css:52
> +    width: auto;

Much nicer than having a fixed width.

> Source/WebInspectorUI/UserInterface/Views/IndexedDatabaseContentView.js:47
> +    {

Can the represented object change during the lifetime of the view? If not this should move to constructor.
Comment 25 Matt Baker 2017-05-03 13:24:21 PDT
Comment on attachment 308868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308868&action=review

>> Source/WebInspectorUI/UserInterface/Views/IndexedDatabaseContentView.js:47
>> +    {
> 
> Can the represented object change during the lifetime of the view? If not this should move to constructor.

Layout is called repeatedly as the view is resized, so this will need to move before landing.
Comment 26 Nikita Vasilyev 2017-05-03 14:31:51 PDT
Created attachment 308954 [details]
Patch
Comment 27 WebKit Commit Bot 2017-05-03 15:13:29 PDT
Comment on attachment 308954 [details]
Patch

Clearing flags on attachment: 308954

Committed r216147: <http://trac.webkit.org/changeset/216147>
Comment 28 WebKit Commit Bot 2017-05-03 15:13:31 PDT
All reviewed patches have been landed.  Closing bug.