| Summary: | Web Inspector: HeapAllocationsTimeline grid should use built-in grid column icons | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||
| Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Matt Baker
2016-04-22 15:31:48 PDT
Created attachment 277108 [details]
[Patch] Proposed Fix
Comment on attachment 277108 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=277108&action=review I still don't think it makes sense for DataGrid to special case having an icon for a data grid cell. It feels like too much is getting put into DataGrid. As is, this change looks fine, it just seems like increased complexity for no profit. > Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:40 > + icon: true Style: Should have trailing comma. (In reply to comment #4) > Comment on attachment 277108 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277108&action=review > > I still don't think it makes sense for DataGrid to special case having an > icon for a data grid cell. It feels like too much is getting put into > DataGrid. As is, this change looks fine, it just seems like increased > complexity for no profit. I disagree. Adding icon elements is a common operation, and pushing it into the grid adds very little complexity while ensuring consistently styled cell content. Created attachment 277110 [details]
[Patch] Proposed Fix
Comment on attachment 277110 [details]
[Patch] Proposed Fix
One solution to Joe's concerns would be to have a subclass of DataGridNode that added the icon support, but didn't bake it into DataGridNode directly. That would be similar to GeneralTreeElement for TreeOutlines. But I think this is fine. I suspect there are other places we could do this.
Since grid cell formatting is the same for all cells in a column, we might want something like this: DataGridColumn (basic formatting for text & numbers) DataGridIconColumn DataGridCustomColumn (for document fragment cells) I'll give it some more thought. Comment on attachment 277110 [details] [Patch] Proposed Fix Clearing flags on attachment: 277110 Committed r199947: <http://trac.webkit.org/changeset/199947> All reviewed patches have been landed. Closing bug. |