Bug 30104 - Inspector should show cookies of sub-resources on the page.
: Inspector should show cookies of sub-resources on the page.
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-10-05 18:56 PST by
Modified: 2009-10-09 12:40 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-05 18:56:53 PST
Created an attachment (id=40677) [details]
iFrame test case

Inspector should show cookies of sub-resources on the page.
------- Comment #1 From 2009-10-05 18:58:27 PST -------
That test case should show the user's cookie's on webkit.org, but it doesn't.
------- Comment #2 From 2009-10-05 19:16:35 PST -------
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).
------- Comment #3 From 2009-10-05 19:41:45 PST -------
(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?
------- Comment #4 From 2009-10-05 19:44:00 PST -------
Also why why are there only two columns in the data grid?
------- Comment #5 From 2009-10-05 19:52:49 PST -------
(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 From 2009-10-05 20:01:18 PST -------
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 From 2009-10-05 20:24:49 PST -------
(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=40678) [details] [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 From 2009-10-05 20:27:39 PST -------
(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 From 2009-10-06 15:18:16 PST -------
Created an attachment (id=40747) [details]
GetRawCookies on windows
------- Comment #10 From 2009-10-06 15:39:02 PST -------
Created an attachment (id=40748) [details]
Fix duplicate variable names
------- Comment #11 From 2009-10-06 15:45:14 PST -------
(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.
------- Comment #12 From 2009-10-06 15:46:59 PST -------
(In reply to comment #11)
> (From update of attachment 40748 [details] [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 From 2009-10-06 15:55:15 PST -------
Part 1 landed in http://trac.webkit.org/changeset/49210.
------- Comment #14 From 2009-10-06 19:26:19 PST -------
Created an attachment (id=40756) [details]
Cookies from Subdomains Screenshot
------- Comment #15 From 2009-10-06 20:04:42 PST -------
Created an attachment (id=40760) [details]
Fix
------- Comment #16 From 2009-10-06 21:44:10 PST -------
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 From 2009-10-06 22:16:16 PST -------
(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 From 2009-10-07 09:04:57 PST -------
(From update of attachment 40760 [details])
> +    // 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 From 2009-10-07 09:27:54 PST -------
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 From 2009-10-07 13:26:27 PST -------
Created an attachment (id=40817) [details]
Addresses Darin's + Joe's comments
------- Comment #21 From 2009-10-07 13:43:05 PST -------
Created an attachment (id=40818) [details]
Style Fixes + deleteCookie support on Windows
------- Comment #22 From 2009-10-07 13:57:56 PST -------
(From update of attachment 40818 [details])
> +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 From 2009-10-07 14:12:54 PST -------
Created an attachment (id=40822) [details]
[V8 bindings] ScriptObject::set(const char*, unsigned)
------- Comment #24 From 2009-10-07 14:30:41 PST -------
(From update of attachment 40818 [details])
> +        this._cookieView = {};

This is better called _cookieViews.
------- Comment #25 From 2009-10-07 14:58:15 PST -------
pfeldman's patch landed in http://trac.webkit.org/changeset/49262, my patch landed in http://trac.webkit.org/changeset/49263.
------- Comment #26 From 2009-10-09 06:21:51 PST -------
Created an attachment (id=40947) [details]
patch fixing Chromium breakage

In Chromium Storage panel is still hidden so the WebInspector.addCookieDomain fails there with exception.
------- Comment #27 From 2009-10-09 06:59:46 PST -------
Created an attachment (id=40948) [details]
 patch fixing Chromium breakage

Fixed Changelog entry
------- Comment #28 From 2009-10-09 09:04:13 PST -------
(From update of attachment 40948 [details])
r=me.
------- Comment #29 From 2009-10-09 12:40:31 PST -------
Sorry guys, I missed the check there. Thanks for the fix!