[chromedriver] Add chrome.detach option for configuring Chrome not to quit

[chromedriver] Add chrome.detach option for configuring Chrome not to quit
when its automation client disconnects. chrome.detach is currently implemented
by using a NamedProxyLauncher. We also add a new switch that accomplishes
the same purpose.
Also, modify pre-post command execution steps in two ways:
1) wait for loads post execution, in case pyauto issues a non-webdriver
command which expects the load to be finished
2) only  return a frame checking error if no alert is present
BUG=87676, 93438
TEST=none


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

[kkania, 2011-08-16 17:43:35]

phajdan: chrome/*/automation
hnguyen: chrome/test/webdriver

[Paweł Hajdan Jr., 2011-08-16 17:59:16]

This is racy. Please design it so that the setting is set at Chrome start-up
time (in fact AutomationProvider creation time).

[kkania, 2011-08-16 19:57:31]

Is the race you are referring to that a client could connect to Chrome and not
get the chance to set the quit mode before it or another client disconnects
(perhaps accidentally)? This doesn't matter for ChromeDriver. Was there some
scenario you imagined where it matters? Or am I misunderstanding you?

The only way I can see to set this at AutomationProvider creation is by means of
command-line. Any other ideas you have? How about creating an automation-args
switch that takes a JSON string which can include small options like this in it,
along with stuff like channel name and type? Although I don't want to do this
unless I see the reason why such an option has to be set at creation time.

[Paweł Hajdan Jr., 2011-08-17 18:05:56]

On 2011/08/16 19:57:31, kkania wrote:
> Is the race you are referring to that a client could connect to Chrome and not
> get the chance to set the quit mode before it or another client disconnects
> (perhaps accidentally)?

Yes.

> This doesn't matter for ChromeDriver.

ChromeDriver is not the only client of automation. And even if it was, making
_assumptions_ like this just makes it harder to make it reliable.

> Was there some scenario you imagined where it matters?

Just the scenario above. You don't even need multiple clients, just one.

> Or am I misunderstanding you?

Possibly. The idea is that if you do something a racy way, _you_ need to
convince people that it's going to be safe. The "right way" generally has a well
defined rationale.

> The only way I can see to set this at AutomationProvider creation is by means
of
> command-line.

Possibly, or maybe rethink the design.

> Any other ideas you have? How about creating an automation-args
> switch that takes a JSON string which can include small options like this in
it,
> along with stuff like channel name and type?

Why a JSON think when you can do things just "standard" way?
--automation-reinitialize-on-channel-error or something like that.

> Although I don't want to do this
> unless I see the reason why such an option has to be set at creation time.

Races are bad. Even if it _seems_ OK now, it's much easier to design for
non-raciness rather that redesign things later when you have N clients using it.

[kkania, 2011-08-18 18:23:55]

use switch now

[Paweł Hajdan Jr., 2011-08-18 22:35:42]

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

http://codereview.chromium.org/7648053/diff/6002/chrome/browser/automation/au...
chrome/browser/automation/automation_provider.cc:127: if
(CommandLine::ForCurrentProcess()->HasSwitch(
Rather: reinitialize_on_channel_error_(CommandLine...HasSwitch in the
initializer list above please.

[Paweł Hajdan Jr., 2011-08-18 23:15:16]

Automation LGTM

[Huyen, 2011-08-18 23:54:48]

http://codereview.chromium.org/7648053/diff/12001/chrome/test/webdriver/test/...
File chrome/test/webdriver/test/chromedriver_tests.py (right):

http://codereview.chromium.org/7648053/diff/12001/chrome/test/webdriver/test/...
chrome/test/webdriver/test/chromedriver_tests.py:356: os.kill(pid, 0)  # Would
throw if process no longer exists
Maybe assert process exists and fail to be more obvious. It's tough to decipher
if you just get an exception here.

http://codereview.chromium.org/7648053/diff/12001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_automation.cc (right):

http://codereview.chromium.org/7648053/diff/12001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_automation.cc:251: !file_util::Delete(temp_file,
false /* recursive */)) {
I'm not following here, you error out if temp file is not created or if you
can't delete it?

[kkania, 2011-08-19 18:28:32]

http://codereview.chromium.org/7648053/diff/12001/chrome/test/webdriver/test/...
File chrome/test/webdriver/test/chromedriver_tests.py (right):

http://codereview.chromium.org/7648053/diff/12001/chrome/test/webdriver/test/...
chrome/test/webdriver/test/chromedriver_tests.py:356: os.kill(pid, 0)  # Would
throw if process no longer exists
On 2011/08/18 23:54:49, Huyen wrote:
> Maybe assert process exists and fail to be more obvious. It's tough to
decipher
> if you just get an exception here.

Done.

http://codereview.chromium.org/7648053/diff/12001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_automation.cc (right):

http://codereview.chromium.org/7648053/diff/12001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_automation.cc:251: !file_util::Delete(temp_file,
false /* recursive */)) {
that is correct. All I want is a temp filename, not the file. If you know of an
easier and safe way to do this, let me know.

[Huyen, 2011-08-19 19:30:40]

lgtm

[commit-bot: I haz the power, 2011-08-20 04:52:47]

Change committed as 97557