chrome/common/automation_messages_internal.h - Issue 10534163: First pass at refactoring pyautolib in preparation for removing proxy dependencies.

Unified Diff: chrome/common/automation_messages_internal.h

Issue 10534163: First pass at refactoring pyautolib in preparation for removing proxy dependencies. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Applied Nirnimesh's suggestions. Created 8 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« chrome/browser/automation/testing_automation_provider.cc ('K') | « chrome/browser/automation/testing_automation_provider.cc ('k') | chrome/test/functional/test_pyauto.py » ('j') | chrome/test/pyautolib/pyautolib.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/common/automation_messages_internal.h

diff --git a/chrome/common/automation_messages_internal.h b/chrome/common/automation_messages_internal.h

index a3924009212b9ab6d06de6a6c1255bd7a185eb8d..3812d04eeecf7300d335e1a9ec53871bc1a5614b 100644

--- a/chrome/common/automation_messages_internal.h

+++ b/chrome/common/automation_messages_internal.h

@@ -950,6 +950,14 @@ IPC_SYNC_MESSAGE_CONTROL0_2(AutomationMsg_GetTracingOutput,

IPC_SYNC_MESSAGE_CONTROL0_1(AutomationMsg_GetMachPortCount,

int /* number of Mach ports */)

+// Generic pyauto pattern to help avoid future addition of

+// automation messages.

+IPC_SYNC_MESSAGE_CONTROL2_2(AutomationMsg_SendJSONRequestWithBrowserIndex,

kkania 2012/06/18 16:30:33 You eventually want to delete AutomationMsg_SendJS

craigdh 2012/06/19 00:19:35 Chromedriver uses AutomationMsg_SendJSONRequest us

kkania 2012/06/19 16:20:12 ChromeDriver always uses AutomationMsg_SendJSONReq

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.

craigdh 2012/06/19 23:28:52 I did not realize Chromedriver only passes -1 as t

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.

+ int /* window_index */,

+ std::string /* JSON request */,

+ std::string /* JSON response */,

+ bool /* success */)

// Browser -> renderer messages.

// Requests a snapshot.