[chromedriver] Logging tweaks and fixes.

[chromedriver] Logging tweaks and fix thread leak bug.
-move command logging from http_handler to execute session command
-fix bug where didn't delete thread if session creation failed
-add logging for prefs
-LOG to driver log if on appropriate thread
BUG=none

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

[chrisgao (Use stgao instead), 2013-09-06 21:20:13]

lgtm with nits.

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

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome_launcher.cc:283: Status status =
chrome_desktop->WaitForPageToLoad(
Will this also wait for the automation extension?

If so, we may remove waiting for the automation extension in some other
place.(Forgot where it is...)

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

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:70: init_session_cmd.Run(params, new_id,
callback);
I like this change, although it is a little hard for me to understand at first
as those pieces are distributed in several files...

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:148: VLOG(0) << "COMMAND " << command_name
<< " "
I like command names than guessing from urls :)
Thanks for getting them back.

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:149: << PrettyPrintValue(*params);
No need to check if log is enabled?

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
File chrome/test/chromedriver/server/http_handler.cc (right):

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
chrome/test/chromedriver/server/http_handler.cc:221:
WrapToCommand("TypeElement", base::Bind(&ExecuteSendKeysToElement))),
Should we follow the command names used in the open-source selenium project?
Users may get confused by the difference somehow though.

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
chrome/test/chromedriver/server/http_handler.cc:430: WrapToCommand("DblClick",
base::Bind(&ExecuteMouseDoubleClick))),
"dbl" is kind of inconsistency here.

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
chrome/test/chromedriver/server/http_handler.cc:491:
WrapToCommand("GetLotTypes",
typo, 'lot' -> 'log'

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
chrome/test/chromedriver/session_commands.cc:99: Status status =
capabilities.Parse(*desired_caps);
If there is some warning during parsing like "loadAsync", we still can't log it
to session log at this point.

Can we create the driver_log earlier and update its log level after parsing?

[kkania, 2013-09-06 23:09:29]

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

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome_launcher.cc:283: Status status =
chrome_desktop->WaitForPageToLoad(
On 2013/09/06 21:20:13, chrisgao wrote:
> Will this also wait for the automation extension?
> 
> If so, we may remove waiting for the automation extension in some other
> place.(Forgot where it is...)

It does not.

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

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:70: init_session_cmd.Run(params, new_id,
callback);
On 2013/09/06 21:20:13, chrisgao wrote:
> I like this change, although it is a little hard for me to understand at first
> as those pieces are distributed in several files...

Done.

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:148: VLOG(0) << "COMMAND " << command_name
<< " "
On 2013/09/06 21:20:13, chrisgao wrote:
> I like command names than guessing from urls :)
> Thanks for getting them back.

Done.

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:149: << PrettyPrintValue(*params);
On 2013/09/06 21:20:13, chrisgao wrote:
> No need to check if log is enabled?

Done.

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
File chrome/test/chromedriver/server/http_handler.cc (right):

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
chrome/test/chromedriver/server/http_handler.cc:221:
WrapToCommand("TypeElement", base::Bind(&ExecuteSendKeysToElement))),
On 2013/09/06 21:20:13, chrisgao wrote:
> Should we follow the command names used in the open-source selenium project?
> Users may get confused by the difference somehow though.

The command names and methods vary by client I think. I chose Type just because
it is shorter and conveys the same idea.

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
chrome/test/chromedriver/server/http_handler.cc:430: WrapToCommand("DblClick",
base::Bind(&ExecuteMouseDoubleClick))),
On 2013/09/06 21:20:13, chrisgao wrote:
> "dbl" is kind of inconsistency here.

Done.

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
chrome/test/chromedriver/server/http_handler.cc:491:
WrapToCommand("GetLotTypes",
On 2013/09/06 21:20:13, chrisgao wrote:
> typo, 'lot' -> 'log'

Done.

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/23467010/diff/3001/chrome/test/chromedriver/s...
chrome/test/chromedriver/session_commands.cc:99: Status status =
capabilities.Parse(*desired_caps);
On 2013/09/06 21:20:13, chrisgao wrote:
> If there is some warning during parsing like "loadAsync", we still can't log
it
> to session log at this point.
> 
> Can we create the driver_log earlier and update its log level after parsing?

OK

[commit-bot: I haz the power, 2013-09-06 23:25:46]

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

[commit-bot: I haz the power, 2013-09-07 02:57:37]

Change committed as 221868