1188198 - Remove m prefix from ThemedView.java.frag template & generate results

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

ThemedView.java.frag [1] is a template that auto-generates m/a/b/widget/Themed*.java. We use an old convention in that file that member variables have an `m` prefix. Your mission:

1) Remove the `m` prefix from all variables.
2) Regenerate the output files. `./mach python mobile/android/base/widget/generate_themed_views.py` is probably the easiest way.
3) Commit and add it all to version control!

[1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ThemedView.java.frag

Comment 1

[raunaqabhyankar]

Attachment: bug-1188198-fix.patch

Comment 2

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

Comment on attachment 8706924 [details] [diff] [review]
bug-1188198-fix.patch

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

::: mobile/android/base/java/org/mozilla/gecko/widget/themed/ThemedView.java.frag
@@ +21,5 @@
>  import android.util.AttributeSet;
>  
>  public class Themed@VIEW_NAME_SUFFIX@ extends @BASE_TYPE@
>                                       implements LightweightTheme.OnChangeListener {
> +    private LightweightTheme Theme;

Sorry, if I wasn't clear – member variables should start with lowercase letters.

Comment 3

[raunaqabhyankar]

(In reply to Michael Comella (:mcomella) from comment #2)
> Comment on attachment 8706924 [details] [diff] [review]
> bug-1188198-fix.patch
> 
> Review of attachment 8706924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/widget/themed/ThemedView.java.frag
> @@ +21,5 @@
> >  import android.util.AttributeSet;
> >  
> >  public class Themed@VIEW_NAME_SUFFIX@ extends @BASE_TYPE@
> >                                       implements LightweightTheme.OnChangeListener {
> > +    private LightweightTheme Theme;
> 
> Sorry, if I wasn't clear – member variables should start with lowercase
> letters.

 if ((isLight && IsLight != isLight) || (!isLight && IsDark == isLight))
What to do here? If member variable is named "isLight" then we will have same variable names.

Comment 4

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

By the way, you can use the "Need more information from" field to give a user a persistent notification so they'll be more likely to see your comments or questions.

(In reply to raunaqabhyankar from comment #3)
>  if ((isLight && IsLight != isLight) || (!isLight && IsDark == isLight))
> What to do here? If member variable is named "isLight" then we will have
> same variable names.

isLight vs. IsLight is also pretty confusing, to be fair. :P

A common convention is to differentiate arguments from member variables by using the `this` reference, i.e.
  (isLight && this.isLight != isLight) || ...

Comment 5

[raunaqabhyankar]

Attachment: bug-1188198-fix.patch

Changed to lowercase..!

Comment 6

[raunaqabhyankar]

(In reply to Michael Comella (:mcomella) from comment #4)
> By the way, you can use the "Need more information from" field to give a
> user a persistent notification so they'll be more likely to see your
> comments or questions.
> 
> (In reply to raunaqabhyankar from comment #3)
> >  if ((isLight && IsLight != isLight) || (!isLight && IsDark == isLight))
> > What to do here? If member variable is named "isLight" then we will have
> > same variable names.
> 
> isLight vs. IsLight is also pretty confusing, to be fair. :P
> 
> A common convention is to differentiate arguments from member variables by
> using the `this` reference, i.e.
>   (isLight && this.isLight != isLight) || ...

Have submitted a patch. Kindly review and let me know of any change to be made..

Comment 7

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

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

Comment 8

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

Comment on attachment 8710072 [details] [diff] [review]
bug-1188198-fix.patch

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

Right on! This looks good to me! Also, I apologize for the delay. It shouldn't be this long in the future.

I made a push to our try test servers (above).

Once the push goes green, you can add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run. Let me know if you need help reading the results.

[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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fefbc8e44b8

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fefbc8e44b8