Closed
Bug 1067060
Opened 10 years ago
Closed 10 years ago
Miscellaneous Robocop code cleanup.
Categories
(Firefox for Android Graveyard :: Testing, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: ckitching, Assigned: ckitching)
Details
Attachments
(5 files)
3.40 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
6.05 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
21.03 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
2.75 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
3.00 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
It turns out that 3 is, in fact, an integer. :P
Attachment #8489066 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=298d47b8e894
Assignee | ||
Comment 8•10 years ago
|
||
Yes.
Assignee | ||
Updated•10 years ago
|
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+
Thanks for the cleanup!
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c51908365510 https://hg.mozilla.org/integration/fx-team/rev/21e0ccc07a97 https://hg.mozilla.org/integration/fx-team/rev/0216f2e858be https://hg.mozilla.org/integration/fx-team/rev/b4c908baae64 https://hg.mozilla.org/integration/fx-team/rev/2b204a30fd9f https://hg.mozilla.org/integration/fx-team/rev/dbbd4188d63e
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c51908365510 https://hg.mozilla.org/mozilla-central/rev/21e0ccc07a97 https://hg.mozilla.org/mozilla-central/rev/0216f2e858be https://hg.mozilla.org/mozilla-central/rev/b4c908baae64 https://hg.mozilla.org/mozilla-central/rev/2b204a30fd9f https://hg.mozilla.org/mozilla-central/rev/dbbd4188d63e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•