Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1048153002:
Add an utility class to improve java logs. (Closed)
DescriptionAdd an utility class to improve java logs.
Add the Log class to org.chromium.base. It is meant to hold different
loggers for each feature. Currently a neutral, ROOT is available.
New loggers will have to be added there as classes migrate to using
base.Log.
BUG=472152
Committed: https://crrev.com/33e4d5203f0d166c78925215a29a1416aacee1b8
Cr-Commit-Position: refs/heads/master@{#323747}
Patch Set 1 #Patch Set 2 : Added the NoSideEffects annotation #Patch Set 3 : Add overrides of the log functions to allow proguard to properly remove arguments #
Total comments: 12
Patch Set 4 : Removed test, make varargs fn private, add file/line numbers #
Total comments: 3
Patch Set 5 : Fixed the code for getting the caller info #Patch Set 6 : Fix for findbugs #
Total comments: 2
Patch Set 7 : Rename Log.info to Log.i #
Messages
Total messages: 30 (10 generated)
dgn@chromium.org changed reviewers: + yfriedman@chromium.org
Yaron: PTAL at everything (2 new files)
https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/Log.java (right): https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:107: * For optimization purposes, use the fixed parameters versions if possible. How does a client decide that? I assume this is just an implementation detail of java which prefers the strict match before varargs or am I missing something? https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:115: public void verbose(String secondaryTag, String messageTemplate, Object... args) { Do we have any examples using more than 7? Perhaps make this private so it's clearer what's going on https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:122: public void v(String secondaryTag, String message) { So for release builds, all of these variants are stripped? https://codereview.chromium.org/1048153002/diff/40001/content/public/android/... File content/public/android/junit/src/org/chromium/base/LogTest.java (right): https://codereview.chromium.org/1048153002/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/base/LogTest.java:5: package org.chromium.base; This should be in a base/ folder if it's package base. If the problem is that there only exists one at the content/ layer we should define a new one in base. If this requires a test-apk on which to run (i.e. content_shell) then we can still have lower layers (base,net,etc) roll up to content. This is what we previously did with things like base_javatests
torne@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/Log.java (right): https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:107: * For optimization purposes, use the fixed parameters versions if possible. On 2015/04/01 20:47:31, Yaron wrote: > How does a client decide that? I assume this is just an implementation detail of > java which prefers the strict match before varargs or am I missing something? It has a different name. https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:115: public void verbose(String secondaryTag, String messageTemplate, Object... args) { On 2015/04/01 20:47:31, Yaron wrote: > Do we have any examples using more than 7? Perhaps make this private so it's > clearer what's going on Yeah, I'd be inclined to make this private and if people need more than 7 parameters, either explain why this is crazy or add more overloads here.
https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/Log.java (right): https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:107: * For optimization purposes, use the fixed parameters versions if possible. On 2015/04/01 20:47:31, Yaron wrote: > How does a client decide that? I assume this is just an implementation detail of > java which prefers the strict match before varargs or am I missing something? The function names are different, so Log.X.v() instead of Log.X.verbose(). But yes, Java also prefers strict match. https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:122: public void v(String secondaryTag, String message) { On 2015/04/01 20:47:31, Yaron wrote: > So for release builds, all of these variants are stripped? Yes. Proguard config (CL here: https://chrome-internal-review.googlesource.com/#/c/211310/) has assumenosideeffects on @NoSideEffects <methods> and on Log {d(...), v(...)} (TODO: need to add verbose() and debug() there) If the return value is unused, proguard should remove the function calls. If all function calls are removed, the function itself is also supposed to go away. https://codereview.chromium.org/1048153002/diff/40001/content/public/android/... File content/public/android/junit/src/org/chromium/base/LogTest.java (right): https://codereview.chromium.org/1048153002/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/base/LogTest.java:5: package org.chromium.base; It's a unit test, no apk required to run it. I can maybe send a CL to enable junit tests in //base, with this file as sample test? I will remove it from this CL, and need it to land first then.
So outstanding is to move the test to base and make the varargs function private. otherwise lgtm https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/Log.java (right): https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:107: * For optimization purposes, use the fixed parameters versions if possible. On 2015/04/02 12:47:20, dgn wrote: > On 2015/04/01 20:47:31, Yaron wrote: > > How does a client decide that? I assume this is just an implementation detail > of > > java which prefers the strict match before varargs or am I missing something? > > The function names are different, so Log.X.v() instead of Log.X.verbose(). But > yes, Java also prefers strict match. Yeesh. Sorry for my poor reading :S https://codereview.chromium.org/1048153002/diff/40001/content/public/android/... File content/public/android/junit/src/org/chromium/base/LogTest.java (right): https://codereview.chromium.org/1048153002/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/base/LogTest.java:5: package org.chromium.base; On 2015/04/02 12:47:20, dgn wrote: > It's a unit test, no apk required to run it. I can maybe send a CL to enable > junit tests in //base, with this file as sample test? I will remove it from this > CL, and need it to land first then. > Ok, sounds good
https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/Log.java (right): https://codereview.chromium.org/1048153002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:115: public void verbose(String secondaryTag, String messageTemplate, Object... args) { On 2015/04/02 10:50:05, Torne wrote: > On 2015/04/01 20:47:31, Yaron wrote: > > Do we have any examples using more than 7? Perhaps make this private so it's > > clearer what's going on > > Yeah, I'd be inclined to make this private and if people need more than 7 > parameters, either explain why this is crazy or add more overloads here. Done. For consistency, I removed the Log.i() variants, since those are going to stay in anyway. https://codereview.chromium.org/1048153002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/Log.java (right): https://codereview.chromium.org/1048153002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:291: private String getCallOrigin() { Following Ted's comment on the design doc, I added this to log filename and line number in debug and verbose logs.
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
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/1048153002/#ps60001 (title: "Removed test, make varargs fn private, add file/line numbers")
The CQ bit was unchecked by dgn@chromium.org
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1048153002/60001
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1048153002/60001
tedchoc@chromium.org changed reviewers: + tedchoc@chromium.org
https://codereview.chromium.org/1048153002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/Log.java (right): https://codereview.chromium.org/1048153002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:302: return st[5].getFileName() + ":" + st[5].getLineNumber(); Is the index stable between art and dalvik? I have alway written it as a loop and looking for the first instance that isn't within org.chromium.base.Log and use that. But if 5 is consistent across OS versions, then that is fine IMO.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1048153002/60001
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1048153002/60001
https://codereview.chromium.org/1048153002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/Log.java (right): https://codereview.chromium.org/1048153002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/Log.java:302: return st[5].getFileName() + ":" + st[5].getLineNumber(); On 2015/04/02 16:19:55, Ted C wrote: > Is the index stable between art and dalvik? > > I have alway written it as a loop and looking for the first instance that isn't > within org.chromium.base.Log and use that. But if 5 is consistent across OS > versions, then that is fine IMO. I was just trying to test that and noticed that 5 doesn't work with openjdk. I guess I'll loop then.
The CQ bit was unchecked by dgn@chromium.org
Test here: https://codereview.chromium.org/1051343002
Yaron: PTAL. I changed the secondary tag for debug and verbose to be the filename and line number.
lgtm
https://codereview.chromium.org/1048153002/diff/100001/base/android/java/src/... File base/android/java/src/org/chromium/base/Log.java (right): https://codereview.chromium.org/1048153002/diff/100001/base/android/java/src/... base/android/java/src/org/chromium/base/Log.java:254: public void info(String secondaryTag, String messageTemplate, Object... args) { err, why is "info" not "i" to mirror others?
https://codereview.chromium.org/1048153002/diff/100001/base/android/java/src/... File base/android/java/src/org/chromium/base/Log.java (right): https://codereview.chromium.org/1048153002/diff/100001/base/android/java/src/... base/android/java/src/org/chromium/base/Log.java:254: public void info(String secondaryTag, String messageTemplate, Object... args) { On 2015/04/02 19:55:41, Yaron wrote: > err, why is "info" not "i" to mirror others? That's a mistake, thanks for pointing it out. fixed.
The CQ bit was checked by dgn@chromium.org
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/1048153002/#ps120001 (title: "Rename Log.info to Log.i")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1048153002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/33e4d5203f0d166c78925215a29a1416aacee1b8 Cr-Commit-Position: refs/heads/master@{#323747} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
