1122331 - Move statically initialized String values from StringHelper constructor to declaration

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

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

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! ^_^

Comment 1

[Prabhjyot Sodhi [:psd]]

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! :)

Comment 2

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

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>`).

Comment 3

[Prabhjyot Sodhi [:psd]]

Attachment: 1122331.patch

Comment 4

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

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`)!

Comment 5

[Prabhjyot Sodhi [:psd]]

(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.

Comment 6

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

(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` ?

Comment 7

[Prabhjyot Sodhi [:psd]]

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 !

Comment 8

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

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);

Comment 9

[Prabhjyot Sodhi [:psd]]

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 :)

Comment 10

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

Sure – let me know if you need any help.

Comment 11

[Prabhjyot Sodhi [:psd]]

Attachment: 1122331.patch

Comment 12

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

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?

Comment 13

[Prabhjyot Sodhi [:psd]]

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 :)

Comment 14

[Prabhjyot Sodhi [:psd]]

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.

Comment 15

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

(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).

Comment 16

[Prabhjyot Sodhi [:psd]]

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 :)

Comment 17

[Prabhjyot Sodhi [:psd]]

Attachment: 1122331.patch

Comment 18

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

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.

Comment 19

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

Thanks, Prabhjyot!

https://hg.mozilla.org/integration/fx-team/rev/8a4280e98895

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This also appears to have caused https://treeherder.mozilla.org/logviewer.html#?job_id=1994746&repo=fx-team

Comment 22

[Prabhjyot Sodhi [:psd]]

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

Comment 23

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

(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!

Comment 24

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

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

Comment 25

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

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

Comment 26

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

https://hg.mozilla.org/integration/fx-team/rev/973f507abcf0

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/973f507abcf0