[chromedriver] Implement proxy capability.

[chromedriver] Implement proxy capability.
Added proxy as switches as arguments passed to command line.
BUG=chromedriver:287
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192836

[kkania, 2013-03-29 20:52:46]

https://codereview.chromium.org/13185004/diff/1/chrome/test/chromedriver/comm...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/13185004/diff/1/chrome/test/chromedriver/comm...
chrome/test/chromedriver/commands.cc:48: Status ParseProxy(const
base::DictionaryValue& proxy_dict,
Can we factor the capability parsing out into its own file? Sorta like
webdriver_capabilities_parser?

[chrisgao (Use stgao instead), 2013-04-02 15:37:17]

https://codereview.chromium.org/13185004/diff/5001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/13185004/diff/5001/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:8: #include "base/callback.h"
unused

[kkania, 2013-04-02 16:24:08]

https://codereview.chromium.org/13185004/diff/5001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/capabilities_parser.cc (right):

https://codereview.chromium.org/13185004/diff/5001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities_parser.cc:111: if
(desired_caps.Get("chromeOptions.logPath", &log_path) &&
what do you think about iterating through ChromeOptions instead, so that we can
return an error on an unrecognized capability?

[chrisgao (Use stgao instead), 2013-04-03 18:51:43]

https://codereview.chromium.org/13185004/diff/5001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/capabilities_parser.cc (right):

https://codereview.chromium.org/13185004/diff/5001/chrome/test/chromedriver/c...
chrome/test/chromedriver/capabilities_parser.cc:111: if
(desired_caps.Get("chromeOptions.logPath", &log_path) &&
On 2013/04/02 16:24:08, kkania wrote:
> what do you think about iterating through ChromeOptions instead, so that we
can
> return an error on an unrecognized capability?

Done.

https://codereview.chromium.org/13185004/diff/5001/chrome/test/chromedriver/c...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/13185004/diff/5001/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:8: #include "base/callback.h"
On 2013/04/02 15:37:17, chrisgao wrote:
> unused

Done.

[kkania, 2013-04-03 19:09:35]

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities_parser.cc (right):

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.cc:165: parser_map["proxy"] =
base::Bind(&ParseProxy);
this isn't part of chromeOptions. It's a cross-browser capability

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities_parser.h (right):

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.h:30: base::ListValue args;
how about a std::vector<string> instead or a CommandLine?

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.h:31: const base::DictionaryValue*
prefs;
i think the capabilities object should own these, either in scoped_ptr or a copy

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.h:33: const base::ListValue*
extensions;
how about a std::vector<string>?

[chrisgao (Use stgao instead), 2013-04-06 01:11:25]

ptal

Check unsupported option within "desiredCapabilities.chromeOptions" for desktop
version only, otherwise java tests fail on android.
Don't check unsupported capability under "desiredCapabilities", otherwise java
tests will fail.

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities_parser.h (right):

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.h:30: base::ListValue args;
On 2013/04/03 19:09:35, kkania wrote:
> how about a std::vector<string> instead or a CommandLine?
changed to a CommandLine.

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.h:31: const base::DictionaryValue*
prefs;
On 2013/04/03 19:09:35, kkania wrote:
> i think the capabilities object should own these, either in scoped_ptr or a
copy

Done.

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.h:33: const base::ListValue*
extensions;
On 2013/04/03 19:09:35, kkania wrote:
> how about a std::vector<string>?

Done.

[chrisgao (Use stgao instead), 2013-04-06 01:12:01]

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities_parser.cc (right):

https://codereview.chromium.org/13185004/diff/16002/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.cc:165: parser_map["proxy"] =
base::Bind(&ParseProxy);
On 2013/04/03 19:09:35, kkania wrote:
> this isn't part of chromeOptions. It's a cross-browser capability

Done.

[kkania, 2013-04-06 17:17:51]

lgtm

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities_parser.cc (right):

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.cc:12: #include
"base/stringprintf.h"
base/strings/

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.cc:21: const
base::Callback<bool(const base::FilePath&)>& file_checker,
i notice this is the only place where we use the file checker. We could move
this check out to chrome_launcher.cc, and have this file mostly just do simple
parsing stuff.

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.cc:140: if
(!proxy_dict->GetString("noProxy", & proxy_bypass_list))
no space

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities_parser.h (right):

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.h:5: #ifndef
CHROME_TEST_CHROMEDRIVER_CAPABILITIES_PARSER_H_
how about rename this file capabilities.h, since it doesn't even have a
CapabilitiesParser class anymore.

[chrisgao (Use stgao instead), 2013-04-08 08:26:56]

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities_parser.cc (right):

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.cc:12: #include
"base/stringprintf.h"
On 2013/04/06 17:17:52, kkania wrote:
> base/strings/

no stringprintf.h in directory "base/strings/".
And it should be "base/stringprintf.h".

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.cc:21: const
base::Callback<bool(const base::FilePath&)>& file_checker,
On 2013/04/06 17:17:52, kkania wrote:
> i notice this is the only place where we use the file checker. We could move
> this check out to chrome_launcher.cc, and have this file mostly just do simple
> parsing stuff.

Done.

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.cc:140: if
(!proxy_dict->GetString("noProxy", & proxy_bypass_list))
On 2013/04/06 17:17:52, kkania wrote:
> no space

Done.

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities_parser.h (right):

https://codereview.chromium.org/13185004/diff/34001/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities_parser.h:5: #ifndef
CHROME_TEST_CHROMEDRIVER_CAPABILITIES_PARSER_H_
On 2013/04/06 17:17:52, kkania wrote:
> how about rename this file capabilities.h, since it doesn't even have a
> CapabilitiesParser class anymore.

Done. I had the same idea.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrisgao@chromium.org/13185004/43001

[commit-bot: I haz the power, 2013-04-08 08:36:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 15:41:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrisgao@chromium.org/13185004/43001

[commit-bot: I haz the power, 2013-04-08 15:46:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 18:09:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrisgao@chromium.org/13185004/43001

[commit-bot: I haz the power, 2013-04-08 18:10:09]

Change committed as 192836