[Webkit-unassigned] [Bug 127613] Web Inspector: In a DataGrid, store value of columnIdentifier to DOM node representing a cell

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=127613





--- Comment #12 from Timothy Hatcher <timothy at apple.com>  2014-01-28 00:29:15 PST ---
(From update of attachment 222378)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.