Bug 127613 – Web Inspector: In a DataGrid, store value of columnIdentifier to DOM node representing a cell

• Component: Web Inspector
• Status: RESOLVED FIXED

[Diego Pino]

Currently, cell DOM nodes of a DataGrid store the value of a 'columnIdentifier' in the 'class' attribute. This requires later parsing of attribute 'class' to know the value of 'columnIdentifier'. HTML5 provides a proper way to store custom attributes to a DOM node, which is by using the 'dataset' attribute. 

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement.dataset

Storing 'columnIdentifier' as a dataset attribute won't require later parsing.

[Radar WebKit Bug Importer]

<rdar://problem/15908787>

[Diego Pino]

Created attachment 222197 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 222197 [details]
Patch

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!

[Timothy Hatcher]

Comment on attachment 222197 [details]
Patch

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

I agree with Joe. We need the className for style reasons. I am also not a fan of dataset. There is no reason to use a DOM attribute for internal logic. I would just set cell.__columnIdentifier and use it later.

> Source/WebInspectorUI/UserInterface/DataGrid.js:903
> -                // The first child of the selected element is the <td class="0-column">,
> +                // The first child of the selected element is the <td data-columnIdentifier="0">,

The original comment is not correct, the column identifier is not usually a number, it is a arbitrary string. Just remove this comment.

[Timothy Hatcher]

Lets not use dataset. Using the DOM for this is expensive. A private property on the DOM element is good.

[Diego Pino]

Created attachment 222234 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 222234 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/DataGrid.js:156
> +        td.columnIdentifier = columnIdentifier;

As Tim suggested, we should use "__columnIdentifier".

We typically use "__foo" syntax for property names when one class is storing properties on another object, and that other object has no knowledge of the existence of that property. Though ".columnIdentifier" works, it is not immediately clear if you're debugging this element that the property is something we injected onto the element. And having it show up as obvious in the debugger can sometimes be helpful! Also, if in the further that object wanted to add a "columnIdentifier" property then the double underscore would avoid a naming conflict.

[Diego Pino]

Created attachment 222378 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 222378 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/DataGrid.js:156
> +        td.__columnIdentifier = parseInt(columnIdentifier);

Whoa, there is a parseInt now? Can't columnIdentifier be a string like "name"?

[Timothy Hatcher]

Comment on attachment 222378 [details]
Patch

Yeah, parseInt is not correct. The old code had it in a different spot and it was wrong.

[Diego Pino]

Comment on attachment 222378 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/DataGrid.js:156
>> +        td.__columnIdentifier = parseInt(columnIdentifier);
> 
> Whoa, there is a parseInt now? Can't columnIdentifier be a string like "name"?

I noticed that __columnIdentifier has to be "numeric". If __columnIdentifier is a "string", there's a regression. When I try the following:

   * double-click on a "Key" column 
   * Insert a value, for instance "Key1"
   * Press return

The result is stored in the key column as "Key1" but it's also stored in the value column as "Key1". This is not correct.

In the original code, parseInt was used in "_editingCommited" and "_contextMenuInDataTable" for casting the value of columnIdentifier after retrieving that value from a regular expression.

var columnIdentifier = parseInt(element.className.match(/\b(\d+)-column\b/)[1], 10);

Since there is going to be a field for storing columnIdentifier I thought it was better to store that field as numeric, instead of casting it everytime "_editingCommited" and "_contextMenuInDataTable" were called.

Still, why columnIdentifier has to be numeric? If __columnIdentifier is a string, function "_contextMenuInDataTable" works well. The problem is in function "_editingCommited". _editingCommited calls this._editCallback. _editCallback (DOMStorageContentView.js:215) does a strict comparison of the value of columnIdentifier.

_editingCallback: function(editingNode, columnIdentifier, oldText, newText)
{
    var domStorage = this.representedObject;
    if (columnIdentifier === 0) {
        if (oldText)
            domStorage.removeItem(oldText);

        domStorage.setItem(newText, editingNode.data[1]);
    } else
        domStorage.setItem(editingNode.data[0], newText);

    this.update();
}

If columnIdentifier is a "string" the else branch is always executed (it updates the value of the value column). That's why in the use-case explained above, when updating a key the key column gets updated (visually) and the value column gets updated too.

[Timothy Hatcher]

Comment on attachment 222378 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/DataGrid.js:156
>>> +        td.__columnIdentifier = parseInt(columnIdentifier);
>> 
>> Whoa, there is a parseInt now? Can't columnIdentifier be a string like "name"?
> 
> I noticed that __columnIdentifier has to be "numeric". If __columnIdentifier is a "string", there's a regression. When I try the following:
> 
>    * double-click on a "Key" column 
>    * Insert a value, for instance "Key1"
>    * Press return
> 
> The result is stored in the key column as "Key1" but it's also stored in the value column as "Key1". This is not correct.
> 
> In the original code, parseInt was used in "_editingCommited" and "_contextMenuInDataTable" for casting the value of columnIdentifier after retrieving that value from a regular expression.
> 
> var columnIdentifier = parseInt(element.className.match(/\b(\d+)-column\b/)[1], 10);
> 
> Since there is going to be a field for storing columnIdentifier I thought it was better to store that field as numeric, instead of casting it everytime "_editingCommited" and "_contextMenuInDataTable" were called.
> 
> Still, why columnIdentifier has to be numeric? If __columnIdentifier is a string, function "_contextMenuInDataTable" works well. The problem is in function "_editingCommited". _editingCommited calls this._editCallback. _editCallback (DOMStorageContentView.js:215) does a strict comparison of the value of columnIdentifier.
> 
> _editingCallback: function(editingNode, columnIdentifier, oldText, newText)
> {
>     var domStorage = this.representedObject;
>     if (columnIdentifier === 0) {
>         if (oldText)
>             domStorage.removeItem(oldText);
> 
>         domStorage.setItem(newText, editingNode.data[1]);
>     } else
>         domStorage.setItem(editingNode.data[0], newText);
> 
>     this.update();
> }
> 
> If columnIdentifier is a "string" the else branch is always executed (it updates the value of the value column). That's why in the use-case explained above, when updating a key the key column gets updated (visually) and the value column gets updated too.

Good catch! But DOMStorageContentView.js is the one that should change. Making the 0s and 1s be "0" and "1" would be fine to fix in this patch. Ideally, DOMStorageContentView.js would use identifiers like "key" and "value" instead of numbers.

[Diego Pino]

Created attachment 222430 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 222430 [details]
Patch

Clearing flags on attachment: 222430

Committed r162931: <http://trac.webkit.org/changeset/162931>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.