Implement the target locator commands for ChromeDriver.

Implement the target locator commands for ChromeDriver.
BUG=none
TEST=none

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

[Paweł Hajdan Jr., 2011-02-14 09:29:43]

Drive-by with testing-related comments. Please fix them and let me do another
review before committing.

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:78: for (int i = 0; i < browser_count; ++i)
{
This loop looks worrying. So far no automation code required that kind of loop.
Can you keep a TabProxy around instead of doing a linear lookup every time? If
that's not possible, have you considered alternative solutions that wouldn't
require looping over all browsers?

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:165: void
Automation::GetTabIds(std::vector<int>* tab_ids,
What is this method trying to do, and why? I can infer the first from code, but
the second is still puzzling me a bit (especially why is this _needed_). There
is no comment for this method, neither here or in the header.

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:191: tabs_ = tabs;
I guess bad things happen if a tab gets opened or closed and |tabs_| is not
updated. Have you considered such condition?

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:205: for (size_t i = 0; i < tabs_.size();
++i) {
A linear scan doesn't seem to be an optimal solution here. Can you do better?

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:208: return true;
Now this is weird. We have _both_ a return value and a boolean out parameter,
and sometimes they might not be consistent. I think we should just stick to one
of them, but not both. I generally prefer return values.

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/sessio...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/sessio...
chrome/test/webdriver/session.cc:253: for (size_t i = 0; i < window_ids.size();
++i) {
Another case of linear-scan lookup. Can we make it more elegant?

[kkania, 2011-02-14 17:42:12]

No actual code changes yet. Just wanted to respond to your comments as quick as
possible to get feedback before I start changing stuff. Thanks for the review!

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
File chrome/test/webdriver/automation.cc (right):

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:78: for (int i = 0; i < browser_count; ++i)
{
On 2011/02/14 09:29:43, Paweł Hajdan Jr. wrote:
> This loop looks worrying. So far no automation code required that kind of
loop.
> Can you keep a TabProxy around instead of doing a linear lookup every time? If
> that's not possible, have you considered alternative solutions that wouldn't
> require looping over all browsers?

Yes, I do not like this code either. I am keeping the tab proxy around, but that
is not what I need. I need a BrowserProxy, and there is no way to get that from
the TabProxy currently. Perhaps adding such a method is the best way to solve
this problem.

The other way would be to keep track of all BrowserProxies the user knows about
along with the TabProxies, and know which are associated with which.

I think I prefer the first way better. I'll add it.

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:165: void
Automation::GetTabIds(std::vector<int>* tab_ids,
On 2011/02/14 09:29:43, Paweł Hajdan Jr. wrote:
> What is this method trying to do, and why? I can infer the first from code,
but
> the second is still puzzling me a bit (especially why is this _needed_). There
> is no comment for this method, neither here or in the header.

Will add appropriate comments. This method implements the WebDriver method for
retrieving handles for all the known tabs currently available to the user. The
client can then use these handles to switch between what tab/window they want to
work with.

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:191: tabs_ = tabs;
On 2011/02/14 09:29:43, Paweł Hajdan Jr. wrote:
> I guess bad things happen if a tab gets opened or closed and |tabs_| is not
> updated. Have you considered such condition?

It is ok if tabs_ does  not get updated. We need to give back handles for all
the tabs in the system at this point in time to the client. If a tab closes, and
the user tells us to do something with that tab, we will simply return an error.
If a new tab is created after we return these, the user has no way of
identifying that tab to us without calling this method again.

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:205: for (size_t i = 0; i < tabs_.size();
++i) {
On 2011/02/14 09:29:43, Paweł Hajdan Jr. wrote:
> A linear scan doesn't seem to be an optimal solution here. Can you do better?

Will use a map

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/automa...
chrome/test/webdriver/automation.cc:208: return true;
On 2011/02/14 09:29:43, Paweł Hajdan Jr. wrote:
> Now this is weird. We have _both_ a return value and a boolean out parameter,
> and sometimes they might not be consistent. I think we should just stick to
one
> of them, but not both. I generally prefer return values.

Yes, this is a bit of a shortcut and can be confusing. I'll just keep to the
return value here.

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/sessio...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/6507015/diff/2014/chrome/test/webdriver/sessio...
chrome/test/webdriver/session.cc:253: for (size_t i = 0; i < window_ids.size();
++i) {
On 2011/02/14 09:29:43, Paweł Hajdan Jr. wrote:
> Another case of linear-scan lookup. Can we make it more elegant?

I can add a method to Automation which does something like bool DoesTabExist(int
tab_id).

[kkania, 2011-02-14 19:33:35]

ok, I went ahead and did what I thought best in response to Pawel's comments.
Please review again.

[Jason Leyba, 2011-02-14 21:00:23]

I'm not sure I understand the necessity of the changes to Session/SessionManager
relationship.  These changes are also unrelated to implementing the target
locators, so I'd appreciate it if you could move them to a separate CL.

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

http://codereview.chromium.org/6507015/diff/10003/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:204:
tab_ids->push_back(iter->second->handle());
iter->first?

Also, why not build this list as your building tab_id_map?

http://codereview.chromium.org/6507015/diff/10003/chrome/test/webdriver/comma...
File chrome/test/webdriver/commands/create_session.cc (right):

http://codereview.chromium.org/6507015/diff/10003/chrome/test/webdriver/comma...
chrome/test/webdriver/commands/create_session.cc:19: Session* session = new
Session();
I think these changes unnecessarily obfuscate who owns and is responsible for
the allocated Session.

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

http://codereview.chromium.org/6507015/diff/10003/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:57: delete this;
This looks really suspicious.

http://codereview.chromium.org/6507015/diff/10003/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:65: delete this;
Same as above.  This looks really suspicious.

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

http://codereview.chromium.org/6507015/diff/10003/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.h:35: // Removes this |Session| from the
|SessionManager|.
Not a fan of these changes.  I don't think it's the Session's responsibility to
add/remove itself from the SessionManager.

[kkania, 2011-02-14 22:10:09]

The Session now manages its own lifetime. Added comments about this in several
places.

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

http://codereview.chromium.org/6507015/diff/10003/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:204:
tab_ids->push_back(iter->second->handle());
On 2011/02/14 21:00:23, Jason Leyba wrote:
> iter->first?
> 
> Also, why not build this list as your building tab_id_map?

Done.

[Paweł Hajdan Jr., 2011-02-15 08:50:14]

This is better. Just wondering... how about identifying a tab by a pair
(browser_num, tab_num) instead of the internal tab handle? AFAIK pyauto uses the
pair (browser_num, tab_num), and it should be easier to implement, because you
can already get n-th browser and n-th tab of the browser.

Is it possible to change the protocol so instead of asking for all tab handles,
you can ask for number of browsers, and number of tabs of a given browser? This
is also more like how the automation was designed to work.

http://codereview.chromium.org/6507015/diff/15001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/6507015/diff/15001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:4623: *browser_handle =
0;
Generally automation client is expected to know this. Are you sure this is
needed?

Please also add bool success out-parameter. It's much more obvious than any
special value of |browser_handle|.

[kkania, 2011-02-15 17:34:56]

No code changes, just replying to Pawel's idea.

It is possible to identify a tab by (browser_index, tab_index), but then you run
into the problem that some of your pairs are no longer valid when you close a
window/tab of lower index. To solve this we could change WebDriver so that
instead of returning handles, the client always has to identify to us the
current indices they are working with.

This would involve more than a protocol change, it would require modifying the
WebDriver API as well. We would have to deprecate the old way of identifying
windows and introduce our new way. Eventually tests using the old way would have
to be changed to our new way. Basically, this would be a huge change. I want to
avoid changing the WebDriver spec if possible.

My primary goal with ChromeDriver is to implement the cross-platform standard
WebDriver has already established so that existing tests can be run for Chrome
as well as IE, firefox, etc. We can not easily change the spec without causing
the WebDriver team to have to change all the other browser implementations.
Although I hope we do have some say in the future of the spec, we do not own it,
the WebDriver team does.

[Jason Leyba, 2011-02-15 23:59:26]

LGTM

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

http://codereview.chromium.org/6507015/diff/21002/chrome/test/webdriver/autom...
chrome/test/webdriver/automation.cc:177: TabIdMap tab_id_map;
Move this declaration down after GetBrowserWindowCount.

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

http://codereview.chromium.org/6507015/diff/21002/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:345: if (window_ids.size() == 1u)
Closing a window will fire its unload event. A page could (although its
unlikely) have an unload listener that opens a new window.  In this situation,
you would terminate the session even though there is still a window open.

It's slower, but you really should call GetWindowIds again before terminating
the session.

[Paweł Hajdan Jr., 2011-02-16 17:36:24]

Code I commented in the drive-by LGTM. Thank you.

[kkania, 2013-02-28 21:49:38]

https://codereview.chromium.org/6507015/diff/21002/chrome/test/webdriver/auto...
File chrome/test/webdriver/automation.cc (right):

https://codereview.chromium.org/6507015/diff/21002/chrome/test/webdriver/auto...
chrome/test/webdriver/automation.cc:177: TabIdMap tab_id_map;
On 2011/02/15 23:59:26, Jason Leyba wrote:
> Move this declaration down after GetBrowserWindowCount.

Done.

https://codereview.chromium.org/6507015/diff/21002/chrome/test/webdriver/sess...
File chrome/test/webdriver/session.cc (right):

https://codereview.chromium.org/6507015/diff/21002/chrome/test/webdriver/sess...
chrome/test/webdriver/session.cc:345: if (window_ids.size() == 1u)
On 2011/02/15 23:59:26, Jason Leyba wrote:
> Closing a window will fire its unload event. A page could (although its
> unlikely) have an unload listener that opens a new window.  In this situation,
> you would terminate the session even though there is still a window open.
> 
> It's slower, but you really should call GetWindowIds again before terminating
> the session.

good point, done