Move eventlog_provider from //base/win to //chrome/common/win

Move eventlog_provider from //base/win to //chrome/common/win.

This removes a //chrome dependency in base (process_version_rc_template), 
allowing //base to be used by targets that do not have access to //chrome.

BUG=642115
TEST=eventlog_provider.dll is installed and SYSLOG works.

Review-Url: https://codereview.chromium.org/2946983002
Cr-Commit-Position: refs/heads/master@{#481878}
Committed: https://chromium.googlesource.com/chromium/src/+/fe47600213937d47019402f8bda6198c464d37b3

[proberge, 2017-06-20 18:33:55]

Description was changed from

==========
Move eventlog_provider and eventlog_provider_dll_version out of //base

This is an alternative solution to
https://codereview.chromium.org/2945763003/ for allowing external
projects to depend on //base without having to also depend on //chrome.

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

to

==========
Move eventlog_provider and eventlog_provider_dll_version out of //base

This is an alternative solution to
https://codereview.chromium.org/2945763003/ for allowing external
projects to depend on //base without having to also depend on //chrome.

Note that the the .rc file is output as
out/Default/gen/chrome/eventlog_provider_dll_version.dll instead of
out/Default/gen/base/win/eventlog_provider_dll_version.dll

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

[grt (UTC plus 2), 2017-06-21 09:30:43]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2017-06-21 09:30:44]

https://codereview.chromium.org/2946983002/diff/1/base/win/BUILD.gn
File base/win/BUILD.gn (right):

https://codereview.chromium.org/2946983002/diff/1/base/win/BUILD.gn#newcode31
base/win/BUILD.gn:31: "//chrome:eventlog_provider",
this still has a dependency on chrome in base. looking a bit deeper, i see that
base/win/eventlog_messages.mc and base/syslog_logging.cc both have chrome-ish
things in them. my preference is for base logging to provide a way to generate
events without knowing that it's being used for a browser. it's already set up
so that SYSLOG(severity) doesn't do anything until Chrome calls
SetEventSourceName to tell base logging the name of the event source. i think
this is a good pattern to follow. i think we could sensibly move
eventlog_messages into //chrome/common as well. this would require removing it
as a dep from //base:base, removing the #include of base/win/eventlog_messages.h
from syslog_logging.cc, and adding uint16_t category, uint32_t event_id
parameters (or WORD category, DWORD event_id) params to a gently-renamed
SetEventSourceName method. this allows the embedder of //base:base to completely
tell it "here's how i want you to report SYSLOG messages".

[proberge, 2017-06-21 19:10:05]

https://codereview.chromium.org/2946983002/diff/1/base/win/BUILD.gn
File base/win/BUILD.gn (right):

https://codereview.chromium.org/2946983002/diff/1/base/win/BUILD.gn#newcode31
base/win/BUILD.gn:31: "//chrome:eventlog_provider",
On 2017/06/21 09:30:44, grt (UTC plus 2) wrote:
> this still has a dependency on chrome in base. looking a bit deeper, i see
that
> base/win/eventlog_messages.mc and base/syslog_logging.cc both have chrome-ish
> things in them. my preference is for base logging to provide a way to generate
> events without knowing that it's being used for a browser. it's already set up
> so that SYSLOG(severity) doesn't do anything until Chrome calls
> SetEventSourceName to tell base logging the name of the event source. i think
> this is a good pattern to follow. i think we could sensibly move
> eventlog_messages into //chrome/common as well. this would require removing it
> as a dep from //base:base, removing the #include of
base/win/eventlog_messages.h
> from syslog_logging.cc, and adding uint16_t category, uint32_t event_id
> parameters (or WORD category, DWORD event_id) params to a gently-renamed
> SetEventSourceName method. this allows the embedder of //base:base to
completely
> tell it "here's how i want you to report SYSLOG messages".

This isn't really a dependency (it's a visibility rule). 
Still, I think your suggested solution makes a lot of sense and I've implemented
it in patchset 3. If //chrome/common OWNERS are unhappy with the change we can
go back to patchset #1.

[proberge, 2017-06-21 19:13:46]

proberge@chromium.org changed reviewers:
+ pastarmovj@chromium.org

[proberge, 2017-06-21 19:13:47]

++pastarmovj who added this class. PTAL

[grt (UTC plus 2), 2017-06-21 20:26:15]

this looks pretty good to me. could you update the CL description?

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc
File base/syslog_logging.cc (right):

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:6: #include "base/syslog_logging.h"
nit: this should be the first include with a blank line between it and the other
includes

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:9: #include <stddef.h>
why is this needed?

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:16: #include <cstring>
this doesn't seem to be used, is it?

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:25: std::string* g_event_source_name = nullptr;
now that there's more than one thing in here, please add blank lines between the
opening and closure of the namespace.

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:26: WORD g_category;
please zero-initialize these

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:28: }
nit:
}  // namespace

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:36: #endif  // defined(OS_WIN)
nit: blank line before this

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:80: if (!ReportEventA(event_log_handle, log_type,
g_category, g_event_id, NULL, 1,
NULL -> nullptr throughout

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.h
File base/syslog_logging.h (right):

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.h#n...
base/syslog_logging.h:41: std::ostream& stream() { return log_message_.stream();
}
i think this file needs #include <iosfwd> for std::ostream.

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:182: "//chrome/common/win:eventlog_messages",
i *think* this can be in deps rather than public_deps. does anyone outside of
//chrome/common need to see it?

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/win/BUILD.gn
File chrome/common/win/BUILD.gn (right):

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/win/BUILD...
chrome/common/win/BUILD.gn:1: # Copyright (c) 2017 The Chromium Authors. All
rights reserved.
nit: no " (c)" in the current notice:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[proberge, 2017-06-21 21:33:39]

Thanks for the great review! (as usual)

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc
File base/syslog_logging.cc (right):

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:6: #include "base/syslog_logging.h"
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> nit: this should be the first include with a blank line between it and the
other
> includes

Done.

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:9: #include <stddef.h>
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> why is this needed?

I think I was trying to get WORD/DWORD defined. Removed it.

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:16: #include <cstring>
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> this doesn't seem to be used, is it?

Done.

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:25: std::string* g_event_source_name = nullptr;
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> now that there's more than one thing in here, please add blank lines between
the
> opening and closure of the namespace.

Done.

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:26: WORD g_category;
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> please zero-initialize these

Done.

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:28: }
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> nit:
> }  // namespace

Done.

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:36: #endif  // defined(OS_WIN)
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> nit: blank line before this

Done.

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.cc#...
base/syslog_logging.cc:80: if (!ReportEventA(event_log_handle, log_type,
g_category, g_event_id, NULL, 1,
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> NULL -> nullptr throughout

Done.

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.h
File base/syslog_logging.h (right):

https://codereview.chromium.org/2946983002/diff/40001/base/syslog_logging.h#n...
base/syslog_logging.h:41: std::ostream& stream() { return log_message_.stream();
}
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> i think this file needs #include <iosfwd> for std::ostream.

Done.

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:182: "//chrome/common/win:eventlog_messages",
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> i *think* this can be in deps rather than public_deps. does anyone outside of
> //chrome/common need to see it?

Moved to an is_win block.

There's no non-public deps in this genrule (aside from one in an is_linux
block). Is it worthwhile adding a deps definition just for this rule?

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/win/BUILD.gn
File chrome/common/win/BUILD.gn (right):

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/win/BUILD...
chrome/common/win/BUILD.gn:1: # Copyright (c) 2017 The Chromium Authors. All
rights reserved.
On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> nit: no " (c)" in the current notice:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

[proberge, 2017-06-21 21:37:37]

Description was changed from

==========
Move eventlog_provider and eventlog_provider_dll_version out of //base

This is an alternative solution to
https://codereview.chromium.org/2945763003/ for allowing external
projects to depend on //base without having to also depend on //chrome.

Note that the the .rc file is output as
out/Default/gen/chrome/eventlog_provider_dll_version.dll instead of
out/Default/gen/base/win/eventlog_provider_dll_version.dll

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

to

==========
Move eventlog_provider from //base/win to //chrome/common/win

This is an alternative solution to
https://codereview.chromium.org/2945763003/ for allowing external
projects to depend on //base without having to also depend on //chrome.

Note that the the .rc file is output as
out/Default/gen/chrome/common/win/eventlog_provider_dll_version.rc instead of
out/Default/gen/base/win/eventlog_provider_dll_version.rc 

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

[grt (UTC plus 2), 2017-06-21 21:48:02]

lgtm w/ an is_win deps block. thanks!

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:182: "//chrome/common/win:eventlog_messages",
On 2017/06/21 21:33:39, proberge wrote:
> On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> > i *think* this can be in deps rather than public_deps. does anyone outside
of
> > //chrome/common need to see it?
> 
> Moved to an is_win block.
> 
> There's no non-public deps in this genrule (aside from one in an is_linux
> block). Is it worthwhile adding a deps definition just for this rule?

i don't see why not. i think minimal visibility is a good thing. a chrome OWNER
may disagree, though.

https://codereview.chromium.org/2946983002/diff/60001/base/syslog_logging.cc
File base/syslog_logging.cc (right):

https://codereview.chromium.org/2946983002/diff/60001/base/syslog_logging.cc#...
base/syslog_logging.cc:7: #include "base/debug/stack_trace.h"
nit: move this into the OS_WIN block below?

https://codereview.chromium.org/2946983002/diff/60001/base/syslog_logging.h
File base/syslog_logging.h (right):

https://codereview.chromium.org/2946983002/diff/60001/base/syslog_logging.h#n...
base/syslog_logging.h:12: #if defined(OS_WIN)
#include "build/build_config.h" for the definition of OS_WIN

[grt (UTC plus 2), 2017-06-21 21:53:17]

regarding the CL description, is the following sufficient:

Move eventlog_provider from //base/win to //chrome/common/win.

This allows //base to be used by targets that do not have access to
//chrome.

BUG=642115
TEST=eventlog_provider.dll is installed and SYSLOG works.

[proberge, 2017-06-22 14:35:32]

Description was changed from

==========
Move eventlog_provider from //base/win to //chrome/common/win

This is an alternative solution to
https://codereview.chromium.org/2945763003/ for allowing external
projects to depend on //base without having to also depend on //chrome.

Note that the the .rc file is output as
out/Default/gen/chrome/common/win/eventlog_provider_dll_version.rc instead of
out/Default/gen/base/win/eventlog_provider_dll_version.rc 

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

to

==========
Move eventlog_provider from //base/win to //chrome/common/win.

This allows //base to be used by targets that do not have access to
//chrome.

BUG=642115
TEST=eventlog_provider.dll is installed and SYSLOG works.
==========

[proberge, 2017-06-22 14:35:47]

Thanks!

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2946983002/diff/40001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:182: "//chrome/common/win:eventlog_messages",
On 2017/06/21 21:48:01, grt (UTC plus 2) wrote:
> On 2017/06/21 21:33:39, proberge wrote:
> > On 2017/06/21 20:26:15, grt (UTC plus 2) wrote:
> > > i *think* this can be in deps rather than public_deps. does anyone outside
> of
> > > //chrome/common need to see it?
> > 
> > Moved to an is_win block.
> > 
> > There's no non-public deps in this genrule (aside from one in an is_linux
> > block). Is it worthwhile adding a deps definition just for this rule?
> 
> i don't see why not. i think minimal visibility is a good thing. a chrome
OWNER
> may disagree, though.

Done.

https://codereview.chromium.org/2946983002/diff/60001/base/syslog_logging.cc
File base/syslog_logging.cc (right):

https://codereview.chromium.org/2946983002/diff/60001/base/syslog_logging.cc#...
base/syslog_logging.cc:7: #include "base/debug/stack_trace.h"
On 2017/06/21 21:48:01, grt (UTC plus 2) wrote:
> nit: move this into the OS_WIN block below?

Done.

https://codereview.chromium.org/2946983002/diff/60001/base/syslog_logging.h
File base/syslog_logging.h (right):

https://codereview.chromium.org/2946983002/diff/60001/base/syslog_logging.h#n...
base/syslog_logging.h:12: #if defined(OS_WIN)
On 2017/06/21 21:48:01, grt (UTC plus 2) wrote:
> #include "build/build_config.h" for the definition of OS_WIN

Done.

[pastarmovj, 2017-06-22 19:04:14]

Thanks for the cleanup! :)

LGTM

[proberge, 2017-06-22 19:09:11]

proberge@chromium.org changed reviewers:
+ thakis@chromium.org

[proberge, 2017-06-22 19:09:12]

++thakis for //base, //chrome and //BUILD.gn approval

[Nico, 2017-06-22 19:40:27]

lgtm

Can you expand on "This allows //base to be used by targets that do not have
access to
//chrome." a bit? Did base have a dependency on chrome/ without this change?
Where?

https://codereview.chromium.org/2946983002/diff/80001/base/syslog_logging.h
File base/syslog_logging.h (right):

https://codereview.chromium.org/2946983002/diff/80001/base/syslog_logging.h#n...
base/syslog_logging.h:33: DWORD event_id);
can you make these uint16_t and uint32_t, so that we don't have to pull in
windows.h in the header?

[proberge, 2017-06-22 20:01:59]

Description was changed from

==========
Move eventlog_provider from //base/win to //chrome/common/win.

This allows //base to be used by targets that do not have access to
//chrome.

BUG=642115
TEST=eventlog_provider.dll is installed and SYSLOG works.
==========

to

==========
Move eventlog_provider from //base/win to //chrome/common/win.

This removes a //chrome dependency in base (process_version_rc_template),
allowing
 //base to be used by targets that do not have access to //chrome.

BUG=642115
TEST=eventlog_provider.dll is installed and SYSLOG works.
==========

[proberge, 2017-06-22 20:02:18]

Description was changed from

==========
Move eventlog_provider from //base/win to //chrome/common/win.

This removes a //chrome dependency in base (process_version_rc_template),
allowing
 //base to be used by targets that do not have access to //chrome.

BUG=642115
TEST=eventlog_provider.dll is installed and SYSLOG works.
==========

to

==========
Move eventlog_provider from //base/win to //chrome/common/win.

This removes a //chrome dependency in base (process_version_rc_template), 
allowing //base to be used by targets that do not have access to //chrome.

BUG=642115
TEST=eventlog_provider.dll is installed and SYSLOG works.
==========

[proberge, 2017-06-22 20:13:11]

On 2017/06/22 19:40:27, Nico wrote:
> lgtm
> 
> Can you expand on "This allows //base to be used by targets that do not have
> access to
> //chrome." a bit? Did base have a dependency on chrome/ without this change?
> Where?
> 
> https://codereview.chromium.org/2946983002/diff/80001/base/syslog_logging.h
> File base/syslog_logging.h (right):
> 
>
https://codereview.chromium.org/2946983002/diff/80001/base/syslog_logging.h#n...
> base/syslog_logging.h:33: DWORD event_id);
> can you make these uint16_t and uint32_t, so that we don't have to pull in
> windows.h in the header?

Updated the description ("This removes a //chrome dependency in base
(process_version_rc_template) ...")

[proberge, 2017-06-22 20:13:23]

Thanks for the quick review!

https://codereview.chromium.org/2946983002/diff/80001/base/syslog_logging.h
File base/syslog_logging.h (right):

https://codereview.chromium.org/2946983002/diff/80001/base/syslog_logging.h#n...
base/syslog_logging.h:33: DWORD event_id);
On 2017/06/22 19:40:27, Nico wrote:
> can you make these uint16_t and uint32_t, so that we don't have to pull in
> windows.h in the header?

Done.

[Nico, 2017-06-22 20:17:26]

lgtm++

Scary that we let that dependency in! Let me start a thread on gn-dev if
checkdeps should maybe check import() lines in gn too.

[proberge, 2017-06-22 20:23:11]

The CQ bit was checked by proberge@chromium.org

[proberge, 2017-06-22 20:23:12]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org,
pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2946983002/#ps100001
(title: "Address review comments on #5")

[commit-bot: I haz the power, 2017-06-22 20:23:38]

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-06-22 23:02:37]

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

[commit-bot: I haz the power, 2017-06-22 23:02:39]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[proberge, 2017-06-23 13:24:14]

The CQ bit was checked by proberge@chromium.org

[commit-bot: I haz the power, 2017-06-23 13:24:26]

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-06-23 14:29:03]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498224254756590,
"parent_rev": "e545f6464160070b1b14d61d2ed3e2ef52aff6ed", "commit_rev":
"fe47600213937d47019402f8bda6198c464d37b3"}

[commit-bot: I haz the power, 2017-06-23 14:29:19]

Description was changed from

==========
Move eventlog_provider from //base/win to //chrome/common/win.

This removes a //chrome dependency in base (process_version_rc_template), 
allowing //base to be used by targets that do not have access to //chrome.

BUG=642115
TEST=eventlog_provider.dll is installed and SYSLOG works.
==========

to

==========
Move eventlog_provider from //base/win to //chrome/common/win.

This removes a //chrome dependency in base (process_version_rc_template), 
allowing //base to be used by targets that do not have access to //chrome.

BUG=642115
TEST=eventlog_provider.dll is installed and SYSLOG works.

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

[commit-bot: I haz the power, 2017-06-23 14:29:22]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/fe47600213937d47019402f8bda6...