[webkit-reviews] review denied: [Bug 28185] Inspector: Show Hidden Cookie Data : [Attachment 34592] Working on OS X Leopard

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 11 17:54:43 PDT 2009

Darin Adler <darin at apple.com> has denied Joseph Pecoraro
<joepeck02 at gmail.com>'s request for review:
Bug 28185: Inspector: Show Hidden Cookie Data

Attachment 34592: Working on OS X Leopard

------- Additional Comments from Darin Adler <darin at apple.com>
> +    Vector<Cookie> cookies;
> +    Document* 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,
> +	   cookieObj->putDirect(Identifier(exec, "value"), jsString(exec,
> +	   cookieObj->putDirect(Identifier(exec, "domain"), jsString(exec,
> +	   cookieObj->putDirect(Identifier(exec, "path"), jsString(exec,
> +	   cookieObj->putDirect(Identifier(exec, "expires"), jsString(exec,
> +	   cookieObj->putDirect(Identifier(exec, "httpOnly"),
> +	   cookieObj->putDirect(Identifier(exec, "secure"),
> +	   cookieObj->putDirect(Identifier(exec, "session"),

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&
> +	       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

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.

More information about the webkit-reviews mailing list