[webkit-reviews] review denied: [Bug 127613] Web Inspector: In a DataGrid, store value of columnIdentifier as a dataset attribute : [Attachment 222197] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Diego Pino <dpino at igalia.com>'s
request for review:
Bug 127613: Web Inspector: In a DataGrid, store value of columnIdentifier as a
dataset attribute
https://bugs.webkit.org/show_bug.cgi?id=127613

Attachment 222197: Patch
https://bugs.webkit.org/attachment.cgi?id=222197&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=222197&action=review


r- because I think it breaks styles. But make it so you set classname and
dataset attribute, and it is good!

> Source/WebInspectorUI/UserInterface/DataGrid.js:88
> -	   cell.className = columnIdentifier + "-column";
> +	   cell.dataset["columnIdentifier"] = columnIdentifier;

I think we will need to set the class name for styles. There are a lot of CSS
instances of "<foo>-column":

    shell> ack "\-column" *.css
    DOMTreeDataGrid.css
    62:.dom-tree-data-grid .data-container .name-column {
    66:.dom-tree-data-grid .data-container .name-column .icon {
    74:.dom-tree-data-grid .data-container .name-column .label {
    83:.dom-tree-data-grid .data-container tr:hover .name-column .label {

    DetailsSection.css
    251:.details-section > .content .data-grid td.value-column {
    257:.details-section > .content .data-grid td.value-column > div {
    ...

> Source/WebInspectorUI/UserInterface/DataGrid.js:155
> -	   td.className = columnIdentifier + "-column";
> +	   td.dataset["columnIdentifier"] = columnIdentifier;

Same here.

> Source/WebInspectorUI/UserInterface/DataGrid.js:346
> -	   // FIXME: Better way to do this than regular expressions?
> -	   var columnIdentifier =
parseInt(element.className.match(/\b(\d+)-column\b/)[1], 10);
> +	   var columnIdentifier = element.dataset["columnIdentifier"];

This looks great though!