Bug 30104 - Inspector should show cookies of sub-resources on the page.
Summary: Inspector should show cookies of sub-resources on the page.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-05 18:56 PDT by Brian Weinstein
Modified: 2009-10-09 12:40 PDT (History)
2 users (show)

See Also:


Attachments
iFrame test case (206 bytes, text/html)
2009-10-05 18:56 PDT, Brian Weinstein
no flags Details
Work in Progress Screenshot (97.50 KB, image/png)
2009-10-05 19:16 PDT, Brian Weinstein
no flags Details
GetRawCookies on windows (2.75 KB, patch)
2009-10-06 15:18 PDT, Brian Weinstein
bweinstein: commit-queue-
Details | Formatted Diff | Diff
Fix duplicate variable names (2.77 KB, patch)
2009-10-06 15:39 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Cookies from Subdomains Screenshot (137.59 KB, image/png)
2009-10-06 19:26 PDT, Brian Weinstein
no flags Details
Fix (23.67 KB, patch)
2009-10-06 20:04 PDT, Brian Weinstein
darin: review-
bweinstein: commit-queue-
Details | Formatted Diff | Diff
Addresses Darin's + Joe's comments (25.74 KB, patch)
2009-10-07 13:26 PDT, Brian Weinstein
bweinstein: commit-queue-
Details | Formatted Diff | Diff
Style Fixes + deleteCookie support on Windows (27.17 KB, patch)
2009-10-07 13:43 PDT, Brian Weinstein
timothy: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff
[V8 bindings] ScriptObject::set(const char*, unsigned) (1.70 KB, patch)
2009-10-07 14:12 PDT, Pavel Feldman
timothy: review+
Details | Formatted Diff | Diff
patch fixing Chromium breakage (1.11 KB, patch)
2009-10-09 06:21 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch fixing Chromium breakage (1.09 KB, patch)
2009-10-09 06:59 PDT, Yury Semikhatsky
dglazkov: review+
dglazkov: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2009-10-05 18:56:53 PDT
Created attachment 40677 [details]
iFrame test case

Inspector should show cookies of sub-resources on the page.
Comment 1 Brian Weinstein 2009-10-05 18:58:27 PDT
That test case should show the user's cookie's on webkit.org, but it doesn't.
Comment 2 Brian Weinstein 2009-10-05 19:16:35 PDT
Created attachment 40678 [details]
Work in Progress Screenshot

This is a rough pass at the look/functionality. One issue I am struggling with is that cookies are defined by their full URL, so to differentiate between resources, we need the full URL instead of the domain, but the full URL is long, so it can be cut off (as it is in this image).
Comment 3 Timothy Hatcher 2009-10-05 19:41:45 PDT
(In reply to comment #2)
> Created an attachment (id=40678) [details]
> Work in Progress Screenshot
> 
> This is a rough pass at the look/functionality. One issue I am struggling with
> is that cookies are defined by their full URL, so to differentiate between
> resources, we need the full URL instead of the domain, but the full URL is
> long, so it can be cut off (as it is in this image).

Cookies are not defined by their full URL. Cokkies are defined by a domain suffix and a path prefix. A full URL matches against that.

I proposed we should show all cookies that match the shortest domain.

So if a resource is from images.apple.com, we show all cookies for apple.com as an item in the sidebar.

And if another resource is from ads.doubleclick.net, then we show another item in the sidebar for doubleclick.net.

Make sense?
Comment 4 Timothy Hatcher 2009-10-05 19:44:00 PDT
Also why why are there only two columns in the data grid?
Comment 5 Joseph Pecoraro 2009-10-05 19:52:49 PDT
(In reply to comment #4)
> Also why why are there only two columns in the data grid?

It looks like he is on Windows. My guess is access to raw cookies isn't implemented in Windows (I only did it for Mac) and so its falling back to the JavaScript version?
Comment 6 Timothy Hatcher 2009-10-05 20:01:18 PDT
Ah, makes sense.

Brian, make sure you try on the Mac, since it uses a different code path.

Also you should implent what is need on Windows to get the full data like Mac.
Comment 7 Brian Weinstein 2009-10-05 20:24:49 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=40678) [details] [details]
> > Work in Progress Screenshot
> > 
> > This is a rough pass at the look/functionality. One issue I am struggling with
> > is that cookies are defined by their full URL, so to differentiate between
> > resources, we need the full URL instead of the domain, but the full URL is
> > long, so it can be cut off (as it is in this image).
> 
> Cookies are not defined by their full URL. Cokkies are defined by a domain
> suffix and a path prefix. A full URL matches against that.
> 
> I proposed we should show all cookies that match the shortest domain.
> 
> So if a resource is from images.apple.com, we show all cookies for apple.com as
> an item in the sidebar.
> 
> And if another resource is from ads.doubleclick.net, then we show another item
> in the sidebar for doubleclick.net.
> 
> Make sense?

Yeah, I had my wires crossed about a couple things I was thinking about, it makes sense, and I'll get a new screen shot.
Comment 8 Brian Weinstein 2009-10-05 20:27:39 PDT
(In reply to comment #6)
> Ah, makes sense.
> 
> Brian, make sure you try on the Mac, since it uses a different code path.
> 
> Also you should implent what is need on Windows to get the full data like Mac.

I'm testing on both, but I'm not sure if we have the data from CFNetwork to get all that cookie information, but I'll see what I can do.
Comment 9 Brian Weinstein 2009-10-06 15:18:16 PDT
Created attachment 40747 [details]
GetRawCookies on windows
Comment 10 Brian Weinstein 2009-10-06 15:39:02 PDT
Created attachment 40748 [details]
Fix duplicate variable names
Comment 11 Brady Eidson 2009-10-06 15:45:14 PDT
Comment on attachment 40748 [details]
Fix duplicate variable names

Looks fine.

Are you sure we use kCFAbsoluteTimeIntervalSince1970 and not ReferenceDate?  (Jan 1 2001)?

I'm not sure, I'm just asking.
Comment 12 Brian Weinstein 2009-10-06 15:46:59 PDT
(In reply to comment #11)
> (From update of attachment 40748 [details])
> Looks fine.
> 
> Are you sure we use kCFAbsoluteTimeIntervalSince1970 and not ReferenceDate? 
> (Jan 1 2001)?
> 
> I'm not sure, I'm just asking.

The mac uses NSTimeInterval expires = [[cookie expiresDate] timeIntervalSince1970] * 1000, and the JavaScript parses it based on 1970, so the kCFAbsoluteTimeIntervalSince1970 is necessary. Thanks for the review!
Comment 13 Brian Weinstein 2009-10-06 15:55:15 PDT
Part 1 landed in http://trac.webkit.org/changeset/49210.
Comment 14 Brian Weinstein 2009-10-06 19:26:19 PDT
Created attachment 40756 [details]
Cookies from Subdomains Screenshot
Comment 15 Brian Weinstein 2009-10-06 20:04:42 PDT
Created attachment 40760 [details]
Fix
Comment 16 Joseph Pecoraro 2009-10-06 21:44:10 PDT
A few comments and questions for my own education.

> this._cookieView = [];
> ... later on ...
> var view = this._cookieView[cookieDomain];

You're using _cookieView like an object / hashtable but initialize it as an array. Although it works as an array "[]", it would be clearer as an object "{}".

> this._cookieDomains = [];
> ...
> // Eliminate duplicate domains from the list.
> if (this._cookieDomains.indexOf(domain) != -1)
>   return;

Although its probably negligible in this case because the # of domains is likely to be very small, is it better to turn this into an Object with properties and look for duplicates that way?  Is there a heuristic for choosing one approach over the other?

  if (!this._cookieDomains[domain]) {
    this._cookieDomains[domain] = true; // Prevent duplicates
    // ...
  }


Any ideas on implementing deleteCookie for Windows?  I honestly can't find a scrap of documentation on CFHTTPCookieStorage stuff. And when the trail finally ended it was in a binary lib file =(.
Comment 17 Brian Weinstein 2009-10-06 22:16:16 PDT
(In reply to comment #16)
> A few comments and questions for my own education.
> 
> > this._cookieView = [];
> > ... later on ...
> > var view = this._cookieView[cookieDomain];
> 
> You're using _cookieView like an object / hashtable but initialize it as an
> array. Although it works as an array "[]", it would be clearer as an object
> "{}".

I was trying to make it behave like an associative array, this._cookieView['string'] = CookieTreeElement or null, wasn't sure the best way to handle this in JavaScript.

> 
> > this._cookieDomains = [];
> > ...
> > // Eliminate duplicate domains from the list.
> > if (this._cookieDomains.indexOf(domain) != -1)
> >   return;
> 
> Although its probably negligible in this case because the # of domains is
> likely to be very small, is it better to turn this into an Object with
> properties and look for duplicates that way?  Is there a heuristic for choosing
> one approach over the other?
> 
>   if (!this._cookieDomains[domain]) {
>     this._cookieDomains[domain] = true; // Prevent duplicates
>     // ...
>   }
> 

Not sure which way is the best, like I mentioned above. I figure I'll take Tim's opinion when he reviews the patch.

> Any ideas on implementing deleteCookie for Windows?  I honestly can't find a
> scrap of documentation on CFHTTPCookieStorage stuff. And when the trail finally
> ended it was in a binary lib file =(.

Yeah, if you want to file a bug about it, I should be able to implement that tomorrow, assuming CFNetwork has the API I hope it does. You can assign it to me if you do want to file it.

Thanks for the feedback! I'll see what Tim has to say when he reviews it, and then go from there with his and your feedback.
Comment 18 Darin Adler 2009-10-07 09:04:57 PDT
Comment on attachment 40760 [details]
Fix

> +    // If we can't get raw cookies - fallback to String representation

The verb "fall back" is two words.

> +    String cookieList = String();

Strings automatically start out with null value, so there's no need to say "= String()".

Naming cookiesList and cookieList almost the same is a bad idea. Give them more clearly different names, please.

> +        Document* doc = it->second->frame()->document();

I prefer document rather than "doc".

> +        if (doc->url().host() == host)
> +        {

Brace goes on same line as if statement.

> +                    if (cookiesList.find(docCookiesList[i]) == WTF::notFound)
> +                        cookiesList.append(docCookiesList);

If you're going to do a find like, it seems to me you should be use a ListHashSet rather than a Vector so it won't get super-slow.

You should be able to use just notFound rather than WTF::notFound. If not, this should be fixed in WTF by adding a using in the file that defines notFound.

> +    if (!isImplemented)
> +        m_frontend->didGetCookies(callId, m_frontend->newScriptArray(), cookieList);
> +    else
> +        m_frontend->didGetCookies(callId, buildArrayForCookies(cookiesList), String());

I find the variable name "isImplemented" confusing in this longer function although it was clear in the shorter function you created it from. It's also strange that it gets set repeatedly each time through the loop. Are you sure it will have the same value every time? Could it return different values? I think it's ugly that you get the value repeatedly and assume it's the same every time.

> +    unsigned length = cookiesList.size();

Some places you are using unsigned and other places int. Normally I suggest using size_t to iterate a Vector.

> +        const Cookie& cookie = cookiesList[i];
> +        cookies.set(i, buildObjectForCookie(cookie));

I don't think the local variable adds value here. You should just write it in one line.

> +    ScriptObject value = m_frontend->newScriptObject();
> +    value.set("name", cookie.name);
> +    value.set("value", cookie.value);
> +    value.set("domain", cookie.domain);
> +    value.set("path", cookie.path);
> +    value.set("expires", cookie.expires);
> +    value.set("size", static_cast<int>(cookie.name.length() + cookie.value.length()));
> +    value.set("httpOnly", cookie.httpOnly);
> +    value.set("secure", cookie.secure);
> +    value.set("session", cookie.session);
> +    return value;

Does the static_cast<int> really need to be there? Could you explain why?

> +        if (document->url().host() == domain) {
> +            WebCore::deleteCookie(document, document->cookieURL(), cookieName);
> +        }

No braces on a single-line if statement.

> +    ScriptObject buildObjectForCookie(const Cookie& cookie);
> +    ScriptArray buildArrayForCookies(const Vector<Cookie>& cookiesList);

We normally omit argument names in cases like this where the type makes it clear alone.

> +        void addCookieDomainForDocument(const Document* document);

Normally we would just use Document*, not const Document* and not give a name for the argument.

> -        delete this._cookieView;
> +        this._cookieDomains = [];
> +        this._cookieView = [];

It seems strange to set _cookieView to an empty array. Why that instead of setting it to an empty object, or to null, or empty string, or deleting the property?

> +        // Eliminate duplicate domains from the list.
> +        if (this._cookieDomains.indexOf(domain) != -1)
> +            return;

This is only efficient if we are guaranteed there are a small number of domains. If there's a larger number, then you should keep another object that uses the domains as property names so you can use hashing to quickly check if the domain is in the list.

> +        bool operator==(const Cookie& cookie) const
> +        {
> +            return cookie.name == name && cookie.domain == domain && cookie.path == path && cookie.secure == secure;
> +        }

Normally it's better to make operator== a non-member function instead of a member.

Normally we define != too if we define ==.

review- because there are enough comments that I think you'll want to make at least some changes
Comment 19 Joseph Pecoraro 2009-10-07 09:27:54 PDT
Some of these comments refer to my older code that Brian moved between files.

> > +    value.set("size", static_cast<int>(cookie.name.length() + cookie.value.length()));
> 
> Does the static_cast<int> really need to be there? Could you explain why?

ScriptObject.set() does not work with an unsigned (the return value from String.length()). int can sufficiently hold the value and satisfied the compiler warnings.  Perhaps a better solution would be to add that signature and implementation to ScriptObject?
Comment 20 Brian Weinstein 2009-10-07 13:26:27 PDT
Created attachment 40817 [details]
Addresses Darin's + Joe's comments
Comment 21 Brian Weinstein 2009-10-07 13:43:05 PDT
Created attachment 40818 [details]
Style Fixes + deleteCookie support on Windows
Comment 22 Pavel Feldman 2009-10-07 13:57:56 PDT
Comment on attachment 40818 [details]
Style Fixes + deleteCookie support on Windows

> +bool ScriptObject::set(const char* name, unsigned value)
> +{
> +    JSLock lock(SilenceAssertionsOnly);
> +    PutPropertySlot slot;
> +    jsObject()->put(m_scriptState, Identifier(m_scriptState, name), jsNumber(m_scriptState, value), slot);
> +    return handleException(m_scriptState);
> +}

I'd like to do that for v8 prior to landing this.

> +void InspectorFrontend::addCookieDomainForDocument(Document* document)
> +{
> +    OwnPtr<ScriptFunctionCall> function(newFunctionCall("addCookieDomain"));
> +    function->appendArgument(document->url().host());
> +    function->call();
> +}
> +

Names of InspectorFrontend::* and corresponding WebInspector.* methods should match. InspectorFrontend is a C++ API to the frontend. It is pretty much reflecting the WebInspector.* API in JS that InspectorController is calling into. We could even try generating it. I'd suggest introducing

void InspectorFrontend::addCookieDomain(const String& domain)

instead.
Comment 23 Pavel Feldman 2009-10-07 14:12:54 PDT
Created attachment 40822 [details]
[V8 bindings] ScriptObject::set(const char*, unsigned)
Comment 24 Timothy Hatcher 2009-10-07 14:30:41 PDT
Comment on attachment 40818 [details]
Style Fixes + deleteCookie support on Windows

> +        this._cookieView = {};

This is better called _cookieViews.
Comment 25 Brian Weinstein 2009-10-07 14:58:15 PDT
pfeldman's patch landed in http://trac.webkit.org/changeset/49262, my patch landed in http://trac.webkit.org/changeset/49263.
Comment 26 Yury Semikhatsky 2009-10-09 06:21:51 PDT
Created attachment 40947 [details]
patch fixing Chromium breakage

In Chromium Storage panel is still hidden so the WebInspector.addCookieDomain fails there with exception.
Comment 27 Yury Semikhatsky 2009-10-09 06:59:46 PDT
Created attachment 40948 [details]
 patch fixing Chromium breakage

Fixed Changelog entry
Comment 28 Dimitri Glazkov (Google) 2009-10-09 09:04:13 PDT
Comment on attachment 40948 [details]
 patch fixing Chromium breakage

r=me.
Comment 29 Brian Weinstein 2009-10-09 12:40:31 PDT
Sorry guys, I missed the check there. Thanks for the fix!