First pass at refactoring pyautolib in preparation for removing proxy dependencies.

First pass at refactoring pyautolib and TestingAutomationProvider in preparation for removing proxy dependencies.

This breaks IPC backwards compatibility.

BUG=132789, 135059
TEST=functional/apptest.py, functional/browser.py, functional/navigation.py

[Nirnimesh, 2012-06-14 22:16:41]

http://codereview.chromium.org/10534163/diff/1/chrome/browser/automation/test...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/10534163/diff/1/chrome/browser/automation/test...
chrome/browser/automation/testing_automation_provider.cc:1635: void
TestingAutomationProvider::BuildJSONHandlerMaps(
The whole handler map gets initialized for each automation call. While you're at
it, it's better to initialize on first-use and save it.

http://codereview.chromium.org/10534163/diff/1/chrome/browser/automation/test...
chrome/browser/automation/testing_automation_provider.cc:2052: void
TestingAutomationProvider::SendJSONRequestWithIndex(
It's not clear index of what.
Rename to SendJSONRequestWithBrowserIndex?

http://codereview.chromium.org/10534163/diff/1/chrome/browser/automation/test...
chrome/browser/automation/testing_automation_provider.cc:2087: "The supplied
browser window does not/no longer exists.");
It would be nice to specify which browser index.

Also: just say the browser window does not exist. (No reason to make it sound
nice)

http://codereview.chromium.org/10534163/diff/1/chrome/test/pyautolib/pyautoli...
File chrome/test/pyautolib/pyautolib.cc (right):

http://codereview.chromium.org/10534163/diff/1/chrome/test/pyautolib/pyautoli...
chrome/test/pyautolib/pyautolib.cc:404: if (duration >= timeout) {
please add a test for timeout in test_pyauto.py

http://codereview.chromium.org/10534163/diff/1/chrome/test/pyautolib/pyautoli...
chrome/test/pyautolib/pyautolib.cc:404: if (duration >= timeout) {
There should be better way to determine if it timed out instead of determining
ourselves.  The Send() call knows if it times out. Can we use that instead?

http://codereview.chromium.org/10534163/diff/1/chrome/test/pyautolib/pyautoli...
chrome/test/pyautolib/pyautolib.cc:405: ErrorResponse("Request timed out",
request, response);
print out duration as well

[craigdh, 2012-06-15 19:24:52]

https://chromiumcodereview.appspot.com/10534163/diff/1/chrome/browser/automat...
File chrome/browser/automation/testing_automation_provider.cc (right):

https://chromiumcodereview.appspot.com/10534163/diff/1/chrome/browser/automat...
chrome/browser/automation/testing_automation_provider.cc:1635: void
TestingAutomationProvider::BuildJSONHandlerMaps(
On 2012/06/14 22:16:42, Nirnimesh wrote:
> The whole handler map gets initialized for each automation call. While you're
at
> it, it's better to initialize on first-use and save it.

Done.

https://chromiumcodereview.appspot.com/10534163/diff/1/chrome/browser/automat...
chrome/browser/automation/testing_automation_provider.cc:2052: void
TestingAutomationProvider::SendJSONRequestWithIndex(
On 2012/06/14 22:16:42, Nirnimesh wrote:
> It's not clear index of what.
> Rename to SendJSONRequestWithBrowserIndex?

Done.

https://chromiumcodereview.appspot.com/10534163/diff/1/chrome/browser/automat...
chrome/browser/automation/testing_automation_provider.cc:2087: "The supplied
browser window does not/no longer exists.");
On 2012/06/14 22:16:42, Nirnimesh wrote:
> It would be nice to specify which browser index.
> 
> Also: just say the browser window does not exist. (No reason to make it sound
> nice)

Done.

https://chromiumcodereview.appspot.com/10534163/diff/1/chrome/test/pyautolib/...
File chrome/test/pyautolib/pyautolib.cc (right):

https://chromiumcodereview.appspot.com/10534163/diff/1/chrome/test/pyautolib/...
chrome/test/pyautolib/pyautolib.cc:404: if (duration >= timeout) {
On 2012/06/14 22:16:42, Nirnimesh wrote:
> please add a test for timeout in test_pyauto.py

Done.

https://chromiumcodereview.appspot.com/10534163/diff/1/chrome/test/pyautolib/...
chrome/test/pyautolib/pyautolib.cc:404: if (duration >= timeout) {
On 2012/06/14 22:16:42, Nirnimesh wrote:
> There should be better way to determine if it timed out instead of determining
> ourselves.  The Send() call knows if it times out. Can we use that instead?

Yes, but right now this is still going through the proxy. My next pass is going
to remove use of the proxy altogether and then we'll find out firsthand about
timeouts and the connection breaking.

https://chromiumcodereview.appspot.com/10534163/diff/1/chrome/test/pyautolib/...
chrome/test/pyautolib/pyautolib.cc:405: ErrorResponse("Request timed out",
request, response);
On 2012/06/14 22:16:42, Nirnimesh wrote:
> print out duration as well

Done.

[Nirnimesh, 2012-06-15 19:52:49]

LGTM. Thanks for writing the test.

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
File chrome/browser/automation/testing_automation_provider.cc (right):

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:2047: if (!browser &&
handle >= 0) {
Why handle >= 0?

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/test/pyautol...
File chrome/test/pyautolib/pyautolib.cc (right):

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/test/pyautol...
chrome/test/pyautolib/pyautolib.cc:391: LOG(WARNING) << "Error during
automation: " << response;
LOG(ERROR)?

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/test/pyautol...
chrome/test/pyautolib/pyautolib.cc:404: if (duration >= timeout) {
Add a TODO to deduce timeout from the ipc Send() call

[kkania, 2012-06-18 16:30:33]

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

http://codereview.chromium.org/10534163/diff/4001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2017: DictionaryValue*
TestingAutomationProvider::ParseJSONRequestCommand(
i think it would be preferable to return scoped_ptr

http://codereview.chromium.org/10534163/diff/4001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2023: if
(!error->empty())
I think it would be better to check values.get() here

http://codereview.chromium.org/10534163/diff/4001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2033: if
(!dict_value->GetStringASCII(std::string("command"), command)) {
you don't need the explicit call to the std::string constructor.

http://codereview.chromium.org/10534163/diff/4001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2046: browser =
browser_tracker_->GetResource(handle);
indent

http://codereview.chromium.org/10534163/diff/4001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/10534163/diff/4001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:67: //
?

http://codereview.chromium.org/10534163/diff/4001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:274: static
DictionaryValue* ParseJSONRequestCommand(
i think static funcs are supposed to go above non-statics, check the style guide

http://codereview.chromium.org/10534163/diff/4001/chrome/common/automation_me...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/10534163/diff/4001/chrome/common/automation_me...
chrome/common/automation_messages_internal.h:955:
IPC_SYNC_MESSAGE_CONTROL2_2(AutomationMsg_SendJSONRequestWithBrowserIndex,
You eventually want to delete AutomationMsg_SendJSONRequest, right? How about
you make this one AutomationMsg_SendJSONRequestWithBrowserHandle, and let the
other one be the index one. Then you can get rid of the handle >= 0 check that
Nirnimesh commented on and I don't have to make compat changes when you delete
the other message.

[craigdh, 2012-06-19 00:19:34]

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
File chrome/browser/automation/testing_automation_provider.cc (right):

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:2017: DictionaryValue*
TestingAutomationProvider::ParseJSONRequestCommand(
On 2012/06/18 16:30:33, kkania wrote:
> i think it would be preferable to return scoped_ptr

Done.

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:2023: if
(!error->empty())
On 2012/06/18 16:30:33, kkania wrote:
> I think it would be better to check values.get() here

Done.

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:2033: if
(!dict_value->GetStringASCII(std::string("command"), command)) {
On 2012/06/18 16:30:33, kkania wrote:
> you don't need the explicit call to the std::string constructor.

Done.

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:2046: browser =
browser_tracker_->GetResource(handle);
On 2012/06/18 16:30:33, kkania wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:2047: if (!browser &&
handle >= 0) {
On 2012/06/15 19:52:49, Nirnimesh wrote:
> Why handle >= 0?

That means the user tried to pass a valid handle. The automation_proxy passes -1
when a browser is not needed. We don't want to throw an error about not finding
a browser when one wasn't supplied.

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
File chrome/browser/automation/testing_automation_provider.h (right):

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.h:67: //
On 2012/06/18 16:30:33, kkania wrote:
> ?

Good question.

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.h:274: static
DictionaryValue* ParseJSONRequestCommand(
On 2012/06/18 16:30:33, kkania wrote:
> i think static funcs are supposed to go above non-statics, check the style
guide

I went and looked it up: the style guide only treats static const data members
differently. It lumps the declaration order of "Methods, including static
methods" together. That said, if you think it's preferable I'd be happy to move
it.

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/common/autom...
File chrome/common/automation_messages_internal.h (right):

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/common/autom...
chrome/common/automation_messages_internal.h:955:
IPC_SYNC_MESSAGE_CONTROL2_2(AutomationMsg_SendJSONRequestWithBrowserIndex,
On 2012/06/18 16:30:33, kkania wrote:
> You eventually want to delete AutomationMsg_SendJSONRequest, right? How about
> you make this one AutomationMsg_SendJSONRequestWithBrowserHandle, and let the
> other one be the index one. Then you can get rid of the handle >= 0 check that
> Nirnimesh commented on and I don't have to make compat changes when you delete
> the other message.

Chromedriver uses AutomationMsg_SendJSONRequest using browser handles. Wouldn't
moving that IPC here change the IPC id and hence break backwards compatibility
for Chromedriver? Also, renaming AutomationMsg_SendJSONRequest to
AutomationMsg_SendJSONRequestWithBrowserHandle would create a problem if I
missed a rename somewhere, it's safer to add a new one with new functionality
than switch the behavior of the old one. Also, the check for handle >=0 would
still have to stay because automation_json_requests.cc uses -1 as a handle to
indicate the automation call doesn't require a browser.

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/test/pyautol...
File chrome/test/pyautolib/pyautolib.cc (right):

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/test/pyautol...
chrome/test/pyautolib/pyautolib.cc:391: LOG(WARNING) << "Error during
automation: " << response;
On 2012/06/15 19:52:49, Nirnimesh wrote:
> LOG(ERROR)?

Done.

https://chromiumcodereview.appspot.com/10534163/diff/4001/chrome/test/pyautol...
chrome/test/pyautolib/pyautolib.cc:404: if (duration >= timeout) {
On 2012/06/15 19:52:49, Nirnimesh wrote:
> Add a TODO to deduce timeout from the ipc Send() call

Done.

[kkania, 2012-06-19 16:20:11]

http://codereview.chromium.org/10534163/diff/4001/chrome/common/automation_me...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/10534163/diff/4001/chrome/common/automation_me...
chrome/common/automation_messages_internal.h:955:
IPC_SYNC_MESSAGE_CONTROL2_2(AutomationMsg_SendJSONRequestWithBrowserIndex,
On 2012/06/19 00:19:35, craigdh wrote:
> On 2012/06/18 16:30:33, kkania wrote:
> > You eventually want to delete AutomationMsg_SendJSONRequest, right? How
about
> > you make this one AutomationMsg_SendJSONRequestWithBrowserHandle, and let
the
> > other one be the index one. Then you can get rid of the handle >= 0 check
that
> > Nirnimesh commented on and I don't have to make compat changes when you
delete
> > the other message.
> 
> Chromedriver uses AutomationMsg_SendJSONRequest using browser handles.
Wouldn't
> moving that IPC here change the IPC id and hence break backwards compatibility
> for Chromedriver? Also, renaming AutomationMsg_SendJSONRequest to
> AutomationMsg_SendJSONRequestWithBrowserHandle would create a problem if I
> missed a rename somewhere, it's safer to add a new one with new functionality
> than switch the behavior of the old one. Also, the check for handle >=0 would
> still have to stay because automation_json_requests.cc uses -1 as a handle to
> indicate the automation call doesn't require a browser.

ChromeDriver always uses AutomationMsg_SendJSONRequest using the browser handle
as -1. It does on a few occasions use the automation proxy and browser proxy
directly, but those uses are for Chrome 17- and I need to go ahead and delete
those from the code. So I don't see why the handle message needs to check for
>=0.

I don't want you to move AutomationMsg_SendJSONRequest to here, just leave it
where it is but change the comment and use so that the first arg is a
window_index instead of browser_handle.

About your comment about being safe, if you are really worried about missing
something, you could just change the name of the index one to
AutomationMsg_SendJSONRequestWithBrowserIndex. That should break compile for
anything you forgot. You could also just change the name of the new one to
AutomationMsg_SendJSONRequest if you want.

Basically, doing all of this would be better for ChromeDriver. I don't have to
add an additional hack to change the message IDs when the old one is removed.
I'm ok with letting the automation messages change if necessary, but in this
case I don't see any cost in making it easy for ChromeDriver. If there is a cost
I haven't seen, let me know and we can just follow what you have.

http://codereview.chromium.org/10534163/diff/10001/chrome/browser/automation/...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/10534163/diff/10001/chrome/browser/automation/...
chrome/browser/automation/testing_automation_provider.h:273: static
scoped_ptr<DictionaryValue*> ParseJSONRequestCommand(
I think you want scoped_ptr<DictionaryValue>

[craigdh, 2012-06-19 23:28:52]

http://codereview.chromium.org/10534163/diff/4001/chrome/common/automation_me...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/10534163/diff/4001/chrome/common/automation_me...
chrome/common/automation_messages_internal.h:955:
IPC_SYNC_MESSAGE_CONTROL2_2(AutomationMsg_SendJSONRequestWithBrowserIndex,
On 2012/06/19 16:20:12, kkania wrote:
> On 2012/06/19 00:19:35, craigdh wrote:
> > On 2012/06/18 16:30:33, kkania wrote:
> > > You eventually want to delete AutomationMsg_SendJSONRequest, right? How
> about
> > > you make this one AutomationMsg_SendJSONRequestWithBrowserHandle, and let
> the
> > > other one be the index one. Then you can get rid of the handle >= 0 check
> that
> > > Nirnimesh commented on and I don't have to make compat changes when you
> delete
> > > the other message.
> > 
> > Chromedriver uses AutomationMsg_SendJSONRequest using browser handles.
> Wouldn't
> > moving that IPC here change the IPC id and hence break backwards
compatibility
> > for Chromedriver? Also, renaming AutomationMsg_SendJSONRequest to
> > AutomationMsg_SendJSONRequestWithBrowserHandle would create a problem if I
> > missed a rename somewhere, it's safer to add a new one with new
functionality
> > than switch the behavior of the old one. Also, the check for handle >=0
would
> > still have to stay because automation_json_requests.cc uses -1 as a handle
to
> > indicate the automation call doesn't require a browser.
> 
> ChromeDriver always uses AutomationMsg_SendJSONRequest using the browser
handle
> as -1. It does on a few occasions use the automation proxy and browser proxy
> directly, but those uses are for Chrome 17- and I need to go ahead and delete
> those from the code. So I don't see why the handle message needs to check for
> >=0.
> 
> I don't want you to move AutomationMsg_SendJSONRequest to here, just leave it
> where it is but change the comment and use so that the first arg is a
> window_index instead of browser_handle.
> 
> About your comment about being safe, if you are really worried about missing
> something, you could just change the name of the index one to
> AutomationMsg_SendJSONRequestWithBrowserIndex. That should break compile for
> anything you forgot. You could also just change the name of the new one to
> AutomationMsg_SendJSONRequest if you want.
> 
> Basically, doing all of this would be better for ChromeDriver. I don't have to
> add an additional hack to change the message IDs when the old one is removed.
> I'm ok with letting the automation messages change if necessary, but in this
> case I don't see any cost in making it easy for ChromeDriver. If there is a
cost
> I haven't seen, let me know and we can just follow what you have.

I did not realize Chromedriver only passes -1 as the browser handle. It has been
switched to the way you suggested.

http://codereview.chromium.org/10534163/diff/10001/chrome/browser/automation/...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/10534163/diff/10001/chrome/browser/automation/...
chrome/browser/automation/testing_automation_provider.h:273: static
scoped_ptr<DictionaryValue*> ParseJSONRequestCommand(
On 2012/06/19 16:20:12, kkania wrote:
> I think you want scoped_ptr<DictionaryValue>

Yes, compiler caught that one but I forgot to re-upload with it corrected :-)

[kkania, 2012-06-20 00:19:45]

lgtm thanks

http://codereview.chromium.org/10534163/diff/17001/chrome/common/automation_m...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/10534163/diff/17001/chrome/common/automation_m...
chrome/common/automation_messages_internal.h:956: int /* window_index */,
You need to update this, since it's a handle, not a window index. Also, you need
to update the comment for SendJSONRequest that it takes a index, not a handle.

[craigdh, 2012-06-20 00:33:16]

http://codereview.chromium.org/10534163/diff/17001/chrome/common/automation_m...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/10534163/diff/17001/chrome/common/automation_m...
chrome/common/automation_messages_internal.h:956: int /* window_index */,
On 2012/06/20 00:19:45, kkania wrote:
> You need to update this, since it's a handle, not a window index. Also, you
need
> to update the comment for SendJSONRequest that it takes a index, not a handle.

Done.

[commit-bot: I haz the power, 2012-06-27 22:11:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/craigdh@chromium.org/10534163/22001

[commit-bot: I haz the power, 2012-06-27 23:48:25]

Change committed as 144610

[craigdh, 2012-06-29 19:13:41]

Please take another look. Changed the IPCs to be backwards compatible with old
chrome binaries (to the best of my knowledge).

[kkania, 2012-07-02 17:57:00]

i'm not sure it's recommended practice to re-use the same issue for multiple
CLs. I've already taken a look and it lgtm. Just create a new issue and I'll
approve it.