Use CHROME_LOG_FILE environment variable for headless chrome logging

Use CHROME_LOG_FILE environment variable for headless chrome logging.

Chromedriver/Selenium expects to be able to set the log file using the CHROME_LOG_FILE environment variable.

BUG=698095
Review-Url: https://codereview.chromium.org/2736883002
Cr-Commit-Position: refs/heads/master@{#455625}
Committed: https://chromium.googlesource.com/chromium/src/+/ea2f917390a886772e5252da913d7998a60bba69

[irisu, 2017-03-07 08:30:58]

The CQ bit was checked by irisu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 08:31:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 09:26:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 09:26:37]

Dry run: This issue passed the CQ dry run.

[irisu, 2017-03-07 23:42:15]

Description was changed from

==========
Add chrome log file to headless

BUG=
==========

to

==========
Use CHROME_LOG_FILE environment variable for headless chrome logging.

Chromedriver/Selenium expects to be able to set the log file using the
CHROME_LOG_FILE environment variable.

BUG=698095
==========

[irisu, 2017-03-07 23:42:15]

irisu@chromium.org changed reviewers:
+ skyostil@chromium.org

[irisu, 2017-03-07 23:47:47]

Hi Sami, I've modified the headless chrome InitLogging to use the
CHROME_LOG_FILE environment variable because chromedriver/selenium expects to be
able to set it, see
https://cs.chromium.org/chromium/src/chrome/test/chromedriver/test/run_py_tes...

Do you think it makes sense to add this, or should we modify chromedriver to set
the enable-logging flag as well?

[Sami, 2017-03-08 10:38:14]

Thanks, I think it makes sense to support this here too. lgtm with two small
tweaks.

https://codereview.chromium.org/2736883002/diff/1/headless/lib/headless_conte...
File headless/lib/headless_content_main_delegate.cc (right):

https://codereview.chromium.org/2736883002/diff/1/headless/lib/headless_conte...
headless/lib/headless_content_main_delegate.cc:133: if
(env->HasVar("CHROME_LOG_FILE") &&
nit: no need to call HasVar since GetVar will just return false if the variable
isn't there.

https://codereview.chromium.org/2736883002/diff/1/headless/lib/headless_conte...
headless/lib/headless_content_main_delegate.cc:134:
env->GetVar("CHROME_LOG_FILE", &filename) && !filename.empty()) {
Could you make this a named constant at the top of this file (e.g., like here:
https://cs.chromium.org/chromium/src/chrome/common/env_vars.cc?rcl=ae2ff2acd1...)

[irisu, 2017-03-08 23:56:08]

https://codereview.chromium.org/2736883002/diff/1/headless/lib/headless_conte...
File headless/lib/headless_content_main_delegate.cc (right):

https://codereview.chromium.org/2736883002/diff/1/headless/lib/headless_conte...
headless/lib/headless_content_main_delegate.cc:133: if
(env->HasVar("CHROME_LOG_FILE") &&
On 2017/03/08 10:38:14, Sami wrote:
> nit: no need to call HasVar since GetVar will just return false if the
variable
> isn't there.

Done.

https://codereview.chromium.org/2736883002/diff/1/headless/lib/headless_conte...
headless/lib/headless_content_main_delegate.cc:134:
env->GetVar("CHROME_LOG_FILE", &filename) && !filename.empty()) {
On 2017/03/08 10:38:13, Sami wrote:
> Could you make this a named constant at the top of this file (e.g., like here:
>
https://cs.chromium.org/chromium/src/chrome/common/env_vars.cc?rcl=ae2ff2acd1...)

Done.

[irisu, 2017-03-08 23:56:15]

The CQ bit was checked by irisu@chromium.org

[irisu, 2017-03-08 23:56:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2736883002/#ps20001
(title: "Fix nits")

[commit-bot: I haz the power, 2017-03-08 23:56:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 01:34:29]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489017375323520,
"parent_rev": "2484950e1caae6eda284acc57c5de06cfd8b82fd", "commit_rev":
"ea2f917390a886772e5252da913d7998a60bba69"}

[commit-bot: I haz the power, 2017-03-09 01:35:28]

Description was changed from

==========
Use CHROME_LOG_FILE environment variable for headless chrome logging.

Chromedriver/Selenium expects to be able to set the log file using the
CHROME_LOG_FILE environment variable.

BUG=698095
==========

to

==========
Use CHROME_LOG_FILE environment variable for headless chrome logging.

Chromedriver/Selenium expects to be able to set the log file using the
CHROME_LOG_FILE environment variable.

BUG=698095

Review-Url: https://codereview.chromium.org/2736883002
Cr-Commit-Position: refs/heads/master@{#455625}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea2f917390a886772e5252da913d...
==========

[commit-bot: I haz the power, 2017-03-09 01:35:29]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ea2f917390a886772e5252da913d...