Convert cookie and download automation commands to the JSON interface.

Convert cookie and download automation commands to the JSON interface.

GetCookie, SetCookie, DeleteCookie, SetDownloadShelfVisible,
IsDownloadShelfVisible, GetDownloadDirectory

BUG=132794
TEST=PyAuto CONTINUOUS suite


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

[craigdh, 2012-07-19 23:40:08]

and today's conversions...

[Nirnimesh, 2012-07-20 02:19:46]

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

https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:6711: void
TestingAutomationProvider::SetTabCookieJSON(DictionaryValue* args,
What is setTabCookie for? I thought cookies were keyed against URLs, not tabs.

[craigdh, 2012-07-20 15:27:43]

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

https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:6711: void
TestingAutomationProvider::SetTabCookieJSON(DictionaryValue* args,
On 2012/07/20 02:19:46, Nirnimesh wrote:
> What is setTabCookie for? I thought cookies were keyed against URLs, not tabs.

If you think of a better name, let me know. The name is to differentiate these
commands, which set a cookie in the context of a tab, and the chromedriver
functions, which take a different set of parameters and set the cookie in the
context of a profile. They all take a url that the cookie is applicable to.

[craigdh, 2012-07-20 18:49:59]

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

https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:6711: void
TestingAutomationProvider::SetTabCookieJSON(DictionaryValue* args,
On 2012/07/20 15:27:43, craigdh wrote:
> On 2012/07/20 02:19:46, Nirnimesh wrote:
> > What is setTabCookie for? I thought cookies were keyed against URLs, not
tabs.
> 
> If you think of a better name, let me know. The name is to differentiate these
> commands, which set a cookie in the context of a tab, and the chromedriver
> functions, which take a different set of parameters and set the cookie in the
> context of a profile. They all take a url that the cookie is applicable to.

How about 'SetCookieInTab'?

[Nirnimesh, 2012-07-20 20:52:26]

On 2012/07/20 18:49:59, craigdh wrote:
>
https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/browser/auto...
> File chrome/browser/automation/testing_automation_provider.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/browser/auto...
> chrome/browser/automation/testing_automation_provider.cc:6711: void
> TestingAutomationProvider::SetTabCookieJSON(DictionaryValue* args,
> On 2012/07/20 15:27:43, craigdh wrote:
> > On 2012/07/20 02:19:46, Nirnimesh wrote:
> > > What is setTabCookie for? I thought cookies were keyed against URLs, not
> tabs.
> > 
> > If you think of a better name, let me know. The name is to differentiate
these
> > commands, which set a cookie in the context of a tab, and the chromedriver
> > functions, which take a different set of parameters and set the cookie in
the
> > context of a profile. They all take a url that the cookie is applicable to.
> 
> How about 'SetCookieInTab'?

What does 'Tab' have to do with setting a cookie?

[craigdh, 2012-07-20 21:07:05]

On 2012/07/20 20:52:26, Nirnimesh wrote:
> On 2012/07/20 18:49:59, craigdh wrote:
> >
>
https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/browser/auto...
> > File chrome/browser/automation/testing_automation_provider.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/browser/auto...
> > chrome/browser/automation/testing_automation_provider.cc:6711: void
> > TestingAutomationProvider::SetTabCookieJSON(DictionaryValue* args,
> > On 2012/07/20 15:27:43, craigdh wrote:
> > > On 2012/07/20 02:19:46, Nirnimesh wrote:
> > > > What is setTabCookie for? I thought cookies were keyed against URLs, not
> > tabs.
> > > 
> > > If you think of a better name, let me know. The name is to differentiate
> these
> > > commands, which set a cookie in the context of a tab, and the chromedriver
> > > functions, which take a different set of parameters and set the cookie in
> the
> > > context of a profile. They all take a url that the cookie is applicable
to.
> > 
> > How about 'SetCookieInTab'?
> 
> What does 'Tab' have to do with setting a cookie?

A tab corresponds to a WebContents. A cookie is set in the context of a
WebContents: for example if you have an incognito window and two profiles open,
the cookie is only set in the one the tab corresponds to.

[Nirnimesh, 2012-07-20 22:16:20]

https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/test/pyautol...
File chrome/test/pyautolib/pyauto.py (right):

https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1388: return
self._GetResultFromJSONRequest(cmd_dict, windex=windex)['cookies']
this should probably be windex=None?

[craigdh, 2012-07-20 23:23:41]

https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/test/pyautol...
File chrome/test/pyautolib/pyauto.py (right):

https://chromiumcodereview.appspot.com/10804038/diff/3001/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1388: return
self._GetResultFromJSONRequest(cmd_dict, windex=windex)['cookies']
On 2012/07/20 22:16:20, Nirnimesh wrote:
> this should probably be windex=None?

Done.

[Nirnimesh, 2012-07-21 05:08:34]

LGTM

[craigdh, 2012-07-24 21:31:56]

+jam: owners approval for more refactoring towards reducing the automation to
one IPC channel.

[jam, 2012-07-24 21:38:28]

why aren't these changes deleting the unused methods on BrowserProxy, marking
the IPCs as deprecated, and removing the IPC dispatching code in
TestingAutomationProvider?

[craigdh, 2012-07-24 21:44:41]

On 2012/07/24 21:38:28, John Abd-El-Malek wrote:
> why aren't these changes deleting the unused methods on BrowserProxy, marking
> the IPCs as deprecated, and removing the IPC dispatching code in
> TestingAutomationProvider?

The BrowserProxy (and TabProxy, etc) are used by some performance binaries that
run new test binaries against old chrome binaries. So I am first transitioning
all of PyAuto to the JSON interface, then I will delete all the proxy calls in
one change so that binary compatibility is only broken once (with all the
updating of expectation files, etc that that entails).

[jam, 2012-07-24 21:50:18]

On 2012/07/24 21:44:41, craigdh wrote:
> On 2012/07/24 21:38:28, John Abd-El-Malek wrote:
> > why aren't these changes deleting the unused methods on BrowserProxy,
marking
> > the IPCs as deprecated, and removing the IPC dispatching code in
> > TestingAutomationProvider?
> 
> The BrowserProxy (and TabProxy, etc) are used by some performance binaries
that
> run new test binaries against old chrome binaries. So I am first transitioning
> all of PyAuto to the JSON interface, then I will delete all the proxy calls in
> one change so that binary compatibility is only broken once (with all the
> updating of expectation files, etc that that entails).

My suggestion doesn't break automation IPC compatibility. i.e. marking the IPC
as deprecated and removing the other code on
BrowserProxy/TabProxy/TestingAutomationProvider.

[craigdh, 2012-07-24 22:03:09]

On 2012/07/24 21:50:18, John Abd-El-Malek wrote:
> On 2012/07/24 21:44:41, craigdh wrote:
> > On 2012/07/24 21:38:28, John Abd-El-Malek wrote:
> > > why aren't these changes deleting the unused methods on BrowserProxy,
> marking
> > > the IPCs as deprecated, and removing the IPC dispatching code in
> > > TestingAutomationProvider?
> > 
> > The BrowserProxy (and TabProxy, etc) are used by some performance binaries
> that
> > run new test binaries against old chrome binaries. So I am first
transitioning
> > all of PyAuto to the JSON interface, then I will delete all the proxy calls
in
> > one change so that binary compatibility is only broken once (with all the
> > updating of expectation files, etc that that entails).
> 
> My suggestion doesn't break automation IPC compatibility. i.e. marking the IPC
> as deprecated and removing the other code on
> BrowserProxy/TabProxy/TestingAutomationProvider.

Possibly I'm misunderstanding your suggestion. I removed the unused functions
from pyautolib.cc, but I can not remove the functions from BrowserProxy,
TabProxy, or handlers from TestingAutomationProvider as this code is used to
build performance test binaries. New versions of these test binaries are run
against both current and old chrome binaries, so removing the code on either
side of the IPC breaks compatibility.

[jam, 2012-07-25 00:20:04]

On 2012/07/24 22:03:09, craigdh wrote:
> On 2012/07/24 21:50:18, John Abd-El-Malek wrote:
> > On 2012/07/24 21:44:41, craigdh wrote:
> > > On 2012/07/24 21:38:28, John Abd-El-Malek wrote:
> > > > why aren't these changes deleting the unused methods on BrowserProxy,
> > marking
> > > > the IPCs as deprecated, and removing the IPC dispatching code in
> > > > TestingAutomationProvider?
> > > 
> > > The BrowserProxy (and TabProxy, etc) are used by some performance binaries
> > that
> > > run new test binaries against old chrome binaries. So I am first
> transitioning
> > > all of PyAuto to the JSON interface, then I will delete all the proxy
calls
> in
> > > one change so that binary compatibility is only broken once (with all the
> > > updating of expectation files, etc that that entails).
> > 
> > My suggestion doesn't break automation IPC compatibility. i.e. marking the
IPC
> > as deprecated and removing the other code on
> > BrowserProxy/TabProxy/TestingAutomationProvider.
> 
> Possibly I'm misunderstanding your suggestion. I removed the unused functions
> from pyautolib.cc, but I can not remove the functions from BrowserProxy,
> TabProxy, or handlers from TestingAutomationProvider as this code is used to
> build performance test binaries. New versions of these test binaries are run
> against both current and old chrome binaries, so removing the code on either
> side of the IPC breaks compatibility.

I understand what you're trying to guard against. How that's prevented is by not
removing the unused IPCs in automation_messages_internal.h. The convention for
doing that is to add DEPRECATED in the IPC message file for unused messages,
i.e. see
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/automation_m...

As long as you leave these IPCs in place, and don't change the line numbers in
that file, then the automation IPCs from new to old binaries will remain
unchanged. Since in trunk the methods in BrowserProxy/TabProxy are not called by
any code, removing them won't impact with old binaries. Similarly, since the
deprecated IPCs aren't being dispatched by the code in trunk, then you can
remove the code that handles them.

[craigdh, 2012-07-25 19:32:47]

On 2012/07/25 00:20:04, John Abd-El-Malek wrote:
> On 2012/07/24 22:03:09, craigdh wrote:
> > On 2012/07/24 21:50:18, John Abd-El-Malek wrote:
> > > On 2012/07/24 21:44:41, craigdh wrote:
> > > > On 2012/07/24 21:38:28, John Abd-El-Malek wrote:
> > > > > why aren't these changes deleting the unused methods on BrowserProxy,
> > > marking
> > > > > the IPCs as deprecated, and removing the IPC dispatching code in
> > > > > TestingAutomationProvider?
> > > > 
> > > > The BrowserProxy (and TabProxy, etc) are used by some performance
binaries
> > > that
> > > > run new test binaries against old chrome binaries. So I am first
> > transitioning
> > > > all of PyAuto to the JSON interface, then I will delete all the proxy
> calls
> > in
> > > > one change so that binary compatibility is only broken once (with all
the
> > > > updating of expectation files, etc that that entails).
> > > 
> > > My suggestion doesn't break automation IPC compatibility. i.e. marking the
> IPC
> > > as deprecated and removing the other code on
> > > BrowserProxy/TabProxy/TestingAutomationProvider.
> > 
> > Possibly I'm misunderstanding your suggestion. I removed the unused
functions
> > from pyautolib.cc, but I can not remove the functions from BrowserProxy,
> > TabProxy, or handlers from TestingAutomationProvider as this code is used to
> > build performance test binaries. New versions of these test binaries are run
> > against both current and old chrome binaries, so removing the code on either
> > side of the IPC breaks compatibility.
> 
> I understand what you're trying to guard against. How that's prevented is by
not
> removing the unused IPCs in automation_messages_internal.h. The convention for
> doing that is to add DEPRECATED in the IPC message file for unused messages,
> i.e. see
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/automation_m...
> 
> As long as you leave these IPCs in place, and don't change the line numbers in
> that file, then the automation IPCs from new to old binaries will remain
> unchanged. Since in trunk the methods in BrowserProxy/TabProxy are not called
by
> any code, removing them won't impact with old binaries. Similarly, since the
> deprecated IPCs aren't being dispatched by the code in trunk, then you can
> remove the code that handles them.

Functions not used by the perf tests have been removed and the relevant IPCs
marked DEPRECIATED. Both GetCookies and GetCookiesByName could not be removed as
they are used by perf/url_fetch_test.cc.

[jam, 2012-07-25 20:53:21]

lgtm

given that the automation interface will be deprecated, it seems switching from
IPC to json is work that'll be thrown out later. I recommend not spending more
time on this.

[Nirnimesh, 2012-07-25 20:58:11]

On 2012/07/25 20:53:21, John Abd-El-Malek wrote:
> lgtm
> 
> given that the automation interface will be deprecated, it seems switching
from
> IPC to json is work that'll be thrown out later. I recommend not spending more
> time on this.

Actually there's pressing need for chromeos automation to improve automation
proxy errors. Right now due to all the Browser/TabProxy it makes the failure
messages useless. Craig's refactor is to address these problems.

[jam, 2012-07-25 21:05:32]

On 2012/07/25 20:58:11, Nirnimesh wrote:
> On 2012/07/25 20:53:21, John Abd-El-Malek wrote:
> > lgtm
> > 
> > given that the automation interface will be deprecated, it seems switching
> from
> > IPC to json is work that'll be thrown out later. I recommend not spending
more
> > time on this.
> 
> Actually there's pressing need for chromeos automation to improve automation
> proxy errors. Right now due to all the Browser/TabProxy it makes the failure
> messages useless. Craig's refactor is to address these problems.

I'm not sure I understand the kind of errors you're trying to solve against?
There are a few scenarios I can think of:
-changes to IPC serialization: this won't fix it since there's still at least
one IPC used
-IPC messages being reordered and so their IDs change: this hurts chrome tests
that use reference builds, so we avoid doing this. I did it a few months ago to
clear up all the deprecated messages that had built up over the last year (the
last time the message IDs changed). I don't see the message IDs changing from
now until we remove the automation code.

Am I missing another scenario?

[Nirnimesh, 2012-07-25 21:11:16]

On 2012/07/25 21:05:32, John Abd-El-Malek wrote:
> On 2012/07/25 20:58:11, Nirnimesh wrote:
> > On 2012/07/25 20:53:21, John Abd-El-Malek wrote:
> > > lgtm
> > > 
> > > given that the automation interface will be deprecated, it seems switching
> > from
> > > IPC to json is work that'll be thrown out later. I recommend not spending
> more
> > > time on this.
> > 
> > Actually there's pressing need for chromeos automation to improve automation
> > proxy errors. Right now due to all the Browser/TabProxy it makes the failure
> > messages useless. Craig's refactor is to address these problems.
> 
> I'm not sure I understand the kind of errors you're trying to solve against?
> There are a few scenarios I can think of:
> -changes to IPC serialization: this won't fix it since there's still at least
> one IPC used
> -IPC messages being reordered and so their IDs change: this hurts chrome tests
> that use reference builds, so we avoid doing this. I did it a few months ago
to
> clear up all the deprecated messages that had built up over the last year (the
> last time the message IDs changed). I don't see the message IDs changing from
> now until we remove the automation code.
> 
> Am I missing another scenario?

The kind of issues I'm talking about involve BrowserProxy & TabProxy. It's
different from the ones you've mentioned above.
Craig is trying to totally get rid of them. The motivation is:

- When executing a BrowserProxy/TabProxy automation call, if the corresponding
window/tab is invalid (missing or crashed), the output that we get from the test
run is just a bunch of memory addresses. The automation calls in the json
interface use window and tab indices, with corresponding checks that reply with
the right error message in such cases. These cases are very common on chromeos
at the moment, since it's very crashy.
- Getting rid of the BrowserProxy/TabProxy and all the automation handle
tracking will actually help with the transition to the new automation system.
One less dependency.

[commit-bot: I haz the power, 2012-07-27 17:48:24]

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

[commit-bot: I haz the power, 2012-07-27 18:33:19]

Try job failure for 10804038-20001 on linux_chromeos for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

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

[commit-bot: I haz the power, 2012-07-27 18:44:23]

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

[commit-bot: I haz the power, 2012-07-27 21:14:59]

Change committed as 148809