[chromedriver] Load the automation extension as a component extension.

[chromedriver] Use single path for handling android/desktop args.

-also allow extensions to be loaded via --load-extension
-change useExistingBrowser to debuggerAddress
BUG=chromedriver:488
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220610

[chrisgao (Use stgao instead), 2013-08-28 17:01:58]

https://codereview.chromium.org/23643005/diff/3001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/23643005/diff/3001/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome_launcher.cc:523:
command->AppendSwitchNative("load-component-extension",
If the users pass in an argument "--load-component-extension=path", it fails
like the current case in the bug.
I remember there was a crash if use "load-component-extension" without
"disable-extensions", although not 100% sure which version of chrome it was.

How about doing it this way?
If "load-component-extension" is passed in, we update its value instead of
adding a switch with the same name.

[chrisgao (Use stgao instead), 2013-08-29 22:53:32]

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:53: if (str.empty())
I remember one webdriver client pass empty value for chromeOption.binary by
default.
If it isn't fixed yet, this may lead to break.

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:89:
capabilities->switches.SetSwitch(arg_string);
An arg_string could be "--load-extension=/path/to/ext".
We need to parse it though.
Using SetUnparsedSwitch instead?

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:102: Status ParseLocalState(const
base::Value& option, Capabilities* capabilities) {
Add a new function ParseDict instead of both ParsePrefs and ParseLocalState?

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:223: return Status(kUnknownError, "port
must be >= 0");
">=" to ">"

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:255: parser_map["extensions"] =
base::Bind(&IgnoreCapability);
This function is for chromeOptions.*
But args, binary, and extensions here are in the same level as chromeOptions,
right?
They should be in Capabilities::Parse instead.

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:268: parser_map["useExistingBrowser"] =
base::Bind(&ParseUseExistingBrowser);
not "debugger_address"?

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:282: parser_map["switches"] =
base::Bind(&ParseSwitches);
What's this new "switches" for? Equivalent to "args"?

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.h:37: void SetUnparsedSwitch(const
std::string& unparsed_switch);
Add a comment on its difference from those above?

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.h:91: std::set<std::string>
exclude_switches;
exclude_switches has no effect anymore?

[chrisgao (Use stgao instead), 2013-08-30 02:12:55]

lgtm

Fix issues discussed offline.

https://codereview.chromium.org/23643005/diff/19001/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/23643005/diff/19001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_launcher.cc:129:
switches->SetFromSwitches(capabilities.switches);
including excludeSwitches,
automation-extension might be overwritten.

[kkania, 2013-08-30 03:14:56]

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:53: if (str.empty())
On 2013/08/29 22:53:32, chrisgao wrote:
> I remember one webdriver client pass empty value for chromeOption.binary by
> default.
> If it isn't fixed yet, this may lead to break.

Let's have them fix their side.

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:89:
capabilities->switches.SetSwitch(arg_string);
On 2013/08/29 22:53:32, chrisgao wrote:
> An arg_string could be "--load-extension=/path/to/ext".
> We need to parse it though.
> Using SetUnparsedSwitch instead?

Done.

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:102: Status ParseLocalState(const
base::Value& option, Capabilities* capabilities) {
On 2013/08/29 22:53:32, chrisgao wrote:
> Add a new function ParseDict instead of both ParsePrefs and ParseLocalState?

Done.

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:223: return Status(kUnknownError, "port
must be >= 0");
On 2013/08/29 22:53:32, chrisgao wrote:
> ">=" to ">"

Done.

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:255: parser_map["extensions"] =
base::Bind(&IgnoreCapability);
On 2013/08/29 22:53:32, chrisgao wrote:
> This function is for chromeOptions.*
> But args, binary, and extensions here are in the same level as chromeOptions,
> right?
> They should be in Capabilities::Parse instead.

talked offline

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:268: parser_map["useExistingBrowser"] =
base::Bind(&ParseUseExistingBrowser);
On 2013/08/29 22:53:32, chrisgao wrote:
> not "debugger_address"?

Done.

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.cc:282: parser_map["switches"] =
base::Bind(&ParseSwitches);
On 2013/08/29 22:53:32, chrisgao wrote:
> What's this new "switches" for? Equivalent to "args"?

It's a synonym for args. Deleted.

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/capabilities.h (right):

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.h:37: void SetUnparsedSwitch(const
std::string& unparsed_switch);
On 2013/08/29 22:53:32, chrisgao wrote:
> Add a comment on its difference from those above?

Done.

https://codereview.chromium.org/23643005/diff/8001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities.h:91: std::set<std::string>
exclude_switches;
On 2013/08/29 22:53:32, chrisgao wrote:
> exclude_switches has no effect anymore?

Done.

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities.cc:133: CommandLine::StringType
proxy_pac_url;
string

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities.cc:392: value =
base::GetDoubleQuotedJson(value);
will this work with adb_impl?

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_launcher.cc:106: if
(!switches->HasSwitch("user-data-dir")) {
capabilities.switches

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_launcher.cc:373: for (size_t i = 0; i <
arraysize(kCommonSwitches); i++)
++i

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_launcher.cc:513: Switches* switches,
required_switches

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_launcher.h (right):

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_launcher.h:44: Switches* switches,
required_

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_launcher_unittest.cc (right):

https://codereview.chromium.org/23643005/diff/25001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_launcher_unittest.cc:143: Switches switches;
?

[commit-bot: I haz the power, 2013-08-30 04:21:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/23643005/41001

[commit-bot: I haz the power, 2013-08-30 07:55:53]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-30 17:07:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/23643005/41001

[commit-bot: I haz the power, 2013-08-30 18:16:46]

Change committed as 220610