Let pyauto create an attached webdriver instance to manipulate web pages.

Let pyauto create an attached webdriver instance to manipulate web pages.
BUG=49379
TEST=none

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

[kkania, 2011-07-29 19:09:44]

hnguyen: chrome/test/webdriver
nirnimesh: all else

[Huyen, 2011-07-29 20:28:36]

LGTM on webdriver side

[Nirnimesh, 2011-07-29 20:47:13]

Since this is a rather large change, I'll take some time to respond.
Meanwhile adding dtu to go over the launcher changes.

[Paweł Hajdan Jr., 2011-07-29 20:48:38]

Drive-by with automation comments.

Please always wait for my review, chrome/test/OWNERS.

http://codereview.chromium.org/7523060/diff/3001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/7523060/diff/3001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider.cc:159:
base::ThreadRestrictions::ScopedAllowIO allow_io;
Let's do it on the right thread. Who knows what races we might create otherwise.

http://codereview.chromium.org/7523060/diff/3001/chrome/test/automation/proxy...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/7523060/diff/3001/chrome/test/automation/proxy...
chrome/test/automation/proxy_launcher.h:295: bool wait_for_server_channel,
3 bool parameters next to each other are a recipe for some mix-up. Please use a
less error-prone solution.

Actually, if you don't wait here, how do you know when the channel is ready?

[dtu, 2011-07-29 21:57:30]

ProxyLauncher/AutomationProvider/PyAuto stuff LGTM, but also wait for phajdan.jr
and nirnimesh.

http://codereview.chromium.org/7523060/diff/3001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/7523060/diff/3001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider.cc:425:
base::ThreadRestrictions::ScopedAllowIO allow_io;
Remove this line since it's going in InitializeChannel() now.

http://codereview.chromium.org/7523060/diff/3001/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/7523060/diff/3001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3755: global _WEBDRIVER_FACTORY
nirnimesh: will we also need to add this to the PyAuto dep for autotest?

[kkania, 2011-07-29 22:44:18]

Pawel: do you want an owner to be put on all chrome/test changes? If so, you
could set noparent in the OWNERS file? Or are you just talking about this CL?

http://codereview.chromium.org/7523060/diff/3001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/7523060/diff/3001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider.cc:159:
base::ThreadRestrictions::ScopedAllowIO allow_io;
I believe it is on the right thread.

This is only needed on POSIX when creating a MODE_NAMED_SERVER channel. When
creating a NAMED_SERVER channel, ipc_channel_posix.cc calls
file_util::CreateDirectory to ensure the channel name (which is a path), has a
parent dir. Usually an IPC::Channel is initialized only on the IO thread, so
this is fine. However, when using a ChannelProxy in SERVER mode, the channel is
created immediately to avoid a race (see ChannelProxy::Init). Thus, we end up
calling file_util on the UI thread in normal conditions. There's only 1 other
use of MODE_NAMED_SERVER, which is not in the browser process and looks like it
doesn't have any thread restrictions.

So here, I do not think this is a race. It is just a case of doing IO on the UI
thread, which you should avoid. If you agree, I see three options:
1) keep this here and comment
2) move this allow IO to ChannelProxy::Init when creating a channel immediately
3) change the IPC code to, in the case of a NAMED_SERVER, not create the channel
immediately, since this requires possibly creating a directory on POSIX

We already do ScopedAllowIO in AutomationProvider::ReinitializeChannel, as dtu
points out below, for I assume the same reason.

http://codereview.chromium.org/7523060/diff/3001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider.cc:425:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2011/07/29 21:57:31, Dave Tu wrote:
> Remove this line since it's going in InitializeChannel() now.

Good catch. I'll move if Pawel is ok with having it there in the first place.

http://codereview.chromium.org/7523060/diff/3001/chrome/test/automation/proxy...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/7523060/diff/3001/chrome/test/automation/proxy...
chrome/test/automation/proxy_launcher.h:295: bool wait_for_server_channel,
Changed to struct.

We know the channel is ready because the
TestingAutomationProvider::CreateNewAutomationProvider method does not return
until the server socket is listening, because the ChannelProxy creates it
immediately on the UI thread.

[Paweł Hajdan Jr., 2011-07-30 00:03:03]

Sorry, by "please always wait for my review, chrome/test/OWNERS." I meant
"please do not commit without my explicit approval", just to avoid any possible
"shortcuts".

http://codereview.chromium.org/7523060/diff/3001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/7523060/diff/3001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider.cc:159:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2011/07/29 22:44:19, kkania wrote:
> I believe it is on the right thread.

Then you shouldn't need ScopedAllowIO here.

> So here, I do not think this is a race. It is just a case of doing IO on the
UI
> thread, which you should avoid. If you agree, I see three options:
> 1) keep this here and comment

No, comment is not a fix.

> 2) move this allow IO to ChannelProxy::Init when creating a channel
immediately

I believe it's too risky, but I'm fine if ipc/ OWNERS are fine with that.

> 3) change the IPC code to, in the case of a NAMED_SERVER, not create the
channel
> immediately, since this requires possibly creating a directory on POSIX

Hmm, or how about just doing the thing on the right thread?

> We already do ScopedAllowIO in AutomationProvider::ReinitializeChannel, as dtu
> points out below, for I assume the same reason.

Bleh, existing code that's bad is no excuse. Ideally we should fix both. I'd
like that. :-D

[Nirnimesh, 2011-08-01 19:21:24]

Love this CL.
Excited to see this coming.

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

http://codereview.chromium.org/7523060/diff/13002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:1157: //   output: none
remove this line

http://codereview.chromium.org/7523060/diff/13002/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:300: bool wait_for_server_channel;
Why is this necessary? In what case would you set this to false?

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
File chrome/test/functional/pyauto_webdriver.py (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
chrome/test/functional/pyauto_webdriver.py:1: #!/usr/bin/python
Love this file

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
chrome/test/functional/pyauto_webdriver.py:16: self.assertEquals(1,
driver.execute_script('return 1'))
Could you have it do something more webdriver-y? maybe fetch the value of a dom
element?

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
chrome/test/functional/pyauto_webdriver.py:20: driver = self.NewWebDriver()
driver.Stop()?

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:56: os.path.dirname(__file__),
this should go before third_party

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:89: from webdriver_factory import
WebDriverFactory
This would work if you have all the webdriver files available on the machine,
which is true for the buildbots. But it'll break on chromeos, where we hand pick
which files need to end up in the autotest package. For now, could you move this
import inside the function please?

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
File chrome/test/pyautolib/webdriver_factory.py (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:1: #!/usr/bin/python
I see 'webdriver' and 'chromedriver' used interchangeably. Please pick one and
use throughout this file. Should the filename be chromedriver_factory.py?

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:28: """Creates a new remote WebDriver
instance.
Why 'remote'?

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:31: running Chrome.
nit: leave a blank line before Args: and Returns:

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:40: channel_id =
os.path.join(tempfile.gettempdir(), channel_id)
where does this file reside on linux?

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:49: if exe is None:
assert exe, 'Cannot find chromedriver exe. Did you build it?'

... will produce equivalent effect

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:54: def Stop(self):
Should the destructor call Stop() automatically to prevent a pileup of
chromedriver processes?

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:56: if self._chromedriver_server is
not None:
Add: self._chromedriver_server = None after this line

[kkania, 2011-08-02 15:10:44]

Pawel,

I went ahead and created a new CL addressing the ScopedAllowIO in
InitializeChannel and added you as a reviewer. I don't want that to block this
CL.

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

http://codereview.chromium.org/7523060/diff/13002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:1157: //   output: none
On 2011/08/01 19:21:24, Nirnimesh wrote:
> remove this line

Done.

http://codereview.chromium.org/7523060/diff/13002/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:300: bool wait_for_server_channel;
This is necessary because the code for waiting for the server channel is broken
on windows. I have another CL that fixes that
(http://codereview.chromium.org/7486007), but it is going slowly and currently
waiting on Darin's comments.

Once that CL is in and I or dtu fixes a race in the named proxy launcher, I can
remove this arg.

I do not need to wait for the server channel for this CL because the
CreateNewAutomationProvider call guarantees the channel will be listening before
it returns (added a comment). It does so because ChannelProxy creates a
listening server channel in its constructor.

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
File chrome/test/functional/pyauto_webdriver.py (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
chrome/test/functional/pyauto_webdriver.py:16: self.assertEquals(1,
driver.execute_script('return 1'))
Changed to get title.

Are you concerned that other features will not work, or do you just want a
normal example testers can look to? If the former, I've run all the webdriver py
tests through pyauto and didn't see any problems (I might add this as a test
step later). If the latter, I can grab dyu's two webdriver tests that he wrote
and stick them in a dom_ui file testers can look at.

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
chrome/test/functional/pyauto_webdriver.py:20: driver = self.NewWebDriver()
this isn't necessary, since pyauto controls the browser and webdriver just
attaches to it. I wasn't planning to support letting chromedriver create/kill
the browser and pyauto to attach to it.

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:56: os.path.dirname(__file__),
On 2011/08/01 19:21:24, Nirnimesh wrote:
> this should go before third_party

Done.

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:89: from webdriver_factory import
WebDriverFactory
On 2011/08/01 19:21:24, Nirnimesh wrote:
> This would work if you have all the webdriver files available on the machine,
> which is true for the buildbots. But it'll break on chromeos, where we hand
pick
> which files need to end up in the autotest package. For now, could you move
this
> import inside the function please?

Done.

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
File chrome/test/pyautolib/webdriver_factory.py (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:1: #!/usr/bin/python
changed to chromedriver

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:28: """Creates a new remote WebDriver
instance.
The remote webdriver is the most configurable. The python bindings for the
ChromeDriver start and stop the chromedriver server for each session, which is
not what we want. The ChromeDriver is just a wrapper around the remote one
anyways.

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:31: running Chrome.
On 2011/08/01 19:21:24, Nirnimesh wrote:
> nit: leave a blank line before Args: and Returns:

Done.

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:40: channel_id =
os.path.join(tempfile.gettempdir(), channel_id)
On 2011/08/01 19:21:24, Nirnimesh wrote:
> where does this file reside on linux?

http://docs.python.org/library/tempfile.html#tempfile.tempdir

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:49: if exe is None:
On 2011/08/01 19:21:24, Nirnimesh wrote:
> assert exe, 'Cannot find chromedriver exe. Did you build it?'
> 
> ... will produce equivalent effect

Done.

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:54: def Stop(self):
It could, but since __del__ isn't called until the garbage collector runs, I
wanted the client to make sure they shut it down properly themselves. Do you
want me to add it?

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:56: if self._chromedriver_server is
not None:
On 2011/08/01 19:21:24, Nirnimesh wrote:
> Add: self._chromedriver_server = None after this line

Done.

[Nirnimesh, 2011-08-02 18:07:13]

LGTM

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
File chrome/test/functional/pyauto_webdriver.py (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
chrome/test/functional/pyauto_webdriver.py:16: self.assertEquals(1,
driver.execute_script('return 1'))
I wanted a real hello-world example. 'return 1' just looks like javascript.

The following would suffice for what I want:

- navigate to a url
- do something webdriver-y. (fill up a form or find a dom element and get its
value)

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
File chrome/test/pyautolib/webdriver_factory.py (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:54: def Stop(self):
On 2011/08/02 15:10:44, kkania wrote:
> It could, but since __del__ isn't called until the garbage collector runs, I
> wanted the client to make sure they shut it down properly themselves. Do you
> want me to add it?

Yes please

__del__ is guaranteed to get called when your object goes out of scope, or is
deleted.

[kkania, 2011-08-02 23:19:19]

just need approval from Pawel now

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
File chrome/test/functional/pyauto_webdriver.py (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/functional/pyau...
chrome/test/functional/pyauto_webdriver.py:16: self.assertEquals(1,
driver.execute_script('return 1'))
On 2011/08/02 18:07:13, Nirnimesh wrote:
> I wanted a real hello-world example. 'return 1' just looks like javascript.
> 
> The following would suffice for what I want:
> 
> - navigate to a url
> - do something webdriver-y. (fill up a form or find a dom element and get its
> value)

Done.

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
File chrome/test/pyautolib/webdriver_factory.py (right):

http://codereview.chromium.org/7523060/diff/13002/chrome/test/pyautolib/webdr...
chrome/test/pyautolib/webdriver_factory.py:54: def Stop(self):
Done.

[Paweł Hajdan Jr., 2011-08-02 23:31:23]

http://codereview.chromium.org/7523060/diff/20020/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/7523060/diff/20020/chrome/browser/automation/a...
chrome/browser/automation/automation_provider.cc:159:
base::ThreadRestrictions::ScopedAllowIO allow_io;
My NAK on this still holds.

[kkania, 2011-08-03 17:38:00]

http://codereview.chromium.org/7523060/diff/20020/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/7523060/diff/20020/chrome/browser/automation/a...
chrome/browser/automation/automation_provider.cc:159:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2011/08/02 23:31:24, Paweł Hajdan Jr. wrote:
> My NAK on this still holds.

Fixing this problem will require some significant changes, which is why I split
it into a separate CL. I don't see a reason to block this CL waiting for that to
occur. This CL does not introduce the core problem here, although it reuses some
bad code. What do you think?

Added TODO to remove this when underlying issue is fixed.

[Paweł Hajdan Jr., 2011-08-03 19:09:16]

http://codereview.chromium.org/7523060/diff/20020/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/7523060/diff/20020/chrome/browser/automation/a...
chrome/browser/automation/automation_provider.cc:159:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2011/08/03 17:38:00, kkania wrote:
> On 2011/08/02 23:31:24, Paweł Hajdan Jr. wrote:
> > My NAK on this still holds.
> 
> Fixing this problem will require some significant changes

Then please do those changes. What is so hard about it?

[Nirnimesh, 2011-08-04 00:44:54]

Pawel, what is the race you are referring to here?

If I understand correctly (and Dave can correct me if I'm wrong), the initial
TestingAutomationProvider gets instantiated before any of the thread
restrictions are imposed. So it's channels are all set up without any problems.
Later when the webdriver automation channel is spun off, it seems only fair that
it does the same things, except that it now has to forgo the thread
restrictions.

Remember, this is test automation code, which consciously allows the channels to
get created, by design. base::ThreadRestrictions::ScopedAllowIO exists for a
reason.

[Paweł Hajdan Jr., 2011-08-04 18:05:46]

On 2011/08/04 00:44:54, Nirnimesh wrote:
> Pawel, what is the race you are referring to here?

I mentioned races by saying "Who knows what races we might create otherwise."
The idea is that even if there are no races now, the number of clients of
automation is growing. There is a work in progress to have multiple automation
providers for multiple automation clients, so we may be well after startup at
that point.

If it's hard to do it correctly now, imagine how much of a pain it's going to be
when even more people will start using it.

> If I understand correctly (and Dave can correct me if I'm wrong), the initial
> TestingAutomationProvider gets instantiated before any of the thread
> restrictions are imposed.

I have no idea if that's true. However, if it is, why would you need
ScopedAllowIO at all?

> So it's channels are all set up without any problems.
> Later when the webdriver automation channel is spun off, it seems only fair
that
> it does the same things, except that it now has to forgo the thread
> restrictions.

Which is bad. Even if it works now, it's just fragile. We should be very
confident that the automation is solid, and at least I can't when things are
done on unexpected threads.

> Remember, this is test automation code, which consciously allows the channels
to
> get created, by design. base::ThreadRestrictions::ScopedAllowIO exists for a
> reason.

It seems to me it's only there to make transition of existing code easier. Also,
"this is automation code" does not mean people can do anything with it. It's
used by ChromeFrame and other pieces of code, so it's important to keep it in
good shape.

[Paweł Hajdan Jr., 2011-08-05 17:51:32]

Okay, so only remaining issue is ScopedAllowIO here. When it disappears from
this CL, feel free to commit without waiting for me.

I was talking with Ken just now and ideally please wait with checking this in
until the issue with ScopedAllowIO is solved properly. Thanks!

[kkania, 2011-08-10 20:48:05]

I started on a few changes to fix the ScopedAllowIO problem but found it was a
bit harder than I originally expected. 
I've moved the ScopedAllowIO from AutomationProvider::InitializeChannel to
TestingAutomationProvider::CreateNewAutomationChannel since the latter function
is the only place that really needs it.

Fixing the ScopedAllowIO problem is most likely not worthwhile. It is only used
in testing code currently (see discussion about this below).

I would like to go ahead and commit this. If you still object Pawel, I can add
another test or IPC owner to get their opinion.



----Here's my overall findings and summary of the issue----

The ScopedAllowIO problem is that we are creating a named server channel on the
UI thread. Creating and closing a named server channel on POSIX requires some
disk operations (see CreateServerUnixDomainSocket). Named server channels are
only used in the automation code (when using the NamedTestingInterface path,
which is not in ChromeFrame) and in the chrome service process, which allows
disk IO on its thread. The reason a named server channel can be created at all
in AutomationProvider::InitializeChannel is that on startup disk IO is allowed
until the main message loop is run (see browser_main.cc RunUIMessageLoop).

I think allowing disk IO on the UI thread only in testing code is not preferable
but is ok, given the difficulty of the solution. Currently there are several
uses of ScopedAllowIO in automation and there are times when we just block the
UI thread with a WaitableEvent that is signaled on a different thread we post a
task to (see automation_util's cookie code).

Ideally, to solve the problem, I think IPC::Channel and IPC::ChannelProxy should
be changed to handle creating and closing domain server sockets on the file
thread. This is a bit complicated, as the IPC code (not to mention any IPC
clients) expects creating a channel to be synchronous. Also, the IPC code
doesn't have any concept of a file thread. We would need to pass a file message
loop ptr through ChannelProxy to Channel, where it could be used when creating
and closing the socket. There are several complications here that would need to
be addressed. For example, clients may call ChannelProxy::Send before the
channel is ready. This only worked earlier because channel creation and sending
messages happened on the same thread, and the former message was guaranteed to
get there before the latter. This would be no longer valid.

Here's other solutions I tried or were suggested:
1) Create the ChannelProxy on the file thread
This does not work because the ChannelProxy calls all the listener methods on
the thread it is constructed on. Having the listener repost on the UI thread
would be wasteful. Changing ChannelProxy to allow the client to specify the
listener thread would solve that, but would require having to queue messages
sent before the channel is ready and locking/waitiable event. I think
ScopedAllowIO is better than that.

2) Let automation provider be a named client channel, which does not require
disk IO. I almost finished this approach, but it kept requiring more code and
hacks in chromedriver, so I gave it up.

3) Move the ScopedAllowIO to the IPC code. I think this is a bad idea, because
it hides to named server channel users that the disk will be accessed and could
cause jank.

[Paweł Hajdan Jr., 2011-08-11 22:36:57]

On 2011/08/10 20:48:05, kkania wrote:
> I think allowing disk IO on the UI thread only in testing code is not
preferable
> but is ok, given the difficulty of the solution. Currently there are several
> uses of ScopedAllowIO in automation and there are times when we just block the
> UI thread with a WaitableEvent that is signaled on a different thread we post
a
> task to (see automation_util's cookie code).

Such blocking is bad. Could you file a bug about this (and reference places
where the code does it) and assign the bug to me? It may change behavior of the
browser, or even result in deadlocks.

> Here's other solutions I tried or were suggested:
> Changing ChannelProxy to allow the client to specify the
> listener thread would solve that, but would require having to queue messages
> sent before the channel is ready and locking/waitiable event. I think
> ScopedAllowIO is better than that.

Have you checked that it's really so complicated? I think we may have an
infrastructure to forward things to another thread in a simple way. Have you
tried asking on chromium-dev?

> 2) Let automation provider be a named client channel, which does not require
> disk IO. I almost finished this approach, but it kept requiring more code and
> hacks in chromedriver, so I gave it up.

Do you have more details?

[Paweł Hajdan Jr., 2011-08-11 23:22:42]

LGTM