[webkit-reviews] review denied: [Bug 30104] Inspector should show cookies of sub-resources on the page. : [Attachment 40760] Fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 30104: Inspector should show cookies of sub-resources on the page.
https://bugs.webkit.org/show_bug.cgi?id=30104

Attachment 40760: Fix
https://bugs.webkit.org/attachment.cgi?id=40760&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // 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