778247 - GeckoApp.mAppContext shouldn't be a "public static" member

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

With a public static status, we are making a singleton out of GeckoApp. This makes everything tightly coupled and get crashes when something changes at Android level -- which changes the context.

It's better not to be dependent on mAppContext as much as possible.

Here are few things:
1. Every view can access UI thread by call getHandler() on itself. This can reduce calls to GeckoApp.mAppContext.mMainHandler. If there is a bunch of things to be done (say in BrowserToolbar, BrowserToolbar should take care of overriding getHandler()).

2. DisplayMetrics can be obtained from Resources too (which in turn can be obtained from Context). http://developer.android.com/reference/android/content/res/Resources.html#getDisplayMetrics%28%29
Hence a call for getWindowManager() from GeckoApp.mAppContext isn't needed.

.. and on and on.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (1/X): Event registration

This removes unnecessary reference to mAppContext on event registration.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2/X): DisplayMetrics

This kills most of the dependencies of GeckoApp.mAppContext for DisplayMetrics. They are obtained from their context's getResources().

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (3/X): BrowserToolbar

BrowserToolbar is available only for BrowserApp. Hence it's enough if it has a reference to BrowserApp and makes calls through it. GeckoApp.mAppContext isn't needed.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (4/X): TextSelection

This removes text-selection references to mAppContext.
If there is a top-level controller for the handles (like a view or something), it's better to post it to that view -- which I'm not aware of.

Comment 5

[:Margaret Leibovic]

Comment on attachment 646728 [details] [diff] [review]
Patch (4/X): TextSelection

Nice, I didn't know you could do this. Why haven't we done this from the start?

(In reply to Sriram Ramasubramanian [:sriram] from comment #4)

> If there is a top-level controller for the handles (like a view or
> something), it's better to post it to that view -- which I'm not aware of.

There isn't a top-level view that manages the handles (the TextSelection class manages them, but that's not a view).

Comment 6

[Sriram Ramasubramanian [:sriram]]

(In reply to Margaret Leibovic [:margaret] from comment #5)
> Comment on attachment 646728 [details] [diff] [review]
> Patch (4/X): TextSelection
> 
> Nice, I didn't know you could do this. Why haven't we done this from the
> start?
> 

:D Now we know.. and I'm cleaaaaaaaaaning up everything :)

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (5/X): TabsPanel

This is for TabsPanel.
The PanelView (interfaces) should know the parent to inform the parent to close. (Or in ideal world, it should inform its parent it has closed -- which we don't need).
Hence a reference to TabsPanel is placed in TabsTray and RemoteTabs. Now they call autoHidePanel() (like I said in previous statement -- parent is Panel).
The panel knows it should call the GeckoApp's autoHideTabs() <-- which is a reference again.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (6/X): BrowserApp

BrowserApp knows mAppContext and it doesnt need to refer it from GeckoApp.
AboutHomeContent's update() runs on GeckoAppShell's handler. An unnecessary UI thread posting is not needed.
I'll be killing the mAppContext left behind in next patch that cleans AboutHomeContent.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (7/X): AboutHomeContent

AboutHomeContent never needed an Activity all the time. Now it's got from context (safe) and holds on to it. When a new activity is launched -- a new context - and hence a new mActivity for AboutHomeContent. All is well! :)

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (8/X): Form Assistant

This uses views post() method.
IME can be got from Context.
mActivity is needed only for viewport stuff. If we can get the viewport in the message, we can kill that too.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 646750 [details] [diff] [review]
Patch (7/X): AboutHomeContent

>     public AboutHomeContent(Context context) {
>         super(context);
>+        mContext = context;
>     }

Do we ever use this constructor?  If so, we should set mActivity here too.  If not, can we just remove it?

>     public AboutHomeContent(Context context, AttributeSet attrs) {
>         super(context, attrs);
>+        mContext = context;
>+
>+        if (context instanceof BrowserApp)
>+            mActivity = (BrowserApp) context;
>     }

Since a lot of the code in this patch depends on mActivity being set, I think we should remove the "if" statement here, or replace it with an assert.  (Basically, we're going to crash if it's not set, so we might as well crash here instead of later.)

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 646740 [details] [diff] [review]
Patch (5/X): TabsPanel

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

Nice!  I think we can improve this even more by moving some methods from GeckoApp into TabsPanel, but that can be done in a separate bug if you prefer.  Details below...

::: mobile/android/base/TabsPanel.java
@@ +60,5 @@
>          mContext = context;
>  
> +        if (context instanceof GeckoApp)
> +            mActivity = (GeckoApp) context;
> +

I think we should remove the "if" here too (for the same reason as in the other patch).

@@ +96,2 @@
>                  else
> +                    mActivity.showRemoteTabs();

I think we could just call this.showTabs(...) directly here, and maybe remove the GeckoApp showLocalTabs/showRemoteTabs methods.  (The only other caller is BrowserToolbar, and we could change it to access the TabsPanel directly rather than through GeckoApp.)

@@ +163,4 @@
>          mPanel.show();
>          mListContainer.addView(mPanel.getLayout());
>  
> +        if (mActivity.hasTabsSideBar()) {

This could change to "if (isSideBar())".

@@ +218,5 @@
>  
> +    public void autoHidePanel() {
> +        mActivity.autoHideTabs();
> +    }
> +

I think perhaps we should just move the code from autoHideTabs into this method, and remove it from GeckoApp.  It would be very simple:

public boolean autoHidePanel() {
    if (!isSideBar() && isShown()) {
        hide();
        return true;
    }
    return false;
}

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 646716 [details] [diff] [review]
Patch (2/X): DisplayMetrics

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

::: mobile/android/base/PromptService.java
@@ +47,5 @@
> +    private static int mGroupPaddingSize = 0;
> +    private static int mLeftRightTextWithIconPadding = 0;
> +    private static int mTopBottomTextWithIconPadding = 0;
> +    private static int mIconTextPadding = 0;
> +    private static int mIconSize = 0;

Drive-by: these should either be non-static (my preference) or start with s rather than m

Comment 14

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (9/X): Tabs

This cleans up Tabs.java
mMainHandler should ultimately made private -- but too many references -- hence doing it in parts.

Comment 15

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (9/X): Tabs

This is same as previous patch. Just realized that, instead of a getHandler(), I could use "runOnUiThread()".

Comment 16

[:Margaret Leibovic]

Comment on attachment 646755 [details] [diff] [review]
Patch (8/X): Form Assistant

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

::: mobile/android/base/FormAssistPopup.java
@@ +226,5 @@
>  
>          int popupWidth = RelativeLayout.LayoutParams.FILL_PARENT;
>          int popupLeft = left < 0 ? 0 : left;
>  
> +        FloatSize viewport = mActivity.getLayerController().getViewportSize();

This will run into problems if mActivity is never initialized. If we're depending on this class being created from a GeckoApp instance, perhaps we should just put a GeckoApp parameter in the constructor.

Also, is there a reason to have both mActivity and mContext?

Comment 17

[Sriram Ramasubramanian [:sriram]]

(In reply to Matt Brubeck (:mbrubeck) from comment #12)
> Comment on attachment 646740 [details] [diff] [review]
> @@ +96,2 @@
> >                  else
> > +                    mActivity.showRemoteTabs();
> 
> I think we could just call this.showTabs(...) directly here, and maybe
> remove the GeckoApp showLocalTabs/showRemoteTabs methods.  (The only other
> caller is BrowserToolbar, and we could change it to access the TabsPanel
> directly rather than through GeckoApp.)

I tried doing this. Now if we give access of TabsPanel to BrowserToolbar -- we are again messing up things. It's better for the activity to be a bridge -- so that BrowserToolbar/AboutHomeContent can inform Activity to take action -- and Activity can do what is required.

> @@ +218,5 @@
> >  
> > +    public void autoHidePanel() {
> > +        mActivity.autoHideTabs();
> > +    }
> > +
> 
> I think perhaps we should just move the code from autoHideTabs into this
> method, and remove it from GeckoApp.  It would be very simple:
> 
> public boolean autoHidePanel() {
>     if (!isSideBar() && isShown()) {
>         hide();
>         return true;
>     }
>     return false;
> }

There are many references to autoHideTabs() of GeckoApp. I could probably take this as a followup.

Comment 18

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a598781db2f2
http://hg.mozilla.org/integration/mozilla-inbound/rev/151f4c56c608
http://hg.mozilla.org/integration/mozilla-inbound/rev/b2a7b60ef3b1
http://hg.mozilla.org/integration/mozilla-inbound/rev/985bfd3aea74
http://hg.mozilla.org/integration/mozilla-inbound/rev/123e9052f37e
http://hg.mozilla.org/integration/mozilla-inbound/rev/05e6057b43b0
http://hg.mozilla.org/integration/mozilla-inbound/rev/e98e417adc2f

Comment 19

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/a598781db2f2
https://hg.mozilla.org/mozilla-central/rev/151f4c56c608
https://hg.mozilla.org/mozilla-central/rev/b2a7b60ef3b1
https://hg.mozilla.org/mozilla-central/rev/985bfd3aea74
https://hg.mozilla.org/mozilla-central/rev/123e9052f37e
https://hg.mozilla.org/mozilla-central/rev/05e6057b43b0
https://hg.mozilla.org/mozilla-central/rev/e98e417adc2f

Comment 20

[:Margaret Leibovic]

Comment on attachment 646755 [details] [diff] [review]
Patch (8/X): Form Assistant

Clearing review, since this bug has been closed. You can move this patch to a new bug if you still want to land it, but I had questions about it in comment 16.

Comment 21

[Sriram Ramasubramanian [:sriram]]

(In reply to Margaret Leibovic [:margaret] from comment #16)
> Comment on attachment 646755 [details] [diff] [review]
> Patch (8/X): Form Assistant
> 
> Review of attachment 646755 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/FormAssistPopup.java
> @@ +226,5 @@
> >  
> >          int popupWidth = RelativeLayout.LayoutParams.FILL_PARENT;
> >          int popupLeft = left < 0 ? 0 : left;
> >  
> > +        FloatSize viewport = mActivity.getLayerController().getViewportSize();
> 
> This will run into problems if mActivity is never initialized. If we're
> depending on this class being created from a GeckoApp instance, perhaps we
> should just put a GeckoApp parameter in the constructor.
> 
> Also, is there a reason to have both mActivity and mContext?

I started replying, but I forgot.
1. Using mActivity as a parameter in a constructor is a wrong idea. We should do something like 
mFormPopupAssistant.registerActivity(this);
from GeckoApp, if you feel casting could be a problem.

2. They both are the same. But I don't like keep casting it every time. Also, that would let us know "these are ones dependent on context, and these are on activity", which can help us while killing the mActivity, if we ever come up with something like that.

I'll move this to a separate bug.