[webkit-reviews] review denied: [Bug 27202] Inspector: Cookies in Storage Panel : [Attachment 32643] Add Cookies to Database Panel

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Joseph Pecoraro
<joepeck02 at gmail.com>'s request for review:
Bug 27202: Inspector: Cookies in Storage Panel
https://bugs.webkit.org/show_bug.cgi?id=27202

Attachment 32643: Add Cookies to Database Panel
https://bugs.webkit.org/attachment.cgi?id=32643&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
You should describe more about the change in the ChangeLog. I know this was one
of your first patches, you do great now!

> + * Copyright (C) 2009 Apple Inc.  All rights reserved.

You should add your and not Apple on all new files.

> +WebInspector.CookieDataGrid.prototype = {
> +
> +    deleteSelectedRow: function(callback)
> +    {
> +	   var node = this.selectedNode;
> +	   var key = node.data[0];
> +	   var expire = 'Thu, 01 Jan 1970 00:00:00 GMT'; // (new
Date(0)).toGMTString();
> +	   var evalStr = 'document.cookie = "'+ key + '=; expires=' + expire +
'; path=/";' +
> +			 'document.cookie = "'+ key + '=; expires=' + expire +
'";';
> +	   WebInspector.console.doEvalInWindow(evalStr, callback);
> +    }
> +}

You should need to make a new DataGrid subclass just for this method. Can the
CookieItemsView just handle this one thing?

> +	   if (this._dataGrid) {
> +	       this._dataGrid.deleteSelectedRow( this.update.bind(this) );
> +	   }

No need for the braces. No need for the spaces.

> +    buildCookies: function()
> +    {
> +	   var rawCookieString =
InspectorController.inspectedWindow().document.cookie;
> +	   var rawCookies = rawCookieString.split(/;\s*/);
> +	   var cookies = {};
> +	   cookies.list = [];
> +
> +	   if (!(/^\s*$/.test(rawCookieString))) {
> +	       for (var i = 0, len = rawCookies.length; i < len; ++i) {
> +		   var cookie = rawCookies[i];
> +		   var sep = cookie.indexOf('=');
> +		   var name = cookie.substring(0, sep);
> +		   var value = cookie.substring(sep+1);
> +		   cookies.list.push(new WebInspector.Cookie(name, value));
> +	       }		
> +	   }
> +
> +	   this._cookies = cookies;
> +	   return cookies;
> +    },

I would be good if the View did this work and not the DatabasesPanel.

> +    dataGridForCookies: function()
> +    {
> +	   this.buildCookies();
> +	   var cookies = this._cookies;
> +	   if ( !cookies.list.length )
> +	       return null;
> +	   
> +	   var columns = {};
> +	   columns[0] = {};
> +	   columns[1] = {};
> +	   columns[0].title = WebInspector.UIString("Key");
> +	   columns[0].width = columns[0].title.length;
> +	   columns[1].title = WebInspector.UIString("Value");
> +	   columns[1].width = columns[0].title.length;
> +	   
> +	   var nodes = [];
> +	   
> +	   var list = cookies.list;
> +	   var length = list.length;
> +	   for (var index = 0; index < list.length; index++) {
> +	       var cookie = list[index];
> +	       var data = {};
> +	       
> +	       var key = cookie.key;
> +	       data[0] = key;
> +	       if (key.length > columns[0].width)
> +		   columns[0].width = key.length;
> +	       
> +	       var value = cookie.value;
> +	       data[1] = value;
> +	       if (value.length > columns[1].width)
> +		   columns[1].width = value.length;
> +	   
> +	       var node = new WebInspector.DataGridNode(data, false);
> +	       node.selectable = true;
> +	       nodes.push(node);
> +	 }
> +
> +	 var totalColumnWidths = columns[0].width + columns[1].width;
> +	 width = Math.round((columns[0].width * 100) / totalColumnWidths);
> +	 const minimumPrecent = 15;
> +	 if (width < minimumPrecent)
> +	     width = minimumPrecent;
> +	 if (width > 100 - minimumPrecent)
> +	     width = 100 - minimumPrecent;
> +	 columns[0].width = width;
> +	 columns[1].width = 100 - width;
> +	 columns[0].width += "%";
> +	 columns[1].width += "%";
> +
> +	 var dataGrid = new WebInspector.CookieDataGrid(columns);
> +	 var length = nodes.length;
> +	 for (var i = 0; i < length; ++i)
> +	     dataGrid.appendChild(nodes[i]);
> +	 if (length > 0)
> +	     nodes[0].selected = true;
> +	 return dataGrid;
> +
> +    },

I know this matches how the other data grids are made, but I think this should
be up to the views. This model was done for databases so two views could share
code. But some day we should make DatabasesPanel a very slim Panel subclass
that just manages the sidebar and active view.