Bug 156934 – Web Inspector: HeapAllocationsTimeline grid should use built-in grid column icons

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
HeapAllocationsTimeline grid should use built-in grid column icons. To avoid subtle differences in styling, grid nodes should avoid creating their own icon elements for document fragment cells.

[Matt Baker]

Created attachment 277108 [details]
[Patch] Proposed Fix

[Radar WebKit Bug Importer]

<rdar://problem/25887367>

[Radar WebKit Bug Importer]

<rdar://problem/25887363>

[Joseph Pecoraro]

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.

[Matt Baker]

(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.

[Matt Baker]

Created attachment 277110 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

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.

[Matt Baker]

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.

[WebKit Commit Bot]

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

Clearing flags on attachment: 277110

Committed r199947: <http://trac.webkit.org/changeset/199947>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.