Closed Bug 959184 Opened 6 years ago Closed 6 years ago

Use enum (EDIT, DISPLAY) instead of boolean to track BrowserToolbar mode

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: lucasr, Unassigned)

References

Details

Attachments

(1 file)

Using an enum will make the code easier to read and it's more future-proof.
Attachment #8361092 - Flags: review?(michael.l.comella)
Comment on attachment 8361092 [details] [diff] [review]
Use enum instead of boolean to track BrowserToolbar's mode (r=mcomella)

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

Looks good to me with the var convention change.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +144,5 @@
>  
>      final private BrowserApp mActivity;
>      private boolean mHasSoftMenuButton;
>  
> +    private UIMode mUiMode;

s/UiMode/UIMode/

We should be consistent, here and throughout the file.

uiMode (i.. as a local var) should probably stay like that.

@@ +848,5 @@
>              }
>          }
>      }
>  
> +    private void setUiMode(UIMode uiMode) {

nit: final
Attachment #8361092 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #2)
> Comment on attachment 8361092 [details] [diff] [review]
> Use enum instead of boolean to track BrowserToolbar's mode (r=mcomella)
> 
> Review of attachment 8361092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me with the var convention change.
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +144,5 @@
> >  
> >      final private BrowserApp mActivity;
> >      private boolean mHasSoftMenuButton;
> >  
> > +    private UIMode mUiMode;
> 
> s/UiMode/UIMode/
> 
> We should be consistent, here and throughout the file.
> 
> uiMode (i.. as a local var) should probably stay like that.

Done.

> @@ +848,5 @@
> >              }
> >          }
> >      }
> >  
> > +    private void setUiMode(UIMode uiMode) {
> 
> nit: final

final what?
Pushed with suggested changes (and assumed the final is for setUIMode's argument).

https://hg.mozilla.org/integration/fx-team/rev/b87878a98887
https://hg.mozilla.org/mozilla-central/rev/b87878a98887
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
(In reply to Lucas Rocha (:lucasr) from comment #3)
> final what?

Method arg.

But yes, definitely a nit. :)
You need to log in before you can comment on or make changes to this bug.