Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1806963002:
Reduce APK size by disabling WebRTC. (Closed)
DescriptionReduce APK size by disabling WebRTC.
Reduced official build size from 7.9MB to 6.4MB. Can reenable WebRTC by changing the BUILD.gn file.
BUG=595038
Committed: https://crrev.com/e8f2d518b2337b2ae8736ee0f032d2fffc8b16b8
Cr-Commit-Position: refs/heads/master@{#381636}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Change macro name, use separate config #
Total comments: 1
Patch Set 3 : Reviewer's Feedback #Patch Set 4 : add condition for GYP #
Total comments: 15
Patch Set 5 : Reviewer's feedback #
Messages
Total messages: 24 (6 generated)
yuweih@chromium.org changed reviewers: + sergeyu@chromium.org
PTAL:)
lambroslambrou@chromium.org changed reviewers: + lambroslambrou@chromium.org
Just some drive-by comments - I'll defer final LG to Sergey. https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn#newcode16 remoting/BUILD.gn:16: defines += [ "DISABLE_WEBRTC=1" ] I don't think this is the right way. We already have a #define called "ENABLE_WEBRTC" and a GN flag called enable_webrtc here: https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD... But I think if you were to disable that flag, libjingle would be knocked out as well and our client wouldn't build at all. So we need to define a new GN flag, named something like "enable_webrtc_in_remoting_client" (Sergey can maybe suggest a better name for this). The way to define new flags in GN is to add them to a declare_args() section. We have a file remoting/remoting_options.gni so that seems like the best place to put the new flag. Default the flag to true, but add something that sets it to false on Android. I would also prefer that the #define be named ENABLE_XXX rather than DISABLE_XXX.
The CQ bit was checked by yuweih@chromium.org
The CQ bit was unchecked by yuweih@chromium.org
Just replied for the comment... https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn#newcode16 remoting/BUILD.gn:16: defines += [ "DISABLE_WEBRTC=1" ] On 2016/03/16 18:12:20, Lambros wrote: > I don't think this is the right way. > > We already have a #define called "ENABLE_WEBRTC" and a GN flag called > enable_webrtc here: > https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD... > > But I think if you were to disable that flag, libjingle would be knocked out as > well and our client wouldn't build at all. > > So we need to define a new GN flag, named something like > "enable_webrtc_in_remoting_client" > (Sergey can maybe suggest a better name for this). > > The way to define new flags in GN is to add them to a declare_args() section. We > have a file remoting/remoting_options.gni so that seems like the best place to > put the new flag. Default the flag to true, but add something that sets it to > false on Android. > > I would also prefer that the #define be named ENABLE_XXX rather than > DISABLE_XXX. To follow the pattern in remoting_options.gni, I think I should call it remoting_enable_webrtc or enable_remoting_webrtc? I can make it like: declare_args() { ... remoting_enable_webrtc = true } and later: if (is_android) { remoting_enable_webrtc = false } but then we are not able to set GN argument to enable WebRTC on Android... (Or maybe we don't care?) Should the `defines += [...]` thing also be in remoting_options?
https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn#newcode15 remoting/BUILD.gn:15: if (is_android) { config(version) is not the right place to add this define. This config is there to add to define VERSION. I suggest adding a new config, e.g. in remoting_options.gni. Then you'll just need to use it in remoting/client/BUILD.gn and remoting/android/BUILD.gn. https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn#newcode16 remoting/BUILD.gn:16: defines += [ "DISABLE_WEBRTC=1" ] I think ideally we want to disable it only for offical builds. Also it should always be disabled for NaCl builds: if (!is_official_build && !is_nacl) { defines += [ "ENABLE_WEBRTC_REMOTING_CLIENT=1" ] } https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn#newcode16 remoting/BUILD.gn:16: defines += [ "DISABLE_WEBRTC=1" ] Suggest calling it ENABLE_WEBRTC_REMOTING_CLIENT to make it clear what it's used for. Also "enable" flags are easier to work with. https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn#newcode16 remoting/BUILD.gn:16: defines += [ "DISABLE_WEBRTC=1" ] You need similar change for GYP. Just add a condition similar to this: https://code.google.com/p/chromium/codesearch#chromium/src/remoting/remoting_... here: https://code.google.com/p/chromium/codesearch#chromium/src/remoting/remoting_... Note that remoting_client.gyp is not used for NaCl build, so you just need to disable this flag for non-official builds. https://codereview.chromium.org/1806963002/diff/1/remoting/client/chromoting_... File remoting/client/chromoting_client.cc (right): https://codereview.chromium.org/1806963002/diff/1/remoting/client/chromoting_... remoting/client/chromoting_client.cc:76: LOG(FATAL) << "WebRTC is not supported in webapp."; We want this LOG(FATAL) on all platforms: #if !defined(ENABLED_WEBRTC_CLIENT) LOG(FATAL) << "WebRTC is not supported"; #elif .... #endif https://codereview.chromium.org/1806963002/diff/1/remoting/client/jni/chromot... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/1806963002/diff/1/remoting/client/jni/chromot... remoting/client/jni/chromoting_jni_instance.cc:420: #endif // if !defined(DISABLE_WEBRTC) don't need 'if'. two spaces before // please
https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn#newcode16 remoting/BUILD.gn:16: defines += [ "DISABLE_WEBRTC=1" ] On 2016/03/16 19:12:36, Sergey Ulanov wrote: > I think ideally we want to disable it only for offical builds. Also it should > always be disabled for NaCl builds: > if (!is_official_build && !is_nacl) { > defines += [ "ENABLE_WEBRTC_REMOTING_CLIENT=1" ] > } So we are disabling WebRTC on all platform, right? It seems the `defines += [...]` thing only works in the config("version") {...} section in remoting/BUILD.gn. Putting it somewhere else will not define the marco. Not sure whether it is the right place
https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1806963002/diff/1/remoting/BUILD.gn#newcode16 remoting/BUILD.gn:16: defines += [ "DISABLE_WEBRTC=1" ] On 2016/03/16 19:45:34, yuweih wrote: > On 2016/03/16 19:12:36, Sergey Ulanov wrote: > > I think ideally we want to disable it only for offical builds. Also it should > > always be disabled for NaCl builds: > > if (!is_official_build && !is_nacl) { > > defines += [ "ENABLE_WEBRTC_REMOTING_CLIENT=1" ] > > } > > So we are disabling WebRTC on all platform, right? Correct. Beside Android that only makes difference on iOS. > It seems the `defines += [...]` thing only works in the config("version") {...} > section in remoting/BUILD.gn. Putting it somewhere else will not define the > marco. Not sure whether it is the right place The targets that use this config have to explicitly pull it, e.g.: configs += [ "//remoting:version" ] If you add another config then you will need to enable it in the same way for the targets that need this define, i.e. in remoting/client (client/BUILD.gn) and remoting/android:remoting_client_jni (android/BUILD.gn)
Followed reviewer's feedback. Used `ENABLE_WEBRTC_REMOTING_CLIENT` as the macro name. Macro defined in a separate config section.
https://codereview.chromium.org/1806963002/diff/20001/remoting/client/chromot... File remoting/client/chromoting_client.cc (right): https://codereview.chromium.org/1806963002/diff/20001/remoting/client/chromot... remoting/client/chromoting_client.cc:75: #if defined(OS_NACL) Please see my previous comment. We no longer need OS_NACL here. Instead this ifdef should use ENABLE_WEBRTC_REMOTING_CLIENT, so the LOG(FATAL) statement is compiled whenever webrtc is disabled.
PTAL https://codereview.chromium.org/1806963002/diff/1/remoting/client/chromoting_... File remoting/client/chromoting_client.cc (right): https://codereview.chromium.org/1806963002/diff/1/remoting/client/chromoting_... remoting/client/chromoting_client.cc:76: LOG(FATAL) << "WebRTC is not supported in webapp."; On 2016/03/16 19:12:36, Sergey Ulanov wrote: > We want this LOG(FATAL) on all platforms: > #if !defined(ENABLED_WEBRTC_CLIENT) > LOG(FATAL) << "WebRTC is not supported"; > #elif > .... > #endif Done. https://codereview.chromium.org/1806963002/diff/1/remoting/client/jni/chromot... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/1806963002/diff/1/remoting/client/jni/chromot... remoting/client/jni/chromoting_jni_instance.cc:420: #endif // if !defined(DISABLE_WEBRTC) On 2016/03/16 19:12:36, Sergey Ulanov wrote: > don't need 'if'. > two spaces before // please Done.
On 2016/03/16 21:21:00, yuweih wrote: > PTAL > > https://codereview.chromium.org/1806963002/diff/1/remoting/client/chromoting_... > File remoting/client/chromoting_client.cc (right): > > https://codereview.chromium.org/1806963002/diff/1/remoting/client/chromoting_... > remoting/client/chromoting_client.cc:76: LOG(FATAL) << "WebRTC is not supported > in webapp."; > On 2016/03/16 19:12:36, Sergey Ulanov wrote: > > We want this LOG(FATAL) on all platforms: > > #if !defined(ENABLED_WEBRTC_CLIENT) > > LOG(FATAL) << "WebRTC is not supported"; > > #elif > > .... > > #endif > > Done. > > https://codereview.chromium.org/1806963002/diff/1/remoting/client/jni/chromot... > File remoting/client/jni/chromoting_jni_instance.cc (right): > > https://codereview.chromium.org/1806963002/diff/1/remoting/client/jni/chromot... > remoting/client/jni/chromoting_jni_instance.cc:420: #endif // if > !defined(DISABLE_WEBRTC) > On 2016/03/16 19:12:36, Sergey Ulanov wrote: > > don't need 'if'. > > two spaces before // please > > Done. looks like GN is being touched, reminder to take a look at GYP too if it is applicable. iOS does not use GN yet.
On 2016/03/16 21:29:42, nicholss wrote: > On 2016/03/16 21:21:00, yuweih wrote: > > PTAL > > > > > https://codereview.chromium.org/1806963002/diff/1/remoting/client/chromoting_... > > File remoting/client/chromoting_client.cc (right): > > > > > https://codereview.chromium.org/1806963002/diff/1/remoting/client/chromoting_... > > remoting/client/chromoting_client.cc:76: LOG(FATAL) << "WebRTC is not > supported > > in webapp."; > > On 2016/03/16 19:12:36, Sergey Ulanov wrote: > > > We want this LOG(FATAL) on all platforms: > > > #if !defined(ENABLED_WEBRTC_CLIENT) > > > LOG(FATAL) << "WebRTC is not supported"; > > > #elif > > > .... > > > #endif > > > > Done. > > > > > https://codereview.chromium.org/1806963002/diff/1/remoting/client/jni/chromot... > > File remoting/client/jni/chromoting_jni_instance.cc (right): > > > > > https://codereview.chromium.org/1806963002/diff/1/remoting/client/jni/chromot... > > remoting/client/jni/chromoting_jni_instance.cc:420: #endif // if > > !defined(DISABLE_WEBRTC) > > On 2016/03/16 19:12:36, Sergey Ulanov wrote: > > > don't need 'if'. > > > two spaces before // please > > > > Done. > > looks like GN is being touched, reminder to take a look at GYP too if it is > applicable. iOS does not use GN yet. I'm not sure how the GYP thing works, but I just tried to add a condition for remoting_client, not sure how I can test this...
To try GYP build you can follow the "GYP build" instructions on https://www.chromium.org/developers/how-tos/get-the-code Basically you just need to run gyp instead of GN to generate ninja files: ./build/gyp_chromium -D OS=android gyp puts results in out/Debug and out/Release and then you can run ninja in these directories same as when using GN. https://codereview.chromium.org/1806963002/diff/60001/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1806963002/diff/60001/remoting/BUILD.gn#newco... remoting/BUILD.gn:17: config("webrtc") { Use more specific name please. E.g. enable_webrtc_client https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... File remoting/client/chromoting_client.cc (right): https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... remoting/client/chromoting_client.cc:75: #if !defined(ENABLE_WEBRTC_REMOTING_CLIENT) // !defined(ENABLE_WEBRTC_REMOTING_CLIENT) Don't need the comment. https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... remoting/client/chromoting_client.cc:76: LOG(FATAL) << "WebRTC is not supported"; nit: add . at the end of the message. https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... remoting/client/chromoting_client.cc:79: #endif // defined(ENABLE_WEBRTC_REMOTING_CLIENT) nit: add comment for the #else statement or remove the comment from here. The comments for short #ifdef blocks like this aren't required (because it's clear what's going on without them), but we want to be consistent, i.e. you should either have comments for both #else and #endif or neither. https://codereview.chromium.org/1806963002/diff/60001/remoting/remoting_clien... File remoting/remoting_client.gypi (right): https://codereview.chromium.org/1806963002/diff/60001/remoting/remoting_clien... remoting/remoting_client.gypi:26: ['buildtype!="Official"', { Also please add this for the remoting_client_jni in remoting_android.gypi.
I'm following these instructions for GYP. Thanks! https://codereview.chromium.org/1806963002/diff/60001/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1806963002/diff/60001/remoting/BUILD.gn#newco... remoting/BUILD.gn:17: config("webrtc") { On 2016/03/16 23:15:07, Sergey Ulanov wrote: > Use more specific name please. E.g. enable_webrtc_client I was thinking of maybe we can later put other webrtc configs in the section... I'll just use enable_webrtc_client for now https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... File remoting/client/chromoting_client.cc (right): https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... remoting/client/chromoting_client.cc:75: #if !defined(ENABLE_WEBRTC_REMOTING_CLIENT) // !defined(ENABLE_WEBRTC_REMOTING_CLIENT) On 2016/03/16 23:15:07, Sergey Ulanov wrote: > Don't need the comment. Acknowledged. https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... remoting/client/chromoting_client.cc:76: LOG(FATAL) << "WebRTC is not supported"; On 2016/03/16 23:15:07, Sergey Ulanov wrote: > nit: add . at the end of the message. Acknowledged. https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... remoting/client/chromoting_client.cc:79: #endif // defined(ENABLE_WEBRTC_REMOTING_CLIENT) On 2016/03/16 23:15:07, Sergey Ulanov wrote: > nit: add comment for the #else statement or remove the comment from here. The > comments for short #ifdef blocks like this aren't required (because it's clear > what's going on without them), but we want to be consistent, i.e. you should > either have comments for both #else and #endif or neither. Acknowledged. https://codereview.chromium.org/1806963002/diff/60001/remoting/remoting_clien... File remoting/remoting_client.gypi (right): https://codereview.chromium.org/1806963002/diff/60001/remoting/remoting_clien... remoting/remoting_client.gypi:26: ['buildtype!="Official"', { On 2016/03/16 23:15:08, Sergey Ulanov wrote: > Also please add this for the remoting_client_jni in remoting_android.gypi. Acknowledged.
https://codereview.chromium.org/1806963002/diff/60001/remoting/BUILD.gn File remoting/BUILD.gn (right): https://codereview.chromium.org/1806963002/diff/60001/remoting/BUILD.gn#newco... remoting/BUILD.gn:17: config("webrtc") { On 2016/03/16 23:15:07, Sergey Ulanov wrote: > Use more specific name please. E.g. enable_webrtc_client Done. https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... File remoting/client/chromoting_client.cc (right): https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... remoting/client/chromoting_client.cc:75: #if !defined(ENABLE_WEBRTC_REMOTING_CLIENT) // !defined(ENABLE_WEBRTC_REMOTING_CLIENT) On 2016/03/16 23:15:07, Sergey Ulanov wrote: > Don't need the comment. Done. https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... remoting/client/chromoting_client.cc:76: LOG(FATAL) << "WebRTC is not supported"; On 2016/03/16 23:15:07, Sergey Ulanov wrote: > nit: add . at the end of the message. Done. https://codereview.chromium.org/1806963002/diff/60001/remoting/client/chromot... remoting/client/chromoting_client.cc:79: #endif // defined(ENABLE_WEBRTC_REMOTING_CLIENT) On 2016/03/16 23:15:07, Sergey Ulanov wrote: > nit: add comment for the #else statement or remove the comment from here. The > comments for short #ifdef blocks like this aren't required (because it's clear > what's going on without them), but we want to be consistent, i.e. you should > either have comments for both #else and #endif or neither. Done. https://codereview.chromium.org/1806963002/diff/60001/remoting/remoting_clien... File remoting/remoting_client.gypi (right): https://codereview.chromium.org/1806963002/diff/60001/remoting/remoting_clien... remoting/remoting_client.gypi:26: ['buildtype!="Official"', { On 2016/03/16 23:15:08, Sergey Ulanov wrote: > Also please add this for the remoting_client_jni in remoting_android.gypi. Done.
lgtm
The CQ bit was checked by yuweih@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1806963002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1806963002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Reduce APK size by disabling WebRTC. Reduced official build size from 7.9MB to 6.4MB. Can reenable WebRTC by changing the BUILD.gn file. BUG=595038 ========== to ========== Reduce APK size by disabling WebRTC. Reduced official build size from 7.9MB to 6.4MB. Can reenable WebRTC by changing the BUILD.gn file. BUG=595038 Committed: https://crrev.com/e8f2d518b2337b2ae8736ee0f032d2fffc8b16b8 Cr-Commit-Position: refs/heads/master@{#381636} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e8f2d518b2337b2ae8736ee0f032d2fffc8b16b8 Cr-Commit-Position: refs/heads/master@{#381636} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
