Summary: | Web Inspector: Convert DatabaseContentView to use View base class | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bburg, buildbot, commit-queue, graouts, joepeck, mattbaker, nvasilyev, rniwa, timothy, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Matt Baker
2015-11-05 15:09:54 PST
Created attachment 264905 [details]
[Patch] Proposed Fix
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. 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 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
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? 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? 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(). (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. Created attachment 264961 [details]
[Image] 'No Results' UI
Currently we show nothing when a query succeeds and returns no rows. This is better.
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. Created attachment 264972 [details]
[Patch] Proposed Fix
Comment on attachment 264972 [details] [Patch] Proposed Fix Clearing flags on attachment: 264972 Committed r192165: <http://trac.webkit.org/changeset/192165> All reviewed patches have been landed. Closing bug. |