979166 - Refactor dynamic toolbar code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Attachment: Refactor dynamic toolbar code

Carried over from bug 962103. The dynamic toolbar code is kind of difficult to digest right now as it's spread all over BrowserApp. This is an attempt to clean this up by moving some of this logic into a separate class.

Comment 1

[Brian Nicholson (:bnicholson)]

Just to be clear, the code is still spread out all over BrowserApp; the many dependencies of dynamic toolbar will make this hard to change. But by encapsulating some of the state and wrapping the logic with concise method names, hopefully it's at least a bit easier to follow.

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8385153 [details] [diff] [review]
Refactor dynamic toolbar code

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

Nice refactoring. Needs some fixes/tweaks before getting r+.

::: mobile/android/base/DynamicToolbar.java
@@ +5,5 @@
> +
> +import android.os.Bundle;
> +
> +public class DynamicToolbar {
> +    private static final String sStateEnabled = "dynamic_toolbar";

This is a constant, all caps. Rename it to STATE_ENABLED or something.

@@ +6,5 @@
> +import android.os.Bundle;
> +
> +public class DynamicToolbar {
> +    private static final String sStateEnabled = "dynamic_toolbar";
> +    private static final String sChromePref = "browser.chrome.dynamictoolbar";

Ditto. CHROME_PREF

@@ +11,5 @@
> +
> +    public static final int PIN_RELAYOUT =    1 << 0;
> +    public static final int PIN_ACTION_MODE = 1 << 1;
> +
> +    private boolean mPrefEnabled;

Not sure this is an actual possibility but what is the expected behaviour if, say, a isEnabled() call happens before the PrefsHelper.PrefHandlerBase() is initially called? Maybe you should be more explicit about the initial state?

@@ +16,5 @@
> +    private Integer mPrefObserverId;
> +    private boolean mAccessibilityEnabled;
> +    private int mPinned;
> +    private LayerView mLayerView;
> +    private OnEnabledChangedListener mEnabledChangedListener;

I thought we decided to get rid of the member prefixes for new code?

@@ +31,5 @@
> +
> +    public DynamicToolbar() {}
> +
> +    public void setLayerView(LayerView layerView) {
> +        mLayerView = layerView;

I assume there's no good way to get the LayerView directly from the DynamicToolbar's constructor instead of setting it later with this method?

@@ +38,5 @@
> +    public void setEnabledChangedListener(OnEnabledChangedListener listener) {
> +        mEnabledChangedListener = listener;
> +    }
> +
> +    public void initialize() {

The split between class constructor and this 'initialization' is a bit unexpected here. Why not just only create the DynamicToolbar instance when it can/should be initialized?

@@ +47,5 @@
> +                if (value == mPrefEnabled) {
> +                    return;
> +                }
> +
> +                mPrefEnabled = value;

mPrefEnabled is used from multiple threads, make it volatile?

@@ +54,5 @@
> +                    @Override
> +                    public void run() {
> +                        // If accessibility is enabled, the dynamic toolbar is
> +                        // forced to be off.
> +                        if (!mAccessibilityEnabled && mEnabledChangedListener != null) {

Using isEnabled() here would probably make this code a little easier to follow (unless I'm missing something here). Maybe factor out this code into a separate method like triggerOnEnabledListener() that always does:

if (mEnabledChangedListener != null) {
    mEnabledChangedListener.onEnabledChanged(isEnabled());
}

To ensure consistent behaviour?

@@ +70,5 @@
> +            }
> +        });
> +    }
> +
> +    public void destroy() {

nit: I usually place destruction-type methods at the end of the class.

@@ +87,5 @@
> +            mPrefEnabled = savedInstanceState.getBoolean(sStateEnabled);
> +        }
> +    }
> +
> +    public boolean isEnabled() {

Why no UI thread assertion here?

@@ +101,5 @@
> +
> +        // Disable the dynamic toolbar when accessibility features are enabled,
> +        // and re-read the preference when they're disabled.
> +        mAccessibilityEnabled = enabled;
> +        if (mPrefEnabled && mEnabledChangedListener != null) {

I don folow. Why would you only call the listener if mPrefEnabled is true here?

@@ +106,5 @@
> +            mEnabledChangedListener.onEnabledChanged(enabled);
> +        }
> +    }
> +
> +    public void show(boolean animate) {

Maybe a setVisible(boolean visible, boolean animate) would avoid some of the code duplication here? I realize this was not introduced by your patch but the use of a boolean for 'animate' here looks a bit cryptic on the caller side. Maybe turn this into a flag or something?

@@ +122,5 @@
> +            mLayerView.getLayerMarginsAnimator().hideMargins(!animate);
> +        }
> +    }
> +
> +    public void pin(int reason) {

Maybe a setPinned(boolean, reason) method would avoid some of the code duplication here?

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Refactor dynamic toolbar code, v2

Made suggested changes unless otherwise mentioned below:

(In reply to Lucas Rocha (:lucasr) from comment #2)
> ::: mobile/android/base/DynamicToolbar.java
> @@ +5,5 @@
> > +
> > +import android.os.Bundle;
> > +
> > +public class DynamicToolbar {
> > +    private static final String sStateEnabled = "dynamic_toolbar";
> 
> This is a constant, all caps. Rename it to STATE_ENABLED or something.

Hmph, fixed. Android should fix their docs at https://source.android.com/source/code-style.html#follow-field-naming-conventions. s/Public static final fields/Static final fields/

> @@ +11,5 @@
> > +
> > +    public static final int PIN_RELAYOUT =    1 << 0;
> > +    public static final int PIN_ACTION_MODE = 1 << 1;
> > +
> > +    private boolean mPrefEnabled;
> 
> Not sure this is an actual possibility but what is the expected behaviour
> if, say, a isEnabled() call happens before the PrefsHelper.PrefHandlerBase()
> is initially called?

The pref defaults to off, so isEnabled() will return false. We don't enable dynamic toolbar until we explicitly get the pref from Gecko telling us to turn it on. I added comment saying this.

> @@ +16,5 @@
> I thought we decided to get rid of the member prefixes for new code?

Wasn't sure whether we ever officially decided on that. I guess we came close enough, so fixed.

> @@ +31,5 @@
> > +
> > +    public DynamicToolbar() {}
> > +
> > +    public void setLayerView(LayerView layerView) {
> > +        mLayerView = layerView;
> 
> I assume there's no good way to get the LayerView directly from the
> DynamicToolbar's constructor instead of setting it later with this method?

Unfortunately, no; the toolbar must be constructed by onCreate (because of onSaveInstanceState), but the LayerView doesn't come in until initializeChrome.

> @@ +70,5 @@
> nit: I usually place destruction-type methods at the end of the class.

It's a mirror of initialize, so I think it makes more sense to put it next to it. Since one method is the opposite of another, and since changing one will often result in changing the other, it's helpful to be able to refer to the counterpart without scrolling through the whole file.

> @@ +101,5 @@
> > +
> > +        // Disable the dynamic toolbar when accessibility features are enabled,
> > +        // and re-read the preference when they're disabled.
> > +        mAccessibilityEnabled = enabled;
> > +        if (mPrefEnabled && mEnabledChangedListener != null) {
> 
> I don folow. Why would you only call the listener if mPrefEnabled is true
> here?

The dynamic toolbar is enabled iff prefEnabled is true *and* accessibilityEnabled is false. If prefEnabled is false, changing the accessibility state will have no effect since the toolbar will still be disabled regardless. (Note that the other place we fire the OnEnabledChangedListener has a `!accessibilityEnabled` check for similar reasons.)

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 8385583 [details] [diff] [review]
Refactor dynamic toolbar code, v2

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

::: mobile/android/base/BrowserApp.java
@@ +277,5 @@
>              (event.getSource() & InputDevice.SOURCE_GAMEPAD) == InputDevice.SOURCE_GAMEPAD) {
>              switch (keyCode) {
>                  case KeyEvent.KEYCODE_BUTTON_Y:
>                      // Toggle/focus the address bar on gamepad-y button.
> +

Not sure how this newline got here. Removed!

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8385583 [details] [diff] [review]
Refactor dynamic toolbar code, v2

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

::: mobile/android/base/DynamicToolbar.java
@@ +12,5 @@
> +    public static final int PIN_RELAYOUT =    1 << 0;
> +    public static final int PIN_ACTION_MODE = 1 << 1;
> +
> +    public static final int TRANSITION_NONE =    1 << 0;
> +    public static final int TRANSITION_ANIMATE = 1 << 1;

I tend to prefer EnumSets for these flags for the extra type-safety. But I can live with those int flags.

@@ +37,5 @@
> +    }
> +
> +    public DynamicToolbar() {
> +        // Listen to the dynamic toolbar pref
> +        prefObserverId = PrefsHelper.getPref(CHROME_PREF, new PrefsHelper.PrefHandlerBase() {

nit: I'd move this anon class into a private class so that the constructor is easier to read:

prefObserverId = PrefsHelper.getPref(CHROME_PREF, new PrefsHandler());

Then at the bottom of the class:

private class PrefsHandler {
   ...
}

Comment 6

[Brian Nicholson (:bnicholson)]

Landed with both suggested changes: https://hg.mozilla.org/integration/fx-team/rev/d8018c23945a

Comment 7

[Brian Nicholson (:bnicholson)]

Backed out for NPEs:
PROCESS-CRASH | java-exception | java.lang.NullPointerException at org.mozilla.gecko.DynamicToolbar.setPinned(DynamicToolbar.java:125)

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

Comment 8

[Brian Nicholson (:bnicholson)]

Relanded with NPE fixed:

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

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/76d856e4ec61