Bug 150959 – Web Inspector: Convert DatabaseContentView to use View base class

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
Convert DatabaseContentView to use View base class. The prompt element is wrapped in a div for styling purposes, preventing it from being added as a subview. The wrapper can be eliminated with some modifications to the database view CSS.

[Radar WebKit Bug Importer]

<rdar://problem/23420787>

[Matt Baker]

Created attachment 264905 [details]
[Patch] Proposed Fix

[Matt Baker]

DatabaseUserQueryView has static "builder" methods for creating views for error and success queries. This is to avoid having to overload the constructor like this:

class DatabaseUserQueryView
{
    constructor(query, columnNamesOrMessage, values)
    {
        this._query = query;
        if (columnNamesOrMessage instanceof String)
            this._message = columnNamesOrMessage;
        else
            this._dataGrid = WebInspector.DataGrid.createSortableDataGrid(columnNames, values);

        ...
    }
};

let errorResult = new WebInspector.DatabaseUserQueryView(query, errorMessage);
let successResult = new WebInspector.DatabaseUserQueryView(query, columnNames, values);

I'm open to suggestions if someone has a more elegant approach.

[Build Bot]

Comment on attachment 264905 [details]
[Patch] Proposed Fix

Attachment 264905 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/388238

New failing tests:
fast/hidpi/image-set-background-repeat.html

[Build Bot]

Created attachment 264912 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Timothy Hatcher]

Comment on attachment 264905 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/DatabaseUserQueryView.js:63
> +            queryView._resultElement.appendChild(dataGrid.element);

Can this be a subview? Perhaps addSubview needs the ability to add to a sub-element of a view?

[Blaze Burg]

Comment on attachment 264905 [details]
[Patch] Proposed Fix

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

I would prefer that we have separate View subclasses for error and success, since they have different view hierarchies and not much in common. Right?

It is awkward that we use a factory method and then dig into the internals to append a data grid.

> Source/WebInspectorUI/UserInterface/Views/DatabaseUserQueryView.js:26
> +WebInspector.DatabaseUserQueryView = class View extends WebInspector.View

Nit: class DatabaseUserQueryView

> Source/WebInspectorUI/UserInterface/Views/DatabaseUserQueryView.js:35
> +        commandTextElement.className = "database-query-text";

Nit: pls classList.add

> Source/WebInspectorUI/UserInterface/Views/DatabaseUserQueryView.js:61
> +        if (dataGrid) {

Why could this return falsey? In that case, can we do anything useful?

[Matt Baker]

Comment on attachment 264905 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/DatabaseUserQueryView.js:61
>> +        if (dataGrid) {
> 
> Why could this return falsey? In that case, can we do anything useful?

It returns null if columnNames.length == 0, otherwise you always get a DataGrid. A valid query can return an empty result set, which we should handle by showing a "No results returned" message. Currently the database content view throws out the query result if it's valid but empty.

>> Source/WebInspectorUI/UserInterface/Views/DatabaseUserQueryView.js:63
>> +            queryView._resultElement.appendChild(dataGrid.element);
> 
> Can this be a subview? Perhaps addSubview needs the ability to add to a sub-element of a view?

Having an optional second parameter would be a simple solution. I've also considered adding a "container element" concept to View, which would be useful for views with chrome (scrollbars, etc) that are vulnerable to subclasses naively calling: this.element.removeChildren().

[Matt Baker]

(In reply to comment #7)
> Comment on attachment 264905 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264905&action=review
> 
> I would prefer that we have separate View subclasses for error and success,
> since they have different view hierarchies and not much in common. Right?
> 
> It is awkward that we use a factory method and then dig into the internals
> to append a data grid.

For me the awkward aspect is that clients can bypass the factory and construct a useless DatabaseUserQueryView. It's fine for static factory/builder methods to access the private internals of the class to which they belong. That said, having subclasses that do the work in their constructors is much cleaner. The base class can construct the DOM element tree that they have in common.

[Matt Baker]

Created attachment 264961 [details]
[Image] 'No Results' UI

Currently we show nothing when a query succeeds and returns no rows. This is better.

[Matt Baker]

Comment on attachment 264905 [details]
[Patch] Proposed Fix

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

>>> Source/WebInspectorUI/UserInterface/Views/DatabaseUserQueryView.js:63
>>> +            queryView._resultElement.appendChild(dataGrid.element);
>> 
>> Can this be a subview? Perhaps addSubview needs the ability to add to a sub-element of a view?
> 
> Having an optional second parameter would be a simple solution. I've also considered adding a "container element" concept to View, which would be useful for views with chrome (scrollbars, etc) that are vulnerable to subclasses naively calling: this.element.removeChildren().

Allowing more than one View DOM element to host child views complicates View.prototype.insertSubviewBefore. Let's revisit this once View can distinguish between its root element and the element which can host subviews. See https://bugs.webkit.org/show_bug.cgi?id=150982.

[Matt Baker]

Created attachment 264972 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 264972 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 264972

Committed r192165: <http://trac.webkit.org/changeset/192165>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.