838793 - Convert AboutHomeContent to a Fragment

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

We always hold onto the AboutHomeContent view, meaning we have increased memory usage from the thumbnails, logo, personas, and other views when about:home isn't visible. Additionally, the observers are still registered when about:home isn't displayed, causing unnecessary overhead in certain situations (such rotating the device and clearing private data).

We should look into destroying/reinflating about:home between views. Though there may be a performance hit when navigating to about:home, the memory gains could outweigh the cons (for example, OOMS will be reduced).

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Destroy about:home between views

This was the approach I was hoping we could go with, but adding AboutHomeContent dynamically results in a brief white screen at startup instead of the blue about:home background. Personally, it doesn't bother me too much since it makes about:home look like any other web page (which always start off as blank when loading), but I'm sure others will disagree.

Performance-wise, I haven't seen any perceptible difference when showing about:home. Rotating the phone on any non-about:home page is noticeably smoother/faster.

Build available at https://dl.dropbox.com/u/35559547/fennec-about-home-inflate.apk

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Can you check with Kats for ways to collect some memory usage numbers? (with and without patch).

Comment 3

[Wesley Johnston (:wesj)]

We'll want to test this on some larger profiles too. I'm surprised there's no perceptible difference showing about:home with it. But maybe the providers cache enough in memory to make up for any slowness.

Comment 4

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

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Can you check with Kats for ways to collect some memory usage numbers? (with
> and without patch).

The simplest way to get memory numbers it to do this:

https://wiki.mozilla.org/Mobile/Fennec/Android#about:memory

Compare the "resident" memory usage from the about:memory dump before and after and see if it's any different. It sounds like you'll want to be off the about:home screen for this to help, so I would suggest putting a small HTML file on the SD card and load that to eliminate any discrepancies from network packets.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Destroy about:home between views, v2

This patch isn't actually much more complicated, and I didn't notice any UI differences like I did with the first patch. The difference is that this creates a persistent ScrollView that acts as the container for AboutHomeContent (which now extends RelativeLayout); previously, AboutHomeContent was itself a ScrollView. Either way, though, we have a RelativeLayout inside of ScrollView, so I don't think this results in the creation of any more views.

Since AboutHomeContent now corresponds to the view inside of the ScrollView, it means we can keep the blue background color on the ScrollView, which fixes the blank screen problem in the first patch. We can still destroy/inflate AboutHomeContent each time by adding and removing it from the parent ScrollView.

To measure memory, I went to about:home, loaded about:blank (to hide about:home), did a GC, and dumped the heap. Before the patch, the heap total was 14.8MB, and at least 4.2MB were from layouts and views on about:home. After the patch, the heap total was 10MB, and the layouts and views from about:home were not shown on the heap as expected.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 711009 [details] [diff] [review]
Destroy about:home between views, v2

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

My recommendation:
1. Leave AboutHomeContent as it is.
2. Have show() and hide() methods inside AboutHomeContent, that would do the deletion and re-inflation.
TabsPanel works that way.

::: mobile/android/base/AboutHomeContent.java
@@ +70,1 @@
>                                implements TabsAccessor.OnQueryTabsCompleteListener,

This could be a ScrollView. But what's inflated could be a RelativeLayout (like what's there now).

::: mobile/android/base/BrowserApp.java
@@ +55,5 @@
>      private static final String LOGTAG = "GeckoBrowserApp";
>  
>      public static BrowserToolbar mBrowserToolbar;
>      private AboutHomeContent mAboutHomeContent;
> +    private ViewGroup mAboutHomeContainer;

This might not be needed. Reasons listed below.

@@ +229,5 @@
>          if (mTabsPanel != null)
>              mTabsPanel.setTabsLayoutChangeListener(this);
>  
>          mFindInPageBar = (FindInPageBar) findViewById(R.id.find_in_page);
> +        mAboutHomeContainer = (ViewGroup) mGeckoLayout.findViewById(R.id.abouthome_container);

This could be mAboutHomeContent.

@@ +345,5 @@
>          mTabsPanel.refresh();
>  
> +        if (mAboutHomeContent != null) {
> +            hideAboutHome();
> +            showAboutHome();

mAboutHomeContent.hide();
mAboutHomeContent.show();

@@ +692,5 @@
>              return;
>  
> +        mAboutHomeContainer.setVisibility(View.VISIBLE);
> +        mAboutHomeContent = (AboutHomeContent) LayoutInflater.from(BrowserApp.this)
> +                .inflate(R.layout.abouthome_content, mAboutHomeContainer, false);

The inflate should go inside AboutHomeContent. It could stay there, in its whole, instead of spilling over here.

@@ +693,5 @@
>  
> +        mAboutHomeContainer.setVisibility(View.VISIBLE);
> +        mAboutHomeContent = (AboutHomeContent) LayoutInflater.from(BrowserApp.this)
> +                .inflate(R.layout.abouthome_content, mAboutHomeContainer, false);
> +        mAboutHomeContainer.addView(mAboutHomeContent);

This is good if its inside AboutHomeContent.

::: mobile/android/base/resources/layout/gecko_app.xml.in
@@ +30,5 @@
>  
> +            <ScrollView android:id="@+id/abouthome_container"
> +                        android:layout_width="fill_parent"
> +                        android:layout_height="fill_parent"
> +                        android:background="@drawable/abouthome_bg_repeat"/>

There's one more gecko_app.xml.in I guess :D

Comment 7

[Brian Nicholson (:bnicholson)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> Comment on attachment 711009 [details] [diff] [review]
> Destroy about:home between views, v2
> 
> Review of attachment 711009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My recommendation:
> 1. Leave AboutHomeContent as it is.
> 2. Have show() and hide() methods inside AboutHomeContent, that would do the
> deletion and re-inflation.
> TabsPanel works that way.
> 

Thanks for the feedback. While I agree it would be nice for AboutHomeContent logic to be contained within AboutHomeContent, we have to take care to properly destroy it when we call mAboutHomeContent.hide(). AboutHomeContent is more than just a collection of views; it's a class that actually uses and holds references to these views, and removing the views won't automatically remove the references since AboutHomeContent stays alive. We're making AboutHomeContent more fragile by making it responsible for nullifying all of these references in onDestroy, which we don't normally need to do. Destroying it and recreating it entirely means we don't need to be concerned with meticulous cleanup.

If this is all about encapsulating the logic, maybe we could create an AboutHome class that extends ScrollView, and it could be responsible for everything BrowserApp is doing in this patch? We could then say mAboutHome.show()/hide() (which would internally add/remove an AboutHomeContent child view).

> ::: mobile/android/base/resources/layout/gecko_app.xml.in
> @@ +30,5 @@
> >  
> > +            <ScrollView android:id="@+id/abouthome_container"
> > +                        android:layout_width="fill_parent"
> > +                        android:layout_height="fill_parent"
> > +                        android:background="@drawable/abouthome_bg_repeat"/>
> 
> There's one more gecko_app.xml.in I guess :D

Thanks - forgot about that one!

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Some possible issues with this patch:
* Does it regress the "promo changes on rotation" bug (bug 837142) ?
* I would like to hide most of these changes from the GeckoApp/BrowserApp parent. I wonder how if we could move more of these changes into AboutHomeContent, like Sriram suggested.

Comment 9

[Lucas Rocha (:lucasr)]

Quick idea: wouldn't it better if you detached the AboutHomeContent view from the parent and kept a soft reference to it? This way we'd leave the about:home view available to be garbage collected but would still be able to reuse it if it hasn't been disposed between about:home visits.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> Quick idea: wouldn't it better if you detached the AboutHomeContent view
> from the parent and kept a soft reference to it? This way we'd leave the
> about:home view available to be garbage collected but would still be able to
> reuse it if it hasn't been disposed between about:home visits.

Yeah, we were talking about this on IRC. I think one disadvantage to doing this is that we'd still have to make sure the view doesn't get stale (after clearing private data, orientation changes, etc.) when it's in the background. But maybe we could just drop our reference when this happens to force a new inflation at the next visit.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Destroy about:home between views, v3

Creates an AboutHome class that acts as a kind of proxy to its inner AboutHomeContent. It also uses a SoftReference for AboutHomeContent to keep it around in memory. I've seen the reference cleared in certain situtations, and I've verified with MAT that the AboutHomeContent view has no other GC roots, so we shouldn't be leaking anywhere. I can also confirm that changing orientations does not change the promo box.

Comment 12

[Brian Nicholson (:bnicholson)]

Attachment: Destroy about:home between views, v4

minor fix

Comment 13

[Brian Nicholson (:bnicholson)]

Hmm...there's still problems with getting a stale view with this patch from using a SoftReference. Since the personas and remote tabs listeners are unregistered when we're not looking at about:home, it won't refresh with the changes.

We could just ditch the SoftReference and reinflate every time like in the original patch. Or we could move the listeners to AboutHome, then call the appropriate methods on AboutHomeContent if about:home is visible. If it's not visible, we could ditch the SoftReference to trigger a reinflation whenever it's next viewed - maybe this could be done in a follow-up?

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #13)
> Hmm...there's still problems with getting a stale view with this patch from
> using a SoftReference. Since the personas and remote tabs listeners are
> unregistered when we're not looking at about:home, it won't refresh with the
> changes.
> 
> We could just ditch the SoftReference and reinflate every time like in the
> original patch. Or we could move the listeners to AboutHome, then call the
> appropriate methods on AboutHomeContent if about:home is visible. If it's
> not visible, we could ditch the SoftReference to trigger a reinflation
> whenever it's next viewed - maybe this could be done in a follow-up?

Drive-by: moving the listeners to AboutHome would probably be a more robust approach IMO.

Comment 15

[Brian Nicholson (:bnicholson)]

Attachment: Destroy about:home between views, v5

This registers the listeners in AboutHome instead of AboutHomeContent, and it works correctly with personas. Since we're looking into using Fragments, though, I won't put this up for review, but we can use it to compare the two implementations.

Comment 16

[Aaron Train [:aaronmt]]

Happy to try a build with this out to verify if bug 847348 is fixed as a result.

Comment 17

[Brian Nicholson (:bnicholson)]

Attachment: Convert AboutHomeContent to a Fragment

This converts AboutHomeContent to a fragment so it can be destroyed/recreated as its visibility changes. There are a few things worth discussing in the patch, so I'll add some review comments of my own to clarify...

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 734922 [details] [diff] [review]
Convert AboutHomeContent to a Fragment

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

To aid with the review, I tried to annotate the less obvious parts of the patch:

::: mobile/android/base/BrowserApp.java
@@ +467,5 @@
> +        // so we can't use Fragment#isVisible() to determine whether the
> +        // about:home is shown. Instead, we use Fragment#getUserVisibleHint()
> +        // with the hint we set ourselves.
> +        mAboutHomeContent = AboutHomeContent.newInstance();
> +        mAboutHomeContent.setUserVisibleHint(false);

Rather than adding <AboutHomeContent/> to the XML and having it automatically inflated with the rest of the layout, I created an instance of it here. When the fragment was added to the XML directly, the view was automatically inflated at startup, which we don't want if we're not showing about:home. Creating the fragment programmatically seemed to be the only way to avoid that.

@@ -725,5 @@
>          updateSideBarState();
>          mTabsPanel.refresh();
> -
> -        if (mAboutHomeContent != null)
> -            mAboutHomeContent.refresh();

This will now be handled internally in the fragment in onConfigurationChanged.

::: mobile/android/base/GeckoApp.java
@@ +1346,5 @@
> +                mProfile = GeckoProfile.get(this, profileName, profilePath);
> +            }
> +        }
> +
> +        BrowserDB.initialize(getProfile().getName());

I moved this from initialize() to onCreate() because I was running into problems with views on about:home trying to use BrowserDB before it's initialized, and we don't GeckoApp#initialize() to race with other code trying to use the database. Another (maybe better) solution might be to make BrowserDB a singleton whose getInstance() method guarantees that it is initialized, but this seemed sufficient for now.

::: mobile/android/base/Tab.java
@@ +95,5 @@
>          mZoomConstraints = new ZoomConstraints(false);
>          mPluginViews = new ArrayList<View>();
>          mPluginLayers = new HashMap<Object, Layer>();
>          mState = shouldShowProgress(url) ? STATE_SUCCESS : STATE_LOADING;
> +        mBackgroundColor = getBackgroundColorForUrl(url);

Previously, AboutHomeContent was always inflated at startup, regardless of whether we were actually showing it. This patch changes that by creating the about:home view only when we're showing about:home.

Before, the background color of AboutHomeContent was set to "background_normal". At startup, this kept the screen blue between the window creation and about:home being shown. But since we no longer inflate the about:home view at startup, we end up exposing the LayerView during the period before about:home is shown.

The LayerView's background color is set to some default color when it's created (previously white), and once a tab is created, the color changes again to match the background of the tab (which was also white by default). So to prevent a white screen from appearing at startup before about:home is shown, we need to do two things: 1) change the default color drawn by the LayerView from white to background_normal, and 2) change the background color of newly created tabs from white to background_normal (but only for about: tabs). This part here takes care of the latter.

::: mobile/android/base/gfx/LayerView.java
@@ +182,5 @@
>          // is fully created to avoid flickering (see bug 801477).
>          if (shouldUseTextureView()) {
>              mTextureView = new TextureView(getContext());
>              mTextureView.setSurfaceTextureListener(new SurfaceTextureListener());
> +            mTextureView.setBackgroundResource(R.color.background_normal);

And this takes care of changing LayerView's default color.

::: mobile/android/base/resources/layout-xlarge-land-v11/abouthome_content.xml
@@ +5,5 @@
>  
> +<org.mozilla.gecko.widget.AboutHomeContentView
> +        xmlns:android="http://schemas.android.com/apk/res/android"
> +        xmlns:gecko="http://schemas.android.com/apk/res-auto"
> +		android:id="@+id/abouthome_content"

Oops...tabs somehow made it into this patch (I made this change before changing my Eclipse settings). I just converted these to spaces locally.

::: mobile/android/base/widget/AboutHomeContent.java
@@ +41,5 @@
> +    private ViewHolder mViewHolder;
> +    private int mPaddingLeft;
> +    private int mPaddingRight;
> +    private int mPaddingTop;
> +    private int mPaddingBottom;

We recreate AboutHomeContentView every time the fragment is shown, so I think we need to keep track of these values so we can restore the padding of the view. Without them,

@@ +83,5 @@
> +    public void onAttach(Activity activity) {
> +        super.onAttach(activity);
> +
> +        try {
> +            mUriLoadCallback = (UriLoadCallback) activity;

Having the activity implement the fragment interface for callbacks wasn't something I had initially considered, but this is the pattern suggested by http://developer.android.com/training/basics/fragments/communicating.html. And it's probably the best option; you aren't supposed to create custom constructors for fragments since the system may instantiate them on its own in certain situations.

@@ +151,5 @@
>      }
>  
> +    @Override
> +    public void onDestroyView() {
> +        mLightweightTheme.removeListener(mViewHolder.contentView);

We can't just do removeListener(getView()) here because the v4 Fragment implementation of getView() actually returns a NoSaveStateFrameLayout wrapper instead of the view itself, and we get a ClassCastException if we try to cast it to a LightweightTheme.OnChangeListener.

::: mobile/android/base/widget/AboutHomeContentView.java
@@ +7,5 @@
> +import android.content.Context;
> +import android.util.AttributeSet;
> +import android.widget.ScrollView;
> +
> +public class AboutHomeContentView extends ScrollView implements LightweightTheme.OnChangeListener {

I hate having to use a custom view for about:home rather than just a standard ScrollView, but personas themes weren't always appearing unless onLayout() was overridden. Sriram agreed that we had to do this, but I'd love to find a way to get rid of it!

Comment 19

[Brian Nicholson (:bnicholson)]

Test build here: https://dl.dropbox.com/u/35559547/fennec-abouthome-fragments.apk

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 734922 [details] [diff] [review]
Convert AboutHomeContent to a Fragment

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

Looking pretty good overall. Some aspects of the patch need more clarification though.

::: mobile/android/base/AwesomeBar.java
@@ +279,5 @@
> +     * Overriding onCreateView() here allows us to dispatch view creation to
> +     * both factories.
> +     */
> +    @Override
> +    public View onCreateView(String name, Context context, AttributeSet attrs) {

Not clear to me how this change in awesomebar is related to the main goal of this patch.

::: mobile/android/base/BrowserApp.java
@@ +207,5 @@
>                      loadFavicon(tab);
>                  }
>                  break;
> +            default:
> +                break;

How is this related?

@@ +467,5 @@
> +        // so we can't use Fragment#isVisible() to determine whether the
> +        // about:home is shown. Instead, we use Fragment#getUserVisibleHint()
> +        // with the hint we set ourselves.
> +        mAboutHomeContent = AboutHomeContent.newInstance();
> +        mAboutHomeContent.setUserVisibleHint(false);

Yep, that's the right thing to do ;-)

Just one important background bit here: the reason why you can "simply" create the fragment instance on BrowserApp's onCreate() is because we're handling configuration changes ourselves. This means the activity is not recreated on device rotation, for example. If we were recreating the activity on device rotation, you'd need to first check if you already had an about:home fragment instance in the FragmentManager.

@@ +1128,5 @@
>      }
>  
> +    private void attachAboutHome() {
> +        getSupportFragmentManager().beginTransaction()
> +                .add(R.id.gecko_layout, mAboutHomeContent).commitAllowingStateLoss();

What's the rationale for using commitAllowingStateLoss() here? commit() will only cause a crash if called after the user left the activity. Do you expect any case like that? This can potentially cause state loss on the about:home fragment, no?

Unless you have a good reason otherwise, I'd prefer us to start with commit() and see what kind of crashes/bugs we get. In many cases, those crashes will simply expose actual problems in our activity life cycle.

::: mobile/android/base/GeckoApp.java
@@ +1344,5 @@
> +            }
> +            if (profileName != null || profilePath != null) {
> +                mProfile = GeckoProfile.get(this, profileName, profilePath);
> +            }
> +        }

Not from your patch but maybe this code to initialize mProfile could be factored out to a separate method? Do it in a separate patch though.

@@ +1346,5 @@
> +                mProfile = GeckoProfile.get(this, profileName, profilePath);
> +            }
> +        }
> +
> +        BrowserDB.initialize(getProfile().getName());

Making BrowserDB a singleton wouldn't make much sense because the BrowserDB API is just a collection of static methods. There's no code interacting with a BrowserDB "instance" anywhere. Given that GeckoProfile.get() is potentially an IO blocking call, does this change affect startup time in any relevant way?

::: mobile/android/base/Tab.java
@@ +95,5 @@
>          mZoomConstraints = new ZoomConstraints(false);
>          mPluginViews = new ArrayList<View>();
>          mPluginLayers = new HashMap<Object, Layer>();
>          mState = shouldShowProgress(url) ? STATE_SUCCESS : STATE_LOADING;
> +        mBackgroundColor = getBackgroundColorForUrl(url);

It's probably worth adding the explanation of how the bg color is handled on startup as a comment somewhere. Maybe BrowserApp? LayerView?

@@ +553,5 @@
>          return "about:home".equals(url) || ReaderModeUtils.isAboutReader(url);
>      }
>  
> +    private int getBackgroundColorForUrl(String uri) {
> +        if (uri != null && uri.startsWith("about:")) {

Does the same apply to about:blank? I guess it doesn't.

@@ +555,5 @@
>  
> +    private int getBackgroundColorForUrl(String uri) {
> +        if (uri != null && uri.startsWith("about:")) {
> +            return mContext.getResources().getColor(R.color.background_normal);
> +        }

nit: add empty line here.

::: mobile/android/base/resources/layout-xlarge-land-v11/abouthome_content.xml
@@ +5,5 @@
>  
> +<org.mozilla.gecko.widget.AboutHomeContentView
> +        xmlns:android="http://schemas.android.com/apk/res/android"
> +        xmlns:gecko="http://schemas.android.com/apk/res-auto"
> +		android:id="@+id/abouthome_content"

That's unacceptable! ;-)

::: mobile/android/base/util/GeckoBackgroundThread.java
@@ +5,5 @@
>  package org.mozilla.gecko.util;
>  
>  import android.os.Handler;
>  import android.os.Looper;
> +import android.util.Log;

Unrelated?

::: mobile/android/base/widget/AboutHomeContent.java
@@ +26,2 @@
>  
> +public class AboutHomeContent extends Fragment {

Can we just rename this to AboutHome? The "Content" part has always bugged me. I would probably name it StartPage but I'm sure there will be people against it :-)

@@ +41,5 @@
> +    private ViewHolder mViewHolder;
> +    private int mPaddingLeft;
> +    private int mPaddingRight;
> +    private int mPaddingTop;
> +    private int mPaddingBottom;

I guess you forgot to complete the comment here :-)

In any case, this padding-saving bits look quite suspicious to me. I'll wait for a proper explanation before deciding what I think though.

@@ +46,3 @@
>  
>      public interface UriLoadCallback {
> +        public void onAboutHomeUriLoad(String uriSpec);

I tend to prefer calling these things listeners instead of callbacks. A callbacks makes me thing of functions, not interfaces.

@@ +49,3 @@
>      }
>  
> +    public interface LoadCompleteCallback {

Same here.

@@ +53,3 @@
>      }
>  
> +    static class ViewHolder {

Why is this ViewHolder necessary here? I mean, why not simply make each component a member of AboutHomeContent? Is it because it makes it simpler to check if the view is set on the fragment? You could simply use getView() on the fragment to check if you have an associated view. If it's non-null, it's probably safe to use mLastTabsSection, mAddonsSection, mRemoteTabsSection, etc. This way you'd be relying more on the platform behavior instead of handling the state ourselves.

Or maybe I'm missing something here?

@@ +83,5 @@
> +    public void onAttach(Activity activity) {
> +        super.onAttach(activity);
> +
> +        try {
> +            mUriLoadCallback = (UriLoadCallback) activity;

Looks like a sane approach for now. This is a case where a message bus would definitely help us decouple fragments from activities more cleanly btw.

@@ +114,5 @@
> +                R.layout.abouthome_content, container, false);
> +
> +        // Synchronized with onUpdate(), which runs in a background thread.
> +        synchronized (this) {
> +            mViewHolder = new ViewHolder(view);

I really don't like the fact that the views are expected to be accessed from a background thread here. The situation will hopefully improve once we move to a ViewPager backed by a FragmentPagerAdapter. i.e. each page will be a fragment too.

@@ +144,5 @@
>              @Override
>              public void onChange(boolean selfChange) {
>                  update(EnumSet.of(AboutHomeContent.UpdateFlags.REMOTE_TABS));
>              }
>          };

nit: add empty line here.

@@ +151,5 @@
>      }
>  
> +    @Override
> +    public void onDestroyView() {
> +        mLightweightTheme.removeListener(mViewHolder.contentView);

Nice catch.

@@ +169,5 @@
> +
> +        // Reattach the fragment, forcing a reinflation of its view. We don't
> +        // need to save anything in the state at this point, so it's safe to
> +        // call commitAllowingStateLoss() (which we must do to allow this to be
> +        // called after onSaveInstanceState()).

What are the situations where we may call commit() after the user has left the activity?

@@ +174,5 @@
> +        if (isVisible()) {
> +            getFragmentManager().beginTransaction()
> +                                .detach(this)
> +                                .attach(this)
> +                                .commitAllowingStateLoss();

This will cause not only the view to be recreated but the whole fragment state. i.e. it will probably requery database, etc. This is not a regression but it would be good think of ways we can retain more of the fragment state to avoid redundant work on device rotation.

Comment 22

[Brian Nicholson (:bnicholson)]

Comment on attachment 734922 [details] [diff] [review]
Convert AboutHomeContent to a Fragment

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

::: mobile/android/base/AwesomeBar.java
@@ +279,5 @@
> +     * Overriding onCreateView() here allows us to dispatch view creation to
> +     * both factories.
> +     */
> +    @Override
> +    public View onCreateView(String name, Context context, AttributeSet attrs) {

After making GeckoActivity extend FragmentActivity instead of Activity, we were crashing in onCreate() with our "LayoutInflater.from(...)" call. The reason is that you can only call setFactory() once, and FragmentActivity already calls setFactory() internally in onCreate().

Simply moving the "LayoutInflater.from(...)" call before "super.onCreate()" fixes this crash since FragmentActivity checks for the existence of a factory before setting it: http://androidxref.com/4.2.2_r1/xref/frameworks/support/v4/java/android/support/v4/app/FragmentActivity.java#195. But without it, any behavior relying on the fragment factory will be broken in AwesomeBar. This implementation of onCreateView() should allow us to use both our factory for our views along with the fragment factory for any fragment-related inflation.

Since AwesomeBar currently doesn't use fragments, we could probably remove this without causing any problems. But if we decide to add fragments to AwesomeBar in the future, we'd need to be aware of the fact that we're overriding FragmentActivity's factory.

::: mobile/android/base/BrowserApp.java
@@ +207,5 @@
>                      loadFavicon(tab);
>                  }
>                  break;
> +            default:
> +                break;

Just a warning reported by Eclipse for not handling all of the possible cases in the switch statement. I've been incrementally fixing these warnings as I touch these files, but I can drop it for now if you'd rather.

@@ +1128,5 @@
>      }
>  
> +    private void attachAboutHome() {
> +        getSupportFragmentManager().beginTransaction()
> +                .add(R.id.gecko_layout, mAboutHomeContent).commitAllowingStateLoss();

Oops, I meant to add comments here about this. But yeah, I've already hit crashes using commit() (which was what I wanted to use originally). From what I remember, this was one way to hit the crash:

1) Open about:home
2) Click the Android home button
3) Rotate the phone
4) Reopen Fennec

When we send Fennec to the background, we trigger onSaveInstanceState(). When we reopen Fennec, we hit the Activity's onConfigurationChanged() because of the rotation. Since onConfigurationChanged() is fired before onResume(), we get an IllegalStateException because we try to commit before resuming.

I don't like having to use commitAllowingStateLoss() either. It shouldn't affect us now since we aren't retaining any state, but I'd happily replace this with commit() if you can think of a way to avoid the crash.

::: mobile/android/base/GeckoApp.java
@@ +1346,5 @@
> +                mProfile = GeckoProfile.get(this, profileName, profilePath);
> +            }
> +        }
> +
> +        BrowserDB.initialize(getProfile().getName());

Right, but couldn't we just make the methods non-static if we were to convert it to a singleton? Not that I necessarily want to do this (I'm not a fan of singletons).

From my benchmarks, this takes about 30-40ms. I think that's a pretty small penalty to pay for guaranteed DB initialization correctness. This shouldn't affect reported startup performance at all since we call "onAboutHomeLoadComplete()" at the end of loadTopSites(), and by that point, the database needs to be initialized either way.

::: mobile/android/base/Tab.java
@@ +553,5 @@
>          return "about:home".equals(url) || ReaderModeUtils.isAboutReader(url);
>      }
>  
> +    private int getBackgroundColorForUrl(String uri) {
> +        if (uri != null && uri.startsWith("about:")) {

It does apply to about:firefox, about:downloads, about:addons, about:apps, and about:feedback, but it doesn't apply to some other about: pages such as about:blank, about:config, and about:support. I chose to do it for all about: pages since the majority of the user-facing about:pages are that color, but I can limit it to about:home if you'd prefer.

::: mobile/android/base/widget/AboutHomeContent.java
@@ +26,2 @@
>  
> +public class AboutHomeContent extends Fragment {

Renaming to AboutHome (fragment) and AboutHomeView (view). I never liked the Content part either.

@@ +41,5 @@
> +    private ViewHolder mViewHolder;
> +    private int mPaddingLeft;
> +    private int mPaddingRight;
> +    private int mPaddingTop;
> +    private int mPaddingBottom;

Err, let me try that again...

The dynamic toolbar was written in a way that assumes AboutHomeContent is always present, so it calls setPadding() without worrying about whether the view exists. We now recreate AboutHomeContentView every time the fragment is shown, and there are cases where we call setPadding() before onCreateView() has run (e.g., after leaving about:home and then clicking back). That means the view is null at the time, so we aren't getting the proper padding values when it's created, and the top of the view ends up being cut off. So the padding here is "pending padding", not unlike the pending thumbnails we need in bug 859584.

There's probably a way integrate the dynamic toolbar logic with the fragment/view callbacks so things happen in a more logical order, but I didn't feel like grokking the dynamic toolbar code and making this refactor even bigger.

@@ +53,3 @@
>      }
>  
> +    static class ViewHolder {

It's not necessary, but I felt it was slightly cleaner to encapsulate all of the view references/findViewById() calls into a container class. And in onDestroyView(), we can destroy all of the references at once by setting the ViewHolder to null instead of clearing the reference for each view individually.

I don't have a strong opinion about it though, so if you think it's better to drop the ViewHolder container and make these AboutHome members, I can do that.

On the other hand, we could go even further with the ViewHolder pattern and attach the ViewHolder to the view's tag like we do elsewhere. We wouldn't have to worry about setting any references to null in onDestroyView() for cleanup since the references would live with the view. It also gets rid of the weird coupling we have now between the references in the AboutHome fragment and the actual view state since accesses to all these views would go through getView(). What do you think?

@@ +114,5 @@
> +                R.layout.abouthome_content, container, false);
> +
> +        // Synchronized with onUpdate(), which runs in a background thread.
> +        synchronized (this) {
> +            mViewHolder = new ViewHolder(view);

I don't either. Ideally, I think everything in this class would happen on the UI thread, and each view would be responsible for handling any background operations on its own. I was debating making these changes here, but I didn't want to complicate this patch any more. BTW, this is the approach I was taking in bug 845542, so perhaps we can fix it there (though it will need to be completely rewritten after all of the recent refactoring).

@@ +169,5 @@
> +
> +        // Reattach the fragment, forcing a reinflation of its view. We don't
> +        // need to save anything in the state at this point, so it's safe to
> +        // call commitAllowingStateLoss() (which we must do to allow this to be
> +        // called after onSaveInstanceState()).

See above.

@@ +174,5 @@
> +        if (isVisible()) {
> +            getFragmentManager().beginTransaction()
> +                                .detach(this)
> +                                .attach(this)
> +                                .commitAllowingStateLoss();

Yeah...unfortunately, it's not an easy problem to fix since each of these expensive operations are local to each view; TopSitesView needs to requery for thumbnails, AddonsSection needs to read and parse the addons from the profile, and LastTabsSection needs to parse the session store file.

This is probably something we can address when we move each section to its own fragment. One solution would be for each view to save its state between being destroyed and recreated, and that's exactly what fragments offer.

Comment 23

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #22)
> Comment on attachment 734922 [details] [diff] [review]
> Convert AboutHomeContent to a Fragment
> 
> Review of attachment 734922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/AwesomeBar.java
> @@ +279,5 @@
> > +     * Overriding onCreateView() here allows us to dispatch view creation to
> > +     * both factories.
> > +     */
> > +    @Override
> > +    public View onCreateView(String name, Context context, AttributeSet attrs) {
> 
> After making GeckoActivity extend FragmentActivity instead of Activity, we
> were crashing in onCreate() with our "LayoutInflater.from(...)" call. The
> reason is that you can only call setFactory() once, and FragmentActivity
> already calls setFactory() internally in onCreate().
> 
> Simply moving the "LayoutInflater.from(...)" call before "super.onCreate()"
> fixes this crash since FragmentActivity checks for the existence of a
> factory before setting it:
> http://androidxref.com/4.2.2_r1/xref/frameworks/support/v4/java/android/
> support/v4/app/FragmentActivity.java#195. But without it, any behavior
> relying on the fragment factory will be broken in AwesomeBar. This
> implementation of onCreateView() should allow us to use both our factory for
> our views along with the fragment factory for any fragment-related inflation.
> 
> Since AwesomeBar currently doesn't use fragments, we could probably remove
> this without causing any problems. But if we decide to add fragments to
> AwesomeBar in the future, we'd need to be aware of the fact that we're
> overriding FragmentActivity's factory.

Ok, I'm fine with keeping it.

> ::: mobile/android/base/BrowserApp.java
> @@ +207,5 @@
> >                      loadFavicon(tab);
> >                  }
> >                  break;
> > +            default:
> > +                break;
> 
> Just a warning reported by Eclipse for not handling all of the possible
> cases in the switch statement. I've been incrementally fixing these warnings
> as I touch these files, but I can drop it for now if you'd rather.

Please, put this in a separate patch. As you've probably noticed, I'm very picky in terms of keeping patches focused on one thing only :-)

> @@ +1128,5 @@
> >      }
> >  
> > +    private void attachAboutHome() {
> > +        getSupportFragmentManager().beginTransaction()
> > +                .add(R.id.gecko_layout, mAboutHomeContent).commitAllowingStateLoss();
> 
> Oops, I meant to add comments here about this. But yeah, I've already hit
> crashes using commit() (which was what I wanted to use originally). From
> what I remember, this was one way to hit the crash:
> 
> 1) Open about:home
> 2) Click the Android home button
> 3) Rotate the phone
> 4) Reopen Fennec
> 
> When we send Fennec to the background, we trigger onSaveInstanceState().
> When we reopen Fennec, we hit the Activity's onConfigurationChanged()
> because of the rotation. Since onConfigurationChanged() is fired before
> onResume(), we get an IllegalStateException because we try to commit before
> resuming.

Yeah, that's the problem with not letting the platform handle configuration changes for us. It would be great if we could keep our BrowserApp handling its configuration changes but still be able have fragments that have their config changes handles by the platform. Have you read the support library's source code to know that's possible to do?

> I don't like having to use commitAllowingStateLoss() either. It shouldn't
> affect us now since we aren't retaining any state, but I'd happily replace
> this with commit() if you can think of a way to avoid the crash.
> 
> ::: mobile/android/base/GeckoApp.java
> @@ +1346,5 @@
> > +                mProfile = GeckoProfile.get(this, profileName, profilePath);
> > +            }
> > +        }
> > +
> > +        BrowserDB.initialize(getProfile().getName());
> 
> Right, but couldn't we just make the methods non-static if we were to
> convert it to a singleton? Not that I necessarily want to do this (I'm not a
> fan of singletons).
> 
> From my benchmarks, this takes about 30-40ms. I think that's a pretty small
> penalty to pay for guaranteed DB initialization correctness. This shouldn't
> affect reported startup performance at all since we call
> "onAboutHomeLoadComplete()" at the end of loadTopSites(), and by that point,
> the database needs to be initialized either way.

Ok.

> ::: mobile/android/base/Tab.java
> @@ +553,5 @@
> >          return "about:home".equals(url) || ReaderModeUtils.isAboutReader(url);
> >      }
> >  
> > +    private int getBackgroundColorForUrl(String uri) {
> > +        if (uri != null && uri.startsWith("about:")) {
> 
> It does apply to about:firefox, about:downloads, about:addons, about:apps,
> and about:feedback, but it doesn't apply to some other about: pages such as
> about:blank, about:config, and about:support. I chose to do it for all
> about: pages since the majority of the user-facing about:pages are that
> color, but I can limit it to about:home if you'd prefer.

Yeah, I'd start with about:home here which is the specific case you want to handle here. Only applies for the startup case, right?

> ::: mobile/android/base/widget/AboutHomeContent.java
> @@ +26,2 @@
> >  
> > +public class AboutHomeContent extends Fragment {
> 
> Renaming to AboutHome (fragment) and AboutHomeView (view). I never liked the
> Content part either.

\o/

> @@ +41,5 @@
> > +    private ViewHolder mViewHolder;
> > +    private int mPaddingLeft;
> > +    private int mPaddingRight;
> > +    private int mPaddingTop;
> > +    private int mPaddingBottom;
> 
> Err, let me try that again...
> 
> The dynamic toolbar was written in a way that assumes AboutHomeContent is
> always present, so it calls setPadding() without worrying about whether the
> view exists. We now recreate AboutHomeContentView every time the fragment is
> shown, and there are cases where we call setPadding() before onCreateView()
> has run (e.g., after leaving about:home and then clicking back). That means
> the view is null at the time, so we aren't getting the proper padding values
> when it's created, and the top of the view ends up being cut off. So the
> padding here is "pending padding", not unlike the pending thumbnails we need
> in bug 859584.
> 
> There's probably a way integrate the dynamic toolbar logic with the
> fragment/view callbacks so things happen in a more logical order, but I
> didn't feel like grokking the dynamic toolbar code and making this refactor
> even bigger.

Ok, I'd rename the padding bits (class members and methods) to something less generic-looking then.

> @@ +53,3 @@
> >      }
> >  
> > +    static class ViewHolder {
> 
> It's not necessary, but I felt it was slightly cleaner to encapsulate all of
> the view references/findViewById() calls into a container class. And in
> onDestroyView(), we can destroy all of the references at once by setting the
> ViewHolder to null instead of clearing the reference for each view
> individually.
> 
> I don't have a strong opinion about it though, so if you think it's better
> to drop the ViewHolder container and make these AboutHome members, I can do
> that.
> 
> On the other hand, we could go even further with the ViewHolder pattern and
> attach the ViewHolder to the view's tag like we do elsewhere. We wouldn't
> have to worry about setting any references to null in onDestroyView() for
> cleanup since the references would live with the view. It also gets rid of
> the weird coupling we have now between the references in the AboutHome
> fragment and the actual view state since accesses to all these views would
> go through getView(). What do you think?

The view holder pattern is meant to be used in the context of recycling views where there might be tons of findViewById() calls while scrolling. Using it here seems a bit overkill. I'd rather start with something simpler. So, let's make them AboutHome members for now.

> @@ +114,5 @@
> > +                R.layout.abouthome_content, container, false);
> > +
> > +        // Synchronized with onUpdate(), which runs in a background thread.
> > +        synchronized (this) {
> > +            mViewHolder = new ViewHolder(view);
> 
> I don't either. Ideally, I think everything in this class would happen on
> the UI thread, and each view would be responsible for handling any
> background operations on its own. I was debating making these changes here,
> but I didn't want to complicate this patch any more. BTW, this is the
> approach I was taking in bug 845542, so perhaps we can fix it there (though
> it will need to be completely rewritten after all of the recent refactoring).

I guess it wouldn't be too hard to make loadTopSites(), readLastTabs(), readRecommendedAddons(), loadRemoteTabs() to run their code inside a ThreadUtils.postToBackgroundThread(). I'd prefer this to be fixed before landing. You can submit it as a separate patch in this bug (as it's a self-contained change anyway).

> @@ +169,5 @@
> > +
> > +        // Reattach the fragment, forcing a reinflation of its view. We don't
> > +        // need to save anything in the state at this point, so it's safe to
> > +        // call commitAllowingStateLoss() (which we must do to allow this to be
> > +        // called after onSaveInstanceState()).
> 
> See above.

Maybe file a bug just so we can revisit that later?

> @@ +174,5 @@
> > +        if (isVisible()) {
> > +            getFragmentManager().beginTransaction()
> > +                                .detach(this)
> > +                                .attach(this)
> > +                                .commitAllowingStateLoss();
> 
> Yeah...unfortunately, it's not an easy problem to fix since each of these
> expensive operations are local to each view; TopSitesView needs to requery
> for thumbnails, AddonsSection needs to read and parse the addons from the
> profile, and LastTabsSection needs to parse the session store file.
> 
> This is probably something we can address when we move each section to its
> own fragment. One solution would be for each view to save its state between
> being destroyed and recreated, and that's exactly what fragments offer.

True. I'm fine with landing this for now as it doesn't really cause a regression (this is how it behaves now, right?).

Comment 24

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #23)
> Yeah, that's the problem with not letting the platform handle configuration
> changes for us. It would be great if we could keep our BrowserApp handling
> its configuration changes but still be able have fragments that have their
> config changes handles by the platform. Have you read the support library's
> source code to know that's possible to do?

I don't think Fragments have any special configuration changes handled by the platform. When an Activity is destroyed/recreated in onConfigurationChanged(), its Fragments are automatically destroyed and recreated too since they're children of the Activity's layout.


> True. I'm fine with landing this for now as it doesn't really cause a
> regression (this is how it behaves now, right?).

Right -- in fact, we reinflate a whole lot more right now since we do all of this with *every* onResume, including coming back from the AwesomeScreen (a frequent operation). And that's even when about:home isn't visible!

Comment 25

[Brian Nicholson (:bnicholson)]

Also, try results look good: https://tbpl.mozilla.org/?tree=Try&rev=40dd439852df

Comment 26

[Brian Nicholson (:bnicholson)]

Attachment: Convert AboutHomeContent to a Fragment, v2

Comment 27

[Brian Nicholson (:bnicholson)]

Attachment: Move AboutHome update to foreground thread

This simply wraps the update methods in their own postToBackgroundThread() calls. These should be changed to use AsyncTasks to reduce the thread nesting, but since that requires more refactoring work, we can handle that in bug 845542.

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 737687 [details] [diff] [review]
Convert AboutHomeContent to a Fragment, v2

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

Looks good with the suggested fixes.

::: mobile/android/base/BrowserApp.java
@@ +667,4 @@
>              if (!isExternalURL) {
>                  // show about:home if we aren't restoring previous session
>                  if (mRestoreMode == RESTORE_NONE) {
> +                    Tabs.getInstance().loadUrl("about:home", Tabs.LOADURL_NEW_TAB);

Unrelated fix?

@@ +700,4 @@
>                  // When the dynamic toolbar is enabled, set the padding on the
>                  // about:home widget directly - this is to avoid resizing the
>                  // LayerView, which can cause visible artifacts.
> +                mAboutHome.setPadding(0, aVisibleHeight, 0, 0);

Do you actually need this generic-looking padding setter when you always just set the top padding? I'd prefer a method name that describes its intent more clearly. setToolbarOffset? Then internally you use this value as view padding like you're doing now. I'm ok with landing this as is, just something to consider.

@@ +1184,4 @@
>  
>      /* About:home UI */
>      void updateAboutHomeTopSites() {
> +        if (mAboutHome == null)

Do you still need this null check given that you now create the fragment in onCreate?

::: mobile/android/base/widget/AddonsSection.java
@@ +40,4 @@
>  
>      private Context mContext;
>      private BrowserApp mActivity;
> +    private AboutHome.UriLoadListener mUriLoadCallback = null;

mUriLoadCallback -> mUriLoadListener

::: mobile/android/base/widget/TopSitesView.java
@@ +66,4 @@
>  
>      private Context mContext;
>      private BrowserApp mActivity;
> +    private AboutHome.UriLoadListener mUriLoadCallback = null;

mUriLoadCallback -> mUriLoadListener

@@ +66,5 @@
>  
>      private Context mContext;
>      private BrowserApp mActivity;
> +    private AboutHome.UriLoadListener mUriLoadCallback = null;
> +    private AboutHome.LoadCompleteListener mLoadCompleteCallback = null;

mLoadcompleteCallback -> mLoadCompleteListener

@@ +359,3 @@
>      }
>  
> +    public void setLoadCompleteListener(AboutHome.LoadCompleteListener callback) {

callback -> loadCompleteListener

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 737688 [details] [diff] [review]
Move AboutHome update to foreground thread

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

Nice.

::: mobile/android/base/widget/LastTabsSection.java
@@ +33,4 @@
>      }
>  
>      public void readLastTabs() {
> +        ThreadUtils.postToBackgroundThread(new Runnable() {

Wow, this is a rather gigantic method. Definitely needs some refactoring there. On a follow-up bug, of course.

Comment 30

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #28)
> Comment on attachment 737687 [details] [diff] [review]
> Convert AboutHomeContent to a Fragment, v2
> 
> Do you actually need this generic-looking padding setter when you always
> just set the top padding? I'd prefer a method name that describes its intent
> more clearly. setToolbarOffset? Then internally you use this value as view
> padding like you're doing now. I'm ok with landing this as is, just
> something to consider.

I'm not sure the method name should mention the toolbar. As a whole, about:home is largely decoupled from the rest of the views, so it seems weird to add a method about the browser toolbar when about:home doesn't have anything to do with the toolbar. Even though the reason we're setting the padding here is for the dynamic toolbar, we are still doing only that -- setting the padding. I did add a comment in case someone want to know why we're doing it here, though.

You're right that we don't need the other padding values. This was probably a case of me being over-generic, and I could've gotten away with just having a setTopPadding() or something similar so we only need to keep track of one.

> 
> @@ +1184,4 @@
> >  
> >      /* About:home UI */
> >      void updateAboutHomeTopSites() {
> > +        if (mAboutHome == null)
> 
> Do you still need this null check given that you now create the fragment in
> onCreate?

Nope, thanks.


https://hg.mozilla.org/integration/mozilla-inbound/rev/cacd9189f531
https://hg.mozilla.org/integration/mozilla-inbound/rev/843223330b5e

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cacd9189f531
https://hg.mozilla.org/mozilla-central/rev/843223330b5e

Comment 32

[Brian Nicholson (:bnicholson)]

Comment on attachment 737687 [details] [diff] [review]
Convert AboutHomeContent to a Fragment, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: greater memory usage, and bug 857459 won't be fixed without a separate patch
Testing completed (on m-c, etc.): m-c several weeks
Risk to taking this patch (and alternatives if risky): Fairly low; this bug introduced one bad regression (bug 863803), but it has been fixed.
String or IDL/UUID changes made by this patch: none

If approved, needs to be uplifted with bug 863803.

Comment 33

[Brian Nicholson (:bnicholson)]

Comment on attachment 737688 [details] [diff] [review]
Move AboutHome update to foreground thread

[Approval Request Comment]
See above.

Comment 34

[Aaron Train [:aaronmt]]

QA Note: This will miss Firefox 22 Beta 1

Comment 35

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-beta/rev/64ef2257900a
https://hg.mozilla.org/releases/mozilla-beta/rev/49e34d4e8e8a