Closed Bug 909550 Opened 6 years ago Closed 6 years ago

Lazy load tabs panel

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: shilpanbhagat, Assigned: lucasr)

References

(Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → sbhagat
Blocks: 906952
Attached patch Patch: Lazy load tabs panel (obsolete) — Splinter Review
Sorry about the mix up. This is the patch and it does as described above.
Attachment #795704 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #795704 - Flags: review?(lucasr.at.mozilla) → review+
Attached patch Patch: Lazy load tabs panel (obsolete) — Splinter Review
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).
Attachment #795704 - Attachment is obsolete: true
Attachment #796985 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #796985 - Flags: review?(lucasr.at.mozilla) → review-
Duplicate of this bug: 959634
It would be nice to get someone to look into continuing this work.
Sounds like a job for Lucas! :D
Assignee: shilpanbhagat → lucasr.at.mozilla
Status: NEW → ASSIGNED
Keywords: perf
OS: Mac OS X → Android
Hardware: x86 → All
Version: unspecified → Trunk
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.
Attachment #8370813 - Flags: review?(mark.finkle)
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.
Attachment #8370813 - Flags: review?(mark.finkle) → review+
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.
Attachment #8370878 - Flags: review?(mark.finkle)
Attachment #796985 - Attachment is obsolete: true
Attachment #8370813 - Attachment is obsolete: true
Attachment #8370878 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/f8ad3611031b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Worth uplifting to Aurora?
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
Attachment #8370878 - Flags: approval-mozilla-aurora?
Attachment #8370878 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.