DevTools: Implement raw cookies access for inspector.

DevTools: Implement raw cookies access for inspector.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30457

[pfeldman, 2009-10-20 16:35:46]

Here is the sync version with childsecuritypolicy used.

[darin (slow to review), 2009-10-26 17:06:11]

I'm sorry for the delay.  I actually wrote these comments last week, but somehow
failed to submit them :-(

http://codereview.chromium.org/294025/diff/2001/3004
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/294025/diff/2001/3004#newcode518
Line 518: if (!cookie_monster)
nit: alternatively:

  if (!cookie_monster) {
    NOTREACHED();
    return;
  }

that way the condition isn't repeated.

http://codereview.chromium.org/294025/diff/2001/3004#newcode539
Line 539: const std::string& cookieName)
nit: cookie_name

http://codereview.chromium.org/294025/diff/2001/3005
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/294025/diff/2001/3005#newcode127
Line 127: const std::string& cookieName);
nit: cookie_name

http://codereview.chromium.org/294025/diff/2001/3008
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/294025/diff/2001/3008#newcode134
Line 134: for (std::vector<WebCookie>::iterator it = cookies.begin();
the glue version of WebCookie should be in the webkit_glue namespace. 
webkit/glue isn't very consistent about that today, but i'm trying to push it
toward consistency!

if you make this change, then you can add a 'using WebKit::WebCookie' above.

http://codereview.chromium.org/294025/diff/2001/3008#newcode136
Line 136: result[i++] = WebKit::WebCookie(UTF8ToUTF16(it->name),
WebString::fromUTF8 is slightly more efficient (avoids one copy)

http://codereview.chromium.org/294025/diff/2001/3009
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/294025/diff/2001/3009#newcode41
Line 41: virtual bool getRawCookies(const WebKit::WebURL& url,
nit: webkit style is to exclude the "get" prefix.  for this, just calling it
"rawCookies" seems good.  it is similar to the existing "cookies" method.

http://codereview.chromium.org/294025/diff/2001/3011
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/294025/diff/2001/3011#newcode99
Line 99: std::vector<CanonicalCookie>& raw_cookies);
nit: per google style, out params should be pointer types instead of reference
types

http://codereview.chromium.org/294025/diff/2001/3012
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/294025/diff/2001/3012#newcode164
Line 164: virtual bool getRawCookies(const WebURL& url, WebVector<WebCookie>&
rawCookies) = 0;
should there be a policyURL parameter?  for your application, you could just
pass WebURL(), but future applications might like to narrow the set of returned
cookies based on third-party cookie blocking rules.

nit: no need to name the WebVector parameter since it's pretty apparent from the
type what it is.  webkit style encourages leaving off parameter names in such
cases.

[pfeldman, 2009-10-29 15:28:32]

On 2009/10/26 17:06:11, darin wrote:
> I'm sorry for the delay.  I actually wrote these comments last week, but
somehow
> failed to submit them :-(
> 
> http://codereview.chromium.org/294025/diff/2001/3004
> File chrome/browser/renderer_host/resource_message_filter.cc (right):
> 
> http://codereview.chromium.org/294025/diff/2001/3004#newcode518
> Line 518: if (!cookie_monster)
> nit: alternatively:
> 
>   if (!cookie_monster) {
>     NOTREACHED();
>     return;
>   }
> 
> that way the condition isn't repeated.
> 

Done.

> http://codereview.chromium.org/294025/diff/2001/3004#newcode539
> Line 539: const std::string& cookieName)
> nit: cookie_name

Done.

> 
> http://codereview.chromium.org/294025/diff/2001/3005
> File chrome/browser/renderer_host/resource_message_filter.h (right):
> 
> http://codereview.chromium.org/294025/diff/2001/3005#newcode127
> Line 127: const std::string& cookieName);
> nit: cookie_name

Done.

> 
> http://codereview.chromium.org/294025/diff/2001/3008
> File chrome/renderer/renderer_webkitclient_impl.cc (right):
> 
> http://codereview.chromium.org/294025/diff/2001/3008#newcode134
> Line 134: for (std::vector<WebCookie>::iterator it = cookies.begin();
> the glue version of WebCookie should be in the webkit_glue namespace. 
> webkit/glue isn't very consistent about that today, but i'm trying to push it
> toward consistency!
> 
> if you make this change, then you can add a 'using WebKit::WebCookie' above.

Done.

> 
> http://codereview.chromium.org/294025/diff/2001/3008#newcode136
> Line 136: result[i++] = WebKit::WebCookie(UTF8ToUTF16(it->name),
> WebString::fromUTF8 is slightly more efficient (avoids one copy)
> 

Done.

> http://codereview.chromium.org/294025/diff/2001/3009
> File chrome/renderer/renderer_webkitclient_impl.h (right):
> 
> http://codereview.chromium.org/294025/diff/2001/3009#newcode41
> Line 41: virtual bool getRawCookies(const WebKit::WebURL& url,
> nit: webkit style is to exclude the "get" prefix.  for this, just calling it
> "rawCookies" seems good.  it is similar to the existing "cookies" method.
> 

Done.

> http://codereview.chromium.org/294025/diff/2001/3011
> File net/base/cookie_monster.h (right):
> 
> http://codereview.chromium.org/294025/diff/2001/3011#newcode99
> Line 99: std::vector<CanonicalCookie>& raw_cookies);
> nit: per google style, out params should be pointer types instead of reference
> types
> 

Done.

> http://codereview.chromium.org/294025/diff/2001/3012
> File webkit/api/public/WebKitClient.h (right):
> 
> http://codereview.chromium.org/294025/diff/2001/3012#newcode164
> Line 164: virtual bool getRawCookies(const WebURL& url, WebVector<WebCookie>&
> rawCookies) = 0;
> should there be a policyURL parameter?  for your application, you could just
> pass WebURL(), but future applications might like to narrow the set of
returned
> cookies based on third-party cookie blocking rules.
> 

Done.

> nit: no need to name the WebVector parameter since it's pretty apparent from
the
> type what it is.  webkit style encourages leaving off parameter names in such
> cases.

Done.

[yurys, 2009-10-29 16:00:49]

LGTM with comments

http://codereview.chromium.org/294025/diff/11001/11014
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/294025/diff/11001/11014#newcode517
Line 517: DCHECK(cookie_monster);
remove this line

http://codereview.chromium.org/294025/diff/11001/11014#newcode533
Line 533: it->Value(),
wrong parameters indnentation

http://codereview.chromium.org/294025/diff/11001/11014#newcode550
Line 550: if (!cookie_monster)
be consistent:
if (!cookie_monster) {
   NOTREACHED();
   return;
}

[pfeldman, 2009-10-29 16:08:28]

http://codereview.chromium.org/294025/diff/11001/11014
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/294025/diff/11001/11014#newcode517
Line 517: DCHECK(cookie_monster);
On 2009/10/29 16:00:49, Yury Semikhatsky wrote:
> remove this line

Done.

http://codereview.chromium.org/294025/diff/11001/11014#newcode533
Line 533: it->Value(),
On 2009/10/29 16:00:49, Yury Semikhatsky wrote:
> wrong parameters indnentation 

Done.

http://codereview.chromium.org/294025/diff/11001/11014#newcode550
Line 550: if (!cookie_monster)
On 2009/10/29 16:00:49, Yury Semikhatsky wrote:
> be consistent:
> if (!cookie_monster) {
>    NOTREACHED();
>    return;
> } 

Done.