Define a LoggingSettings struct to use for InitLogging()

Define a LoggingSettings struct to use for InitLogging()

Update all callers of InitLogging() to use LoggingSettings, only
setting fields that need a non-default value.

Turn LoggingDestination enum into a bit field and define add
LOG_DEFAULT and LOG_ALL constants.

Fix erroneous comment saying that the default was to not lock
the log file.

BUG=247594
TBR=brettw@chromium.org, cpu@chromium.org, gene@chromium.org, jam@chromium.org, rch@chromium.org, scherkus@chromium.org, sergeyu@chromium.org, sky@chromium.org, tkent@chromium.org, yfriedman@chromium.org, zea@chromium.org

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

[akalin, 2013-06-07 06:48:30]

+darin for review

You don't have to look at all the files outside of base/ -- I will add OWNERS
reviewers after you LGTM.

[akalin, 2013-06-10 18:16:59]

ping!

[darin (slow to review), 2013-06-11 23:19:02]

Why extract these bits out into a separate file?  Why not just define them at
the top of logging.h?

Historically, we've tried to keep logging.{h,cc} somewhat hermetic and reusable
independently.  I don't know if that goal is really all that valuable.  (It used
to matter when googleurl was hosted outside of the chromium repository.)

+brettw, who should give the final review here.

[darin (slow to review), 2013-06-11 23:19:04]

Why extract these bits out into a separate file?  Why not just define them at
the top of logging.h?

Historically, we've tried to keep logging.{h,cc} somewhat hermetic and reusable
independently.  I don't know if that goal is really all that valuable.  (It used
to matter when googleurl was hosted outside of the chromium repository.)

+brettw, who should give the final review here.

[akalin, 2013-06-11 23:25:39]

On 2013/06/11 23:19:04, darin wrote:
> Why extract these bits out into a separate file?  Why not just define them at
> the top of logging.h?
> 
> Historically, we've tried to keep logging.{h,cc} somewhat hermetic and
reusable
> independently.  I don't know if that goal is really all that valuable.  (It
used
> to matter when googleurl was hosted outside of the chromium repository.)
> 
> +brettw, who should give the final review here.

I don't feel too strongly about separating it out. I just thought it might be
nice to eventually move all the initialization/parameter-setting code to its own
file since most of the files that include logging.h don't need anything but the
logging macros.

It would mean that eventually one could add logging parameters etc. without
triggering a full recompile of everything.

Brett what do you think?

[akalin, 2013-06-12 21:28:05]

ping!

[brettw, 2013-06-12 23:50:25]

I guess I don't see much advantage. We've basically added params like once ever
so we shouldn't optimize for that. And I've been trying to reduce the number of
files in the base parent directory to make stuff easier to find, so a new file
with no clear benefit seems like step backwards.

[brettw, 2013-06-13 03:53:33]

To clarify, I don't see the point of the separate structure but if you like it,
that's fine. I don't think we should have a separate file, though. It would also
be nice if anything you changed was in the "base" namespace (I don't think we
should be using the "logging" one).

[akalin, 2013-06-14 00:51:51]

On 2013/06/13 03:53:33, brettw wrote:
> To clarify, I don't see the point of the separate structure but if you like
it,
> that's fine. I don't think we should have a separate file, though. It would
also
> be nice if anything you changed was in the "base" namespace (I don't think we
> should be using the "logging" one).

Okay. I'm planning to add a few more parameters (some from the body of
InitLogging, some new ones) and I didn't want to have to touch the entire code
base for each of those CLs, so that's why I did it.

But I merged it back into logging.{h,cc}. I can change the 'logging' namespace
into 'base' in a separate CL, as that'll be another 'touch the entire code base'
one, I think.

PTAL!

[akalin, 2013-06-17 17:51:15]

ping!

[brettw, 2013-06-19 18:10:12]

Lgtm but I have some nontrivial suggestions.

https://codereview.chromium.org/16519003/diff/36001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/16519003/diff/36001/base/logging.cc#newcode86
base/logging.cc:86: int g_min_log_level = 0;
This stuff is in an anonymous namespace in a .cc file which I would not consider
"global", so I wouldn't use the global variable naming style.

https://codereview.chromium.org/16519003/diff/36001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/16519003/diff/36001/base/logging.h#newcode189
base/logging.h:189: DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS,
I actually think this would be a good opportunity to delete this enum. The long
names were to make the calls to InitLogging clear (rather than seeing a call
with a bunch of random bools to it).

But with your struct we can have a nicely named bool "enable_..." and just have
people set it to true or false. Maybe log locking state too
("settings.lock_log_file = true;").

https://codereview.chromium.org/16519003/diff/36001/base/logging.h#newcode193
base/logging.h:193: struct BASE_EXPORT LoggingSettings {
I think the settings here should all be explicit about what the defaults are.
One of the things about this patch is that you don't set things when the default
is OK, which means that we should guarantee in the header how such things are
done.

[akalin, 2013-06-19 19:59:17]

https://codereview.chromium.org/16519003/diff/36001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/16519003/diff/36001/base/logging.cc#newcode86
base/logging.cc:86: int g_min_log_level = 0;
On 2013/06/19 18:10:13, brettw wrote:
> This stuff is in an anonymous namespace in a .cc file which I would not
consider
> "global", so I wouldn't use the global variable naming style.

Done. There were already some existing g_ variables here (I think I added them
years ago); I left those alone to keep this CL focused.

https://codereview.chromium.org/16519003/diff/36001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/16519003/diff/36001/base/logging.h#newcode189
base/logging.h:189: DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS,
On 2013/06/19 18:10:13, brettw wrote:
> I actually think this would be a good opportunity to delete this enum. The
long
> names were to make the calls to InitLogging clear (rather than seeing a call
> with a bunch of random bools to it).
> 
> But with your struct we can have a nicely named bool "enable_..." and just
have
> people set it to true or false. Maybe log locking state too
> ("settings.lock_log_file = true;").

I tried this, but I ultimately decided it wasn't a good idea.

Even though InitLogging itself takes a struct, these enums are passed around to
other functions/classes that don't take the struct, and so those would turn into
functions that would take bools, which isn't great.

For LogLockingState in particular, LoggingLock::Init() takes it as a parameter.

For DcheckState in particular, I couldn't think of a better name than 'bool
enable_dcheck_for_non_official_release_builds;', so it wouldn't save much space
anyway. (enable_dcheck by itself could be misleading.)

OldFileDeletionState is used only in LoggingSettings, but I think it's still
useful to have the enum as it's not strictly an on/off thing (it's not
immediately obvious that not deleting == appending).

https://codereview.chromium.org/16519003/diff/36001/base/logging.h#newcode193
base/logging.h:193: struct BASE_EXPORT LoggingSettings {
On 2013/06/19 18:10:13, brettw wrote:
> I think the settings here should all be explicit about what the defaults are.
> One of the things about this patch is that you don't set things when the
default
> is OK, which means that we should guarantee in the header how such things are
> done.

Done. Some of the info is in the comments for the enums, but it's probably good
to put them here anyway.

[akalin, 2013-06-19 21:41:30]

Okay, it's OWNERS time! Please double-check that the right LoggingSettings are
used for your executable in the following directories:

+brettw (if you haven't looked already)
base/*
ppapi/*
tools/*

+phajdan
base/test/*

+sky
chrome/*
ui/views/*

+tommi
chrome_frame/*
courgette/*

+gene
cloud_print/*

+jam
content/*

+sergeyu
jingle/*
remoting/*
third_party/libjingle/*

+scherkus
media/*

+rch
net/*

+zea
sync/*

+yfriedman
tools/android/*

+tkent
webkit/support/*

+cpu
win8/*

[tkent, 2013-06-19 21:45:18]

webkit/support/ lgtm

[Nicolas Zea, 2013-06-19 21:52:07]

sync LGTM

[Sergey Ulanov, 2013-06-19 21:52:30]

lgtm

[gene, 2013-06-19 22:05:55]

cloudprint lgtm

[Ryan Hamilton, 2013-06-19 22:38:16]

net/ lgtm

[Yaron, 2013-06-19 22:47:26]

tools/android lgtm

[scherkus (not reviewing), 2013-06-19 22:52:56]

media lgtm

[brettw, 2013-06-19 23:00:56]

lgtm

[sky, 2013-06-19 23:22:27]

https://codereview.chromium.org/16519003/diff/43002/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_dll.cc (left):

https://codereview.chromium.org/16519003/diff/43002/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_dll.cc:29: logging::DELETE_OLD_LOG_FILE,
Doesn't your change lose DELETE_OLD_LOG_FILE?

https://codereview.chromium.org/16519003/diff/43002/chrome/test/chromedriver/...
File chrome/test/chromedriver/server/chromedriver_server.cc (right):

https://codereview.chromium.org/16519003/diff/43002/chrome/test/chromedriver/...
chrome/test/chromedriver/server/chromedriver_server.cc:149: bool success =
logging::InitLogging(settings);
Don't you lose DELETE_OLD_LOG_FILE?

https://codereview.chromium.org/16519003/diff/43002/chrome/test/webdriver/web...
File chrome/test/webdriver/webdriver_logging.cc (right):

https://codereview.chromium.org/16519003/diff/43002/chrome/test/webdriver/web...
chrome/test/webdriver/webdriver_logging.cc:233: bool success =
logging::InitLogging(settings);
Don't you lose DELETE_OLD_LOG_FILE here too?

https://codereview.chromium.org/16519003/diff/43002/ui/views/examples/content...
File ui/views/examples/content_client/examples_main_delegate.cc (right):

https://codereview.chromium.org/16519003/diff/43002/ui/views/examples/content...
ui/views/examples/content_client/examples_main_delegate.cc:50: bool success =
logging::InitLogging(settings);
And same question.

[jam, 2013-06-20 00:45:55]

lgtm

[akalin, 2013-06-20 07:35:09]

https://codereview.chromium.org/16519003/diff/43002/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_dll.cc (left):

https://codereview.chromium.org/16519003/diff/43002/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_dll.cc:29: logging::DELETE_OLD_LOG_FILE,
On 2013/06/19 23:22:28, sky wrote:
> Doesn't your change lose DELETE_OLD_LOG_FILE?

yes, but it doesn't matter; only LOG_TO_SYSTEM_DEBUG_LOG is set (i.e., there's
no logging file)

https://codereview.chromium.org/16519003/diff/43002/chrome/test/chromedriver/...
File chrome/test/chromedriver/server/chromedriver_server.cc (right):

https://codereview.chromium.org/16519003/diff/43002/chrome/test/chromedriver/...
chrome/test/chromedriver/server/chromedriver_server.cc:149: bool success =
logging::InitLogging(settings);
On 2013/06/19 23:22:28, sky wrote:
> Don't you lose DELETE_OLD_LOG_FILE?

ditto

https://codereview.chromium.org/16519003/diff/43002/chrome/test/webdriver/web...
File chrome/test/webdriver/webdriver_logging.cc (right):

https://codereview.chromium.org/16519003/diff/43002/chrome/test/webdriver/web...
chrome/test/webdriver/webdriver_logging.cc:233: bool success =
logging::InitLogging(settings);
On 2013/06/19 23:22:28, sky wrote:
> Don't you lose DELETE_OLD_LOG_FILE here too?

ditto (not logging to anything)

https://codereview.chromium.org/16519003/diff/43002/ui/views/examples/content...
File ui/views/examples/content_client/examples_main_delegate.cc (right):

https://codereview.chromium.org/16519003/diff/43002/ui/views/examples/content...
ui/views/examples/content_client/examples_main_delegate.cc:50: bool success =
logging::InitLogging(settings);
On 2013/06/19 23:22:28, sky wrote:
> And same question.

ditto

[sky, 2013-06-20 17:34:50]

LGTM

[akalin, 2013-06-20 17:57:08]

On 2013/06/20 17:34:50, sky wrote:
> LGTM

tommi, cpu, ping!

[cpu_(ooo_6.6-7.5), 2013-06-20 22:42:47]

lgtm

[akalin, 2013-06-21 18:51:11]

TBRing tommi

[commit-bot: I haz the power, 2013-06-21 18:52:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/16519003/71001

[akalin, 2013-06-21 21:15:56]

Committed patchset #10 manually as r207920 (presubmit successful).