Closed Bug 1067060 Opened 10 years ago Closed 10 years ago

Miscellaneous Robocop code cleanup.

Categories

(Firefox for Android Graveyard :: Testing, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: ckitching, Assigned: ckitching)

Details

Attachments

(5 files)

For various slightly awful reasons I found myself building robocop with -Xlint:all, and it showed me much badness.

So, let's make it build with Xlint:all (well, probably with -serial because nobody cares) and fix the badness while we're here.
Starting with a juicy one: replace use of SyncQueue with explicit use of the concurrency primitives. Makes it a tad more obvious what we're doing and prevents us from having to write <Object>, which is generally a sign of badness.
Attachment #8489063 - Flags: review?(michael.l.comella)
Having two enum classes for PanelTypes (tablet/phone) isn't great OOP. They're not different classes of 'thing', they're two orderings of the same 'things'. It also pushes you into using a bunch of evilness when trying to make use of it. Here, I replace the two enums with two arrays of PanelTypes, making things rather more concise as well as making the compiler happy.
Attachment #8489064 - Flags: review?(michael.l.comella)
It turns out that 3 is, in fact, an integer. :P
Attachment #8489066 - Flags: review?(michael.l.comella)
StringHelper is a static helper class: You definitely shouldn't be constructing an instance of it! (which you never use, anyway).
Attachment #8489067 - Flags: review?(michael.l.comella)
Browser is a static helper class: Don't instantiate it! (Naughty Android for not having a private constructor for it), and you shouldn't ever reference static members from an instance anyway.
Attachment #8489068 - Flags: review?(michael.l.comella)
Attachment #8489064 - Flags: review?(michael.l.comella) → review+
Can I get a try push for these patches? The secret's in the sauce. :)
Status: NEW → ASSIGNED
Flags: needinfo?(chriskitching)
Yes.
Flags: needinfo?(chriskitching)
Comment on attachment 8489067 [details] [diff] [review]
Don't construct StringHelper

Review of attachment 8489067 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/StringHelper.java
@@ +185,5 @@
>      public static final String BOOKMARK_REMOVED_LABEL = "Bookmark removed";
>      public static final String BOOKMARK_UPDATED_LABEL = "Bookmark updated";
>      public static final String BOOKMARK_OPTIONS_LABEL = "Options";
> +
> +    private StringHelper() {}

nit: I think I'd prefer this near the top of the file, in the usual constructor place - it provides added visibility that this is something that should never be constructed.
Attachment #8489067 - Flags: review?(michael.l.comella) → review+
Attachment #8489068 - Flags: review?(michael.l.comella) → review+
Attachment #8489066 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8489063 [details] [diff] [review]
Remove use of SynchronousQueue from RobocopUtils

Review of attachment 8489063 [details] [diff] [review]:
-----------------------------------------------------------------

Feel free to land all parts if the try push is green.

::: build/mobile/robocop/RobocopUtils.java
@@ +38,5 @@
> +
> +        synchronized (sentinel) {
> +            while (!sentinel.get()) {
> +                try {
> +                    sentinel.wait(MAX_WAIT_MS);

If we woke spuriously, we could wait an extra MAX_WAIT_MS, on top of however much time we've already waited for. Not a big deal though so add a comment that it's not worth your time, or fix it if you think it is.
Attachment #8489063 - Flags: review?(michael.l.comella) → review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: