Avoid having to define Log classes in every layer needing them.

Avoid having to define Log classes in every layer needing them.

The previous usage of Log required layers and components to
redefine Log classes and have them declare log instances. This was
not convenient and made base packages needlessly aware of specialized
layers.

This approach has the callers specify the group tag at every call, removing the need to get any sort if logger instance.

BUG=472152
Committed: https://crrev.com/12a3389e9cc86c6e75d5a4834faaed47a32febd3
Cr-Commit-Position: refs/heads/master@{#325487}

[dgn, 2015-04-10 13:40:14]

dgn@chromium.org changed reviewers:
+ peter@chromium.org, yfriedman@chromium.org

[dgn, 2015-04-10 13:40:15]

For usage, please see this CL:
https://chrome-internal-review.googlesource.com/#/c/212627

[Peter Beverloo, 2015-04-13 15:27:14]

This seems like a better approach to me because (a) base/ doesn't have to know
about other features, and (b) we don't need classes which only contain loggers,
e.g. DebugLog and ChromeLog.

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/Log.java (right):

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/Log.java:31: * Log LOG =
Log.getLogger("Feature");
nit: we don't have all-caps variable names like this. A better example might be
how one'd use it in a class instance:

<pre>
private static Log mLog = Log.getLogger("Feature");

private void myMethod(String awesome) {
  mLog.d("MyTag", "My %s message.", awesome);
}
</pre>

Alternatively, you can of course just s/LOG/log/ in the example :-).

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/Log.java:103: logger = new
Log(featureTag, true);
In what scenario would |debugWithStack| be set to |false|? All uses I've seen so
far enable it, so perhaps we can remove the option altogether?

[dgn, 2015-04-13 15:33:11]

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/Log.java (right):

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/Log.java:31: * Log LOG =
Log.getLogger("Feature");
On 2015/04/13 15:27:14, Peter Beverloo wrote:
> nit: we don't have all-caps variable names like this. A better example might
be
> how one'd use it in a class instance:
> 
> <pre>
> private static Log mLog = Log.getLogger("Feature");
> 
> private void myMethod(String awesome) {
>   mLog.d("MyTag", "My %s message.", awesome);
> }
> </pre>
> 
> Alternatively, you can of course just s/LOG/log/ in the example :-).

Is all caps OK for static final then? For regular static I thought we had to use
the s prefix (eg: sLog)

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/Log.java:103: logger = new
Log(featureTag, true);
On 2015/04/13 15:27:14, Peter Beverloo wrote:
> In what scenario would |debugWithStack| be set to |false|? All uses I've seen
so
> far enable it, so perhaps we can remove the option altogether?

I've been thinking about this yes. disabling it will require a source change and
recompilation, if someone wants to do it locally I guess commenting the tag
substitution works as well but the code stays simple.    I'll do that.

[Peter Beverloo, 2015-04-13 15:39:31]

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/Log.java (right):

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/Log.java:31: * Log LOG =
Log.getLogger("Feature");
On 2015/04/13 15:33:11, dgn wrote:
> On 2015/04/13 15:27:14, Peter Beverloo wrote:
> > nit: we don't have all-caps variable names like this. A better example might
> be
> > how one'd use it in a class instance:
> > 
> > <pre>
> > private static Log mLog = Log.getLogger("Feature");
> > 
> > private void myMethod(String awesome) {
> >   mLog.d("MyTag", "My %s message.", awesome);
> > }
> > </pre>
> > 
> > Alternatively, you can of course just s/LOG/log/ in the example :-).
> 
> Is all caps OK for static final then? For regular static I thought we had to
use
> the s prefix (eg: sLog)

Err yes, s/mLog/sLog/ in my example - sorry.

We generally only use all caps for constants, e.g. the TAG. I wouldn't really
consider this to be a constant like a string or an integer.

[dgn, 2015-04-14 17:41:40]

PTAL

I removed the secondaryTag argument for Log#d and Log#v, since it's now unused.
This breaks the consistency with android.util.Log and with the other log
methods, but it's simpler.

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/Log.java (right):

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/Log.java:31: * Log LOG =
Log.getLogger("Feature");
On 2015/04/13 15:39:31, Peter Beverloo wrote:
> On 2015/04/13 15:33:11, dgn wrote:
> > On 2015/04/13 15:27:14, Peter Beverloo wrote:
> > > nit: we don't have all-caps variable names like this. A better example
might
> > be
> > > how one'd use it in a class instance:
> > > 
> > > <pre>
> > > private static Log mLog = Log.getLogger("Feature");
> > > 
> > > private void myMethod(String awesome) {
> > >   mLog.d("MyTag", "My %s message.", awesome);
> > > }
> > > </pre>
> > > 
> > > Alternatively, you can of course just s/LOG/log/ in the example :-).
> > 
> > Is all caps OK for static final then? For regular static I thought we had to
> use
> > the s prefix (eg: sLog)
> 
> Err yes, s/mLog/sLog/ in my example - sorry.
> 
> We generally only use all caps for constants, e.g. the TAG. I wouldn't really
> consider this to be a constant like a string or an integer.

Done.

https://codereview.chromium.org/1080543002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/Log.java:103: logger = new
Log(featureTag, true);
On 2015/04/13 15:33:11, dgn wrote:
> On 2015/04/13 15:27:14, Peter Beverloo wrote:
> > In what scenario would |debugWithStack| be set to |false|? All uses I've
seen
> so
> > far enable it, so perhaps we can remove the option altogether?
> 
> I've been thinking about this yes. disabling it will require a source change
and
> recompilation, if someone wants to do it locally I guess commenting the tag
> substitution works as well but the code stays simple.    I'll do that.

Done.

[Yaron, 2015-04-15 18:53:10]

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/Log.java (left):

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/Log.java:68: public static final Log
ROOT = new Log(null, true);
So what's the equivalent of ROOT now?

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/Log.java (right):

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/Log.java:90: public static Log
getLogger(String featureTag) {
I'm a little dismayed by adding yet more overhead for logging. This should be
super lightweight and now we're introducing locking for every class which needs
to retrieve a logger. Worse, this happens at static-init time (generally) which
means more overhead every time we're reaching new classes. Compared to previous
where the recommended practice was a single literal per file which had no
overhead (crbug.com/146559), it seems unnecessary to ensure we're sharing a
single log per feature which is just caching a string literal

[dgn, 2015-04-15 20:32:18]

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/Log.java (left):

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/Log.java:68: public static final Log
ROOT = new Log(null, true);
On 2015/04/15 18:53:10, Yaron wrote:
> So what's the equivalent of ROOT now?

That would be making in your class:

private static final Log sLog = Log.getLogger(null);

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/Log.java (right):

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/Log.java:90: public static Log
getLogger(String featureTag) {
Understood, we want to be as lightweight as possible. I was also trying to be as
readable and easy to correctly write, DRY as possible. How about one of those:

1) go back to full static, no state stored, tag is now going to be used as
featureTag rather than classname:
Log.d("Tag", "message") => D/chromium.Tag (9): [Foo.java:42] message
Log.i("Tag", "message") => I/chromium.Tag (9): message

2) new object in each class:
private static final Log sLog = new Log("Foo")
sLog.i("Bar", "message") => I/chromium.Foo (9): [Bar] message

I think 1) would be better. 
We would lose the calling class name for levels above debug. If the message and
the feature tag are not enough to pinpoint the reason for the log, maybe it
should be rewritten?
We would lose checking the tag length, but since we use isLoggable all the time
anyway, that's going to be checked by the framework.
We don't get more duplicated strings than the current android.util.Log way. opt
2) needs to store the 2 tags + whatever comes with a new object. 

What do you think?

[Yaron, 2015-04-15 22:37:27]

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/Log.java (right):

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/Log.java:90: public static Log
getLogger(String featureTag) {
On 2015/04/15 20:32:17, dgn wrote:
> Understood, we want to be as lightweight as possible. I was also trying to be
as
> readable and easy to correctly write, DRY as possible. How about one of those:
> 
> 1) go back to full static, no state stored, tag is now going to be used as
> featureTag rather than classname:
> Log.d("Tag", "message") => D/chromium.Tag (9): [Foo.java:42] message
> Log.i("Tag", "message") => I/chromium.Tag (9): message
> 
> 2) new object in each class:
> private static final Log sLog = new Log("Foo")
> sLog.i("Bar", "message") => I/chromium.Foo (9): [Bar] message
> 
> I think 1) would be better. 
> We would lose the calling class name for levels above debug. If the message
and
> the feature tag are not enough to pinpoint the reason for the log, maybe it
> should be rewritten?
> We would lose checking the tag length, but since we use isLoggable all the
time
> anyway, that's going to be checked by the framework.
> We don't get more duplicated strings than the current android.util.Log way.
opt
> 2) needs to store the 2 tags + whatever comes with a new object. 
> 
> What do you think?

Going with 1 sgtm

[dgn, 2015-04-16 12:00:17]

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/Log.java (right):

https://codereview.chromium.org/1080543002/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/Log.java:90: public static Log
getLogger(String featureTag) {
On 2015/04/15 22:37:27, Yaron wrote:
> On 2015/04/15 20:32:17, dgn wrote:
> > Understood, we want to be as lightweight as possible. I was also trying to
be
> as
> > readable and easy to correctly write, DRY as possible. How about one of
those:
> > 
> > 1) go back to full static, no state stored, tag is now going to be used as
> > featureTag rather than classname:
> > Log.d("Tag", "message") => D/chromium.Tag (9): [Foo.java:42] message
> > Log.i("Tag", "message") => I/chromium.Tag (9): message
> > 
> > 2) new object in each class:
> > private static final Log sLog = new Log("Foo")
> > sLog.i("Bar", "message") => I/chromium.Foo (9): [Bar] message
> > 
> > I think 1) would be better. 
> > We would lose the calling class name for levels above debug. If the message
> and
> > the feature tag are not enough to pinpoint the reason for the log, maybe it
> > should be rewritten?
> > We would lose checking the tag length, but since we use isLoggable all the
> time
> > anyway, that's going to be checked by the framework.
> > We don't get more duplicated strings than the current android.util.Log way.
> opt
> > 2) needs to store the 2 tags + whatever comes with a new object. 
> > 
> > What do you think?
> 
> Going with 1 sgtm

Done.

[dgn, 2015-04-16 13:11:21]

I updated the internal calls in this CL:
https://chrome-internal-review.googlesource.com/#/c/212627

[Yaron, 2015-04-16 17:41:31]

lgtm

https://codereview.chromium.org/1080543002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/Log.java (right):

https://codereview.chromium.org/1080543002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/Log.java:82: else return BASE_TAG + "."
+ groupTag;
Nit: no need for "else"

[dgn, 2015-04-16 17:58:34]

The CQ bit was checked by dgn@chromium.org

[dgn, 2015-04-16 17:59:15]

The CQ bit was unchecked by dgn@chromium.org

[dgn, 2015-04-16 18:01:10]

The CQ bit was checked by dgn@chromium.org

[dgn, 2015-04-16 18:01:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1080543002/#ps60001
(title: "fix nits")

[commit-bot: I haz the power, 2015-04-16 18:03:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1080543002/60001

[commit-bot: I haz the power, 2015-04-16 19:04:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-16 19:05:42]

Patchset 4 (id:??) landed as
https://crrev.com/12a3389e9cc86c6e75d5a4834faaed47a32febd3
Cr-Commit-Position: refs/heads/master@{#325487}