Cookie commands for the webdriver protocol

Cookie commands for the webdriver protocol

Implements the following URLs for WebDriver:
/session/:sessionId/cookie (GET POST DELETE)
/session/:sessionId/cookie/:name (DELETE)

BUG=none
TEST=none

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

[kkania, 2011-01-28 23:25:59]

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
File chrome/test/webdriver/commands/command.cc (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/command.cc:40: bool
Command::GetDictionaryParameter(const std::string& key,
can we just get rid of these functions and let parameters_ be protected? There
doesn't seem to be much point in them.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
File chrome/test/webdriver/commands/command.h (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/command.h:77: bool GetDictionaryParameter(const
std::string& key,
fix formatting

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:13: void tokenize(const
std::string& s, const char& delimiter,
use Tokenize from base/string_util.h and remove this func

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:41:
SET_WEBDRIVER_ERROR(response, "Failed to init Cookie Command",
didn't WebDriverCommand::Init already set the response when it fails?

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:53:
response->set_status(kUnknownError);
use SET_WEBDRIVER_ERROR here and for all other errors in this file

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:80: if
(GetStringASCIIParameter("cookie", &cookie_string)) {
how come you check for this? looking at the spec, it says that it requires a
JSON cookie

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:108: response->set_value(new
StringValue(cookie->ToString()));
how come you send it back as a string? i don't see a return value listed in the
spec

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:158:
SET_WEBDRIVER_ERROR(response, "Failed to init Named Cookie Command",
don't need to set error here

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
File chrome/test/webdriver/commands/cookie_commands.h (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.h:7: 
add '#pragma once' here

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/cookie.cc
File chrome/test/webdriver/cookie.cc (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/cookie.cc...
chrome/test/webdriver/cookie.cc:109: cookie->SetString("expires",
ASCIIToUTF16(expires_));
it looks like they added this to the spec; its name is 'expiry'

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/cookie.h
File chrome/test/webdriver/cookie.h (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/cookie.h#...
chrome/test/webdriver/cookie.h:7: 
#pragma once

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/webdriver...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/webdriver...
chrome/test/webdriver/webdriver_remote_tests.py:145: navigate(SEARCH)
this file seems to be broken; it doesn't run for me. navigate should be
self.navigate

there are some other errors too

[Joe, 2011-02-03 10:02:05]

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
File chrome/test/webdriver/commands/command.cc (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/command.cc:40: bool
Command::GetDictionaryParameter(const std::string& key,
It was like that in the original version but Jason Leyba set this up with the
refractor.  I think this is actually much cleaner since it doesn't litter the
code with a bunch of if statements.

On 2011/01/28 23:25:59, kkania wrote:
> can we just get rid of these functions and let parameters_ be protected? There
> doesn't seem to be much point in them.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
File chrome/test/webdriver/commands/command.h (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/command.h:77: bool GetDictionaryParameter(const
std::string& key,
On 2011/01/28 23:25:59, kkania wrote:
> fix formatting

Done.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:13: void tokenize(const
std::string& s, const char& delimiter,
On 2011/01/28 23:25:59, kkania wrote:
> use Tokenize from base/string_util.h and remove this func

Done.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:41:
SET_WEBDRIVER_ERROR(response, "Failed to init Cookie Command",
We need to set the message to report that the cookie init failed for detailed
info.

On 2011/01/28 23:25:59, kkania wrote:
> didn't WebDriverCommand::Init already set the response when it fails?

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:53:
response->set_status(kUnknownError);
On 2011/01/28 23:25:59, kkania wrote:
> use SET_WEBDRIVER_ERROR here and for all other errors in this file

Done.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:80: if
(GetStringASCIIParameter("cookie", &cookie_string)) {
Since chromium can also handle a string format I included support for both types

On 2011/01/28 23:25:59, kkania wrote:
> how come you check for this? looking at the spec, it says that it requires a
> JSON cookie

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:108: response->set_value(new
StringValue(cookie->ToString()));
I used the default return of chromium since it was not specified in the spec
On 2011/01/28 23:25:59, kkania wrote:
> how come you send it back as a string? i don't see a return value listed in
the
> spec

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:158:
SET_WEBDRIVER_ERROR(response, "Failed to init Named Cookie Command",
On 2011/01/28 23:25:59, kkania wrote:
> don't need to set error here

Done.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
File chrome/test/webdriver/commands/cookie_commands.h (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.h:7: 
On 2011/01/28 23:25:59, kkania wrote:
> add '#pragma once' here

Done.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/cookie.cc
File chrome/test/webdriver/cookie.cc (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/cookie.cc...
chrome/test/webdriver/cookie.cc:109: cookie->SetString("expires",
ASCIIToUTF16(expires_));
On 2011/01/28 23:25:59, kkania wrote:
> it looks like they added this to the spec; its name is 'expiry'

Done.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/cookie.h
File chrome/test/webdriver/cookie.h (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/cookie.h#...
chrome/test/webdriver/cookie.h:7: 
On 2011/01/28 23:25:59, kkania wrote:
> #pragma once

Done.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/webdriver...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/webdriver...
chrome/test/webdriver/webdriver_remote_tests.py:145: navigate(SEARCH)
I added self. and this file runs for me, what errors do u see
On 2011/01/28 23:25:59, kkania wrote:
> this file seems to be broken; it doesn't run for me. navigate should be
> self.navigate
> 
> there are some other errors too

[John Grabowski, 2011-02-04 02:40:23]

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/command.h (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/command.h:78: DictionaryValue** out) const;
bad indent on this line?

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:13: void tokenize(const
std::string& s, const char& delimiter,
seems like something to put in an anonymous namespace.  In any case, add a
comment describing this function.  In particular, mention "modifies |tokens|"

Reference args which are modified by the function is a pattern actively
discouraged in the style guide.  Are you sure this is better than simply having
a return value of std::vector<std::string> ?

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:17: 
Hmm.  Might it be better to create a static method on class Cooke, called
tokenize, which takes a string and delim, then returns a std::vector<Cookie>?

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:121: tokenize(cookies, ':',
tokens);
On line 58 you tokenize cookes on ;.
Here you tokenize them on ;.
Since not consistent, add comments explaining why they differ.

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.h (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:17: class CookieCommand: public
WebDriverCommand {
Here you have
  class CookieCommand:
later in this file you have
  class NamedCookieCommand :   (space between the name and :)

I like the 2nd one better, but you should maintain consistency with the other
commands (and within the same file).

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/cooki...
File chrome/test/webdriver/cookie.cc (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:108: // Chrome specific additons which are not
apart of the JSON over HTTP spec.
a part

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_remote_tests.py:142: 
excellent

[kkania, 2011-02-04 17:04:44]

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:41:
SET_WEBDRIVER_ERROR(response, "Failed to init Cookie Command",
On 2011/02/03 10:02:05, Joe wrote:
> We need to set the message to report that the cookie init failed for detailed
> info.
> 
> On 2011/01/28 23:25:59, kkania wrote:
> > didn't WebDriverCommand::Init already set the response when it fails?
> 

At this point, WebDriverCommand has already set the error saying that the URL
didn't have a real session id in it. I think that is a much better error to
return to the user than Failed to init Cookie command.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:53:
response->set_status(kUnknownError);
On 2011/02/03 10:02:05, Joe wrote:
> On 2011/01/28 23:25:59, kkania wrote:
> > use SET_WEBDRIVER_ERROR here and for all other errors in this file
> 
> Done.

I don't see SET_WEBDRIVER_ERROR still. Please fix.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:80: if
(GetStringASCIIParameter("cookie", &cookie_string)) {
On 2011/02/03 10:02:05, Joe wrote:
> Since chromium can also handle a string format I included support for both
types
> 
> On 2011/01/28 23:25:59, kkania wrote:
> > how come you check for this? looking at the spec, it says that it requires a
> > JSON cookie
> 

That is a nice feature, but please remove it. Only implement webdriver for now.

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:108: response->set_value(new
StringValue(cookie->ToString()));
On 2011/02/03 10:02:05, Joe wrote:
> I used the default return of chromium since it was not specified in the spec
> On 2011/01/28 23:25:59, kkania wrote:
> > how come you send it back as a string? i don't see a return value listed in
> the
> > spec
> 

What happens if you just leave the value blank?

http://codereview.chromium.org/6330012/diff/1/chrome/test/webdriver/commands/...
chrome/test/webdriver/commands/cookie_commands.cc:158:
SET_WEBDRIVER_ERROR(response, "Failed to init Named Cookie Command",
On 2011/02/03 10:02:05, Joe wrote:
> On 2011/01/28 23:25:59, kkania wrote:
> > don't need to set error here
> 
> Done.

I don't see any change here. Please fix.

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:13: void tokenize(const
std::string& s, const char& delimiter,
hmm, looks like you forgot to fix this on the first pass; I wanted you to remove
this and use Tokenize from base/string_util

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.h (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:5: #pragma once
put this after the #define CHROME_TEST....

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:35: GURL current_url_;
need to include GURL header in this file since you have this here

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:63: std::string cookie_name_;
need to #include <string>

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/cookie.h
File chrome/test/webdriver/cookie.h (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:5: #pragma once
put this right after the #define

[kkania, 2011-02-08 18:35:22]

looking better, hopefully we can get this done by today

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:87: void Automation::GetGURL(GURL* gurl,
I don't see where this is used. Isn't the one above enough?

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:101: std::string* cookies,
correct indents:
void Automation::GetCookies(
    const GURL& gurl,
    std::string* cookies,
    bool* success) {

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:106: void Automation::GetCookieByName(const
GURL& gurl,
also correct this and the rest of the indents

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:46: void GetGURL(GURL* gurl, bool* success);
I don't see this used anywhere. Delete?

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:7: #include
"chrome/test/webdriver/commands/cookie_commands.h"
wrong include order. the format is:
#include "chrome/test/webdriver/commands/cookie_commands.h"

#include <string>
#include <vector>

#include "base/string_util.h"
#include "chrome/test/webdriver/cookie.h"

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:11: #include <vector>
you need to add to your includes:
#include "base/scoped_ptr.h"
#include "base/values.h"
#include "chrome/test/webdriver/commands/response.h"
#include "chrome/test/webdriver/session.h"

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:17: if
(session_->GetURL(&current_url_)) {
I think this is broken. The GetURL function takes a std::string*, not a GURL.
The easiest fix would be to change it to GetGURL and use your new function. Or
you could use std::string instead of GURL

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:25:
SET_WEBDRIVER_ERROR(response, "Failed to init Cookie Command",
You don't need this. Use the error set from WebDriverCommand::Init, which is
more helpful for the user.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:42: Tokenize(cookies,
std::string(";"), &tokens);
std::string is unnecessary here, just do ";"

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:43: ListValue* l = new
ListValue();
usually the only time people accept a one character variable is for looping.
change 'l' to 'cookie_list'

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:64: if
(GetStringASCIIParameter("cookie", &cookie_string)) {
get rid of this and follow the spec, which says that the input cookie will be in
JSON format.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:72: response->set_value(new
StringValue("Cookie key not found"));
Use SET_WEBDRIVER_ERROR

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:80: response->set_value(new
StringValue("Invalid cookie"));
use SET_WEBDRIVER_ERROR

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:86:
response->set_status(kUnableToSetCookie);
use SET_WEBDRIVER_ERROR

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:92: response->set_value(new
StringValue(cookie->ToString()));
the spec does not list a return value

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:100:
response->set_status(kUnknownError);
SET_WEBDRIVER_ERR

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:105: Tokenize(cookies,
std::string(":"), &tokens);
std::string is unnecessary

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:111: LOG(INFO) << "Could not
delete cookie: " << cookie.get_name() << "\n"
use VLOG(1) instead of LOG(INFO)

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:147: void
NamedCookieCommand::ExecuteGet(Response* const response) {
GET isn't listed in the spec for named cookie. Why is it here? If it's not for
webdriver, can you please remove it.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:162:
response->set_status(kUnknownError);
use SET_WEBDRIVER_ERROR

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.h (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:5: #pragma once
this should go after the #define statement

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:11: #include
"googleurl/src/gurl.h"
wrong order, do this:
#include "chrome/test/webdriver/commands/webdriver_command.h"
#include "googleurl/src/gurl.h"

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:14: 
you need to add:
class DictionaryValue;

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:15: namespace webdriver {
you need to add:
class Response;

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:23: CookieCommand(const
std::vector<std::string> path_segments,
don't forget the &

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:41: virtual bool
RequiresValidTab() { return true; }
RequiresValidTab is no longer used; remove

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:51: NamedCookieCommand(const
std::vector<std::string> path_segments,
don't forget the &

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:69: virtual bool
RequiresValidTab() { return true; }
RequiresValidTab is no longer used; remove

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
File chrome/test/webdriver/cookie.cc (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:5: #include <time.h>
this is the wrong include order. See my earlier comment about this or check the
style guide.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:24: if (!pc.IsValid()) {
change to:
valid_ = pc.IsValid();
if (!valid_)
  return;

and drop the valid_ = true at the end

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:63: 
limit use of your newlines in code.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:102: cookie->SetString("name",
ASCIIToUTF16(name_));
I don't think you need the ASCII to UTF16 conversion. We try to do everything in
UTF8

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:109: cookie->SetString("expiry",
ASCIIToUTF16(expires_));
expiry is part of the spec now. put it above this comment.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:118: DictionaryValue* cookie;
use scoped_ptr

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:122:
base::JSONWriter::Write(reinterpret_cast<Value*>(&cookie),
use static_cast

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:138: 
limit use of newlines

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cookie.h
File chrome/test/webdriver/cookie.h (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:5: #pragma once
#pragma goes after the #define

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:9: #include <string>
newline after

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:10: #include "base/values.h"
change this to "class DictionaryValue;" and move the #include "base/values.h"
inside of cookie.cc

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:14: // Class used to conveniently convert cookies
to various formats.
drop 'conveniently'

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:20: DictionaryValue* ToJSON();
change ToJSON -> ToDictionary

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:24: bool IsValid() { return valid_; }
bool IsValid() const {

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:25: const std::string get_name() { return name_;
}
std::string get_name() const {

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:24: #include
"chrome/test/webdriver/utility_functions.h"
add include for googleurl

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:184: &success));
indent 2 more here

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:203: url,
fix indent

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:209: bool Session::GetCookieByName(const GURL&
url, const std::string& cookie_name,
bool Session::GetCookieByName(const GURL& url,
                              const std::string& cookie_name,
                              std::string* cookie)

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.h (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:13: 
add:
class GURL;

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_remote_tests.py:144: def testAddCookie(self):
this is fine, but make sure the webdriver cookie tests work too

[Joe, 2011-02-09 06:32:10]

FYI: You should wait until I send an email for a review since I use patches to
keep a backup and move files from my laptop to my desktop.

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/command.h (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/command.h:78: DictionaryValue** out) const;
On 2011/02/04 02:40:23, John Grabowski wrote:
> bad indent on this line?

Done.

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:13: void tokenize(const
std::string& s, const char& delimiter,
On 2011/02/04 02:40:23, John Grabowski wrote:
> seems like something to put in an anonymous namespace.  In any case, add a
> comment describing this function.  In particular, mention "modifies |tokens|"
> 
> Reference args which are modified by the function is a pattern actively
> discouraged in the style guide.  Are you sure this is better than simply
having
> a return value of std::vector<std::string> ?

Done.

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:13: void tokenize(const
std::string& s, const char& delimiter,
On 2011/02/04 02:40:23, John Grabowski wrote:
> seems like something to put in an anonymous namespace.  In any case, add a
> comment describing this function.  In particular, mention "modifies |tokens|"
> 
> Reference args which are modified by the function is a pattern actively
> discouraged in the style guide.  Are you sure this is better than simply
having
> a return value of std::vector<std::string> ?

Done.

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:17: 
On 2011/02/04 02:40:23, John Grabowski wrote:
> Hmm.  Might it be better to create a static method on class Cooke, called
> tokenize, which takes a string and delim, then returns a std::vector<Cookie>?

Done.

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.h (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:5: #pragma once
If you place it after the ifdef doesn't that defeat the purpose of having it? 
From my understanding it's supposed to be an more efficient version of using
ifdef.

http://en.wikipedia.org/wiki/Pragma_once

On 2011/02/04 17:04:44, kkania wrote:
> put this after the #define CHROME_TEST....

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:17: class CookieCommand: public
WebDriverCommand {
I named the classes after the URLS of the JSON webdriver protocol.  In the cases
of cookies there are actually 2 URLS.
/session/:sessionId/cookie
/session/:sessionId/cookie/:name

On 2011/02/04 02:40:23, John Grabowski wrote:
> Here you have
>   class CookieCommand:
> later in this file you have
>   class NamedCookieCommand :   (space between the name and :)
> 
> I like the 2nd one better, but you should maintain consistency with the other
> commands (and within the same file).

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:35: GURL current_url_;
On 2011/02/04 17:04:44, kkania wrote:
> need to include GURL header in this file since you have this here

Done.

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:63: std::string cookie_name_;
On 2011/02/04 17:04:44, kkania wrote:
> need to #include <string>

Done.

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/cooki...
File chrome/test/webdriver/cookie.cc (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:108: // Chrome specific additons which are not
apart of the JSON over HTTP spec.
On 2011/02/04 02:40:23, John Grabowski wrote:
> a part

Done.

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/cookie.h
File chrome/test/webdriver/cookie.h (right):

http://codereview.chromium.org/6330012/diff/10001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:5: #pragma once
See the pervious comment on pragma
On 2011/02/04 17:04:44, kkania wrote:
> put this right after the #define

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:46: void GetGURL(GURL* gurl, bool* success);
It is used cookie_commands.cc

On 2011/02/08 18:35:22, kkania wrote:
> I don't see this used anywhere. Delete?

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:7: #include
"chrome/test/webdriver/commands/cookie_commands.h"
On 2011/02/08 18:35:22, kkania wrote:
> wrong include order. the format is:
> #include "chrome/test/webdriver/commands/cookie_commands.h"
> 
> #include <string>
> #include <vector>
> 
> #include "base/string_util.h"
> #include "chrome/test/webdriver/cookie.h"

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:11: #include <vector>
On 2011/02/08 18:35:22, kkania wrote:
> you need to add to your includes:
> #include "base/scoped_ptr.h"
> #include "base/values.h"
> #include "chrome/test/webdriver/commands/response.h"
> #include "chrome/test/webdriver/session.h"

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:17: if
(session_->GetURL(&current_url_)) {
It's not broken it uses void GetGURL(GURL* gurl, bool* success); in
automation.cc

On 2011/02/08 18:35:22, kkania wrote:
> I think this is broken. The GetURL function takes a std::string*, not a GURL.
> The easiest fix would be to change it to GetGURL and use your new function. Or
> you could use std::string instead of GURL

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:25:
SET_WEBDRIVER_ERROR(response, "Failed to init Cookie Command",
On 2011/02/08 18:35:22, kkania wrote:
> You don't need this. Use the error set from WebDriverCommand::Init, which is
> more helpful for the user.

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:42: Tokenize(cookies,
std::string(";"), &tokens);
On 2011/02/08 18:35:22, kkania wrote:
> std::string is unnecessary here, just do ";"

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:43: ListValue* l = new
ListValue();
On 2011/02/08 18:35:22, kkania wrote:
> usually the only time people accept a one character variable is for looping.
> change 'l' to 'cookie_list'

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:64: if
(GetStringASCIIParameter("cookie", &cookie_string)) {
It does i just support one more format since it needs to be comverted to a
string for chrome
On 2011/02/08 18:35:22, kkania wrote:
> get rid of this and follow the spec, which says that the input cookie will be
in
> JSON format.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:72: response->set_value(new
StringValue("Cookie key not found"));
On 2011/02/08 18:35:22, kkania wrote:
> Use SET_WEBDRIVER_ERROR

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:80: response->set_value(new
StringValue("Invalid cookie"));
On 2011/02/08 18:35:22, kkania wrote:
> use SET_WEBDRIVER_ERROR

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:86:
response->set_status(kUnableToSetCookie);
On 2011/02/08 18:35:22, kkania wrote:
> use SET_WEBDRIVER_ERROR

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:92: response->set_value(new
StringValue(cookie->ToString()));
I set the value for my own debugging, it's ignored in the drivers
On 2011/02/08 18:35:22, kkania wrote:
> the spec does not list a return value

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:100:
response->set_status(kUnknownError);
On 2011/02/08 18:35:22, kkania wrote:
> SET_WEBDRIVER_ERR

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:105: Tokenize(cookies,
std::string(":"), &tokens);
On 2011/02/08 18:35:22, kkania wrote:
> std::string is unnecessary

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:111: LOG(INFO) << "Could not
delete cookie: " << cookie.get_name() << "\n"
On 2011/02/08 18:35:22, kkania wrote:
> use VLOG(1) instead of LOG(INFO)

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:147: void
NamedCookieCommand::ExecuteGet(Response* const response) {
I added it for completeness and would like to also change the spec for
webdriver.  It affects nothing while it's here.
On 2011/02/08 18:35:22, kkania wrote:
> GET isn't listed in the spec for named cookie. Why is it here? If it's not for
> webdriver, can you please remove it.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:162:
response->set_status(kUnknownError);
On 2011/02/08 18:35:22, kkania wrote:
> use SET_WEBDRIVER_ERROR

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.h (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:5: #pragma once
On 2011/02/08 18:35:22, kkania wrote:
> this should go after the #define statement

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:11: #include
"googleurl/src/gurl.h"
On 2011/02/08 18:35:22, kkania wrote:
> wrong order, do this:
> #include "chrome/test/webdriver/commands/webdriver_command.h"
> #include "googleurl/src/gurl.h"

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:15: namespace webdriver {
On 2011/02/08 18:35:22, kkania wrote:
> you need to add:
> class Response;

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:23: CookieCommand(const
std::vector<std::string> path_segments,
This would break because it doesn't match the base class

On 2011/02/08 18:35:22, kkania wrote:
> don't forget the &

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:41: virtual bool
RequiresValidTab() { return true; }
On 2011/02/08 18:35:22, kkania wrote:
> RequiresValidTab is no longer used; remove

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:51: NamedCookieCommand(const
std::vector<std::string> path_segments,
This would break because it doesn't match the base class
On 2011/02/08 18:35:22, kkania wrote:
> don't forget the &

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:69: virtual bool
RequiresValidTab() { return true; }
On 2011/02/08 18:35:22, kkania wrote:
> RequiresValidTab is no longer used; remove

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
File chrome/test/webdriver/cookie.cc (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:5: #include <time.h>
On 2011/02/08 18:35:22, kkania wrote:
> this is the wrong include order. See my earlier comment about this or check
the
> style guide.

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:24: if (!pc.IsValid()) {
On 2011/02/08 18:35:22, kkania wrote:
> change to:
> valid_ = pc.IsValid();
> if (!valid_)
>   return;
> 
> and drop the valid_ = true at the end

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:63: 
On 2011/02/08 18:35:22, kkania wrote:
> limit use of your newlines in code.

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:102: cookie->SetString("name",
ASCIIToUTF16(name_));
On 2011/02/08 18:35:22, kkania wrote:
> I don't think you need the ASCII to UTF16 conversion. We try to do everything
in
> UTF8

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:109: cookie->SetString("expiry",
ASCIIToUTF16(expires_));
On 2011/02/08 18:35:22, kkania wrote:
> expiry is part of the spec now. put it above this comment.

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:118: DictionaryValue* cookie;
On 2011/02/08 18:35:22, kkania wrote:
> use scoped_ptr

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:122:
base::JSONWriter::Write(reinterpret_cast<Value*>(&cookie),
That's not possible
On 2011/02/08 18:35:22, kkania wrote:
> use static_cast

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cookie.h
File chrome/test/webdriver/cookie.h (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:5: #pragma once
On 2011/02/08 18:35:22, kkania wrote:
> #pragma goes after the #define

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:9: #include <string>
On 2011/02/08 18:35:22, kkania wrote:
> newline after

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:10: #include "base/values.h"
On 2011/02/08 18:35:22, kkania wrote:
> change this to "class DictionaryValue;" and move the #include "base/values.h"
> inside of cookie.cc

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:14: // Class used to conveniently convert cookies
to various formats.
On 2011/02/08 18:35:22, kkania wrote:
> drop 'conveniently'

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:20: DictionaryValue* ToJSON();
On 2011/02/08 18:35:22, kkania wrote:
> change ToJSON -> ToDictionary

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:20: DictionaryValue* ToJSON();
On 2011/02/08 18:35:22, kkania wrote:
> change ToJSON -> ToDictionary

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:24: bool IsValid() { return valid_; }
On 2011/02/08 18:35:22, kkania wrote:
> bool IsValid() const {

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:25: const std::string get_name() { return name_;
}
On 2011/02/08 18:35:22, kkania wrote:
> std::string get_name() const {

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:25: const std::string get_name() { return name_;
}
On 2011/02/08 18:35:22, kkania wrote:
> std::string get_name() const {

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:184: &success));
On 2011/02/08 18:35:22, kkania wrote:
> indent 2 more here

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:203: url,
On 2011/02/08 18:35:22, kkania wrote:
> fix indent

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:209: bool Session::GetCookieByName(const GURL&
url, const std::string& cookie_name,
On 2011/02/08 18:35:22, kkania wrote:
> bool Session::GetCookieByName(const GURL& url,
>                               const std::string& cookie_name,
>                               std::string* cookie)

Done.

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/6330012/diff/15001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_remote_tests.py:144: def testAddCookie(self):
OK
On 2011/02/08 18:35:22, kkania wrote:
> this is fine, but make sure the webdriver cookie tests work too

[kkania, 2011-02-09 16:53:13]

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:88: bool* success) {
the indents in this file are still not fixed. fix.

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:15: #include "chrome/test/ui/ui_test.h"
add "class GURL;"

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:3: // found in the LICENSE
file.
add newline here

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:12: 
delete newline here

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:39:
response->set_status(kUnknownError);
use SET_WEBDRIVER_ERROR

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:66: if
(GetStringASCIIParameter("cookie", &cookie_string)) {
i don't understand your reasoning for keeping this. come talk to me

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:70: if
(GetDictionaryParameter(std::string("cookie"), &cookie_dict)) {
unnecessary std::string here

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:82:
SET_WEBDRIVER_ERROR(response, "Invalid cookie",
indent messed up here and below

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:114: << "Contents of cookie: "
<< cookie.ToString();
indent messed up here

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:139:
SET_WEBDRIVER_ERROR(response, "No cookie name specified",
i think this needs to be surrounded by:
if (cookie_name_.empty()) {

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:144:
SET_WEBDRIVER_ERROR(response, "Failed to init Named Cookie Command",
remove this

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:153:
SET_WEBDRIVER_ERROR(response, "Failed to fetch cookie",
fix indent

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:164:
SET_WEBDRIVER_ERROR(response, "Failed to delete cookie",
fix indent

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.h (right):

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:7: 
delete newline here

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:13: 
delete newline here

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/webdriver_command.h (right):

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webdriver_command.h:11: #include
"chrome/test/webdriver/session_manager.h"
move this include to the .cc

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webdriver_command.h:13: 
add "class DictionaryValue;"

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webdriver_command.h:15: 
add "class Response;"
add "class Session;"

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/cooki...
File chrome/test/webdriver/cookie.cc (right):

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:13: 
all includes referring to headers in the chrome tree (except for the .h
associated with the .cc) go in one block, alphabetized, with no newlines

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:14: #include "chrome/test/webdriver/cookie.h"
This goes at the top of the file. Please familiarize yourself with the c++
include style
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...).
I am getting tired of commenting on this.

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.cc:115:
base::JSONWriter::Write(reinterpret_cast<Value*>(&cookie),
actually, you should be able to change "reinterpret_cast<Value*>(&cookie)" to
just "cookie.get()"

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/serve...
File chrome/test/webdriver/server.cc (right):

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/serve...
chrome/test/webdriver/server.cc:28: 
no newline here

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:20: 
no newline here

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:26: 
no newline here

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:181: automation_.get(),
4 spaces not 2 here

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:237: automation_.get(),
4 spaces not 2 here

[John Grabowski, 2011-02-14 21:59:06]

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:128: *success = tab_->SetCookie(gurl,
cookie);
You probably also need a

Automation::SetCookie(const GURL& gurl, bool scout, const std::string& cookies)
{ }

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:47: void GetCookies(const GURL& gurl,
std::string* cookies, bool* success);
Why don't you like return values?
Seems you could have all these methods return the success bool.

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/command.h (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/command.h:77: // Returns the command parameter
with the given |key| as a Dictionary.
Returns --> Provides, or "Get" (so it's not confused with return value)

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.h (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:24: class CookieCommand: public
WebDriverCommand {
classname: and classname<space>: are used in this file.  Choose one or the
other.
I prefer the 2nd one but consistency with the rest of the webdriver code is
better if you generally use the 1st one.

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/cookie.h
File chrome/test/webdriver/cookie.h (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:22: std::string ToJSONString();
Like.
Can you give a brief example in comments as to what a string and json string
look like when converted?  Also, give a brief example of input format string and
dict in comments.

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_remote_tests.py:144: class
testCookies(RemoteWebDriverTest):
like all this

[kkania, 2011-02-14 22:47:08]

a few comments on John's comments

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:47: void GetCookies(const GURL& gurl,
std::string* cookies, bool* success);
On 2011/02/14 21:59:06, John Grabowski wrote:
> Why don't you like return values?
> Seems you could have all these methods return the success bool.  

We don't return bool because these methods are called by posting a
NewRunnableMethod task on the session thread. See session.cc. Alternative:

Session::GetCookies(...) {
  session_thread->PostTask(NewRunnableMethod(&Session::GetCookiesOnSThread))
}

Session::GetCookiesOnSThread(bool* success) {
*success = automation->GetCookies(...)
}

bool Automation::GetCookies(...) { ... }

I do not think it is worthwhile to adopt this alternative, as it requires
writing an extra wrapper in Session. Please let me know if you think otherwise.

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.h (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.h:24: class CookieCommand: public
WebDriverCommand {
On 2011/02/14 21:59:06, John Grabowski wrote:
> classname: and classname<space>: are used in this file.  Choose one or the
> other.
> I prefer the 2nd one but consistency with the rest of the webdriver code is
> better if you generally use the 1st one.
> 

Use the second one (class CookieCommand : public ...). All my new files have
been using this. We'll fix the rest in a separate CL

[John Grabowski, 2011-02-14 22:57:09]

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.h (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.h:47: void GetCookies(const GURL& gurl,
std::string* cookies, bool* success);
On 2011/02/14 22:47:08, kkania wrote:
> On 2011/02/14 21:59:06, John Grabowski wrote:
> > Why don't you like return values?
> > Seems you could have all these methods return the success bool.  
> 
> We don't return bool because these methods are called by posting a
> NewRunnableMethod task on the session thread. See session.cc. Alternative:
> 
> Session::GetCookies(...) {
>   session_thread->PostTask(NewRunnableMethod(&Session::GetCookiesOnSThread))
> }
> 
> Session::GetCookiesOnSThread(bool* success) {
> *success = automation->GetCookies(...)
> }
> 
> bool Automation::GetCookies(...) { ... }
> 
> I do not think it is worthwhile to adopt this alternative, as it requires
> writing an extra wrapper in Session. Please let me know if you think
otherwise.

Seems completely reasonable.
How about a comment at the class declaration stating as such?

[Joe, 2011-02-15 02:30:48]

Please review on more time

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/webdriver_command.h (right):

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webdriver_command.h:11: #include
"chrome/test/webdriver/session_manager.h"
On 2011/02/09 16:53:13, kkania wrote:
> move this include to the .cc

Done.

http://codereview.chromium.org/6330012/diff/31015/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/webdriver_command.h:13: 
On 2011/02/09 16:53:13, kkania wrote:
> add "class DictionaryValue;"

Done.

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/autom...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:128: *success = tab_->SetCookie(gurl,
cookie);
I am not sure why?  I don't think chromedriver needs that case since the current
functions implement what we need.

On 2011/02/14 21:59:06, John Grabowski wrote:
> You probably also need a
> 
> Automation::SetCookie(const GURL& gurl, bool scout, const std::string&
cookies)
> { }

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/command.h (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/command.h:77: // Returns the command parameter
with the given |key| as a Dictionary.
On 2011/02/14 21:59:06, John Grabowski wrote:
> Returns --> Provides, or "Get" (so it's not confused with return value)

Done.

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/command.h:77: // Returns the command parameter
with the given |key| as a Dictionary.
On 2011/02/14 21:59:06, John Grabowski wrote:
> Returns --> Provides, or "Get" (so it's not confused with return value)

Done.

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/cookie.h
File chrome/test/webdriver/cookie.h (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:22: std::string ToJSONString();
On 2011/02/14 21:59:06, John Grabowski wrote:
> Like.
> Can you give a brief example in comments as to what a string and json string
> look like when converted?  Also, give a brief example of input format string
and
> dict in comments.

Done.

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_remote_tests.py (right):

http://codereview.chromium.org/6330012/diff/33018/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_remote_tests.py:144: class
testCookies(RemoteWebDriverTest):
FYI It''s still here I just moved it to a new section

On 2011/02/14 21:59:06, John Grabowski wrote:
> like all this

[kkania, 2011-02-15 03:32:50]

LGTM

[John Grabowski, 2011-02-15 03:39:56]

LGTM if you add the comments suggested here

http://codereview.chromium.org/6330012/diff/34057/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/6330012/diff/34057/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:45: Tokenize(cookies, ";",
&tokens);
Would still like a comment explaining why sometimes we tokenize on ; and
sometimes on :

http://codereview.chromium.org/6330012/diff/34057/chrome/test/webdriver/cookie.h
File chrome/test/webdriver/cookie.h (right):

http://codereview.chromium.org/6330012/diff/34057/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:22: // ToJSONString returns a string form of a
JSON object with the required
ToJSONString() returns a ...

http://codereview.chromium.org/6330012/diff/34057/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:23: // WebDriver tags.  Example { "name"="foo",
"value"="bar"}
Can you use a valid Cookie as an example?  Or document that these are not the
same as browser cookies?

[Joe, 2011-02-15 21:39:26]

http://codereview.chromium.org/6330012/diff/34057/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/cookie_commands.cc (right):

http://codereview.chromium.org/6330012/diff/34057/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/cookie_commands.cc:45: Tokenize(cookies, ";",
&tokens);
On 2011/02/15 03:39:56, John Grabowski wrote:
> Would still like a comment explaining why sometimes we tokenize on ; and
> sometimes on :

Done.

http://codereview.chromium.org/6330012/diff/34057/chrome/test/webdriver/cookie.h
File chrome/test/webdriver/cookie.h (right):

http://codereview.chromium.org/6330012/diff/34057/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:22: // ToJSONString returns a string form of a
JSON object with the required
On 2011/02/15 03:39:56, John Grabowski wrote:
> ToJSONString() returns a ...

Done.

http://codereview.chromium.org/6330012/diff/34057/chrome/test/webdriver/cooki...
chrome/test/webdriver/cookie.h:23: // WebDriver tags.  Example { "name"="foo",
"value"="bar"}
The cookie is valid, it's a name value pair which is the only required arguments
for a cookie

On 2011/02/15 03:39:56, John Grabowski wrote:
> Can you use a valid Cookie as an example?  Or document that these are not the
> same as browser cookies?
>