Fixing loglevel in chrome driver to accept string rather than an integer.

Fixing loglevel in chrome driver to accept string rather than an integer.
Patch from Zori Kostadinova <zori@chromium.org>;
BUG=chromedriver:75
TEST=

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

[kkania, 2012-05-21 17:46:46]

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
File chrome/test/webdriver/webdriver_capabilities_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
chrome/test/webdriver/webdriver_capabilities_parser_unittest.cc:275:
ASSERT_FALSE(parser.Parse());
i guess you wrote this test to demonstrate the bug. You need to change it now to
pass for the correct behavior.

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
chrome/test/webdriver/webdriver_capabilities_parser_unittest.cc:277:
options->SetInteger("driver", kInfoLogLevel);
although people forget to all the time, technically you need to include
"chrome/test/webdriver/webdriver_logging.h" for this constant. You shouldn't
rely on transitive includes (one exception (i think optional) is if it's
included in the associated header, you don't have to include it in your .cc).

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
File chrome/test/webdriver/webdriver_logging.cc (right):

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
chrome/test/webdriver/webdriver_logging.cc:29: LogLevel level = kInfoLogLevel;
If they pass "OFF", this will get set to INFO, which I think is wrong. I think
we should set CONFIG->INFO, FINEST->FINER, and handle ALL and OFF correctly.

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
chrome/test/webdriver/webdriver_logging.cc:30: if (name == "SEVERE") {
although the doc seems to suggest it will be caps, it doesn't say explicitly. i
think an case insensitive compare would be better. what do you think?

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
File chrome/test/webdriver/webdriver_logging.h (right):

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
chrome/test/webdriver/webdriver_logging.h:33: LogLevel LogLevelFromString(const
std::string& name);
Add a short comment here which mentions that the given name should be a
webdriver log level name, such as "INFO".

[zori, 2012-05-21 23:42:33]

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
File chrome/test/webdriver/webdriver_capabilities_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
chrome/test/webdriver/webdriver_capabilities_parser_unittest.cc:275:
ASSERT_FALSE(parser.Parse());
Since CapabilitiesParser::Parse() returns Error*, this is how the correct
behavior is. Changing the comments.
On 2012/05/21 17:46:46, kkania wrote:
> i guess you wrote this test to demonstrate the bug. You need to change it now
to
> pass for the correct behavior.

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
chrome/test/webdriver/webdriver_capabilities_parser_unittest.cc:277:
options->SetInteger("driver", kInfoLogLevel);
I knew this, sorry. Done.
On 2012/05/21 17:46:46, kkania wrote:
> although people forget to all the time, technically you need to include
> "chrome/test/webdriver/webdriver_logging.h" for this constant. You shouldn't
> rely on transitive includes (one exception (i think optional) is if it's
> included in the associated header, you don't have to include it in your .cc).

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
File chrome/test/webdriver/webdriver_logging.cc (right):

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
chrome/test/webdriver/webdriver_logging.cc:29: LogLevel level = kInfoLogLevel;
I think FINEST->ALL.

On 2012/05/21 17:46:46, kkania wrote:
> If they pass "OFF", this will get set to INFO, which I think is wrong. I think
> we should set CONFIG->INFO, FINEST->FINER, and handle ALL and OFF correctly.

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
chrome/test/webdriver/webdriver_logging.cc:30: if (name == "SEVERE") {
On 2012/05/21 17:46:46, kkania wrote:
> although the doc seems to suggest it will be caps, it doesn't say explicitly.
i
> think an case insensitive compare would be better. what do you think?

Done.

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
File chrome/test/webdriver/webdriver_logging.h (right):

https://chromiumcodereview.appspot.com/10411031/diff/1/chrome/test/webdriver/...
chrome/test/webdriver/webdriver_logging.h:33: LogLevel LogLevelFromString(const
std::string& name);
On 2012/05/21 17:46:46, kkania wrote:
> Add a short comment here which mentions that the given name should be a
> webdriver log level name, such as "INFO".

Done.

[kkania, 2012-05-22 16:09:01]

lgtm after one change

http://codereview.chromium.org/10411031/diff/1006/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_logging.cc (right):

http://codereview.chromium.org/10411031/diff/1006/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_logging.cc:33: level = kSevereLogLevel;
I think OFF should be a separate level. We shouldn't see severe log entries if
they specify OFF

[commit-bot: I haz the power, 2012-05-23 00:48:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zori@chromium.org/10411031/1007

[commit-bot: I haz the power, 2012-05-23 03:47:35]

Try job failure for 10411031-1007 (retry) on win_rel for step "runhooks".
It's a second try, previously, step "sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...