Check adblockplus.org for updates

While this works, I'm wondering if my approach makes sense.

First of all: Please note that the update server decides whether the client should install an update, there is no client-side check. Android won't complain about the update as long as the version code is higher, so we're pretty flexible here and can change the user visible version later.

Firefox mobile update requests currently look like this:

/Fennec/34.0/20141208113329/Android_arm-eabi-gcc3/en-US/default/4.4.4/default/default/34.0/update.xml

I thought it makes sense to force fit this onto our usual update request format, as we do it in ABP for Android. Here's the result of that:

/devbuilds/adblockbrowser?type=0&addonName=adblockbrowser&addonVersion=20141208113329&applicationName=android&applicationVersion=4.4.4&platform=Fennec&platformVersion=34.0&locale=en-US

I've tried to keep things similar to ABP for Android, so:

addon(Name|Version): Our app, the browser
application(Name|Version): Android
platform(Name|Version): The app ours is based on, "Fennec" and "34.0" in this case - I'd argue we don't really need to know this on the update server, but the same goes for platform(Name|Version) in ABP for Android.

I can't say I'm particularly happy with that, this would make more sense IMO:

addon(Name|Version): Omitted! (Or maybe the ABP version we use?)
application(Name|Version): Our app, the browser
platform(Name|Version): The Android platform and it's version

However, that'd be confusing in comparison to ABP for Android, so I thought keeping it similar would be the lesser evil. Please let me know what you think makes more sense.

There's also some constants I completely ignored, because I think they're not relevant for now:

- AppConstants.MOZ_APP_ABI + pkgSpecial (arm-eabi-gcc3)
- AppConstants.MOZ_UPDATE_CHANNEL (default)
- AppConstants.MOZILLA_VERSION (34.0)

[Felix Dahlke, 2014-12-09 19:58:34]

Feedback quite welcome :)

http://codereview.adblockplus.org/4865921483014144/diff/5629499534213120/mobi...
File mobile/android/base/updater/UpdateServiceHelper.java (right):

http://codereview.adblockplus.org/4865921483014144/diff/5629499534213120/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:62: "&platform=" +
AppConstants.MOZ_APP_BASENAME +
This just says "Fennec". We can change it to something of our liking of course,
but I feel there's no real point: It's just used internally. Anything
user-visible is taken from the branding.

http://codereview.adblockplus.org/4865921483014144/diff/5629499534213120/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:63: "&platformVersion=" +
AppConstants.MOZ_APP_VERSION +
We can and probably will change this since it's the user visible version. In
that case we could switch to AppConstants.MOZILLA_VERSION which should be
unaffected by this. At the end of the day, I don't think it matters really, I
don't see what the update server would need this for.

http://codereview.adblockplus.org/4865921483014144/diff/5668600916475904/mobi...
File mobile/android/base/updater/UpdateServiceHelper.java (right):

http://codereview.adblockplus.org/4865921483014144/diff/5668600916475904/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:62: "&platform=" +
AppConstants.MOZ_APP_BASENAME +
This just says "Fennec". We can change it to something of our liking of course,
but I feel there's no real point: It's just used internally. Anything
user-visible is taken from the branding.

http://codereview.adblockplus.org/4865921483014144/diff/5668600916475904/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:63: "&platformVersion=" +
AppConstants.MOZ_APP_VERSION +
We can and probably will change this since it's the user visible version. In
that case we could switch to AppConstants.MOZILLA_VERSION which should be
unaffected by this. At the end of the day, I don't think it matters really, I
don't see what the update server would need this for.

http://codereview.adblockplus.org/4865921483014144/diff/5668600916475904/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:95: replace("%OS_VERSION%",
String.valueOf(Build.VERSION.SDK_INT)).
Firefox uses the full version string here, but I feel it makes more sense for us
to be consistent with ABP for Android and use the API level.

[René Jeschke, 2015-02-18 15:15:42]

Regarding: AppConstants.MOZ_APP_ABI

Isn't Fennec already available for x86/intel platforms? If so, we would force an
ARM user to download an APK that's containing twice as much native code, or even
remove x86 compatibility after all?

IMHO we should stay future safe here, shouldn't we?

[Wladimir Palant, 2015-02-18 18:05:55]

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
File mobile/android/base/updater/UpdateServiceHelper.java (right):

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:57: "?type=0" +
What is this type=0 about?

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:63: "&platformVersion=" +
AppConstants.MOZILLA_VERSION +
Technically speaking, isn't this Adblock Plus for Firefox running in Adblock
Browser? Meaning:

addonName=adblockplus
addonVersion=2.6.7
applicationName=adblockbrowser
applicationVersion=AppConstants.MOZ_APP_VERSION
platform=gecko
platformVersion=AppConstants.MOZILLA_VERSION

IMHO that's the parameters we are really interested in, sending OS info is
optional - can be done via some new parameters.

[Felix Dahlke, 2015-02-19 13:40:16]

On 2015/02/18 15:15:42, René Jeschke wrote:
> Regarding: AppConstants.MOZ_APP_ABI
> 
> Isn't Fennec already available for x86/intel platforms? If so, we would force
an
> ARM user to download an APK that's containing twice as much native code, or
even
> remove x86 compatibility after all?
> 
> IMHO we should stay future safe here, shouldn't we?

Well, that's a good point. However, I haven't looked into how to create builds
for anything but arm-eabi-gcc3 yet, and I'm not really sure why we would care
about the gcc3 part e.g.

I'd rather stick to ARM for now and look into supporting x86 in a follow-up.
That'd then mean we have to change the update format, yes. Seems like the best
thing to add this to the value of the platform parameter, we're doing the same
for ABP for IE.

[Felix Dahlke, 2015-02-19 13:40:22]

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
File mobile/android/base/updater/UpdateServiceHelper.java (right):

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:57: "?type=0" +
On 2015/02/18 18:05:55, Wladimir Palant wrote:
> What is this type=0 about?

How I understood it, this means it's an automatic update check, 1 would mean
it's a manual one. Haven't really looked into how to figure that out here
frankly, would it be horrible to hard code automatic for now?

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:63: "&platformVersion=" +
AppConstants.MOZILLA_VERSION +
On 2015/02/18 18:05:55, Wladimir Palant wrote:
> Technically speaking, isn't this Adblock Plus for Firefox running in Adblock
> Browser? Meaning:
> 
> addonName=adblockplus
> addonVersion=2.6.7
> applicationName=adblockbrowser
> applicationVersion=AppConstants.MOZ_APP_VERSION
> platform=gecko
> platformVersion=AppConstants.MOZILLA_VERSION
> 
> IMHO that's the parameters we are really interested in, sending OS info is
> optional - can be done via some new parameters.

No, it's Adblock Browser's update mechanism, just incidentally using the same
format as ABP update requests. We're interested in Adblock Browser's version and
platform here, so we can offer the proper APKs. 

The parameter names make similarly little sense as with ABP for Android, since
it's not an addon of any kind. But I thought it's easier to work with the data
if it's in the same format as everything else. Do you think it'd make more sense
to just stick with Mozilla's format here?

[Felix Dahlke, 2015-02-19 22:21:58]

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
File mobile/android/base/updater/UpdateServiceHelper.java (right):

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:63: "&platformVersion=" +
AppConstants.MOZILLA_VERSION +
On 2015/02/19 13:40:22, Felix H. Dahlke wrote:
> On 2015/02/18 18:05:55, Wladimir Palant wrote:
> > Technically speaking, isn't this Adblock Plus for Firefox running in Adblock
> > Browser? Meaning:
> > 
> > addonName=adblockplus
> > addonVersion=2.6.7
> > applicationName=adblockbrowser
> > applicationVersion=AppConstants.MOZ_APP_VERSION
> > platform=gecko
> > platformVersion=AppConstants.MOZILLA_VERSION
> > 
> > IMHO that's the parameters we are really interested in, sending OS info is
> > optional - can be done via some new parameters.
> 
> No, it's Adblock Browser's update mechanism, just incidentally using the same
> format as ABP update requests. We're interested in Adblock Browser's version
and
> platform here, so we can offer the proper APKs. 
> 
> The parameter names make similarly little sense as with ABP for Android, since
> it's not an addon of any kind. But I thought it's easier to work with the data
> if it's in the same format as everything else. Do you think it'd make more
sense
> to just stick with Mozilla's format here?

Note that we probably want to go with your variant for the
notification/subscription update requests happening from within ABP itself -
otherwise we can't distinguish between Adblock Browser users and ABP for Firefox
users.

This request here is for devbuild updates of the browser though, so I don't see
why it should pretend to be from ABP.

[Wladimir Palant, 2015-02-23 12:32:24]

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
File mobile/android/base/updater/UpdateServiceHelper.java (right):

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:57: "?type=0" +
On 2015/02/19 13:40:22, Felix H. Dahlke wrote:
> How I understood it, this means it's an automatic update check, 1 would mean
> it's a manual one.

Yes, that's what I remembered as well which is why I was surprised to see this
parameter hardcoded.

> Haven't really looked into how to figure that out here
> frankly, would it be horrible to hard code automatic for now?

Should we send this parameter at all if its value would be bogus?

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:63: "&platformVersion=" +
AppConstants.MOZILLA_VERSION +
On 2015/02/19 22:21:58, Felix H. Dahlke wrote:
> This request here is for devbuild updates of the browser though, so I don't
see
> why it should pretend to be from ABP.

Fair enough. Still, the platform should be "gecko" rather than "mozilla".

[Felix Dahlke, 2015-02-23 22:24:00]

New patch set up.

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
File mobile/android/base/updater/UpdateServiceHelper.java (right):

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:57: "?type=0" +
On 2015/02/23 12:32:24, Wladimir Palant wrote:
> On 2015/02/19 13:40:22, Felix H. Dahlke wrote:
> > How I understood it, this means it's an automatic update check, 1 would mean
> > it's a manual one.
> 
> Yes, that's what I remembered as well which is why I was surprised to see this
> parameter hardcoded.
> 
> > Haven't really looked into how to figure that out here
> > frankly, would it be horrible to hard code automatic for now?
> 
> Should we send this parameter at all if its value would be bogus?

Yeah that's a good point, I've removed it.

http://codereview.adblockplus.org/4865921483014144/diff/5757334940811264/mobi...
mobile/android/base/updater/UpdateServiceHelper.java:63: "&platformVersion=" +
AppConstants.MOZILLA_VERSION +
On 2015/02/23 12:32:24, Wladimir Palant wrote:
> On 2015/02/19 22:21:58, Felix H. Dahlke wrote:
> > This request here is for devbuild updates of the browser though, so I don't
> see
> > why it should pretend to be from ABP.
> 
> Fair enough. Still, the platform should be "gecko" rather than "mozilla".

Done.

[Wladimir Palant, 2015-02-24 14:45:10]

LGTM

[Felix Dahlke, 2015-02-25 03:29:22]

René: Here's a follow-up for building for other platforms and sending that
information: https://trello.com/c/gaG7n3sd/51-build-for-other-platforms-e-g-x86

[René Jeschke, 2015-02-25 06:22:27]

LGTM