Expose cookie get/set to pyauto

Expose cookie get/set to pyauto

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

[Alyssa, 2010-07-12 23:35:32]

Functionality was already in AutomationProvider - just exposed it to PyAuto.

[Nirnimesh, 2010-07-13 00:00:44]

Please update description to something like: "Expose cookie get/set to pyauto"

http://codereview.chromium.org/2903008/diff/2001/3002
File chrome/test/functional/cookies.py (right):

http://codereview.chromium.org/2903008/diff/2001/3002#newcode13
chrome/test/functional/cookies.py:13: """Tests that getting/setting cookies
works correctly."""
"Tests for Cookies"

There can be other tests as well

http://codereview.chromium.org/2903008/diff/2001/3003
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/2001/3003#newcode304
chrome/test/pyautolib/pyautolib.cc:304: scoped_refptr<TabProxy>
tab_proxy(GetActiveTab());
Do not use GetActiveTab(). Please take window index and tab index from the user.

http://codereview.chromium.org/2903008/diff/2001/3003#newcode313
chrome/test/pyautolib/pyautolib.cc:313: scoped_refptr<TabProxy>
tab_proxy(GetActiveTab());
ditto

[Paweł Hajdan Jr., 2010-07-13 03:34:07]

Drive-by with minor automation comments. No need to wait for another review by
me.

http://codereview.chromium.org/2903008/diff/8001/6004
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/8001/6004#newcode306
chrome/test/pyautolib/pyautolib.cc:306: scoped_refptr<BrowserProxy>
browser_proxy = GetBrowserWindow(window_index);
You need to NULL-check browser_proxy to avoid a crash.

http://codereview.chromium.org/2903008/diff/8001/6004#newcode318
chrome/test/pyautolib/pyautolib.cc:318: scoped_refptr<BrowserProxy>
browser_proxy = GetBrowserWindow(window_index);
Same here.

[Alyssa, 2010-07-13 18:14:46]

http://codereview.chromium.org/2903008/diff/2001/3002
File chrome/test/functional/cookies.py (right):

http://codereview.chromium.org/2903008/diff/2001/3002#newcode13
chrome/test/functional/cookies.py:13: """Tests that getting/setting cookies
works correctly."""
On 2010/07/13 00:00:46, Nirnimesh wrote:
> "Tests for Cookies"
> 
> There can be other tests as well

Done.

http://codereview.chromium.org/2903008/diff/2001/3003
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/2001/3003#newcode304
chrome/test/pyautolib/pyautolib.cc:304: scoped_refptr<TabProxy>
tab_proxy(GetActiveTab());
On 2010/07/13 00:00:46, Nirnimesh wrote:
> Do not use GetActiveTab(). Please take window index and tab index from the
user.

Done.

http://codereview.chromium.org/2903008/diff/2001/3003#newcode313
chrome/test/pyautolib/pyautolib.cc:313: scoped_refptr<TabProxy>
tab_proxy(GetActiveTab());
On 2010/07/13 00:00:46, Nirnimesh wrote:
> ditto

Done.

http://codereview.chromium.org/2903008/diff/8001/6004#newcode306
chrome/test/pyautolib/pyautolib.cc:306: if (!tab_proxy.get())
On 2010/07/13 03:34:07, Paweł Hajdan Jr. wrote:
> You need to NULL-check browser_proxy to avoid a crash.

Done.

http://codereview.chromium.org/2903008/diff/8001/6004#newcode318
chrome/test/pyautolib/pyautolib.cc:318: return cookie_val;
On 2010/07/13 03:34:07, Paweł Hajdan Jr. wrote:
> Same here.

Done.

[Nirnimesh, 2010-07-13 18:53:14]

http://codereview.chromium.org/2903008/diff/11001/12004
File chrome/test/pyautolib/pyautolib.h (right):

http://codereview.chromium.org/2903008/diff/11001/12004#newcode158
chrome/test/pyautolib/pyautolib.h:158: int tab_index);
move all args to this line, or align 'int tab_index' like line 136 above.

It's good to give default args (=0) to window_index, and tab_index. Default args
are explicitly permitted in this file. See comment at the top of this file.

http://codereview.chromium.org/2903008/diff/11001/12005
File chrome/test/pyautolib/pyautolib.i (right):

http://codereview.chromium.org/2903008/diff/11001/12005#newcode316
chrome/test/pyautolib/pyautolib.i:316: %feature("docstring", "Set the value of
the cookie at the given url to the "
Mention about the window/tab index args

http://codereview.chromium.org/2903008/diff/11001/12005#newcode320
chrome/test/pyautolib/pyautolib.i:320: int window_index, int tab_index);
fix alignment

http://codereview.chromium.org/2903008/diff/11001/12005#newcode323
chrome/test/pyautolib/pyautolib.i:323: "Returns empty string on error or if
there is no value for the "
mention about the default window/tab index args

http://codereview.chromium.org/2903008/diff/11001/12005#newcode326
chrome/test/pyautolib/pyautolib.i:326: int tab_index);
fix alignment

[Alyssa, 2010-07-13 19:07:02]

http://codereview.chromium.org/2903008/diff/11001/12004
File chrome/test/pyautolib/pyautolib.h (right):

http://codereview.chromium.org/2903008/diff/11001/12004#newcode158
chrome/test/pyautolib/pyautolib.h:158: int tab_index);
On 2010/07/13 18:53:14, Nirnimesh wrote:
> move all args to this line, or align 'int tab_index' like line 136 above.
> 
> It's good to give default args (=0) to window_index, and tab_index. Default
args
> are explicitly permitted in this file. See comment at the top of this file.

Done.

http://codereview.chromium.org/2903008/diff/11001/12005
File chrome/test/pyautolib/pyautolib.i (right):

http://codereview.chromium.org/2903008/diff/11001/12005#newcode316
chrome/test/pyautolib/pyautolib.i:316: %feature("docstring", "Set the value of
the cookie at the given url to the "
On 2010/07/13 18:53:14, Nirnimesh wrote:
> Mention about the window/tab index args

Done.

http://codereview.chromium.org/2903008/diff/11001/12005#newcode320
chrome/test/pyautolib/pyautolib.i:320: int window_index, int tab_index);
On 2010/07/13 18:53:14, Nirnimesh wrote:
> fix alignment

Done.

http://codereview.chromium.org/2903008/diff/11001/12005#newcode323
chrome/test/pyautolib/pyautolib.i:323: "Returns empty string on error or if
there is no value for the "
On 2010/07/13 18:53:14, Nirnimesh wrote:
> mention about the default window/tab index args

Done.

http://codereview.chromium.org/2903008/diff/11001/12005#newcode326
chrome/test/pyautolib/pyautolib.i:326: int tab_index);
On 2010/07/13 18:53:14, Nirnimesh wrote:
> fix alignment

Done.

[Nirnimesh, 2010-07-13 19:12:00]

LGTM

http://codereview.chromium.org/2903008/diff/22001/1010
File chrome/test/pyautolib/pyautolib.i (right):

http://codereview.chromium.org/2903008/diff/22001/1010#newcode319
chrome/test/pyautolib/pyautolib.i:319: bool SetCookie(const GURL& cookie_url,
const std::string& value,
You forgot the default args here

http://codereview.chromium.org/2903008/diff/22001/1010#newcode326
chrome/test/pyautolib/pyautolib.i:326: std::string GetCookie(const GURL&
cookie_url, int window_index,
and here

[Paweł Hajdan Jr., 2010-07-13 19:58:29]

http://codereview.chromium.org/2903008/diff/22001/1008
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/22001/1008#newcode307
chrome/test/pyautolib/pyautolib.cc:307: if (!browser_proxy.get())
nit: Shouldn't you EXPECT_TRUE(browser_proxy) above this final NULL-check?

http://codereview.chromium.org/2903008/diff/22001/1008#newcode321
chrome/test/pyautolib/pyautolib.cc:321: if (!browser_proxy.get())
nit: Same here.

http://codereview.chromium.org/2903008/diff/22001/1008#newcode322
chrome/test/pyautolib/pyautolib.cc:322: return cookie_val;
Can we have a bool return code and return the cookie value via an out parameter
to avoid a possible confusion? Currently it's impossible to differentiate
between an empty cookie value and failure case.

[Nirnimesh, 2010-07-13 20:14:33]

http://codereview.chromium.org/2903008/diff/22001/1008
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/22001/1008#newcode322
chrome/test/pyautolib/pyautolib.cc:322: return cookie_val;
On 2010/07/13 19:58:29, Paweł Hajdan Jr. wrote:
> Can we have a bool return code and return the cookie value via an out
parameter
> to avoid a possible confusion? Currently it's impossible to differentiate
> between an empty cookie value and failure case.

Pawel, this is supposed to be called from python, where the use case is to call
the function and have it return the cookie (in line with some of the other
functions here). It's difficult to fit the return-by-out-param analogy.

[Alyssa, 2010-07-14 00:51:56]

http://codereview.chromium.org/2903008/diff/22001/1008
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/22001/1008#newcode307
chrome/test/pyautolib/pyautolib.cc:307: if (!browser_proxy.get())
On 2010/07/13 19:58:29, Paweł Hajdan Jr. wrote:
> nit: Shouldn't you EXPECT_TRUE(browser_proxy) above this final NULL-check?

Done.

http://codereview.chromium.org/2903008/diff/22001/1008#newcode321
chrome/test/pyautolib/pyautolib.cc:321: if (!browser_proxy.get())
On 2010/07/13 19:58:29, Paweł Hajdan Jr. wrote:
> nit: Same here.

Done.

http://codereview.chromium.org/2903008/diff/22001/1008#newcode322
chrome/test/pyautolib/pyautolib.cc:322: return cookie_val;
On 2010/07/13 20:14:33, Nirnimesh wrote:
> On 2010/07/13 19:58:29, Paweł Hajdan Jr. wrote:
> > Can we have a bool return code and return the cookie value via an out
> parameter
> > to avoid a possible confusion? Currently it's impossible to differentiate
> > between an empty cookie value and failure case.
> 
> Pawel, this is supposed to be called from python, where the use case is to
call
> the function and have it return the cookie (in line with some of the other
> functions here). It's difficult to fit the return-by-out-param analogy.
Is there a standard "error string" I should initialize to, or is it okay for it
to return empty in either case?

http://codereview.chromium.org/2903008/diff/22001/1010
File chrome/test/pyautolib/pyautolib.i (right):

http://codereview.chromium.org/2903008/diff/22001/1010#newcode319
chrome/test/pyautolib/pyautolib.i:319: bool SetCookie(const GURL& cookie_url,
const std::string& value,
On 2010/07/13 19:12:00, Nirnimesh wrote:
> You forgot the default args here

Done.

http://codereview.chromium.org/2903008/diff/22001/1010#newcode326
chrome/test/pyautolib/pyautolib.i:326: std::string GetCookie(const GURL&
cookie_url, int window_index,
On 2010/07/13 19:12:00, Nirnimesh wrote:
> and here

Done.

[Paweł Hajdan Jr., 2010-07-14 00:55:33]

http://codereview.chromium.org/2903008/diff/22001/1008
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/22001/1008#newcode322
chrome/test/pyautolib/pyautolib.cc:322: return cookie_val;
On 2010/07/13 20:14:33, Nirnimesh wrote:
> On 2010/07/13 19:58:29, Paweł Hajdan Jr. wrote:
> > Can we have a bool return code and return the cookie value via an out
> parameter
> > to avoid a possible confusion? Currently it's impossible to differentiate
> > between an empty cookie value and failure case.
> 
> Pawel, this is supposed to be called from python, where the use case is to
call
> the function and have it return the cookie (in line with some of the other
> functions here). It's difficult to fit the return-by-out-param analogy.

Would it be hard to return a tuple (success, value) or something like that? I'm
just not familiar with the C++ to python bindings we use.

[Nirnimesh, 2010-07-14 01:16:57]

http://codereview.chromium.org/2903008/diff/22001/1008
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/22001/1008#newcode322
chrome/test/pyautolib/pyautolib.cc:322: return cookie_val;
On 2010/07/14 00:55:33, Paweł Hajdan Jr. wrote:
> On 2010/07/13 20:14:33, Nirnimesh wrote:
> > On 2010/07/13 19:58:29, Paweł Hajdan Jr. wrote:
> > > Can we have a bool return code and return the cookie value via an out
> > parameter
> > > to avoid a possible confusion? Currently it's impossible to differentiate
> > > between an empty cookie value and failure case.
> > 
> > Pawel, this is supposed to be called from python, where the use case is to
> call
> > the function and have it return the cookie (in line with some of the other
> > functions here). It's difficult to fit the return-by-out-param analogy.
> 
> Would it be hard to return a tuple (success, value) or something like that?
I'm
> just not familiar with the C++ to python bindings we use.

It's possible to return a tuple, but is definitely not straight-forward.
Besides, it breaks the pattern followed by  the other hooks in this file, and
also the pattern a python user expects it in.

Ideally, a python exception should be raised in the case of an automation error,
which is what we do with the new hooks on the json interface.

[Paweł Hajdan Jr., 2010-07-14 01:19:39]

http://codereview.chromium.org/2903008/diff/22001/1008
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/22001/1008#newcode322
chrome/test/pyautolib/pyautolib.cc:322: return cookie_val;
On 2010/07/14 01:16:59, Nirnimesh wrote:
> Ideally, a python exception should be raised in the case of an automation
error,
> which is what we do with the new hooks on the json interface.

I see. If possible, please make it throw a python exception. Otherwise, please
just add a TODO to make it unambiguously report the error condition.

[Nirnimesh, 2010-07-14 01:37:24]

http://codereview.chromium.org/2903008/diff/22001/1008
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/22001/1008#newcode322
chrome/test/pyautolib/pyautolib.cc:322: return cookie_val;
On 2010/07/14 01:19:39, Paweł Hajdan Jr. wrote:
> On 2010/07/14 01:16:59, Nirnimesh wrote:
> > Ideally, a python exception should be raised in the case of an automation
> error,
> > which is what we do with the new hooks on the json interface.
> 
> I see. If possible, please make it throw a python exception. Otherwise, please
> just add a TODO to make it unambiguously report the error condition.

I'd like to bridge this glue such that EXPECT_TRUE/ASSERT_TRUE raises a
corresponding exception in python, but I've not been able to figure out how this
can be done with swig, without making it unbearably ugly and unusable. Sorry!

[Alyssa, 2010-07-14 17:52:08]

I added a TODO. Pawel - Did you have any other comments for me or is this good
to go? Thanks for the review!

[Paweł Hajdan Jr., 2010-07-14 17:53:51]

LGTM with a nit.

http://codereview.chromium.org/2903008/diff/31001/3008
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/2903008/diff/31001/3008#newcode323
chrome/test/pyautolib/pyautolib.cc:323: // TODO: figure out a way to
unambiguously report error condition.
nit: TODO(phajdan.jr)