[webkit-reviews] review granted: [Bug 28185] Inspector: Show Hidden Cookie Data : [Attachment 34634] Fixed Based on Review

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Joseph Pecoraro
<joepeck02 at gmail.com>'s request for review:
Bug 28185: Inspector: Show Hidden Cookie Data
https://bugs.webkit.org/show_bug.cgi?id=28185

Attachment 34634: Fixed Based on Review
https://bugs.webkit.org/attachment.cgi?id=34634&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    Document* document =
ic->inspectedPage()->mainFrame()->domWindow()->document();

>From mainFrame() you can get to the document directly. No need to go through
domWindow().

> +    const Identifier nameIdentifier = Identifier(exec, "name");
> +    const Identifier valueIdentifier = Identifier(exec, "value");
> +    const Identifier domainIdentifier = Identifier(exec, "domain");
> +    const Identifier pathIdentifier = Identifier(exec, "path");
> +    const Identifier expiresIdentifier = Identifier(exec, "expires");
> +    const Identifier sizeIdentifier = Identifier(exec, "size");
> +    const Identifier httpOnlyIdentifier = Identifier(exec, "httpOnly");
> +    const Identifier secureIdentifier = Identifier(exec, "secure");
> +    const Identifier sessionIdentifier = Identifier(exec, "session");

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.

    Identifier nameIdentifier(exec, "name");

and so on.

> +	   const String name;
> +	   const String value;
> +	   const String domain;
> +	   const String path;
> +	   const String expires;
> +	   const bool httpOnly;
> +	   const bool secure;
> +	   const bool session;
> +	   const unsigned size;

Making all these data members const isn't really all that good an idea. If
someone wants to use a Cookie in some non-const way, perhaps modifying one of
these, then declaring it this way would get in the way, for no good reason. On
the other hand, clients who don't want to modify a Cookie object get the same
effect you got here by just saying "const Cookie". I'd leave off all the const
on the individual members.

> +void getRawCookies(const Document*, const KURL& url, Vector<Cookie>&
rawCookies)
> +{
> +    rawCookies.clear();
> +    BEGIN_BLOCK_OBJC_EXCEPTIONS;
> +
> +    NSURL *cookieURL = url;
> +    NSArray *cookies = [[NSHTTPCookieStorage sharedHTTPCookieStorage]
cookiesForURL:cookieURL];
> +
> +    NSUInteger count = [cookies count];

For best memory use and performance, you will want to do this here:

    rawCookies.reserveCapacity(count);

This allocates the memory for the vector rather than sizing it up each time you
hit the limit while appending. It's faster and saves a bit on extra storage.
Once you've done that change, it is then safe to use the slightly faster
uncheckedAppend function below inside the loop instead of append. But even if
you chose not to do uncheckedAppend, reserveCapacity is a good idea.

> +    for (NSUInteger i = 0; i < count; ++i) {
> +	   NSHTTPCookie *cookie = (NSHTTPCookie *)[cookies objectAtIndex:i];
> +	   NSString *name = [cookie name];
> +	   NSString *value = [cookie value];
> +	   NSString *domain = [cookie domain];
> +	   NSString *path = [cookie path];
> +	   NSString *expires = [[cookie expiresDate] description];
> +	   bool httpOnly = [cookie isHTTPOnly];
> +	   bool secure = [cookie isSecure];
> +	   bool session = [cookie isSessionOnly];
> +	   Cookie cookieData = Cookie(name, value, domain, path, expires,
httpOnly, secure, session);

Again could use construction syntax here:

    Cookie cookieData(name, value, domain, path, expires, httpOnly, secure,
session);

> +	   rawCookies.append(cookieData);

Or you could construct right in the append function call:

    rawCookies.uncheckedAppend(Cookie(name, value, domain, path, expires,
httpOnly, secure, session));

Otherwise this looks really great.

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.