[chromedriver] Support clicking an element in sub frames.

[chromedriver] Support click an element in sub frames.

BUG=170998
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187920

[kkania, 2013-03-12 04:04:07]

also, what tests do you have?

https://codereview.chromium.org/12764021/diff/1/chrome/test/chromedriver/elem...
File chrome/test/chromedriver/element_util.cc (right):

https://codereview.chromium.org/12764021/diff/1/chrome/test/chromedriver/elem...
chrome/test/chromedriver/element_util.cc:193: base::StringToInt(border_left_str,
border_left);
check return value?

https://codereview.chromium.org/12764021/diff/1/chrome/test/chromedriver/elem...
chrome/test/chromedriver/element_util.cc:529: xpath +=
base::StringPrintf("[@name=\"%s\"]", it->name.c_str());
why do we support both name and index?  i don't think relying on index is good. 
how about we create a random attribute like the old chromedriver and look up the
frame based on that?

https://codereview.chromium.org/12764021/diff/1/chrome/test/chromedriver/run_...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12764021/diff/1/chrome/test/chromedriver/run_...
chrome/test/chromedriver/run_py_tests.py:67:
self._driver.Load(self.GetHttpUrlForFile('/chromedriver/frame_test.html'))
?

[chrisgao (Use stgao instead), 2013-03-12 17:41:18]

The testcase might be flanking because of the frame switch problem. But it runs
well locally.

https://codereview.chromium.org/12764021/diff/1/chrome/test/chromedriver/elem...
File chrome/test/chromedriver/element_util.cc (right):

https://codereview.chromium.org/12764021/diff/1/chrome/test/chromedriver/elem...
chrome/test/chromedriver/element_util.cc:193: base::StringToInt(border_left_str,
border_left);
On 2013/03/12 04:04:07, kkania wrote:
> check return value?

Done.

https://codereview.chromium.org/12764021/diff/1/chrome/test/chromedriver/elem...
chrome/test/chromedriver/element_util.cc:529: xpath +=
base::StringPrintf("[@name=\"%s\"]", it->name.c_str());
On 2013/03/12 04:04:07, kkania wrote:
> why do we support both name and index?  i don't think relying on index is
good. 
> how about we create a random attribute like the old chromedriver and look up
the
> frame based on that?

Because name of frame is optional in the Inspector.json

Do the same way as the old chromedriver.

https://codereview.chromium.org/12764021/diff/1/chrome/test/chromedriver/run_...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12764021/diff/1/chrome/test/chromedriver/run_...
chrome/test/chromedriver/run_py_tests.py:67:
self._driver.Load(self.GetHttpUrlForFile('/chromedriver/frame_test.html'))
On 2013/03/12 04:04:07, kkania wrote:
> ?

Whoops, forgot to remove this...

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/r...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/r...
chrome/test/chromedriver/run_py_tests.py:253:
self._driver.ExecuteScript('arguments[0].src = "empty.html";', frame)
A workaround for the hanging issue during switching to sub-frame.

[kkania, 2013-03-12 18:34:25]

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/b...
File chrome/test/chromedriver/basic_types.h (right):

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/b...
chrome/test/chromedriver/basic_types.h:43: struct FrameInfo {
i don't think i'd call this a basic type. how about move it to session.h?

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/b...
chrome/test/chromedriver/basic_types.h:43: struct FrameInfo {
can you think of a better name?

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/b...
chrome/test/chromedriver/basic_types.h:47: std::string xpath;
at first read, it is a bit unclear what this refers to. can you think of a
better name

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/e...
File chrome/test/chromedriver/element_util.cc (right):

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/e...
chrome/test/chromedriver/element_util.cc:104: WebPoint* location) {
make this const WebPoint&; also, change all the CreateValueFrom to take a const
ref instead of a pointer

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/e...
chrome/test/chromedriver/element_util.cc:147: if (!ParseFromValue(result.get(),
location))
usually we've been trying to keep to the convention if status is error, the
output will not be modified

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/s...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/s...
chrome/test/chromedriver/session.h:37: std::string frame;
this is a bit messy now that the client must remember to keep frame_elements and
frame in sync. Fix this.

[chrisgao (Use stgao instead), 2013-03-12 23:17:41]

ptal

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/b...
File chrome/test/chromedriver/basic_types.h (right):

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/b...
chrome/test/chromedriver/basic_types.h:43: struct FrameInfo {
On 2013/03/12 18:34:25, kkania wrote:
> i don't think i'd call this a basic type. how about move it to session.h?

Done.

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/b...
chrome/test/chromedriver/basic_types.h:43: struct FrameInfo {
On 2013/03/12 18:34:25, kkania wrote:
> can you think of a better name?

FrameElement.

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/b...
chrome/test/chromedriver/basic_types.h:47: std::string xpath;
On 2013/03/12 18:34:25, kkania wrote:
> at first read, it is a bit unclear what this refers to. can you think of a
> better name

chromedriver_frame_id, holding the random value of attribute cd_frame_id_.

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/e...
File chrome/test/chromedriver/element_util.cc (right):

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/e...
chrome/test/chromedriver/element_util.cc:104: WebPoint* location) {
On 2013/03/12 18:34:25, kkania wrote:
> make this const WebPoint&; also, change all the CreateValueFrom to take a
const
> ref instead of a pointer

Done.

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/e...
chrome/test/chromedriver/element_util.cc:147: if (!ParseFromValue(result.get(),
location))
On 2013/03/12 18:34:25, kkania wrote:
> usually we've been trying to keep to the convention if status is error, the
> output will not be modified

Done.

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/s...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/12764021/diff/9001/chrome/test/chromedriver/s...
chrome/test/chromedriver/session.h:37: std::string frame;
On 2013/03/12 18:34:25, kkania wrote:
> this is a bit messy now that the client must remember to keep frame_elements
and
> frame in sync. Fix this.

Remove frame, and only maintain frame_elements.

[kkania, 2013-03-13 00:06:20]

lgtm

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
File chrome/test/chromedriver/basic_types.h (right):

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/basic_types.h:14: int X() const;
why do we need these?

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/basic_types.h:26: int Width() const;
why?

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/run_py_tests.py:253:
self._driver.ExecuteScript('arguments[0].src = "empty.html";', frame)
can you remove this now? optional, but you can check

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
File chrome/test/chromedriver/session.cc (right):

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/session.cc:19: : parent_frame_id(parent_frame_id),
frame_id(frame_id),
normal style is to put these on separate lines if they can't all fit on one

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/session.cc:67: std::string parent_frame_id = "";
remove = ""

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/session.h:25: struct FrameElement {
actually, i think FrameInfo is better than FrameElement

[chrisgao (Use stgao instead), 2013-03-13 00:36:48]

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
File chrome/test/chromedriver/basic_types.h (right):

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/basic_types.h:14: int X() const;
On 2013/03/13 00:06:20, kkania wrote:
> why do we need these?

Whoops, i thought direct access to const object member leads to compile error.

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/basic_types.h:26: int Width() const;
On 2013/03/13 00:06:20, kkania wrote:
> why?

Removed.

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/run_py_tests.py:253:
self._driver.ExecuteScript('arguments[0].src = "empty.html";', frame)
On 2013/03/13 00:06:20, kkania wrote:
> can you remove this now? optional, but you can check

Removed. It is not needed anymore.

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
File chrome/test/chromedriver/session.cc (right):

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/session.cc:19: : parent_frame_id(parent_frame_id),
frame_id(frame_id),
On 2013/03/13 00:06:20, kkania wrote:
> normal style is to put these on separate lines if they can't all fit on one

Done.

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/session.cc:67: std::string parent_frame_id = "";
On 2013/03/13 00:06:20, kkania wrote:
> remove = ""

Done.

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/12764021/diff/13001/chrome/test/chromedriver/...
chrome/test/chromedriver/session.h:25: struct FrameElement {
On 2013/03/13 00:06:20, kkania wrote:
> actually, i think FrameInfo is better than FrameElement

Ok, take FrameInfo.

[commit-bot: I haz the power, 2013-03-13 19:31:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrisgao@chromium.org/12764021/43001

[commit-bot: I haz the power, 2013-03-13 19:35:50]

Change committed as 187920