Bug 27202 – Inspector: Cookies in Storage Panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Joseph Pecoraro]

This is a continuation of:
Bug 21051: "Databases panel should turn into a general Storage panel"
https://bugs.webkit.org/show_bug.cgi?id=21051

I didn't want to Reopen that bug because things have changed slightly since then and the most recent update on that was over 5 months old.

[Joseph Pecoraro]

Created attachment 32643 [details]
Add Cookies to Database Panel

The first take!  Here are some comments:

- I have little to no image skills.  If anyone could help make a better image or point me to someone who can that would be awesome!  I just made a new image to get the proper CSS rules out of the way.
- The Cookie TreeElement disappears if the user has the Inspector open and navigates to another page or reloads the current page.  I don't know which functions the Inspector is calling.  It doesn't seem to call the Panel's show() or populateInterface() methods on page loads, only when the currentPanel is changed to the Databases panel or the WebInspector is opened after being closed.
- This is an all js version. I hope to work up to switching over to C++ to access more of the cookie data and to do some processing without having to resort to evaluating code in the inspected window.
- A page has only 1 set of cookies but can have >1 DOMStorage and >1 Databases.  This is why WebInspector.DatabasesPanel._cookies is one object that is constantly updated, and why nothing is passed into the CookieView.  Whenever the view needs the list of cookies it must resort to checking the inspected page's document.cookie string.  A C++ version would probably make this cleaner.
- The Datagrid has the ability to delete cookies and will automatically update the display once a cookie has been deleted.  Again this jumps into the inspected window to run a command.
- I tried to keep in line with the coding standards from DOMStorage and Databases as much as possible.
- I used the Apple Copyright with the year 2009.  However, I'm not actually affiliated with Apple.  I just noticed writing this bug report that it says "please also add your copyright (name and year) to the relevant files".  Is the Apple Copyright fine to keep or should I replace it with my own?

If you want to test it out you can easily add/remove cookies, and at the same time get a quick reminder on how to work with them via javascript at this site.  I've been using it for testing and its helped me squash some bugs:
http://www.quirksmode.org/js/cookies.html

Thanks for taking the time to look this over.  Cheers!

[Timothy Hatcher]

Attach a screenshot.

[Joseph Pecoraro]

Created attachment 32646 [details]
Screenshot of the Cookies on the Apple Startpage

Hopefully nothing in there is sensitive =)

[Joseph Pecoraro]

Leaving a note while I think of it:  I should be a lot more careful when I delete a cookie to escape any quotes... since this passes through an eval.  This would hopefully be avoided with C++ interaction with cookies.

[Joseph Pecoraro]

Here is a quick video of what it looks like:
http://screencast.com/t/T4buGsyXd3

[Anthony Ricaud]

You should include path and date columns. They are very useful.

[Timothy Hatcher]

That info isn't available to JS via document.cookie.

[Timothy Hatcher]

Comment on attachment 32643 [details]
Add Cookies to Database Panel

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.

[Timothy Hatcher]

Also we need bugs to track adding the other cookie fields with a C++ implementation and showing cookies for other domains on the same page.

[Joseph Pecoraro]

> Also we need bugs to track adding the other cookie fields with a C++
> implementation and showing cookies for other domains on the same page.

Thanks for the review!  I'll work on polishing up this JavaScript only version, submit a patch, and afterwards I'll open some C++ bugs up once I get this out of the way.

Also, this probably needs a better cookies icon then the one I made.

[Joseph Pecoraro]

Created attachment 34377 [details]
Fixed Based on Review

[Joseph Pecoraro]

(In reply to comment #4)
> Leaving a note while I think of it:  I should be a lot more careful when I
> delete a cookie to escape any quotes... since this passes through an eval. 

Done. I escape quotes.

(In reply to comment #8)
> You should describe more about the change in the ChangeLog.

Done. And the correct files now!

> You should add you and not Apple on all new files.

Done. Added myself for Cookie.js and both Apple + myself on CookiesItemView.js due to similar code.

> You should[n't] need to make a new DataGrid subclass just for this method. Can the
> CookieItemsView just handle this one thing?

Done.

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

Done. Including many other style fixes.

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

Done. Moved the bulk of the cookie code into the view.

(In reply to comment #1)
> - The Cookie TreeElement disappears if the user has the Inspector open and
> navigates to another page or reloads the current page.  I don't know which
> functions the Inspector is calling.  It doesn't seem to call the Panel's show()
> or populateInterface() methods on page loads, only when the currentPanel is
> changed to the Databases panel or the WebInspector is opened after being
> closed.

This is still a minor problem.

> - The Datagrid has the ability to delete cookies and will automatically update
> the display once a cookie has been deleted.  Again this jumps into the
> inspected window to run a command.

Deleting isn't perfect yet. I have to "guess" the Cookie's path, so I attempt both an empty path and the "/" path.  This should be solved with a C++ version.

[Eric Seidel (no email)]

Comment on attachment 34377 [details]
Fixed Based on Review

Rejecting patch from commit queue: 34377

Failure, rejecting patch from commit queue.  This patch will require manual landing.
Patch https://bugs.webkit.org/attachment.cgi?id=34377 from bug 27202 failed to download and apply.

[Eric Seidel (no email)]

Here is the log.  Sorry the commit-queue/bugzilla-tool is not smart enough to know how to post the log yet:
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/inspector/front-end/Cookie.js
patching file WebCore/inspector/front-end/CookieItemsView.js
patching file WebCore/inspector/front-end/DatabasesPanel.js
patch: **** Only garbage was found in the patch input.
patch -p0 "WebCore/inspector/front-end/Images/cookie.png" returned 2.  Pass --force to ignore patch failures.

[Adam Barth]

http://trac.webkit.org/changeset/47181