Issue 3135 - Show the first run slide about the beta community to users that installed via Play and…

Issue 3135 - Show the first run slide about the beta community to users that installed via Play and…

[anton, 2016-09-12 11:36:11]

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

https://codereview.adblockplus.org/29341330/diff/29341331/mobile/android/base...
mobile/android/base/BrowserApp.java:3561: AppConstants.ABB_RELEASE_BUILD &&
org.adblockplus.browser.BrowserAppUtils.wasInstalledFromPlayStore(this)) {
what the point of using full classname with package? Can we use "import
org.adblockplus.browser.BrowserAppUtils;" and
"BrowserAppUtils.wasInstalledFromPlayStore(..)" here?

[diegocarloslima, 2016-10-20 12:18:08]

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

https://codereview.adblockplus.org/29341330/diff/29341331/mobile/android/base...
mobile/android/base/BrowserApp.java:3561: AppConstants.ABB_RELEASE_BUILD &&
org.adblockplus.browser.BrowserAppUtils.wasInstalledFromPlayStore(this)) {
On 2016/09/12 11:36:11, anton wrote:
> what the point of using full classname with package? Can we use "import
> org.adblockplus.browser.BrowserAppUtils;" and
> "BrowserAppUtils.wasInstalledFromPlayStore(..)" here?

I agree with you that using an import + simple class name is better. I was
trying to keep up with the previous pattern that was being used in this file,
without adding any imports from our classes. I believe that this was done to
change the minimum amount of lines to this file, but I believe that it won't
hurt to add the imports to improve readability. I have created an issue #4555
which this one will depends on for next patch submitted

[diegocarloslima, 2016-10-21 13:22:50]

On 2016/10/20 12:18:08, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29341330/diff/29341331/mobile/android/base...
> File mobile/android/base/BrowserApp.java (right):
> 
>
https://codereview.adblockplus.org/29341330/diff/29341331/mobile/android/base...
> mobile/android/base/BrowserApp.java:3561: AppConstants.ABB_RELEASE_BUILD &&
> org.adblockplus.browser.BrowserAppUtils.wasInstalledFromPlayStore(this)) {
> On 2016/09/12 11:36:11, anton wrote:
> > what the point of using full classname with package? Can we use "import
> > org.adblockplus.browser.BrowserAppUtils;" and
> > "BrowserAppUtils.wasInstalledFromPlayStore(..)" here?
> 
> I agree with you that using an import + simple class name is better. I was
> trying to keep up with the previous pattern that was being used in this file,
> without adding any imports from our classes. I believe that this was done to
> change the minimum amount of lines to this file, but I believe that it won't
> hurt to add the imports to improve readability. I have created an issue #4555
> which this one will depends on for next patch submitted

As we discussed on IRC, I'm reverting back to patch set 1 to keep the changes on
upstream code minimal. If we ever add a more significant amount of custom code,
we might reopen the discussion for using imports then.

[anton, 2016-11-02 14:09:35]

On 2016/10/21 13:22:50, diegocarloslima wrote:
> On 2016/10/20 12:18:08, diegocarloslima wrote:
> >
>
https://codereview.adblockplus.org/29341330/diff/29341331/mobile/android/base...
> > File mobile/android/base/BrowserApp.java (right):
> > 
> >
>
https://codereview.adblockplus.org/29341330/diff/29341331/mobile/android/base...
> > mobile/android/base/BrowserApp.java:3561: AppConstants.ABB_RELEASE_BUILD &&
> > org.adblockplus.browser.BrowserAppUtils.wasInstalledFromPlayStore(this)) {
> > On 2016/09/12 11:36:11, anton wrote:
> > > what the point of using full classname with package? Can we use "import
> > > org.adblockplus.browser.BrowserAppUtils;" and
> > > "BrowserAppUtils.wasInstalledFromPlayStore(..)" here?
> > 
> > I agree with you that using an import + simple class name is better. I was
> > trying to keep up with the previous pattern that was being used in this
file,
> > without adding any imports from our classes. I believe that this was done to
> > change the minimum amount of lines to this file, but I believe that it won't
> > hurt to add the imports to improve readability. I have created an issue
#4555
> > which this one will depends on for next patch submitted
> 
> As we discussed on IRC, I'm reverting back to patch set 1 to keep the changes
on
> upstream code minimal. If we ever add a more significant amount of custom
code,
> we might reopen the discussion for using imports then.

LGTM

[Felix Dahlke, 2016-12-01 17:30:41]

LGTM