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

RESOLVED FIXED in Firefox 17

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sriram, Unassigned)

Tracking

unspecified
Firefox 17
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 646671 [details] [diff] [review]
Patch (1/X): Event registration

This removes unnecessary reference to mAppContext on event registration.
Attachment #646671 - Flags: review?(mbrubeck)
Attachment #646671 - Flags: review?(mbrubeck) → review+
(Reporter)

Comment 2

6 years ago
Created attachment 646716 [details] [diff] [review]
Patch (2/X): DisplayMetrics

This kills most of the dependencies of GeckoApp.mAppContext for DisplayMetrics. They are obtained from their context's getResources().
Attachment #646716 - Flags: review?(mbrubeck)
(Reporter)

Comment 3

6 years ago
Created attachment 646723 [details] [diff] [review]
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.
Attachment #646723 - Flags: review?(mbrubeck)
(Reporter)

Comment 4

6 years ago
Created attachment 646728 [details] [diff] [review]
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.
Attachment #646728 - Flags: review?(margaret.leibovic)
Attachment #646723 - Flags: review?(mbrubeck) → review+

Comment 5

6 years ago
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).
Attachment #646728 - Flags: review?(margaret.leibovic) → review+
(Reporter)

Comment 6

6 years ago
(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 :)
(Reporter)

Comment 7

6 years ago
Created attachment 646740 [details] [diff] [review]
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.
Attachment #646740 - Flags: review?(mbrubeck)
(Reporter)

Comment 8

6 years ago
Created attachment 646743 [details] [diff] [review]
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.
Attachment #646743 - Flags: review?(mbrubeck)
Attachment #646743 - Flags: review?(mbrubeck) → review+
(Reporter)

Comment 9

6 years ago
Created attachment 646750 [details] [diff] [review]
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! :)
Attachment #646750 - Flags: review?(mbrubeck)
(Reporter)

Comment 10

6 years ago
Created attachment 646755 [details] [diff] [review]
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.
Attachment #646755 - Flags: review?(margaret.leibovic)
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.)
Attachment #646750 - Flags: review?(mbrubeck) → review+
(Reporter)

Updated

6 years ago
Attachment #646755 - Attachment description: Patch → Patch (8/X): Form Assistant
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;
}
Attachment #646740 - Flags: review?(mbrubeck) → review+
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
Attachment #646716 - Flags: review?(mbrubeck) → review+
(Reporter)

Comment 14

6 years ago
Created attachment 646771 [details] [diff] [review]
Patch (9/X): Tabs

This cleans up Tabs.java
mMainHandler should ultimately made private -- but too many references -- hence doing it in parts.
Attachment #646771 - Flags: review?(mbrubeck)
Attachment #646771 - Flags: review?(mbrubeck) → review+
(Reporter)

Comment 15

6 years ago
Created attachment 646778 [details] [diff] [review]
Patch (9/X): Tabs

This is same as previous patch. Just realized that, instead of a getHandler(), I could use "runOnUiThread()".
Attachment #646778 - Flags: review?(mbrubeck)
Attachment #646778 - Flags: review?(mbrubeck) → review+

Comment 16

6 years ago
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?
(Reporter)

Updated

6 years ago
Attachment #646771 - Attachment is obsolete: true
(Reporter)

Comment 17

6 years ago
(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 20

6 years ago
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.
Attachment #646755 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 21

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.