1110310 - Replace LightweightTheme member variable with getTheme() calls

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

org.mozilla.gecko.widget.Themed* (e.g. ThemedLinearLayout, ThemedEditText) are a series of Views that override the basic Android classes with LightweightTheme functionality. As such, these views keep a reference to the current theme. Until recently, this member was private and any overriding classes would have to duplicate the member. However, now a protected `getTheme()` method exists that overriding classes can call.

There are two places that these members still exist and can be removed [1]:
  * org.mozilla.gecko.toolbar.BrowserToolbar [2]
  * org.mozilla.gecko.toolbar.ShapedButton [3]

Remove these members, instead using getTheme() where the members are used. For readability/performance, storing the Theme in a function-local varibale is also acceptable.

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

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

[1]: https://mxr.mozilla.org/mozilla-central/search?string=p.*+LightweightTheme+.*%3B&regexp=on&find=\.java%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java?rev=d18d4ecda9c3#143
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ShapedButton.java?rev=06379dc1d3b2#26

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

Hi Michael I'm interested in picking up this bug to get use to firefox for android.

Does that sounds ok?

Comment 2

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

Sure, Francisco - you've been assigned. Let me know if you have any questions!

Comment 3

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: patch.diff

Hi,

had some problems with latest gradle plugin for android studio 1.0, but just updated locally the changes needed.

Here is the first version of the patch, ready for feedback

Comment 4

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

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #3)
> had some problems with latest gradle plugin for android studio 1.0, but just
> updated locally the changes needed.

We don't officially support Android Studio at the moment - we support Eclipse [1] and Intellij [2]. You can still use Android Studio (it helps that it's based off Intellij), but if you encounter any build issues, you're unfortunately on your own!

[1]: http://www.ncalexander.net/blog/2014/06/24/better-fennec-builds-with-an-eclipse-plugin/
[2]: http://www.ncalexander.net/blog/2014/10/23/building-fennec-with-gradle-and-intellij-first-steps/

Comment 5

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

Comment on attachment 8536412 [details] [diff] [review]
patch.diff

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

::: mobile/android/app/build.gradle
@@ +13,4 @@
>  
>      buildTypes {
>          release {
> +            minifyEnabled false

Part of maintaining your own development environment is making sure you don't commit any changes to the build system in your patches (unless they're useful to others).

You can use the `hg qcrefresh` command (which requires the crecord extension to be enabled [1]) to remove these changes from your patch, then `hg qnew` them into a separate patch that you can use to modify the environment to use Android Studio (you may need to reorder your patches by popping your patches and using `hg qpush --move`). It's not the most intuitive process so let me know if you have any questions.

[1]: http://mercurial.selenic.com/wiki/CrecordExtension

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +140,4 @@
>      private final Paint shadowPaint;
>      private final int shadowSize;
>  
> +    private final LightweightTheme mTheme;

While this is an irrelevant comment due to my next comment, note that the "m" in mTheme stands for "member" and is a form of Hungarian notation. Our convention is to match what the current file does and, for new files, don't use the notation. In this case, we're not using the notation so you would not want to introduce the notation again.

@@ +179,4 @@
>          setWillNotDraw(false);
>  
>          isNewTablet = NewTabletUI.isEnabled(context);
> +        mTheme = getTheme();

While this will work, the idea here is to remove the excess reference from the sub-classes, i.e. remove mTheme and call `getTheme()` where `mTheme` is used.

This removes redundancy (which often leads to mysterious errors, e.g. why are the two references to mTheme different?) and is (negligibly) more memory-efficient.

Comment 6

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: patch-v1.diff

Sorry in the previous patch I wrongly added some changes to the build system that I needed in Android Studio (intellij at the of the day), hope now are looking better.

Got ride of the member variables on the files where not needed, and that took me to modify as well |NavButton| as it was using the protected variable, now using the getter to retrieve the theme.

Comment 7

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

Comment on attachment 8537341 [details] [diff] [review]
patch-v1.diff

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

Right on!

Comment 8

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

To check the patch into the tree, you need to add the "checkin-needed" [1] keyword. However, in order to use that keyword, your patch needs an associated green push to our try test server. I'm not on my dev machine right now, but I'll give it a push when I get to it later today.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 9

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

Here's the try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a36506dcc3bc

When the push goes green, add the "checkin-needed" keyword! If you need help reading the results (they're a little unobvious), let me know!

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Thanks Michael,

push green :)

Comment 11

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

Thanks for the help, Francisco! :D

If you're looking to continue working on some bugs, I would suggest bug 1065484 (back-end) or bug 871593 (front-end - this one needs an updated mentor so it'll probably be ~24 hours to figure it out). Of course, if there's something else you'd like to work on, feel free!

Comment 12

[Francisco Jordano [:arcturus] [:francisco]]

Definitely happy to continue helping, wouldn't mind to wait for bug 871593.

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/f9e29a6834cd