1243855 - Add Java HTTP client for interacting with autopush service endpoint

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

As it says on the tin: an AutopushClient, similar in spirit to https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java and other examples in the tree.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1243855 - Add Java client for interacting with autopush endpoint service. r?rnewman,sebastian

A few notes: the test is live, so I've marked it @Ignore, so that it
doesn't run during |mach gradle test|.  There's some value in mocking
the service endpoint, but this is how I verify that the server works,
so it has more value right now as a live test than a mocked test.  In
the future, that probably won't be true.

There are issues running the test locally because Robolectric doesn't
provide all the cipher suites we use in GlobalConstants: in
particular, the GCM suites aren't supported.  This may improve as
Robolectric matures, or we may add a work-around in the code (like at
http://androidxref.com/4.4.4_r1/xref/libcore/support/src/test/java/libcore/java/security/StandardNames.java#68),
or we may add a test-specific flag.  For now, I'm not going to address
it directly.

Finally, I put the code in mobile/android/services, simply because the
less that goes into base, the better our build times will be.

Review commit: https://reviewboard.mozilla.org/r/33061/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33061/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8714387 [details]
MozReview Request: Bug 1243855 - Add Java client for interacting with autopush endpoint service. r?rnewman,sebastian

https://reviewboard.mozilla.org/r/33061/#review29991

This code is pretty nice to read!

I feel a bit uncomfortable with all the generic exception catching (I'd rather like to see things like NullPointerException explode Nightly/Aurora/Beta builds) but it seems like this is needed to guarantee that the error handler is always called.

::: mobile/android/services/src/main/java/org/mozilla/gecko/push/autopush/AutopushClient.java:72
(Diff revision 1)
> +            throw new IllegalArgumentException("Must provide a server URI.");

I recently read a long article/discussion about whether to use IllegalArgumentException or NullPointerException for null parameters. It seems like this totally divides the Java community. :)

I don't have a definite opinion myself though.

::: mobile/android/services/src/main/java/org/mozilla/gecko/push/autopush/AutopushClient.java:92
(Diff revision 1)
> +        final String[] parts = serverURI.split("/", -1); // The -1 keeps the trailing empty part.
> +        if (parts.length < 3) {
> +            throw new AutopushClientException("Could not get sender ID from autopush server URI: " + serverURI);
> +        }
> +        if (!parts[parts.length - 1].isEmpty()) {
> +            // We guarantee a trailing slash, so we should always have an empty part at the tail.
> +            throw new AutopushClientException("Could not get sender ID from autopush server URI: " + serverURI);
> +        }
> +        if (!"gcm".equals(parts[parts.length - 3])) {
> +            // We should always have /gcm/senderID/.
> +            throw new AutopushClientException("Could not get sender ID from autopush server URI: " + serverURI);
> +        }
> +        final String senderID = parts[parts.length - 2];
> +        if (senderID.isEmpty()) {
> +            // Something is horribly wrong -- we have /gcm//.  Abort.
> +            throw new AutopushClientException("Could not get sender ID from autopush server URI: " + serverURI);
> +        }

Is there a strong reason why you are dissecting the string instead of using regex? Just curious. The current code is very readable like that!

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/WaitHelper.java:173
(Diff revision 1)
> +  public static Executor newSynchronousExecutor() {
> +    return new Executor() {
> +
> +      @Override
> +      public void execute(Runnable runnable) {
> +        runnable.run();
> +      }
> +    };
> +  }

This is super smart.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8714387 [details]
MozReview Request: Bug 1243855 - Add Java client for interacting with autopush endpoint service. r?rnewman,sebastian

https://reviewboard.mozilla.org/r/33061/#review30141

This looks good to me; the proof is in the running, I suppose.

::: mobile/android/services/src/main/java/org/mozilla/gecko/push/autopush/AutopushClient.java:39
(Diff revision 1)
> + * This client is written against a work-in-progress, un-deployed upstream commit.

Hooray!

::: mobile/android/services/src/main/java/org/mozilla/gecko/push/autopush/AutopushClient.java:96
(Diff revision 1)
> +        if (!parts[parts.length - 1].isEmpty()) {

Use TextUtils.isEmpty, just in case.

::: mobile/android/services/src/main/java/org/mozilla/gecko/push/autopush/AutopushClient.java:105
(Diff revision 1)
> +        if (senderID.isEmpty()) {

TextUtils.

::: mobile/android/services/src/main/java/org/mozilla/gecko/push/autopush/AutopushClient.java:269
(Diff revision 1)
> +            request.addHeader(HttpHeaders.ACCEPT_LANGUAGE, Utils.getLanguageTag(locale));

File a bug to replace `sync.Utils.getLanguageTag` (throughout our codebase) with `Locales.getLanguageTag`.

Comment 4

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/900f4a6eebc269410a821a885b505c22213b4fb2
Bug 1243855 - Add Java client for interacting with autopush endpoint service. r=rnewman,sebastian

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/900f4a6eebc2