[chromedriver] Implement SwitchToFrame command.

[chromedriver] Implement SwitchToFrame command.

This doesn't include switching to a frame by element yet.
BUG=166797
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175592

[chrisgao (Use stgao instead), 2012-12-19 22:29:32]

lgtm

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/d...
File chrome/test/chromedriver/dom_tracker.cc (right):

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/d...
chrome/test/chromedriver/dom_tracker.cc:10: #include "base/logging.h"
Remove logging.

[kkania, 2012-12-20 00:51:23]

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/d...
File chrome/test/chromedriver/dom_tracker.cc (right):

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/d...
chrome/test/chromedriver/dom_tracker.cc:10: #include "base/logging.h"
On 2012/12/19 22:29:32, Shuotao Gao wrote:
> Remove logging.

I use LOG(ERROR) below.

[frankf, 2012-12-20 19:23:53]

LGTM with a request.

+craigdh

I think it'd be easier to have a reviewer added to every CL, not intermittently
for the sake of continuity, otherwise craigdh needs to review the CL he missed
after the fact.

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome_impl.h (right):

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome_impl.h:43: virtual Status EvaluateScript(const
std::string& frame,
I know it's not convention in C++ to have a doc for every function, but it helps
readability a lot. WDTY?

[craigdh, 2012-12-20 20:03:38]

lgtm with design philosophy question/suggestion.

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:129: "  return document.evaluate(xpath,
document, null, "
Constructing these xpaths in C++ would reduce the injected Javascript down to a
single method invocation. In general I think it's worth keeping as much logic as
possible in C++, debugging generated code sent to an OOP interpreter is less
than ideal. Thoughts?

[commit-bot: I haz the power, 2013-01-08 18:09:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/11639019/17001

[commit-bot: I haz the power, 2013-01-08 18:25:25]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[kkania, 2013-01-08 18:30:10]

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome_impl.h (right):

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome_impl.h:43: virtual Status EvaluateScript(const
std::string& frame,
On 2012/12/20 19:23:53, frankf wrote:
> I know it's not convention in C++ to have a doc for every function, but it
helps
> readability a lot. WDTY?

I don't use too many comments, but I think it might be helpful to comment this
as you suggest. I'll comment the base class though.

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/11639019/diff/2001/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:129: "  return document.evaluate(xpath,
document, null, "
On 2012/12/20 20:03:38, craigdh wrote:
> Constructing these xpaths in C++ would reduce the injected Javascript down to
a
> single method invocation. In general I think it's worth keeping as much logic
as
> possible in C++, debugging generated code sent to an OOP interpreter is less
> than ideal. Thoughts?

I've gone and ahead and switched to C++ since it makes it a bit simpler.
Sometimes, if it is easier to do more in JS and it can be tested better, I think
its fine doing it in JS.

[commit-bot: I haz the power, 2013-01-08 18:31:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/11639019/32001

[commit-bot: I haz the power, 2013-01-08 18:33:56]

Step "update" is always a major failure.
Look at the try server FAQ for more details.