907592 - Lazy load View Pager when it's not shown on startup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Shilpan Bhagat]

I did some profiling and we spend around ~28ms inflating ViewPager and HomePagerStrip even when about:home is not loaded initially. This loading should be made smarter so that ViewPager loads only when needed.

Comment 1

[Shilpan Bhagat]

Attachment: [WIP] Using viewstubs for Home Pager

Here a rough patch doing that. There are no major visible delays. And we save ~30ms in certain situations :)

Comment 2

[Shilpan Bhagat]

Attachment: [WIP] Replacing the use of INIParsers with SharedPreferences

Oops, missed out on a small detail which caused a regression. This should fix it.

Comment 3

[Shilpan Bhagat]

Attachment: [WIP] Using viewstubs for Home Pager

Ah, wrong description title, I should sleep :|

Comment 4

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

Comment on attachment 793395 [details] [diff] [review]
[WIP] Using viewstubs for Home Pager

Drive by

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

>-                        if (isDynamicToolbarEnabled() && !mHomePager.isVisible()) {
>+                        if (isDynamicToolbarEnabled() && !(mHomePager != null && mHomePager.isVisible())) {

You use | mHomePager != null && mHomePager.isVisible() | so much, I think you should make a helper function | isHomePagerVisible() |

>         ((GeckoApp.MainLayout) mMainLayout).setMotionEventInterceptor(new MotionEventInterceptor() {
>             @Override
>             public boolean onInterceptMotionEvent(View view, MotionEvent event) {
>                 // If we get a gamepad panning MotionEvent while the focus is not on the layerview,
>                 // put the focus on the layerview and carry on
>                 if (mLayerView != null && !mLayerView.hasFocus() && GamepadUtils.isPanningControl(event)) {
>-                    if (mHomePager.isVisible()) {
>+                    if (mHomePager != null && mHomePager.isVisible()) {
>                         mLayerView.requestFocus();
>-                    } else {
>+                    } else if (mHomePager != null) {
>                         mHomePager.requestFocus();
>                     }
>                 }
>                 return false;
>             }
>         });

In this method nothing can be done if mHomePager is null, so just check for it right away and return early

Comment 5

[Shilpan Bhagat]

Attachment: Patch: Viewstub for HomePager

With changes mentioned above

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 793615 [details] [diff] [review]
Patch: Viewstub for HomePager

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

Nice. Just needs some extra cleanups.

::: mobile/android/base/BrowserApp.java
@@ +441,5 @@
>                  return false;
>              }
>          });
>  
> +        mHomePagerStub = (ViewStub) findViewById(R.id.home_pager);

You don't really need mHomePagerStub. Simply findViewById(R.id.home_pager) when you inflate it below.

@@ +1336,5 @@
>          mBrowserToolbar.cancelEdit();
>      }
>  
> +    private boolean isHomePagerVisible() {
> +        return mHomePager != null && mHomePager.isVisible();

nit: add ()'s around the expression.

@@ +1472,5 @@
>          if (isDynamicToolbarEnabled() && mLayerView != null) {
>              mLayerView.getLayerMarginsAnimator().showMargins(true);
>          }
>  
> +        if (mHomePager == null) {

final ViewStub homePagerStub = (ViewStub) findViewById(R.id.home_pager);

@@ +1473,5 @@
>              mLayerView.getLayerMarginsAnimator().showMargins(true);
>          }
>  
> +        if (mHomePager == null) {
> +            mHomePager = (HomePager) mHomePagerStub.inflate();

mHomePager = (HomePager) homePagerStub.inflate();

Comment 7

[Shilpan Bhagat]

Attachment: Patch: Viewstub for HomePager

Made changes as mentioned above. Removed mHomePagerViewStub declaration completely.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 795640 [details] [diff] [review]
Patch: Viewstub for HomePager

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

Looks good with the suggested change. Please post a new patch.

::: mobile/android/base/BrowserApp.java
@@ +1471,5 @@
>              mLayerView.getLayerMarginsAnimator().showMargins(true);
>          }
>  
> +        if (mHomePager == null) {
> +            mHomePager = (HomePager) ((ViewStub) findViewById(R.id.home_pager)).inflate();

Please, don't do these embedded castings. They might make code a bit shorter but they are pretty hard to read. Simply do:

final ViewStub homePagerStub = (ViewStub) findViewById(R.id.home_pager);
mHomePager = homepPagerStub.inflate();

Comment 9

[Shilpan Bhagat]

Attachment: Patch: Viewstub for HomePager

Carrying forward r+ with mentioned changes

Comment 10

[Shilpan Bhagat]

Attachment: Patch: Viewstub for HomePager

Carrying forward r+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb3a057c5227