909550 - Lazy load tabs panel

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Shilpan Bhagat]

Currently we take about ~100 to 120 ms loading tabs panel on start. This patch makes tabs panel load lazily. Which means gecko gets started faster, and hence faster everything :)

Applying the patch does not show any visible lag in the user experience.

Comment 1

[Shilpan Bhagat]

Attachment: Patch: Lazy load tabs panel

Sorry about the mix up. This is the patch and it does as described above.

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 795704 [details] [diff] [review]
Patch: Lazy load tabs panel

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

Make sure this doesn't break anything on tablets i.e. the updateSideBarState() bit. Before pushing, could you please post a test build? Just curious about the impact of this change on the framerate of the first tabs panel animation after startup. Also, post a new patch with the nits fixed.

::: mobile/android/base/BrowserApp.java
@@ +1185,5 @@
>          if (Tabs.getInstance().getDisplayCount() == 0)
>              return;
>  
> +        if (mTabsPanel == null) {
> +            // Set up tabs panel.

This comment to be a bit more useful: "Inflate stubbed tabs panel for showing" or something.

@@ +1186,5 @@
>              return;
>  
> +        if (mTabsPanel == null) {
> +            // Set up tabs panel.
> +            mTabsPanel = (TabsPanel) ((ViewStub) findViewById(R.id.tabs_panel)).inflate();

Just like the other patch to stub homepager, please avoid these embedded castings. They are pretty hard to read.

@@ +1189,5 @@
> +            // Set up tabs panel.
> +            mTabsPanel = (TabsPanel) ((ViewStub) findViewById(R.id.tabs_panel)).inflate();
> +            mTabsPanel.setTabsLayoutChangeListener(this);
> +            updateSideBarState();
> +        }

nit: add empty line here.

@@ +1209,5 @@
>      }
>  
>      @Override
>      public boolean areTabsShown() {
> +        return mTabsPanel != null && mTabsPanel.isShown();

nit: Enclose the expression with parens.

Comment 3

[Shilpan Bhagat]

Attachment: Patch: Lazy load tabs panel

Hmmm, you were right about tablets it seems we try to show the tabs panel before layout which leads to weird behaviour in certain cases. This patch does take care of that. 

Here is the build. It looks like it lags a bit. https://dl.dropboxusercontent.com/u/11916346/mozilla/lazyloadtabspanel.apk

If you think the same, I can try something else (like loading on gecko ready).

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 796985 [details] [diff] [review]
Patch: Lazy load tabs panel

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

This looks generally good. Still not sure about the need to wait for a global layout before showing tabs panel. There's an extra delay when you first open the tabs panel. It's not hugely noticeable but it's there. I'd like to at least add a trigger to inflate the tabs panel after gecko is loaded. But we still need to handle the case where the user opens the tabs panel before gecko is up and running.

::: mobile/android/base/BrowserApp.java
@@ +1191,5 @@
>              return;
>  
> +        if (mTabsPanel == null) {
> +            // Inflate stubbed tabs panel.
> +            ViewStub tabsPanelStub = (ViewStub) findViewById(R.id.tabs_panel);

final

@@ +1192,5 @@
>  
> +        if (mTabsPanel == null) {
> +            // Inflate stubbed tabs panel.
> +            ViewStub tabsPanelStub = (ViewStub) findViewById(R.id.tabs_panel);
> +            mTabsPanel = (TabsPanel) (tabsPanelStub).inflate();

No need for parens around tabsPanelStub

@@ +1193,5 @@
> +        if (mTabsPanel == null) {
> +            // Inflate stubbed tabs panel.
> +            ViewStub tabsPanelStub = (ViewStub) findViewById(R.id.tabs_panel);
> +            mTabsPanel = (TabsPanel) (tabsPanelStub).inflate();
> +            mTabsPanel.setTabsLayoutChangeListener(this);

nit: add empty line here.

@@ +1200,5 @@
> +            ViewTreeObserver vto = mTabsPanel.getViewTreeObserver();
> +            if (vto.isAlive()) {
> +                vto.addOnGlobalLayoutListener(new ViewTreeObserver.OnGlobalLayoutListener() {
> +                    @Override
> +                    public void onGlobalLayout() {

Why do you need to wait for a global layout round before showing tabs panel?

Comment 6

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

It would be nice to get someone to look into continuing this work.

Comment 7

[Richard Newman [:rnewman]]

Sounds like a job for Lucas! :D

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Lazy-inflate TabsPanel once the first pageload is finished (r=mfinkle)

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8370813 [details] [diff] [review]
Lazy-inflate TabsPanel once the first pageload is finished (r=mfinkle)

Only inflate the tabs panel after the initial pageload is done or if the user manually opens it before that.

Comment 10

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

Comment on attachment 8370813 [details] [diff] [review]
Lazy-inflate TabsPanel once the first pageload is finished (r=mfinkle)

Looks good. Wes is adding a Gecko:DelayedStartup handler in one of his r+'d patches too. You might get conflicts if he gets there first.

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: Lazy-inflate TabsPanel once the first pageload is finished (r=mfinkle)

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8370878 [details] [diff] [review]
Lazy-inflate TabsPanel once the first pageload is finished (r=mfinkle)

Found a bug on tablets when we try to show tabs just after inflating while in landscape orientation (i.e. sidebar mode). We need to wait for a global layout before showing the view.

Comment 13

[Lucas Rocha (:lucasr)]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

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

Worth uplifting to Aurora?

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8370878 [details] [diff] [review]
Lazy-inflate TabsPanel once the first pageload is finished (r=mfinkle)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Improved startup time
Testing completed (on m-c, etc.): Landed in m-c, no issues found. Improved at one of our talos tests by 2.29%.
Risk to taking this patch (and alternatives if risky): Low risk, just forces a (big) part of the UI to only inflate after the initial pageload. 
String or IDL/UUID changes made by this patch: n/a

Comment 17

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01b6fb4e6ad9