Closed Bug 907592 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: shilpanbhagat, Assigned: shilpanbhagat)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

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.
Here a rough patch doing that. There are no major visible delays. And we save ~30ms in certain situations :)
Attachment #793359 - Flags: review?(lucasr.at.mozilla)
Blocks: 906952
Assignee: nobody → sbhagat
Oops, missed out on a small detail which caused a regression. This should fix it.
Attachment #793359 - Attachment is obsolete: true
Attachment #793359 - Flags: review?(lucasr.at.mozilla)
Attachment #793393 - Flags: review?(lucasr.at.mozilla)
Ah, wrong description title, I should sleep :|
Attachment #793393 - Attachment is obsolete: true
Attachment #793393 - Flags: review?(lucasr.at.mozilla)
Attachment #793395 - Flags: review?(lucasr.at.mozilla)
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
Attached patch Patch: Viewstub for HomePager (obsolete) — Splinter Review
With changes mentioned above
Attachment #793395 - Attachment is obsolete: true
Attachment #793395 - Flags: review?(lucasr.at.mozilla)
Attachment #793615 - Flags: review?(lucasr.at.mozilla)
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();
Attachment #793615 - Flags: review?(lucasr.at.mozilla) → review-
Attached patch Patch: Viewstub for HomePager (obsolete) — Splinter Review
Made changes as mentioned above. Removed mHomePagerViewStub declaration completely.
Attachment #793615 - Attachment is obsolete: true
Attachment #795640 - Flags: review?(lucasr.at.mozilla)
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();
Attachment #795640 - Flags: review?(lucasr.at.mozilla) → review+
Attached patch Patch: Viewstub for HomePager (obsolete) — Splinter Review
Carrying forward r+ with mentioned changes
Attachment #795640 - Attachment is obsolete: true
Attachment #796526 - Flags: review+
Keywords: checkin-needed
Carrying forward r+
Attachment #796526 - Attachment is obsolete: true
Attachment #796527 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fb3a057c5227
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.