Inspecting cookie data currently uses "document.cookie" in the inspected window and thus only has access to the names and values of cookies. However, there is far more information, namely path, expires, domain, httpOnly, sessionOnly, and secure. This information should be made available to the Inspector.

Created attachment 34592[details]
Working on OS X Leopard
This is pretty much my first experience with C++, so I'd appreciate it if you be explicit about any of the C++ mistakes I made, or build fixes that will be needed for other platforms. I changed the CookieJar.h header so I expect that I will need to fix a number of other files. Thanks in advance.

Patch looks pretty good. I think rawCookies will be better named as cookies. And rename the existing cookies to cokkiesString or somthing more specific.
I'll review more later and provide guidance o nwhat files to edit for other platforms.

Comment on attachment 34592[details]
Working on OS X Leopard
> + Vector<Cookie> cookies;
> + Document* document = ic->inspectedPage()->mainFrame()->domWindow()->document();
> + cookies = rawCookies(document, document->cookieURL());
You should declare the cookies vector on the same line where you initialize it. But, you should not use Vector for a return value; copying it to return it is very inefficient. Instead you should name this function getRawCookies and use a Vector<Cookie>& as an out parameter.
> + Cookie cookie = cookies[i];
To avoid making an extra copy of each cookie in the vector this can be a "const Cookie&" instead of Cookie.
> + JSObject* cookieObj = constructEmptyObject(exec);
We prefer not to abbreviate, so please say cookieObject.
> + cookieObj->putDirect(Identifier(exec, "name"), jsString(exec, cookie.name()));
> + cookieObj->putDirect(Identifier(exec, "value"), jsString(exec, cookie.value()));
> + cookieObj->putDirect(Identifier(exec, "domain"), jsString(exec, cookie.domain()));
> + cookieObj->putDirect(Identifier(exec, "path"), jsString(exec, cookie.path()));
> + cookieObj->putDirect(Identifier(exec, "expires"), jsString(exec, cookie.expires()));
> + cookieObj->putDirect(Identifier(exec, "httpOnly"), jsBoolean(cookie.isHttpOnly()));
> + cookieObj->putDirect(Identifier(exec, "secure"), jsBoolean(cookie.isSecure()));
> + cookieObj->putDirect(Identifier(exec, "session"), jsBoolean(cookie.isSession()));
This will be relatively slow since it's going to create each of these identifiers from the constant string each time through the loop, which is a hash table lookup every time. It might be good to instead create local variables for each identifier so each will be created exactly once. On the other hand, the jsString operations are relatively costly too, so it's probably not all that important to optimize.
> + result.append( cookieObj );
The spaces around the argument are not the correct style.
> + // return jsNumber(exec, cookies.size());
Oops, left in this commented code!
> + class Cookie {
> + public:
> + Cookie(const String& name, const String& value, const String& domain,
> + const String& path, const String& expires, const bool httpOnly,
> + const bool secure, const bool session)
No need for the "const" in "const bool". Would be nice to indent this so the lines don't exactly line up with the ":". I often use indent by an extra tab stop.
For a pure data holder like this, you might want to consider just using a struct instead of a class with getter functions.
> class KURL;
> class String;
> class Document;
> + class Cookie;
These should be sorted alphabetically. Not sure why Document wasn't already.
> #import "config.h"
> +#import "Cookie.h"
> #import "CookieJar.h"
The new include should be added to the paragraph below, not put above the file's own header. The file's own header always comes first.
> + NSString* name = [cookie name];
> + NSString* value = [cookie value];
> + NSString* domain = [cookie domain];
> + NSString* path = [cookie path];
> + NSString* expires = [[cookie expiresDate] description];
It's a bit strange to put this date into this string form, but I don't have a better suggestion at the moment.
Our coding style is to put the * next to the variable name for Objective-C types like this one.
> + bool httpOnly = [cookie isHTTPOnly];
> + bool secure = [cookie isSecure];
> + bool session = [cookie isSessionOnly];
> + Cookie cookieData = Cookie(name, value, domain, path, expires, httpOnly, secure, session);
> + rawCookies.append(cookieData);
> + }
> +
> + return rawCookies;
You shouldn't need this return statement here. It's OK to fall through to the return outside the block exceptions macro.
This patch will break all platforms other than Mac OS X. You can't land it like that, so you'll need to figure out what to do for those other platforms.

(In reply to comment #3)
> You should declare the cookies vector on the same line where you initialize it.
> But, you should not use Vector for a return value; copying it to return it is
> very inefficient. Instead you should name this function getRawCookies and use a
> Vector<Cookie>& as an out parameter.
Done. Made the Vector an out parameter, but that requires the declaration to again be on another line.
> To avoid making an extra copy of each cookie in the vector this can be a "const
> Cookie&" instead of Cookie.
Done.
> We prefer not to abbreviate, so please say cookieObject.
Done. Noted.
> This will be relatively slow since it's going to create each of these
> identifiers from the constant string each time through the loop, which is a
> hash table lookup every time. It might be good to instead create local
> variables for each identifier so each will be created exactly once. On the
> other hand, the jsString operations are relatively costly too, so it's probably
> not all that important to optimize.
Done. I moved Initializers out and made them const.
> The spaces around the argument are not the correct style.
Done. Including all other style fixes.
> No need for the "const" in "const bool". Would be nice to indent this so the
> lines don't exactly line up with the ":". I often use indent by an extra tab
> stop.
Done.
> For a pure data holder like this, you might want to consider just using a
> struct instead of a class with getter functions.
Done. Made a struct. Including a new field "size" (see below).
> The new include should be added to the paragraph below, not put above the
> file's own header. The file's own header always comes first.
Done. Noted.
> It's a bit strange to put this date into this string form, but I don't have a
> better suggestion at the moment.
Done. Noted.
> Our coding style is to put the * next to the variable name for Objective-C
> types like this one.
Done. Noted.
> This patch will break all platforms other than Mac OS X. You can't land it like
> that, so you'll need to figure out what to do for those other platforms.
Attempted. I searched and identified, hopefully all, other platforms. Most don't really make support for this easy, and I have no way of testing on them at all. What is the best course of action for something like this?
------------
I added another field to the Cookie struct, "size". Firebug's "size" parameter seems to be the (size of the name + size of the value). I decided to put this into the struct, and then make it available in the constructed jsObjects for the inspector.

Oh, one final question. Inside CookieJar.h I include <wtf/Vector.h>. This seems to make it unnecessary to include again in each of the platform specific implementations... but I included them anyways. I could probably do the same for Cookie.h What is the proper way to go about this?
- Include in just the header
- Include in both the header and the implementation
Seeing as it compiled for me without declaring in both places I'm guessing this is a style issue, and I'm guessing certain editors might work better with the declaration in both files.

(In reply to comment #7)
> From mainFrame() you can get to the document directly. No need to go through
> domWindow().
Done. Noted.
> You can use construction syntax instead of assignment for these. Also, no real
> reason to make them const. After all, almost any local variable in most
> programs could be marked const, and for brevity we normally don't do it unless
> there's a special reason to.
Done. This is the first time I've heard of construction syntax. Interesting!
> Making all these data members const isn't really all that good an idea.
Done.
> For best memory use and performance, you will want to do this here:
> rawCookies.reserveCapacity(count);
> Once you've done that change, it is then safe to use the slightly faster
> uncheckedAppend function below inside the loop instead of append.
Done. I had thought of this instead of the clear(), but I realize now that doing both still makes sense.
> Or you could construct right in the append function call:
Done.
> None of my comments are critical, so I'll say r=me on this version, but feel
> free to do a revised version if you like some of the things I propose above.
I don't have commit access so it would be helpful to use the commit-queue. Thanks!
----
Any comments on my header include question from comment #6?

Comment on attachment 34660[details]
Improved Based on Review
> + cookieObject->putDirect(sizeIdentifier, jsNumber(exec, cookie.size));
I'm so sorry I didn't mention this earlier. It seems clear to me that if "size" is just name.length() + value.length() then it should not be stored in a Cookie struct. Instead it should be computed, perhaps in JavaScript code, or perhaps in this function, or perhaps in a function member of the Cookie struct.
Another possibility is that this "name.length() + value.length()" computation is just an estimate that's being used because the cookie API on Mac OS X does not supply the length. In that case, the logic to compute this size estimate should be in the getRawCookies function in CookieJarMac.mm, not in the cross-platform code, and the size should be a constructor argument.
Everything else is perfect, and this is a minor issue.
r=me

(In reply to comment #6)
> Oh, one final question. Inside CookieJar.h I include <wtf/Vector.h>. This
> seems to make it unnecessary to include again in each of the platform specific
> implementations... but I included them anyways. I could probably do the same
> for Cookie.h What is the proper way to go about this?
>
> - Include in just the header
> - Include in both the header and the implementation
It should be included in just the header.

(In reply to comment #10)
> I'm so sorry I didn't mention this earlier. It seems clear to me that if "size"
> is just name.length() + value.length() then it should not be stored in a Cookie
> struct. Instead it should be computed, perhaps in JavaScript code, or perhaps
> in this function, or perhaps in a function member of the Cookie struct.
Done. I moved this to the final point in the chain, inside JSInspectorBackendCustom.cpp. I put it here rather then performing it in the JavaScript so it will hopefully be faster in the C++. It might be useful to throw some ASSERTS() that the name and value are not null, but I'm not entirely clear on WebKit asserts yet. I actually build the Release versions anyways.
(In reply to comment #11)
> It should be included in just the header.
Sounds good. In the latest patch I removed the duplicate includes from all of the implementation files I edited.
----
Thanks for the reviews and help Darin. =)

It probably makes more sense to store the expires as milliseconds since the epoch? Hopefully there is a standard way to calculate this for all platforms and JavaScript can use Date.parse() to convert it into a Date Object. Or, if there is a way to do this in the C++ like jsNumber, jsBoolean etc that would be useful.

(In reply to comment #16)
> (From update of attachment 34692[details])
> > + NSTimeInterval expires = [[cookie expiresDate] timeIntervalSince1970] * 1000;
>
> I'd say double here since this isn't really an NSTimeInterval since it's not in
> the standard units for NSTimeInterval.
I'm not familiar with Obj-C, however the documentation I looked at said that NSTimeInterval was a typedef of a "double". Are you saying because I multiple by 1000 I should change the type to indicate its not really a Time Interval anymore, but should be treated as just a double?
(In reply to comment #14)
> Hopefully there is a standard way to calculate this for all platforms
> and JavaScript can use Date.parse() to convert it into a Date Object. Or, if
> there is a way to do this in the C++ like jsNumber, jsBoolean etc that would be
> useful.
I used jsNumber, although I guess the alternative could have been constructDate
after including <runtime/DateConstructor.h>?

(In reply to comment #17)
> Are you saying because I multiply
> by 1000 I should change the type to indicate its not really a Time Interval
> anymore, but should be treated as just a double?
Yes.
> I used jsNumber, although I guess the alternative could have been constructDate
> after including <runtime/DateConstructor.h>?
That's right. I think Date would be pretty neat for this. But at this point we're in the "tweak forever" mode, so we should get this landed soon!

Okay! If its good to land please flip the commit-queue bit. Or, if you'd rather have me do those last few tweaks let me know.
As a heads up here is what it looks like in the Web Inspector, its what I've been playing with today:
http://bogojoker.com/pub/inspector/cookies.png
This needs a couple of r+ patches to land, including this patch and bug 27202. When all is well I'll start advancing this new stuff.

Created attachment 34713[details]
Merged with ToT
Merged with ToT. There was a merge in the xcodeproj file. Odd, because I only added this to XCode to help others, I don't actually use XCode myself =P.

Did I fail to add the header file to any particular build files?
I took a look at another changeset (http://trac.webkit.org/changeset/47170) where files were added and he specifically added the new files to a couple build files:
- trunk/WebCore/GNUmakefile.am
- trunk/WebCore/WebCore.gypi
Do you think this would solve my problem. Is there a list of build files that I should update whenever I add a new file?

Created attachment 34742[details]
Attempted Build Fix for Windows
aroben made an excellent point in IRC. Even though I change Cookie to a struct, I didn't change the short declaration "class Cookie" to "struct Cookie" inside CookieJar.h. Hopefully this will fix the Windows Build (I noticed someone fixed Chromium shortly after this initially landed).
Let me know about the build files that I should update.

It looks like someone else just included a Haiku build fix for the Haiku port for their CookiesJar implementation, and its better then mine:
https://bugs.webkit.org/show_bug.cgi?id=28253
Hopefully there won't be any conflicts, seeing as his does the same fixes as mine and more. However, his is being applied from a rolled out patch. I think maybe we should cancel his and I'll update mine to include his changes?