Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@matthew-carroll
Copy link
Contributor

Implemented Log proxy that only logs in BuildConfig.DEBUG (#25391).

I also migrated a couple of classes to demonstrate compilation of source.

@mklim mklim requested a review from dnfield May 29, 2019 17:38
@mklim
Copy link
Contributor

mklim commented May 29, 2019

+@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 DEBUG or VERBOSE logs anyway.

}
}

public static void w(@NonNull String tag, @NonNull String message, @NonNull Throwable tr) {
Copy link
Contributor

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.

Copy link
Contributor

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.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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.

@matthew-carroll matthew-carroll merged commit 8b1199c into flutter:master May 30, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request May 30, 2019
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.
huqiuser pushed a commit to huqiuser/engine that referenced this pull request Jun 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants