-
Notifications
You must be signed in to change notification settings - Fork 6k
Implemented Log proxy that only logs in BuildConfig.DEBUG (#25391). #9122
Implemented Log proxy that only logs in BuildConfig.DEBUG (#25391). #9122
Conversation
|
+@dnfield, if I'm remembering right he deliberately didn't want to add a wrapper class. I think the idea was that we generally don't want to be logging a bunch of |
| } | ||
| } | ||
|
|
||
| public static void w(@NonNull String tag, @NonNull String message, @NonNull Throwable tr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to keep WARNING and above logs in all builds. These are more when something's gone actually wrong so it's better to preserve them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
| @@ -0,0 +1,86 @@ | |||
| // Copyright 2013 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much binary size does this add to the engine?
Have we considered implementing tracing at the Android level rather than making it easier to add more logs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider direct logging, which would litter the code with if statements about build config. I think this is a significantly more readable alternative.
As for binary size, I'm pretty sure the binary size impact of this one class is less than if we surrounded every log statement with an if-statement. But I'm also fairly confident that both approaches are negligible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if debug gets compiled out since it's a statically known by the compiler. This won't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added ~700 bytes compressed to the release build of flutter.jar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look for another way. I really, really don't like the idea of dropping if-statements everywhere.
flutter/engine@fa9b5bd...7dd62d6 git log fa9b5bd..7dd62d6 --no-merges --oneline 7dd62d6 Roll src/third_party/skia 1013ecfb3421..859f7108a5af (19 commits) (flutter/engine#9136) e8aa120 New Plugin API PR5: Integrates plugin lifecycle control with FlutterFragment. (flutter/engine#9083) 8b1199c Implemented Log proxy that only logs in BuildConfig.DEBUG (#25391). (flutter/engine#9122) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
Implemented Log proxy that only logs in BuildConfig.DEBUG (#25391).
I also migrated a couple of classes to demonstrate compilation of source.