938845 - Merge StringHelper and org.mozilla.gecko.* Strings

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

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

bug 916507 introduces some changes that allow us to import Fennec classes directly. We should either use the constant Strings directly, or set the values in StringHelper using those Strings.

There can also be some consolidation in Fennec strings, perhaps for another bug (e.g. TabsTray.ABOUT_HOME & BrowserApp.ABOUT_HOME).

Comment 1

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

(In reply to Michael Comella (:mcomella) from comment #0)
> or set the values in StringHelper using those Strings.

We should do this - it'd be really annoying for developers writing tests to find out where some strings are defined (especially if they're defined in JS).

To summarize, the plan is look at the definitions of Strings in StringHelper and see if they're also defined by Android resources in org.mozilla.gecko.R. If so, use the Android resource value. If not, then the duplication is okay (as we cannot retrieve our JS strings, for example) and you can leave it as is.

Note that our Java strings are preprocessed and not stored in .../resources/values/strings.xml, but rather mobile/android/base/locales/en-US/android_strings.dtd.

Comment 2

[Jalpreet Singh Nanda [:imjalpreet]]

Hi, I would like to take this bug. Can you explain me what is a StringHelper?

Comment 3

[Jalpreet Singh Nanda [:imjalpreet]]

Can you explain a bit on what exactly needs to be done. I mean I want to know what changes are to be done.

Comment 4

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

Sorry if my initial comments are unclear.

StringHelper [1] is a class that contains many of the Strings used in the app so that we can reference them in testing. For legacy reasons, these Strings *do not* reference the actual String resources used in the app, but rather duplicate the defined String. Your task is to de-duplicate these Strings.

Essentially, the task is to search where the Strings are defined in the main app's resources, and replace the duplicated Strings with `Resources.getString(<StringResID>)`. Note that the Strings in StringHelper are currently defined statically but Resources are only available with an application Context so you'll need to get around this - I recommend using the Singleton pattern (which requires each reference of StringHelper to be updated) to ensure the Strings in StringHelper are declared final.

Also, we extend Android's localization system so the standard Android String resource file, mobile/android/base/strings.xml.in [2] is pre-processed and points to files in mobile/android/base/locales/en-US/* [3].

To summarize:
  1) Update StringHelper to be a singleton (which requires updating the uses of StringHelper - I recommend using sed, though an IDE might just be able to do it). You'll have to initialize the singleton with a Context in both BaseTest.setUp and UITest.setUp.
  2) Initialize StringHelper with the main app's Strings and resources. I would recommend writing a script to see which Strings we use in StringHelper, and match these against the Strings in the files in [3].

If you're comfortable with hg, it may help to create a separate patch for each part.

Remember you're still in testing code so you'll need to rebuild the Robocop APK (and just the robocop apk) when you make changes.

This seems to be more complicated than I originally thought when I filed the bug (because of the staticness of StringHelper) - if you want to work on something else, please let me know! Otherwise, if you're still game, let me know if you have any questions! :)

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/StringHelper.java?rev=e419afb0dba7#4

[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/strings.xml.in
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/

Comment 5

[Jalpreet Singh Nanda [:imjalpreet]]

I am not that sure about solving this bug right now, so I would like to work on something else for now, but I will certainly come back to this bug in the near future. Till then I will work on some other bug and simultaneously try to understand this bug.

Comment 6

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

Is there anything that I can clarify about the bug?

Comment 7

[Jalpreet Singh Nanda [:imjalpreet]]

Can you please explain the concept of *Singleton* that you talked about. Thanks for the help. :)

Comment 8

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845

I have attached the diff of the changes i have done so far, can you give me a feedback on what all work is still left. 
Thanks :)

Comment 9

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

Comment on attachment 8536217 [details] [diff] [review]
Bug-938845

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

How did you generate this patch? Did you do this by hand or did you write a script? If the latter, it'd be helpful for reviewing to see the script.

Besides the lack of singleton, this is the right idea - nice! :)

::: mobile/android/base/tests/StringHelper.java
@@ +10,4 @@
>  public class StringHelper {
>      private StringHelper() {}
>  
> +    public static final String OK = Resources.getSystem().getString(R.string.button_ok);

Have you tested this? Does this work?

I wouldn't think so, as per the docs for Resources.getSystem():

Return a global shared Resources object that provides access to only system resources (no application resources), and is not configured for the current screen (can not use dimension units, does not change based on orientation, etc).

---

This is why I recommended the use of a singleton - I'll write about that in a moment.

Comment 10

[Jalpreet Singh Nanda [:imjalpreet]]

(In reply to Michael Comella (:mcomella) from comment #9)

> Have you tested this? Does this work?

Yes, I have checked it returns the required string.

Comment 11

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

Actually, I just realized a Singleton is not necessary, but not using it makes the change a bit more tedious - we can construct StringHelper with a Resources instance in setUp of BaseTest and UITest and have each test access that instance. It gets tedious because we have to pass around the StringHelper instance, so let's leave it as a Singleton for now - we can make it a member variable later. So...

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #7)
> Can you please explain the concept of *Singleton* that you talked about.

From Wikipedia:

In software engineering, the singleton pattern is a design pattern that restricts the instantiation of a class to one object.

---

In practice, this let's you access an instance of a class statically. In our case, we would like to access an instance of StringHelper, initialized with a Resources instance, statically...

Before:
  StringHelper.OK
After:
  final StringHelper sh = StringHelper.getInstance();
  sh.OK
  // Or more briefly...
  sh.getInstance().OK

The code to do something like this can be found at [1]. Our Tabs class [2] is an example of this. Some more in-depth reading can be found at [3] (disclaimer: I've only skimmed it myself).

Let me know if you have any questions.

[1]: https://en.wikipedia.org/wiki/Singleton_pattern#Eager_initialization
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java
[3]: http://www.javaworld.com/article/2073352/core-java/simply-singleton.html

Comment 12

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

(In reply to Michael Comella (:mcomella) from comment #11)
>   sh.getInstance().OK

I meant `StringHelper.getInstance().OK`.

Comment 13

[Jalpreet Singh Nanda [:imjalpreet]]

So, I should add the required code in UITest and BaseTest files?

Comment 14

[Jalpreet Singh Nanda [:imjalpreet]]

I have added the Singleton. What are changes that are still left?

Comment 15

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #14)
> I have added the Singleton. What are changes that are still left?

The Singleton should be initialized in BaseTest.setUp and UITest.setUp with a Resource instance from there - look into using getActivity to get the resources (though maybe there's a better way).

After that, each call to StringHelper needs to be replaced with a get to the instance, e.g. StringHelper.getInstance(). That seems a little long so maybe use StringHelper.get() instead.

Does that make sense?

Comment 16

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #10)
> (In reply to Michael Comella (:mcomella) from comment #9)
> 
> > Have you tested this? Does this work?
> 
> Yes, I have checked it returns the required string.

Very interesting! If you've already added the Singleton, I guess we don't need to explore why this works though.

Comment 17

[Jalpreet Singh Nanda [:imjalpreet]]

(In reply to Michael Comella (:mcomella) from comment #15)

> The Singleton should be initialized in BaseTest.setUp and UITest.setUp with
> a Resource instance from there - look into using getActivity to get the
> resources (though maybe there's a better way).

I didn't get this part. Can you please explain again.

Comment 18

[Jalpreet Singh Nanda [:imjalpreet]]

(In reply to Michael Comella (:mcomella) from comment #16)
> (In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #10)
> > (In reply to Michael Comella (:mcomella) from comment #9)
> > 
> > > Have you tested this? Does this work?
> > 
> > Yes, I have checked it returns the required string.
> 
> Very interesting! If you've already added the Singleton, I guess we don't
> need to explore why this works though.

Sorry, I think it will give runtime error if I use Resources.getSystem().getString(). When you suggested I read about it and checked and it gave a runtime error.
So, now I am trying to do it by some other method. Sorry about my earlier reply.

Comment 19

[Jalpreet Singh Nanda [:imjalpreet]]

Now, I have replaced the getSystem().getString() by GeckoApplication.get().getContext().getString(). 
Is this fine?
I am still not sure about my previous doubt that I asked you 2 comments before.

Comment 20

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #17)
> (In reply to Michael Comella (:mcomella) from comment #15)
> 
> > The Singleton should be initialized in BaseTest.setUp and UITest.setUp with
> > a Resource instance from there - look into using getActivity to get the
> > resources (though maybe there's a better way).
> 
> I didn't get this part. Can you please explain again.

Sorry that I'm being unclear.

In JUnit testing, there is a setUp and tearDown method which is run at the beginning and end of every test respectively to initialize and clean up the state of the application for the test. Here, we have access to values used in testing. In particular, we have access to the Activity which can give us an Activity's Context, which has access to our Application's Resources instance.

BaseTest and UITest are our two testing implementations so, in their setUp methods, you can get the Resources (see above) and pass this value into an initialize method on the StringHelper Singleton. To be explicit:

class StringHelper{
  private StringHelper instance = null;
  ...

  public void initialize(final Resources res) {
    // Set string values
    ...
  }

  public StringHelper get() { ... } // Be sure to handle the null case! Consider Exceptions.
}

class BaseTest { // or UITest
  ...
  @Override
  public void setUp(...) {
    // Get the Resources
    ...
    final Resources res = ...
    StringHelper.initialize(res);
    ...
  }  
}

Does this make sense? You can then use the Singleton instance with:

StringHelper.get().varName;

Comment 21

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

Nick, can you double-check my analysis below?

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #19)
> Now, I have replaced the getSystem().getString() by
> GeckoApplication.get().getContext().getString(). 
> Is this fine?

Good code sleuthing! While I'm pretty sure this will work, I don't think it's the most correct approach – each Activity could, in theory, have it's own Resources instance with an entirely different set of Strings and if we're grabbing the Resources object from the Application, it may not apply to the Activity. Therefore, if we get the Context from the Activity we're testing, we're guaranteed to have the correct instance.

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #21)
> Nick, can you double-check my analysis below?
> 
> (In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #19)
> > Now, I have replaced the getSystem().getString() by
> > GeckoApplication.get().getContext().getString(). 
> > Is this fine?
> 
> Good code sleuthing! While I'm pretty sure this will work, I don't think
> it's the most correct approach – each Activity could, in theory, have it's
> own Resources instance with an entirely different set of Strings and if
> we're grabbing the Resources object from the Application, it may not apply
> to the Activity. Therefore, if we get the Context from the Activity we're
> testing, we're guaranteed to have the correct instance.

I'm pretty sure the enclosing Application and each Activity in the same process see the same underlying set of resources, but I see no reason to assume that's the case, since it's easy to use the current Activity context to fetch the resources.

As an aside, +1 to using the actual R.string.* references! \o/

Comment 23

[Jalpreet Singh Nanda [:imjalpreet]]

(In reply to Michael Comella (:mcomella) from comment #21)
> Nick, can you double-check my analysis below?
> 
> (In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #19)
> > Now, I have replaced the getSystem().getString() by
> > GeckoApplication.get().getContext().getString(). 
> > Is this fine?
> 
> Good code sleuthing! While I'm pretty sure this will work, I don't think
> it's the most correct approach – each Activity could, in theory, have it's
> own Resources instance with an entirely different set of Strings and if
> we're grabbing the Resources object from the Application, it may not apply
> to the Activity. Therefore, if we get the Context from the Activity we're
> testing, we're guaranteed to have the correct instance.

So, should I get the context from UITest or BaseTest?

Comment 24

[Nick Alexander :nalexander [he/him]]

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #23)
> (In reply to Michael Comella (:mcomella) from comment #21)
> > Nick, can you double-check my analysis below?
> > 
> > (In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #19)
> > > Now, I have replaced the getSystem().getString() by
> > > GeckoApplication.get().getContext().getString(). 
> > > Is this fine?
> > 
> > Good code sleuthing! While I'm pretty sure this will work, I don't think
> > it's the most correct approach – each Activity could, in theory, have it's
> > own Resources instance with an entirely different set of Strings and if
> > we're grabbing the Resources object from the Application, it may not apply
> > to the Activity. Therefore, if we get the Context from the Activity we're
> > testing, we're guaranteed to have the correct instance.
> 
> So, should I get the context from UITest or BaseTest?

You should be able to use getActivity() regardless of whether you're in UITest or BaseTest.

Comment 25

[Jalpreet Singh Nanda [:imjalpreet]]

I have a doubt. How should I get context in StringHelper as it is static?

Comment 26

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #25)
> I have a doubt. How should I get context in StringHelper as it is static?

That's one purpose of a Singleton - a single instance is constructed for a class that needs to be accessed statically. I guess I was a little unclear with my first example - perhaps this will help:

class StringHelper{
  private StringHelper instance = null;
  ...

  public void initialize(final Resources res) {
    if (instance != null) {
      return;
    }

    instance = new StringHelper(res);

    // Set string values
    ...
  }

  public StringHelper get() {
    // Handle null case w/ Exceptions
    ...
    return instance;
  }
}

---

If this does not help, I'd recommend doing a little more reading on how Singletons work and let me know if you still have questions!

Comment 27

[Jalpreet Singh Nanda [:imjalpreet]]

(In reply to Michael Comella (:mcomella) from comment #26)

>     instance = new StringHelper(res);

How are we sending an argument here?
Thanks for the help.

Comment 28

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #27)
> (In reply to Michael Comella (:mcomella) from comment #26)
> 
> >     instance = new StringHelper(res);
> 
> How are we sending an argument here?
> Thanks for the help.

What do you mean by sending an argument?

Comment 29

[Jalpreet Singh Nanda [:imjalpreet]]

Sorry, I meant how are we implementing StringHelper(res)?

Comment 30

[Jalpreet Singh Nanda [:imjalpreet]]

I think I got that. But when I am initializing the strings to their their string values, I cannot use *public static* final String string_name = value inside the initialize method. Can you explain?

Comment 31

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #30)
> I think I got that. But when I am initializing the strings to their their
> string values, I cannot use *public static* final String string_name = value
> inside the initialize method. Can you explain?

Right - the Strings do not need to be static (just `public final String ...`). Since we have an instance, the singleton, that we can access in a static context, we can also access the member Strings in a static context.

i.e. StringHelper.get().STRING_NAME // static context

Note that you'll have to update all the access points of StringHelper.STRING_NAME to StringHelper.get().STRING_NAME. A little googling brought up [1] (see "Structural replace"), if you're using intellij.

[1]: http://jetbrains.dzone.com/articles/top-20-refactoring-features

Comment 32

[Jalpreet Singh Nanda [:imjalpreet]]

I am still having a problem. I am not able to use public as well. It's giving an error as " Modifier 'public' not allowed here ". Any solutions? I am also trying to find a solution, if you can give any hint, it would be great.

Comment 33

[Nick Alexander :nalexander [he/him]]

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #32)
> I am still having a problem. I am not able to use public as well. It's
> giving an error as " Modifier 'public' not allowed here ". Any solutions? I
> am also trying to find a solution, if you can give any hint, it would be
> great.

Post your code to the ticket.  This is just a Java syntax error; somebody will be able to help.

Comment 34

[Jalpreet Singh Nanda [:imjalpreet]]

public class StringHelper {
    private static StringHelper INSTANCE = null;
    private Resources resources = null;

    private StringHelper (Resources res) {
        this.resources = res;
    }

    public void initialize (final Resources res) {

        if (INSTANCE != null) {
            return;
        }
        INSTANCE = new StringHelper(res);
*       public final String OK = StringHelper.getInstance().resources.getString(R.string.button_ok);
    }

    public static StringHelper getInstance(Resources res) {

        if (INSTANCE == null){
            INSTANCE = new StringHelper(res);
        }
        return INSTANCE;
    }
}

On the starred line, I am getting an error "Modifier 'public' not allowed here".
Please tell where I have made a mistake.

Comment 35

[Nick Alexander :nalexander [he/him]]

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #34)
> public class StringHelper {
>     private static StringHelper INSTANCE = null;

nit: we use sInstance for static, or just instance.  The IDE will tell you what the variable scope is.

>     private Resources resources = null;
> 
>     private StringHelper (Resources res) {
>         this.resources = res;
>     }
> 
>     public void initialize (final Resources res) {
> 
>         if (INSTANCE != null) {
>             return;
>         }
>         INSTANCE = new StringHelper(res);
> *       public final String OK =
> StringHelper.getInstance().resources.getString(R.string.button_ok);

You're defining OK in a method, initialize.  It's scoped to the method, meaning it can't be accessed outside of that method.  It doesn't make sense to declare public/protected/private inside a method.

You'd need to lift the variable declaration outside of the method, like:

public String ok;
public initialize(Resources res) {
...
  ok = ...
}

And at this point, there's no sense having a separate initialize method; just use the constructor, like:

public final String ok;
public StringHelper(Resources res) {
  ...
  ok = ...
}

>     }
> 
>     public static StringHelper getInstance(Resources res) {
> 
>         if (INSTANCE == null){
>             INSTANCE = new StringHelper(res);
>         }
>         return INSTANCE;
>     }
> }
> 
> On the starred line, I am getting an error "Modifier 'public' not allowed
> here".
> Please tell where I have made a mistake.

Comment 36

[Jalpreet Singh Nanda [:imjalpreet]]

Thanks. But now I will not be able to make the string final as we cannot change the value of the string if it's final once it's assigned at declaration. Is there any solution to this?

Comment 37

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch

I have done the required changes. Please give a feedback if any changes are still left.

Comment 38

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #36)
> Thanks. But now I will not be able to make the string final as we cannot
> change the value of the string if it's final once it's assigned at
> declaration. Is there any solution to this?

Java allows you to declare a String and later define the value, provided it's done before it could possibly be used. The most common situations:

// #1
class FinalStringTest {
  public static final String stringy;
  public FinalStringTest() {
    stringy = "I don't like the texture.";
  }
}

// #2
final String dependentValue;
if (thisIsTrue) {
  dependentValue = "thisIsTrue!";
} else {
  dependentValue = "thisIsFalse!";
}

---

The situation in this bug is #1 - we have to define all of our Strings in the constructor.

Comment 39

[Jalpreet Singh Nanda [:imjalpreet]]

Yes, I tried that but I am getting an error that "Cannot assign a value to final variable 'stringname'".
Can there be some mistake in what I am doing?

Comment 40

[Jalpreet Singh Nanda [:imjalpreet]]

I just wanted to ask is it necessary to use the strings as final? The patch that I have attached has all the changes except that the strings are not final.

Comment 41

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #39)
> Yes, I tried that but I am getting an error that "Cannot assign a value to
> final variable 'stringname'".
> Can there be some mistake in what I am doing?

Ah, the Strings declared in the class should not be static - they're instance variables.

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #40)
> I just wanted to ask is it necessary to use the strings as final? The patch
> that I have attached has all the changes except that the strings are not
> final.

While not necessary, it is extremely preferable - these Strings are used in many places so if they accidentally get changed once, anywhere, we could have many tests failing.

Comment 42

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

(In reply to Michael Comella (:mcomella) from comment #41)
> Ah, the Strings declared in the class should not be static - they're
> instance variables.

Note that this forces you to update all the places StringHelper is used to get the Singleton, e.g. "StringHelper.get().STRING_NAME".

Comment 43

[Jalpreet Singh Nanda [:imjalpreet]]

(In reply to Michael Comella (:mcomella) from comment #41)

> Ah, the Strings declared in the class should not be static - they're
> instance variables.
> 

But after doing static also I am still getting the same error. 

(In reply to Michael Comella (:mcomella) from comment #42)
> (In reply to Michael Comella (:mcomella) from comment #41)
> > Ah, the Strings declared in the class should not be static - they're
> > instance variables.
> 
> Note that this forces you to update all the places StringHelper is used to
> get the Singleton, e.g. "StringHelper.get().STRING_NAME".

Yes, this I have already done.

Comment 44

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

Comment on attachment 8540226 [details] [diff] [review]
Bug-938845-Proposed_Patch

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

::: mobile/android/base/tests/UITest.java
@@ +50,5 @@
>      protected AppMenuComponent mAppMenu;
>      protected GeckoViewComponent mGeckoView;
>      protected ToolbarComponent mToolbar;
>  
> +    StringHelper sh;

Good idea - I overlooked how greatly this could simplify the changes you need to make to the tests.

nit: rename -> "mStringHelper"

In classes extending UITest or BaseTest, use: mStringHelper.STRING_NAME

Elsewhere, call upon the Singleton:
StringHelper.get().STRING_NAME

Comment 45

[Jalpreet Singh Nanda [:imjalpreet]]

What about "final" ? The error has still not been removed.

Comment 46

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #43)
> But after doing static also I am still getting the same error. 

By "doing static", do you mean removing the static keyword? e.g.:

private final String ABOUT_HOME;

If so, can you post the code so I might see what's going on?

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #45)
> What about "final" ? The error has still not been removed.

It doesn't make sense to have static final variables defined later because they can be accessed from anywhere. This is unlike instance variables, where there is a single entry point to access (the instance) so we can wait until the instance is constructed to define them.

I'm thinking the issue here is that static wasn't removed properly but feel free to post the code so I can take a look.

Comment 47

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-StringHelper.diff

I have attached only the diff of StringHelper.java file.

Comment 48

[Jalpreet Singh Nanda [:imjalpreet]]

(In reply to Michael Comella (:mcomella) from comment #44)

> In classes extending UITest or BaseTest, use: mStringHelper.STRING_NAME
> 
> Elsewhere, call upon the Singleton:
> StringHelper.get().STRING_NAME

I just wanted to ask what should I use in a class C which extends another class B, when B extends UITest or BaseTest?

Comment 49

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #48)
> I just wanted to ask what should I use in a class C which extends another
> class B, when B extends UITest or BaseTest?

mStringHelper is still accessible, so I'd access that - it's cleaner than using the Singleton.

Comment 50

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

Comment on attachment 8540373 [details] [diff] [review]
Bug-938845-StringHelper.diff

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

Sorry that this is turning out to be harder than I anticipated! Thanks for hanging in there!

Try removing the return statement and seeing if you can add the final keyword again (see my comment below for more). If not, let me know and I'll apply the patch once I get to my dev machine.

::: mobile/android/base/tests/StringHelper.java
@@ +218,5 @@
> +    public String POPUP_MESSAGE;
> +    public String POPUP_ALLOW;
> +    public String POPUP_DENY;
> +
> +    public StringHelper (final Resources res) {

Make this private - we don't want anyone outside the class to instantiate a StringHelper.

@@ +221,5 @@
> +
> +    public StringHelper (final Resources res) {
> +
> +        if (INSTANCE != null) {
> +            return;

This return shouldn't be necessary if the assignment isn't inside this method.

Note that the constructor will still return an instance, even if you return early, though the values may not be initialized.

This may actually be why final cannot be used - if you return here, the member values may never be initialized and all final values must be explicitly initialized.

@@ +223,5 @@
> +
> +        if (INSTANCE != null) {
> +            return;
> +        }
> +        INSTANCE = new StringHelper(res);

As per my previous comment, `INSTANCE = new ...` will set instance to a partially initialized StringHelper. Rather, I think you want:

INSTANCE = this;

Though, the Singleton should not assigned inside the constructor. In general, it can be useful to construct multiple instances of the singleton (e.g. a reset method) as long as only one is being accessed statically.

It looks like the getInstance(...) method is correct so you can just remove this assignment.

@@ +468,5 @@
> +        POPUP_ALLOW = res.getString(R.string.pref_panels_show);
> +        POPUP_DENY = "Don't show";
> +    }
> +
> +    public static StringHelper getInstance(Resources res) {

Passing in the resources here can make invocations annoying long. I was picturing:

class BaseTest/UITest {
  ...
  @Override
  public void setUp() {
    ...
    StringHelper.initialize(resources); // Constructs & assigns the instance
    mStringHelper = StringHelper.getInstance();
  }
}

class StringHelper {
  ...
  private StringHelper(Resources res) {
    // Initialize values
    ...
  }

  public void initialize(Resources res) {
    if (instance != null) {
      throw new IllegalStateException(...);
    }
    instance = new StringHelper(res);
  }

  public void get() { // Note: getInstance is long to use everywhere
    if (instance == null) {
      throw new IllegalStateException(...);
    }
    return instance;
  }
}

Comment 51

[Jalpreet Singh Nanda [:imjalpreet]]

I will get back to you in some time after trying out what you have suggested.

Comment 52

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch

I think I have done all the changes as you told. Please tell if anything is still left.

Comment 53

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

Comment on attachment 8540814 [details] [diff] [review]
Bug-938845-Proposed_Patch

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

::: mobile/android/base/tests/BaseTest.java
@@ +139,5 @@
>          throwIfHttpGetFails();
>          throwIfScreenNotOn();
> +
> +        final Resources res = getActivity().getResources();
> +        StringHelper.get().initialize(res);

StringHelper.initialize() - initialize should be static.

Same in UITest.

::: mobile/android/base/tests/StringHelper.java
@@ +219,5 @@
> +    public final String POPUP_DENY;
> +
> +    private StringHelper (final Resources res) {
> +
> +        INSTANCE = this;

Sorry if I was unclear - you shouldn't need to do this because you set the value in initialize.

@@ +464,5 @@
> +        POPUP_ALLOW = res.getString(R.string.pref_panels_show);
> +        POPUP_DENY = "Don't show";
> +    }
> +
> +    public void initialize (Resources res) {

nit: no space after method names - in get() too.

@@ +466,5 @@
> +    }
> +
> +    public void initialize (Resources res) {
> +        if (INSTANCE != null) {
> +            throw new IllegalStateException();

nit: Add a message w/ the Exception explaining the IllegalState, e.g. this.class.getSimpleName() + " already initialized"

@@ +472,5 @@
> +        INSTANCE = new StringHelper(res);
> +    }
> +
> +    public static StringHelper get () {
> +        if (INSTANCE != null) {

I imagine this will throw a lot - did you mean INSTANCE == null?

Note that this is a separate build process than the application - you should be building the robocop APK and running at least one test to test these changes. See [1] for more information and please let me know if you have questions.

[1]: https://wiki.mozilla.org/Auto-tools/Projects/Robocop#Running_tests

@@ +473,5 @@
> +    }
> +
> +    public static StringHelper get () {
> +        if (INSTANCE != null) {
> +            throw new IllegalStateException();

nit: Again, provide a message explaining the state.

Comment 54

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch

I have done the required changes.

Comment 55

[Jalpreet Singh Nanda [:imjalpreet]]

Is there another bug which you would like to give? I am waiting for the review. :)

Comment 56

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

Comment on attachment 8540889 [details] [diff] [review]
Bug-938845-Proposed_Patch

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

As a standalone, looking great - you just need to fix the nits!

However, the patch doesn't apply on my local copy (pulled this morning) - make sure you pop your patches, pull the latest changes (`hg qpop -a && hg pull <bookmark> && hg up`, and rebase your patch on top of this (push, merge the reject files into the .java files, and qrefresh). There are a lot of files that do not apply - I recommend using the wiggle utility (you can google for it) to merge the files, which manages merges better than mercurial's built-in handler.

Once this patch is rebased, we can run it through try and get this landed! :D

::: mobile/android/base/tests/BaseTest.java
@@ +84,5 @@
>      protected int mScreenMidWidth;
>      protected int mScreenMidHeight;
>      private final HashSet<Integer> mKnownTabIDs = new HashSet<Integer>();
>  
> +    StringHelper mStringHelper;

nit: protected

::: mobile/android/base/tests/StringHelper.java
@@ +8,2 @@
>  public class StringHelper {
> +    private static StringHelper INSTANCE = null;

Sorry if I wrote this in all caps before, but it should really be lower-case. All caps is typically reserved for constants.

This file is a bit silly with how constants are handled so let me know if you have any questions about this.

@@ +253,5 @@
> +        ABOUT_HOME_TITLE = "";
> +
> +        // Context Menu item strings
> +        CONTEXT_MENU_BOOKMARK_LINK = "Bookmark Link";
> +        CONTEXT_MENU_OPEN_LINK_IN_NEW_TAB = "Open Link in New Tab";

I noticed that a few of these resources are not included - since this is such a large change, we can clean up the rest of these in a followup.

@@ +464,5 @@
> +    }
> +
> +    public static void initialize(Resources res) {
> +        if (INSTANCE != null) {
> +            throw new IllegalStateException(StringHelper.class.getSimpleName() + "Already Initialized");

nit: Space before Already

nit: Already -> lowercase

@@ +471,5 @@
> +    }
> +
> +    public static StringHelper get() {
> +        if (INSTANCE == null) {
> +            throw new IllegalStateException(StringHelper.class.getSimpleName() + "INSTANCE is null/not initialized");

nit: space before INSTANCE

nit: INSTANCE -> lowercase

nit: You don't need to specify the null state because someone who sees the log will match the code to the log. So...

StringHleper.class.getSimpleName() + " instance is not yet initialized", or similar. Might be helpful to tell the user to use "StringHelper.initialize(Resources)" first.

::: mobile/android/base/tests/UITest.java
@@ +50,5 @@
>      protected AppMenuComponent mAppMenu;
>      protected GeckoViewComponent mGeckoView;
>      protected ToolbarComponent mToolbar;
>  
> +    StringHelper mStringHelper;

nit: protected

::: mobile/android/base/tests/components/ToolbarComponent.java
@@ +17,5 @@
>  import org.mozilla.gecko.tests.helpers.DeviceHelper;
>  import org.mozilla.gecko.tests.helpers.NavigationHelper;
>  import org.mozilla.gecko.tests.helpers.WaitHelper;
>  
> +import android.content.res.Resources;

nit: You added some unused imports to this file - do you mind removing them?

::: mobile/android/base/tests/testAddSearchEngine.java
@@ +25,5 @@
>      private final int MAX_WAIT_TEST_MS = 5000;
>      private final String SEARCH_TEXT = "Firefox for Android";
>      private final String ADD_SEARCHENGINE_OPTION_TEXT = "Add as Search Engine";
>  
> +    private final Resources res = getActivity().getResources();

This is unused - remove it & the import

Also, it is bad practice to store resources as a member variable - it has the potential to change throughout the application lifecycle depending on which activity is accessing the class instance.

::: mobile/android/base/tests/testBookmark.java
@@ +6,5 @@
>  public class testBookmark extends AboutHomeTest {
>      private static String BOOKMARK_URL;
>      private static final int WAIT_FOR_BOOKMARKED_TIMEOUT = 10000;
>  
> +    private final Resources res = getActivity().getResources();

Unused - remove + import.

@@ +25,5 @@
>      }
>  
>      public void runAboutHomeTest() {
>          blockForGeckoReady();
> +        for (String url:mStringHelper.DEFAULT_BOOKMARKS_URLS) {

nit: spaces between the colon and values: "url : mStringHelper..."

::: mobile/android/base/tests/testLinkContextMenu.java
@@ +2,5 @@
>  
>  
> +import android.content.res.Resources;
> +
> +import org.mozilla.gecko.GeckoApplication;

Unused - remoe these two new imports.

::: mobile/android/base/tests/testMailToContextMenu.java
@@ +1,4 @@
>  package org.mozilla.gecko.tests;
>  
>  
> +import org.mozilla.gecko.GeckoApplication;

Unused import.

::: mobile/android/base/tests/testPictureLinkContextMenu.java
@@ +2,5 @@
>  
>  
> +import android.content.res.Resources;
> +
> +import org.mozilla.gecko.GeckoApplication;

Unused imports.

Comment 57

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #55)
> Is there another bug which you would like to give? I am waiting for the
> review. :)

Perhaps you'd be interested in bug 1116351? If not, let me know and I'll see if I can find something else. Thanks for all of your help! :)

Comment 58

[Jalpreet Singh Nanda [:imjalpreet]]

Ok, Thanks. I will get back to you by tomorrow. Secondly, I am happy to help. :)

Comment 59

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch

I have done the required changes.

Comment 60

[Richard Newman [:rnewman]]

Comment on attachment 8543157 [details] [diff] [review]
Bug-938845-Proposed_Patch

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

A brief comment about the purpose of this bug:

There's actually some value in how this currently works. That is, a build problem that results in a mangled R.string -- and thus a mangled UI -- will cause tests to fail, because we hard-code the expected values in the test.

The only reason I don't want to WONTFIX this bug is because it'll be necessary for localized testing, which I hope someday to do.

All of the mechanical changes look fine. I'll concentrate on StringHelper.

Looks fine with these changes.

::: mobile/android/base/tests/BaseTest.java
@@ +140,5 @@
>          throwIfScreenNotOn();
> +
> +        final Resources res = getActivity().getResources();
> +        StringHelper.initialize(res);
> +        mStringHelper = StringHelper.get();

I question the value of sharing the StringHelper static instance in StringHelper -- if you're actually successfully sharing it, this `initialize` call will throw!

So skip the whole singleton bit, and just do:

  mStringHelper = new StringHelper(getActivity().getResources());

::: mobile/android/base/tests/StringHelper.java
@@ +1,1 @@
>  package org.mozilla.gecko.tests;

License block.

@@ +8,2 @@
>  public class StringHelper {
> +    private static StringHelper instance = null;

Doesn't need = null. But I think we should just get rid of this.

@@ +237,5 @@
> +        // About pages
> +        ABOUT_BLANK_URL = "about:blank";
> +        ABOUT_FIREFOX_URL = res.getString(R.string.bookmarkdefaults_url_aboutfirefox);
> +        ABOUT_RIGHTS_URL = "about:rights";
> +        ABOUT_BUILDCONFIG_URL = "about:buildconfig";

All of these static values -- anything that's not computed from R -- can be lifted back up to the field definition.

@@ +462,5 @@
> +    }
> +
> +    public static void initialize(Resources res) {
> +        if (instance != null) {
> +            throw new IllegalStateException(StringHelper.class.getSimpleName() + " already Initialized");

I don't think this is useful. Every caller just calls `initialize` then `get`.

You could just merge `initialize` and `get`:

  public static StringHelper getInstance(Resources res) {
      if (instance == null) {
          instance = new StringHelper(res);
      }
      return instance;
  }

but I think we should just kill both methods and just use the constructor. Each test will have a `mStringHelper` instance.

Comment 61

[Jalpreet Singh Nanda [:imjalpreet]]

I was told to use the initialize method and the get method as separate by mcomella, so should I wait for his feedback also or should I make the changes?

Comment 62

[Richard Newman [:rnewman]]

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #61)
> I was told to use the initialize method and the get method as separate by
> mcomella, so should I wait for his feedback also or should I make the
> changes?

I think he wanted you to initialize once in the test init, and thereafter use StringHelper.get() everywhere you currently have StringHelper.

Instead you've added an mStringHelper member to every test, and are using that instead, so there's no need for the StringHelper singleton itself.

Comment 63

[Jalpreet Singh Nanda [:imjalpreet]]

Referring to the Comments 11 and 44, he also told me to use the mStringHelper as far as I understand. Can you please check? If still it is not required I will make the required changes.

Comment 64

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

Sorry for the delay - I was on PTO. I'll get to your review in a bit.

(In reply to Richard Newman [:rnewman] from comment #60)
> A brief comment about the purpose of this bug:
> 
> There's actually some value in how this currently works. That is, a build
> problem that results in a mangled R.string -- and thus a mangled UI -- will
> cause tests to fail, because we hard-code the expected values in the test.

Good call - if there's a build issue mangling R, however, it'd likely affect all Strings, right Richard? A test that tests a few hard-coded Strings should cover at least the most common case. If you agree, can you file a follow-up?

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #61)
> I was told to use the initialize method and the get method as separate by
> mcomella, so should I wait for his feedback also or should I make the
> changes?

Sorry for the confusion! I prefer to have these methods used separately - asserting that this is called once here prevents us from doing unnecessary work, which I want to be careful about given how rickety our test framework has been in the past. I could go either way on this though.

(In reply to Richard Newman [:rnewman] from comment #60)
> I question the value of sharing the StringHelper static instance in
> StringHelper

True - this shared reference can be incorrect if the class' static reference (i.e. StringHelper.get) is ever updated. However, my grand vision is to make the StringHelper instance a member variable - I just wanted to do it over multiple bugs because it's a bit of work to get it working properly in both BaseTest and UITest. e.g. for UITest, bug 1121651.

Comment 65

[Jalpreet Singh Nanda [:imjalpreet]]

Hi, Tell me if I have to make any changes and waiting for your review.

Comment 66

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

Comment on attachment 8543157 [details] [diff] [review]
Bug-938845-Proposed_Patch

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

lgtm w/ changes! try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b42b232ed6ba

Looks like testTitleBar got some changes underneath your patch - do you mind rebasing the other files and fixing testTitleBar?

Comment 67

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

Follow-ups, just so we can get this landed:

(In reply to Richard Newman [:rnewman] from comment #60)
> > +    private static StringHelper instance = null;
> 
> Doesn't need = null. But I think we should just get rid of this.

bug 1122329.

> > +        ABOUT_RIGHTS_URL = "about:rights";
> > +        ABOUT_BUILDCONFIG_URL = "about:buildconfig";
> 
> All of these static values -- anything that's not computed from R -- can be
> lifted back up to the field definition.

bug 1122331.

Comment 68

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch.diff

I have done the required changes.

Comment 69

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

Comment on attachment 8550032 [details] [diff] [review]
Bug-938845-Proposed_Patch.diff

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

Sorry for the delay - lgtm w/ green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ab472417bc

Comment 70

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

Jalpreet, looks like we're having issues with the latest push - do you mind taking a look?

Let me know if you need help with accessing/reading the logs, or if you need a push in the right direction.

Comment 71

[Jalpreet Singh Nanda [:imjalpreet]]

Hey, can you help me how can I move forward, i.e, how can I remove these issues?

Comment 72

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #71)
> Hey, can you help me how can I move forward, i.e, how can I remove these
> issues?

Did you have a chance to dig into the logs? Clicking through to the full log [1] (after clicking on a test suite, button second from the left in the status bar at the bottom on the original results page), I searched for the text "Exception" and managed to find the following:

12:52:53     INFO -  01-20 12:16:10.923 E/AndroidRuntime( 1170): Caused by: java.lang.IllegalStateException: StringHelper instance is not yet initialized. Use StringHelper.initialize(Resources) first.
12:52:53     INFO -  01-20 12:16:10.923 E/AndroidRuntime( 1170): 	at org.mozilla.gecko.tests.StringHelper.get(StringHelper.java:478)
12:52:53     INFO -  01-20 12:16:10.923 E/AndroidRuntime( 1170): 	at org.mozilla.gecko.tests.testLinkContextMenu.<clinit>(testLinkContextMenu.java:14)
12:52:53     INFO -  01-20 12:16:10.923 E/AndroidRuntime( 1170): 	... 23 more

[1]: https://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-d2ab472417bc/try-android-api-9/try_ubuntu64_vm_mobile_test-robocop-2-bm68-tests1-linux64-build79.txt.gz

Comment 73

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

Jalpreet, will you have a chance to work on this soon? Work on bug 1122331 is progressing and I don't want to block on this.

Comment 74

[Jalpreet Singh Nanda [:imjalpreet]]

Yeah I will try to work on this asap.

Comment 75

[Jalpreet Singh Nanda [:imjalpreet]]

Sorry for the delay, I will get back to you by tomorrow with a patch.

Comment 76

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

Jalpreet, bug 1122331, a dependent bug, is completed so I'd like to get this landed. Will you have a chance to get a patch out by this weekend?

If not, I'll finish up the remaining bits myself on Monday (NI self).

Comment 77

[Jalpreet Singh Nanda [:imjalpreet]]

Yeah, I will submit the patch by monday.

Comment 78

[Jalpreet Singh Nanda [:imjalpreet]]

Hey, Sorry for the delay, I will try to finish this as soon as possible. Earlier I was able to open the above link you gave me. But now I am not able to open that link. Can you again give me the link?

Comment 79

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #78)
> Hey, Sorry for the delay, I will try to finish this as soon as possible.
> Earlier I was able to open the above link you gave me. But now I am not able
> to open that link. Can you again give me the link?

The test results get purged after a long enough period. However, there should be enough information in comment 72 for you to complete this bug. If you disagree, let me know if you'd like for me to re-run the try build.

Comment 80

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch

I have done some changes and tried to remove the error. Can you try run it?

Comment 81

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

Comment on attachment 8561832 [details] [diff] [review]
Bug-938845-Proposed_Patch

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

::: mobile/android/base/tests/testLinkContextMenu.java
@@ +16,5 @@
>      private static String BLANK_PAGE_URL;
>      private static final String LINK_PAGE_TITLE = "Big Link";
> +    static {
> +        final Resources res = GeckoApplication.get().getResources();
> +        StringHelper.initialize(res);

Rather than making an exception for StringHelper initialization on tests that need to access StringHelper statically, just make the variables member variables instead.

Comment 82

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch.diff

I have done the required changes.

Comment 83

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

Comment on attachment 8562593 [details] [diff] [review]
Bug-938845-Proposed_Patch.diff

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

::: mobile/android/base/tests/BaseTest.java
@@ +113,5 @@
>          throwIfScreenNotOn();
> +
> +        final Resources res = getActivity().getResources();
> +        StringHelper.initialize(res);
> +        mStringHelper = StringHelper.get();

You can move this initialization to BaseRobocopTest (and remove it from UITest too).

::: mobile/android/base/tests/testLinkContextMenu.java
@@ +16,4 @@
>  
>      public void testLinkContextMenu() {
> +        final Resources res = getActivity().getResources();
> +        StringHelper.initialize(res);

You are still are initializing StringHelper at the start of some test.

Sorry, perhaps I can be more clear - you should only have to initialize StringHelper once. My advice from comment 81 might help you with this.

Comment 84

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch

I have done the required changes. But I am not able to change that in the class ToolbarComponent.java as it is not a child class of BaseRobocopTest. Due to this I had to again initialize resources in this class. Is there an alternative to this?

Comment 85

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

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #84)
> I have done the required changes. But I am not able to change that in the
> class ToolbarComponent.java as it is not a child class of BaseRobocopTest.
> Due to this I had to again initialize resources in this class. Is there an
> alternative to this?

ToolbarComponent gets initialized in in UITest.initComponents which is called after super.setUp() (i.e. BaseRobocopTest.setUp), meaning you should just be able to call the singleton via StringHelper.get() and it will already be initialized.

Comment 86

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

Comment on attachment 8563334 [details] [diff] [review]
Bug-938845-Proposed_Patch

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

::: mobile/android/base/tests/components/ToolbarComponent.java
@@ +54,5 @@
>          final String expected;
>          final String absoluteURL = NavigationHelper.adjustUrl(url);
> +
> +        final Resources res = GeckoApplication.get().getResources();
> +        StringHelper.initialize(res);

As per my previous comment, you shouldn't need this. In fact, this should throw because you're initializing StringHelper twice - try running a test that calls assertTitle.

Comment 87

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch

Sorry, I didn't run the tests last time around but now I have done the required changes as told by you.

Comment 88

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10f33a301ab5

Comment 89

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

Comment on attachment 8563464 [details] [diff] [review]
Bug-938845-Proposed_Patch

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

lgtm. Talk about a massive patch - nice work, Jalpreet!

Let me know if you want any follow-up bugs to work on.

Comment 90

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

https://hg.mozilla.org/integration/fx-team/rev/67b3b61d39f3

Comment 91

[Jalpreet Singh Nanda [:imjalpreet]]

Thanks! Right now I am working on some more bugs, as soon as I fix them I will get back to you for more bugs. :)

Comment 92

[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/17e306ef06bf for robocop-4 failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=1994824&repo=fx-team

Comment 93

[Jalpreet Singh Nanda [:imjalpreet]]

Now. what can I do to remove the error?

Comment 94

[Jalpreet Singh Nanda [:imjalpreet]]

I have checked the logs and found out that a null pointer exception is coming in the class testSessionOOMRestore, I am trying to debug it but can you give a little hint on how I can debug this?

13:01:55 INFO - 02-13 13:01:50.414 I/TestRunner( 3059): failed: testSessionOOMRestore(org.mozilla.gecko.tests.testSessionOOMRestore)
13:01:55 INFO - 02-13 13:01:50.414 I/TestRunner( 3059): ----- begin exception -----
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059):
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): java.lang.NullPointerException
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at org.mozilla.gecko.tests.testSessionOOMRestore.setActivityIntent(testSessionOOMRestore.java:20)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at org.mozilla.gecko.tests.BaseRobocopTest.setUp(BaseRobocopTest.java:144)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at org.mozilla.gecko.tests.BaseTest.setUp(BaseTest.java:104)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at org.mozilla.gecko.tests.SessionTest.setUp(SessionTest.java:24)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at junit.framework.TestCase.runBare(TestCase.java:125)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at junit.framework.TestResult$1.protect(TestResult.java:106)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at junit.framework.TestResult.runProtected(TestResult.java:124)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at junit.framework.TestResult.run(TestResult.java:109)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at junit.framework.TestCase.run(TestCase.java:118)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)
13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): ----- end exception -----
13:01:55 INFO - 02-13 13:01:50.429 I/TestRunner( 3059): finished: testSessionOOMRestore(org.mozilla.gecko.tests.testSessionOOMRestore)

Comment 95

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

13:01:55 INFO - 02-13 13:01:50.421 I/TestRunner( 3059): at org.mozilla.gecko.tests.testSessionOOMRestore.setActivityIntent(testSessionOOMRestore.java:20)

Did you check when this method gets called? What do you think is throwing the NullPointerException?

Comment 96

[Jalpreet Singh Nanda [:imjalpreet]]

I am not able to find the cause of this error. :(
Can you please guide me how can I find resolve this?

Comment 97

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

setActivityIntent is called in BaseRobocopTest. The first line of setActivityIntent accesses StringHelper. StringHelper is also initialized in BaseRobocopTest: where is it initialized in relation to where setActivityIntent is called?

Comment 98

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch.diff

I have done the required changes, i.e., initialized StringHelper before calling the setActivityIntent method. Do tell if anything else is required.

Comment 99

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

I pushed the updated patch to try with the previous try run's syntax:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd7992850bb6

Comment 100

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-938845-Proposed_Patch

I am sorry for that last patch. There was some extra character in some line in the SessionTest class. I have removed that now and I have attached the updated patch.

Comment 101

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

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

Comment 102

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

Comment on attachment 8567810 [details] [diff] [review]
Bug-938845-Proposed_Patch

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

Updated patch looks good to me (caveat: I saw something that looked like a compile error but I think this is inter-diff`s fault - building locally appeared to work).

Comment 103

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

Seems the try build failed in comment 101 - did this work for you locally, Jalpreet? I wonder if it's an infrastructure error.

Comment 104

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

Still working on this, Jalpreet?

Comment 105

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

Jalpreet, I'm going to finish up and land this if you don't have time to - thanks for the work you've put in this far!

Comment 106

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

I made some revisions and pushed a green push to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86eda9e029c2

Comment 107

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/28b3258bc0dc
remote:   https://hg.mozilla.org/integration/fx-team/rev/8e8d8f82f9de

Comment 108

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

Note that I didn't get review for my revisions because it was essentially fixing compilation errors and a small test failure.

Comment 109

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/910d4e852706 for "testSettingsMenuItems | Exception caught - java.lang.NullPointerException", https://treeherder.mozilla.org/logviewer.html#?job_id=2353277&repo=fx-team

Comment 110

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

Error is [1]:

 settingsMap.get(PATH_DISPLAY).add(textReflowUi);

But I don't know how settingsMap could be null so I'm guessing something is getting inlined.

[1]: https://hg.mozilla.org/integration/fx-team/file/8e8d8f82f9de/mobile/android/base/tests/testSettingsMenuItems.java#l184

Comment 111

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

Oh, PATH_DISPLAY is null because I accidentally used variable shadowing - fixing.

Comment 112

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

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

Comment 113

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

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

Comment 114

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

https://hg.mozilla.org/integration/fx-team/rev/34167fac3d3a
https://hg.mozilla.org/integration/fx-team/rev/2a3800b9c1b4
https://hg.mozilla.org/integration/fx-team/rev/c1ab1186ce43

Comment 115

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

https://hg.mozilla.org/integration/fx-team/rev/9892f70ad28b

Comment 116

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

https://hg.mozilla.org/mozilla-central/rev/34167fac3d3a
https://hg.mozilla.org/mozilla-central/rev/2a3800b9c1b4
https://hg.mozilla.org/mozilla-central/rev/c1ab1186ce43
https://hg.mozilla.org/mozilla-central/rev/9892f70ad28b