1240579 - Rename Assert.java to ReleaseAssert.java (or remove it entirely)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

I'm finding newer Android toolchains don't appreciate having org.mozilla.gecko.Assert in both the main and androidTest APKs.  The issues manifest in CPU pegs in `dx`, and intermittent errors when deploying to the device with Instant Run.

It's much simpler to rename the one in the main APK.  In fact, we might just remove it altogether, since the idea is mis-guided; but that can be another day.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1240579 - Rename Assert to ReleaseAssert. r?margaret

Having org.mozilla.gecko.Assert in both the main APK and the
androidTest APK is not well supported by the Android toolchain.  It's
much simpler to rename the one in the main APK.

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

Comment 2

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f23e08aeb614

Comment 3

[:Margaret Leibovic]

Comment on attachment 8709138 [details]
MozReview Request: Bug 1240579 - Rename Assert to ReleaseAssert. r?margaret

https://reviewboard.mozilla.org/r/31297/#review28069

::: mobile/android/base/java/org/mozilla/gecko/ReleaseAssert.java:9
(Diff revision 1)
>   * Calls to methods in this class will be stripped by Proguard for release builds, so may be used

It's a bit confusing to call this `ReleaseAssert` when this comment says this will be stripped from release builds. But I suppose there are different levels of "release" going on here.

You should file a bug to remove this if you don't think it's necessary.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:141
(Diff revision 1)
> +import android.widget.Toast;

This looks extaneous.

Comment 4

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/f98d53de194b70822ebf072ece95fa54f4bb7193
Bug 1240579 - Remove org.mozilla.gecko.Assert from Fennec. f=margaret r=me

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f98d53de194b

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(Discussion about assertions came up in #mobile w/ ahunt)

Nick, it seems the patch landed here is not the same patch that Margaret reviewed – was that intentional?

(In reply to Nick Alexander :nalexander from comment #0)
> It's much simpler to rename the one in the main APK.  In fact, we might just
> remove it altogether, since the idea is mis-guided; but that can be another
> day.

Why is the Assert class mis-guided?

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #6)
> (Discussion about assertions came up in #mobile w/ ahunt)
> 
> Nick, it seems the patch landed here is not the same patch that Margaret
> reviewed – was that intentional?

Yeah.  I think I just decided to simplify this -- it wasn't widely used and the comments ("documentation") were wrong.

> (In reply to Nick Alexander :nalexander from comment #0)
> > It's much simpler to rename the one in the main APK.  In fact, we might just
> > remove it altogether, since the idea is mis-guided; but that can be another
> > day.
> 
> Why is the Assert class mis-guided?

Basically, if you're going to throw, throw.  Don't hide it.  The worst possible is to do different things in release and non-release.

We compounded that by stating that Assert's would be stripped in release (by Proguard), which I don't think is true.

So +1 to preconditions (and postconditions), but those are parts of the contract, not hidden things that throw unchecked exceptions some of the time.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I see the change [1] didn't throw – is that intentional since we're just processing the touch icon data?

It's for a JSONException, which (I think) indicates malformed JSON, in which case we should throw.

[1]: https://hg.mozilla.org/integration/fx-team/rev/f98d53de194b70822ebf072ece95fa54f4bb7193#l2.30

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #8)
> I see the change [1] didn't throw – is that intentional since we're just
> processing the touch icon data?

Yes, it's intentional.

> It's for a JSONException, which (I think) indicates malformed JSON, in which
> case we should throw.

Why?  This is lightly massaged data originating from web content...