Convert ChromeDriver to use only the JSON automation interface.

Part2 of Convert ChromeDriver to use only the JSON automation interface.
Move common functions to automation_util.
BUG=none
TEST=none

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

[kkania, 2011-03-04 05:55:06]

jleyba: chrome/test/webdriver
phajdan.jr: all the rest

Sorry for the dense CL. I'm out of town tomorrow, but I'll address your comments
Sunday and Monday and hopefully checkin then...

[Paweł Hajdan Jr., 2011-03-05 11:44:56]

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:2113: void
ExecuteJavascriptObserver::OnDomOperationCompleted(const std::string& json) {
nit: 80 cols.

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:173: bool
GetBrowserFromJSONArgs(
Could you move those helpers to automation_provider_json.cc? I'd like to avoid
cluttering this file which is already too big.

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:179: *error = "'windex'
missing or invalid";
It seems that we should be appending to |error| rather than overwriting it,
given how it is used in GetBrowserAndTabFromJSONArgs (same |error| for both
calls).

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4895: if (!(has_id ^
has_handle)) {
Oh, that looks complicated. Why is it needed (the logic for _both_ id and
handle)?

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:5103: void
TestingAutomationProvider::GetCookiesJSON(
I'm pretty sure this JSON cookie code duplicates some existing code. Could you
avoid that, i.e. either share that code or migrate things to JSON (probably not
possible yet because of the reference build)?

http://codereview.chromium.org/6614023/diff/2002/chrome/test/automation/autom...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/6614023/diff/2002/chrome/test/automation/autom...
chrome/test/automation/automation_json_requests.h:47: int id_or_handle,
This looks like a very unintuitive interface. Can you find a cleaner way?

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

http://codereview.chromium.org/6614023/diff/2002/chrome/test/webdriver/sessio...
chrome/test/webdriver/session.cc:368: bool success = false, does_exist = false;
nit: I'd prefer to avoid declaring two variables with values on the same line.

[kkania, 2011-03-07 04:31:51]

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:2113: void
ExecuteJavascriptObserver::OnDomOperationCompleted(const std::string& json) {
On 2011/03/05 11:44:56, Paweł Hajdan Jr. wrote:
> nit: 80 cols.

Done.

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:173: bool
GetBrowserFromJSONArgs(
On 2011/03/05 11:44:56, Paweł Hajdan Jr. wrote:
> Could you move those helpers to automation_provider_json.cc? I'd like to avoid
> cluttering this file which is already too big.

Done.

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:179: *error = "'windex'
missing or invalid";
On 2011/03/05 11:44:56, Paweł Hajdan Jr. wrote:
> It seems that we should be appending to |error| rather than overwriting it,
> given how it is used in GetBrowserAndTabFromJSONArgs (same |error| for both
> calls).

The error is only set if the func returns false, in which case
GetBrowserAndTabsFromJSONArgs below cannot not overwrite an error.

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:4895: if (!(has_id ^
has_handle)) {
On 2011/03/05 11:44:56, Paweł Hajdan Jr. wrote:
> Oh, that looks complicated. Why is it needed (the logic for _both_ id and
> handle)?

ok, i wrote it in simpler language

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:5103: void
TestingAutomationProvider::GetCookiesJSON(
On 2011/03/05 11:44:56, Paweł Hajdan Jr. wrote:
> I'm pretty sure this JSON cookie code duplicates some existing code. Could you
> avoid that, i.e. either share that code or migrate things to JSON (probably
not
> possible yet because of the reference build)?

Yes, it does. Although really the only thing that is duplicated is posting a
task to an already shared function. Most of the work is in extracting and
packing args. I am not sure it is worth it. But, I was planning on migrating the
other function to JSON anyways, according to the 4-step plan I laid out in an
earlier CL you approved.

http://codereview.chromium.org/6614023/diff/2002/chrome/test/automation/autom...
File chrome/test/automation/automation_json_requests.h (right):

http://codereview.chromium.org/6614023/diff/2002/chrome/test/automation/autom...
chrome/test/automation/automation_json_requests.h:47: int id_or_handle,
On 2011/03/05 11:44:56, Paweł Hajdan Jr. wrote:
> This looks like a very unintuitive interface. Can you find a cleaner way?

Ok, I separated into two functions.

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

http://codereview.chromium.org/6614023/diff/2002/chrome/test/webdriver/sessio...
chrome/test/webdriver/session.cc:368: bool success = false, does_exist = false;
On 2011/03/05 11:44:56, Paweł Hajdan Jr. wrote:
> nit: I'd prefer to avoid declaring two variables with values on the same line.

Done.

[Jason Leyba, 2011-03-07 18:07:56]

Changes in chrome/test/webdriver LGTM

[Nirnimesh, 2011-03-07 19:54:42]

Overall LGTM.
I've made just one comment, but I won't press on it.

http://codereview.chromium.org/6614023/diff/8002/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/6614023/diff/8002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.h:810: 
It's ugly to have all the function names suffixed with "JSON", and it's not even
consistent (GoForward/GoBack don't end in JSON). You need a better style here.
Why not put them inside a namespace and remove the 'JSON' from the name?

[kkania, 2011-03-07 20:15:31]

assuming trybots go green, I want to submit this at 2pm PST today. I'll keep
this CL open in case if anyone has any remaining comments

[Paweł Hajdan Jr., 2011-03-07 21:03:39]

On 2011/03/07 20:15:31, kkania wrote:
> assuming trybots go green, I want to submit this at 2pm PST today. I'll keep
> this CL open in case if anyone has any remaining comments

There is no rush, please avoid saying things like that. If a patch set is fixing
issues pointed out earlier, you should wait for an "LGTM", not for a (short)
timeout.

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/6614023/diff/2002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:5103: void
TestingAutomationProvider::GetCookiesJSON(
On 2011/03/07 04:31:51, kkania wrote:
> On 2011/03/05 11:44:56, Paweł Hajdan Jr. wrote:
> > I'm pretty sure this JSON cookie code duplicates some existing code. Could
you
> > avoid that, i.e. either share that code or migrate things to JSON (probably
> not
> > possible yet because of the reference build)?
> 
> Yes, it does. Although really the only thing that is duplicated is posting a
> task to an already shared function. Most of the work is in extracting and
> packing args. I am not sure it is worth it. But, I was planning on migrating
the
> other function to JSON anyways, according to the 4-step plan I laid out in an
> earlier CL you approved.

The migration may be non-trivial, and I may have missed some parts of earlier
CLs. To be really safe here, can we avoid the duplication? Also, there is a
possible issue with the Task here that the original code probably doesn't have.

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_json.h (right):

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_json.h:46: // Gets the browser
specified by the given dictionary |args|. 'windex' should
nit: There is no |windex| parameter. My understanding is that it should be a key
in the dict. Could you update the comment to avoid any confusion? It applies to
all new functions in this header.

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_json.h:49: bool
GetBrowserFromJSONArgs(DictionaryValue* args,
nit: Bonus points for WARN_UNUSED_RESULT from base/compiler_specific.h.

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:761:
BrowserList::const_iterator iter = BrowserList::begin();
Hmm, now this seems duplicated even across files in this CL. The code is indeed
small, but I can see value in sharing it. Could you do that?

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:5087:
reply.SendError("Couldn't post task to delete the cookie");
That leaks the task it seems (possibly applies to multiple places).

[kkania, 2011-03-07 21:57:50]

I am sorry for pushing this for the past week, but the chromium branch point is
tonight and I really do not want to postpone Chromedriver's release for 12
weeks.

I have addressed your comments. Thanks for the review.

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_json.h (right):

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_json.h:46: // Gets the browser
specified by the given dictionary |args|. 'windex' should
On 2011/03/07 21:03:39, Paweł Hajdan Jr. wrote:
> nit: There is no |windex| parameter. My understanding is that it should be a
key
> in the dict. Could you update the comment to avoid any confusion? It applies
to
> all new functions in this header.

Done.

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_json.h:49: bool
GetBrowserFromJSONArgs(DictionaryValue* args,
On 2011/03/07 21:03:39, Paweł Hajdan Jr. wrote:
> nit: Bonus points for WARN_UNUSED_RESULT from base/compiler_specific.h.

Done.

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:761:
BrowserList::const_iterator iter = BrowserList::begin();
On 2011/03/07 21:03:39, Paweł Hajdan Jr. wrote:
> Hmm, now this seems duplicated even across files in this CL. The code is
indeed
> small, but I can see value in sharing it. Could you do that?

Sure. I realized that this code could be simplified even further however. Take a
look at it now and let me know if you still think it should be shared.

If so, here's the options I see:
1) Add GetBrowserAt as a function in this file and declare it in the header.
2) Add a new file called automation_utils.cc, which contains this function
3) Add a new static function to BrowserList which allows you to retrieve by
index.

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:5087:
reply.SendError("Couldn't post task to delete the cookie");
On 2011/03/07 21:03:39, Paweł Hajdan Jr. wrote:
> That leaks the task it seems (possibly applies to multiple places).

BrowserThread::PostTask takes ownership of the task. The task is deleted even if
the message loop is no longer valid.

[Paweł Hajdan Jr., 2011-03-08 07:29:08]

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:761:
BrowserList::const_iterator iter = BrowserList::begin();
On 2011/03/07 21:57:51, kkania wrote:
> If so, here's the options I see:
> 1) Add GetBrowserAt as a function in this file and declare it in the header.
> 2) Add a new file called automation_utils.cc, which contains this function
> 3) Add a new static function to BrowserList which allows you to retrieve by
> index.

I'd prefer 3), but if we can't get that into BrowserList, automation_utils.cc
sound good to me.

http://codereview.chromium.org/6614023/diff/9003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:5087:
reply.SendError("Couldn't post task to delete the cookie");
On 2011/03/07 21:57:51, kkania wrote:
> On 2011/03/07 21:03:39, Paweł Hajdan Jr. wrote:
> > That leaks the task it seems (possibly applies to multiple places).
> 
> BrowserThread::PostTask takes ownership of the task. The task is deleted even
if
> the message loop is no longer valid.

Right, I missed that info.

[kkania, 2011-03-26 04:19:17]

moved funcs to automation_util, as requested by Pawel

please let me know any further comments you have on this CL.

[Paweł Hajdan Jr., 2011-03-26 15:30:44]

LGTM