Issue 3136 - Only show about:feedback to release build users

Issue 3136 - Only show about:feedback to release build users

This check has to happen _before_ incrementing the launch count - otherwise
users that install the beta and later update to the release version will never
see about:feedback. Moved the Play store check up to make our changes less
intrusive here, but it would have been fine if that one stayed where it was.

[René Jeschke, 2015-10-01 14:38:12]

https://codereview.adblockplus.org/29328791/diff/29328792/mobile/android/base...
File mobile/android/base/AppConstants.java.in (right):

https://codereview.adblockplus.org/29328791/diff/29328792/mobile/android/base...
mobile/android/base/AppConstants.java.in:331: //#ifdef ABB_RELEASE_BUILD
This then works using environment variables, yes? So no changes to configure
needed?

https://codereview.adblockplus.org/29328791/diff/29328792/mobile/android/base...
File mobile/android/base/BrowserApp.java (right):

https://codereview.adblockplus.org/29328791/diff/29328792/mobile/android/base...
mobile/android/base/BrowserApp.java:3544: if (launchCount <
FEEDBACK_LAUNCH_COUNT &&
Is it really necessary to check `wasInstalledFromPlayStore()` on _every_ startup
(when it's a release build and until FEEDBACK_LAUNCH_COUNT is reached)?

The `wasInstalledFromPlayStore()` isn't that resource hungry, but still it
shouldn't be called without a reason IMHO.

So, I would prefer having both checks performed in this if: 
if (launchCount == FEEDBACK_LAUNCH_COUNT) {
->
if (launchCount == FEEDBACK_LAUNCH_COUNT && AppConstants.ABB_RELEASE_BUILD &&
wasInstalledFromPlayStore()) {

[Felix Dahlke, 2015-10-01 15:30:53]

https://codereview.adblockplus.org/29328791/diff/29328792/mobile/android/base...
File mobile/android/base/AppConstants.java.in (right):

https://codereview.adblockplus.org/29328791/diff/29328792/mobile/android/base...
mobile/android/base/AppConstants.java.in:331: //#ifdef ABB_RELEASE_BUILD
On 2015/10/01 14:38:11, René Jeschke wrote:
> This then works using environment variables, yes? So no changes to configure
> needed?

Yup, exactly. The mozconfigs for the release builds in adblockbrowser-build are
setting this, verified that it works.

https://codereview.adblockplus.org/29328791/diff/29328792/mobile/android/base...
File mobile/android/base/BrowserApp.java (right):

https://codereview.adblockplus.org/29328791/diff/29328792/mobile/android/base...
mobile/android/base/BrowserApp.java:3544: if (launchCount <
FEEDBACK_LAUNCH_COUNT &&
On 2015/10/01 14:38:11, René Jeschke wrote:
> Is it really necessary to check `wasInstalledFromPlayStore()` on _every_
startup
> (when it's a release build and until FEEDBACK_LAUNCH_COUNT is reached)?
> 
> The `wasInstalledFromPlayStore()` isn't that resource hungry, but still it
> shouldn't be called without a reason IMHO.
> 
> So, I would prefer having both checks performed in this if: 
> if (launchCount == FEEDBACK_LAUNCH_COUNT) {
> ->
> if (launchCount == FEEDBACK_LAUNCH_COUNT && AppConstants.ABB_RELEASE_BUILD &&
> wasInstalledFromPlayStore()) {

I've changed this mainly to ensure that users that migrated from a beta build to
a release build, or even from a non-Play build to a Play one will still end up
seeing about:feedback, as long as they are on a Play release build. If we run
into performance issues with the Play store check, I'd prefer to optimise this
rather than to end up never showing about:feedback to people that didn't
initially install a Play release build.

[René Jeschke, 2015-10-02 08:22:35]

LGTM

https://codereview.adblockplus.org/29328791/diff/29328792/mobile/android/base...
File mobile/android/base/BrowserApp.java (right):

https://codereview.adblockplus.org/29328791/diff/29328792/mobile/android/base...
mobile/android/base/BrowserApp.java:3544: if (launchCount <
FEEDBACK_LAUNCH_COUNT &&
On 2015/10/01 15:30:53, Felix Dahlke wrote:
> On 2015/10/01 14:38:11, René Jeschke wrote:
> > Is it really necessary to check `wasInstalledFromPlayStore()` on _every_
> startup
> > (when it's a release build and until FEEDBACK_LAUNCH_COUNT is reached)?
> > 
> > The `wasInstalledFromPlayStore()` isn't that resource hungry, but still it
> > shouldn't be called without a reason IMHO.
> > 
> > So, I would prefer having both checks performed in this if: 
> > if (launchCount == FEEDBACK_LAUNCH_COUNT) {
> > ->
> > if (launchCount == FEEDBACK_LAUNCH_COUNT && AppConstants.ABB_RELEASE_BUILD
&&
> > wasInstalledFromPlayStore()) {
> 
> I've changed this mainly to ensure that users that migrated from a beta build
to
> a release build, or even from a non-Play build to a Play one will still end up
> seeing about:feedback, as long as they are on a Play release build. If we run
> into performance issues with the Play store check, I'd prefer to optimise this
> rather than to end up never showing about:feedback to people that didn't
> initially install a Play release build.

I wasn't really concerned about performance issues, it just felt pretty
unnecessary ... I didn't think about that issue you mentioned, though, so it
really makes sense to leave it like this.