Closed Bug 1240579 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(1 file)

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.
Component: Build Config → General
Product: Core → Firefox for Android
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/
Attachment #8709138 - Flags: review?(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.
Attachment #8709138 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/f98d53de194b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(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?
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
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
Flags: needinfo?(nalexander)
(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...
Flags: needinfo?(nalexander)
You need to log in before you can comment on or make changes to this bug.