[Webkit-unassigned] [Bug 28185] Inspector: Show Hidden Cookie Data

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28185


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34592|review?                     |review-
               Flag|                            |




--- Comment #3 from Darin Adler <darin at apple.com>  2009-08-11 17:54:43 PDT ---
(From update of attachment 34592)
> +    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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.