Closed Bug 1122331 Opened 6 years ago Closed 6 years ago

Move statically initialized String values from StringHelper constructor to declaration

Categories

(Firefox for Android :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: mcomella, Assigned: psd, Mentored)

References

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 file, 2 obsolete files)

After bug 938845 lands, there are some values defined in the mobile/android/base/tests/StringHelper.java constructor that can be initialized at their declarations instead. For example:

StringHelper() {
  ...
  ABOUT_RIGHTS_URL = "about:rights";
  ABOUT_BUILDCONFIG_URL = "about:buildconfig";
  ...
}

Can be...

private final String ABOUT_RIGHTS_URL = "about:rights";
private final String ABOUT_BUILDCONFIG_URL = "about:buildconfig";

I recommend writing a regex replace or a script to automate this process - it could be a bit tedious otherwise.

To start, set up a build environment - you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

The tests you would be fixing are in our Robocop test framework, so you'll need to set that up too: https://wiki.mozilla.org/Auto-tools/Projects/Robocop

When you make changes in the mobile/android/base/tests/ directory, you should only need to recompile Robocop, not the entire browser, for these changes.

While you're developing, I recommend running just a single test at a time because the full robocop test suite takes a long time to run (at least 30 minutes).

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks and happy coding! ^_^
Whiteboard: [lang=java][good first bug]
Hey Michael!
I would love to take a stab at this!
I should wait for bug 938845, right?
btw I have setup my fennec, I will setup Robocop now! :)
Hey, Prabhjyot! Welcome aboard - glad you can join us! :D

(In reply to Prabhjyot Sodhi [:psd] from comment #1)
> I should wait for bug 938845, right?

Yep! I just gave it an r+ so we're just waiting for the tests to go green - it should hopefully be landed by the end of the day!

If you'd like to do a bit of extra work to get started early, you can download the patch in that bug (right-click, save as on the attachment name), you can import it via `hg qimport <path-to-file>`. Then run `hg qpush` to apply that patch. You can run `hg qstatus` to see what patches you have in your queue and `hg qapplied` to see which ones are applied. Then you can make your changes on top of this patch - run `hg qnew <patch-name>` to create a new patch. Be sure not to save your changes to the patch from the other bug (i.e. don't run `hg qrefresh` before `hg qnew <patch-name>`).
Attached patch 1122331.patch (obsolete) — Splinter Review
Attachment #8553868 - Flags: review?(michael.l.comella)
Comment on attachment 8553868 [details] [diff] [review]
1122331.patch

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

I get some compile errors when trying to build this - make sure you're building the robocop code too (i.e. `./mach build build/mobile/robocop`)!
Attachment #8553868 - Flags: review?(michael.l.comella) → review-
(In reply to Michael Comella (:mcomella) from comment #4)
> I get some compile errors when trying to build this - make sure you're
> building the robocop code too (i.e. `./mach build build/mobile/robocop`)!

I have built it without errors,
Though when I run ./mach robocop, I get that some tests failed.
(In reply to Prabhjyot Sodhi [:psd] from comment #5)
> I have built it without errors,
> Though when I run ./mach robocop, I get that some tests failed.

How are you building robocop? `./mach build build/mobile/robocop` ?
Flags: needinfo?(prabhjyotsingh95)
Hey michael!
Yes, I built it as you mentioned in the previous comment.
Didn't get any errors. I tried it more than a couple of times, but no error !
Flags: needinfo?(prabhjyotsingh95)
Some thoughts:
  * Have you made any changes that you did not `hg qrefresh` into your patch? Run `hg diff` to check.
  * Is your patch correctly applied? Run `hg qapplied` to see a list of the patches currently applied.
  * Is the patch you uploaded is out of date? Try re-uploading your most recent patch.

I get the following errors when I build:

:$ mach build build/mobile/robocop
 0:00.18 /usr/bin/make -C /home/mcomella/dev/moz/obj-android -j8 -s backend.RecursiveMakeBackend
 0:00.40 /usr/bin/make -C build/mobile/robocop -j8 -s
 0:01.99 /home/mcomella/dev/moz/mobile/android/base/tests/StringHelper.java:308: error: cannot assign a value to final variable SCROLL_TITLE_BAR_LABEL
 0:01.99         SCROLL_TITLE_BAR_LABEL = res.getString(R.string.pref_scroll_title_bar2);
 0:01.99         ^
 0:01.99 /home/mcomella/dev/moz/mobile/android/base/tests/StringHelper.java:329: error: cannot assign a value to final variable LEARN_MORE_LABEL
 0:01.99         LEARN_MORE_LABEL = res.getString(R.string.pref_learn_more);
Flags: needinfo?(prabhjyotsingh95)
Assignee: nobody → prabhjyotsingh95
Status: NEW → ASSIGNED
Hey Michael!
I will get back to you after doing what you have suggested.
I am facing some issue with the os, and am trying to resolve that.
Thanks for you patience :)
Flags: needinfo?(prabhjyotsingh95)
Sure – let me know if you need any help.
Attached patch 1122331.patch (obsolete) — Splinter Review
Attachment #8557318 - Flags: review?(michael.l.comella)
Comment on attachment 8557318 [details] [diff] [review]
1122331.patch

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

::: mobile/android/base/tests/StringHelper.java
@@ +167,3 @@
>  
>      // Desktop default bookmarks folders
> +    public final String BOOKMARKS_UP_TO = "Up to %s";

The original line looks like:

BOOKMARKS_UP_TO = res.getString(R.string.home_move_up_to_filter);

Why did you substitute the String here?
Attachment #8557318 - Flags: review?(michael.l.comella) → feedback+
Hey Hey!
I cannot precisely recollect, 
I wrote a python script to fix this bug, 
but this variable was creating some error, so I copied this definition from a prev revision of StringHelper.java, I think.
I will make the changes right-a-away :)
Okay, Now I am pretty confused!
the changes that my patch hopes to make, I already see them in https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/StringHelper.java

Also, see line 213 in the above link.
(In reply to Prabhjyot Sodhi [:psd] from comment #14)
> the changes that my patch hopes to make, I already see them in
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/
> StringHelper.java

bug 938845 moves things around so that your changes are necessary (e.g. notice the Singleton code is missing).
Oh Right!!
My bad, I just forgot to apply the patch to bug 938845 (looks like I should sleep  more :P)
Will get back to you after making the changes :)
Attached patch 1122331.patchSplinter Review
Attachment #8553868 - Attachment is obsolete: true
Attachment #8557318 - Attachment is obsolete: true
Attachment #8559313 - Flags: review?(michael.l.comella)
Comment on attachment 8559313 [details] [diff] [review]
1122331.patch

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

Looks good - thanks, Prabhjyot!

I'll poke bug 938845 so we can get this landed.
Attachment #8559313 - Flags: review?(michael.l.comella) → review+
I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/6229b91659cf so that I could cleanly back out bug 938845 for robocop-4 failures.
Flags: needinfo?(prabhjyotsingh95)
Flags: needinfo?(michael.l.comella)
Hey!

I am not sure how I can help to move this forward. But it looks like bug 938845 is still blocked. So I'm clearing needinfo
Flags: needinfo?(prabhjyotsingh95)
(In reply to Prabhjyot Sodhi [:psd] from comment #22)
> But it looks like bug 938845 is still blocked.

That's correct - I'll let you know if I need anything after bug 938845 lands. Thanks, Prabhjyot!
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/973f507abcf0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.