Don't animate tab counter during session restore

VERIFIED FIXED in Firefox 11

Status

()

defect
P2
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: dietrich, Assigned: bnicholson)

Tracking

unspecified
Firefox 13
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 verified, firefox12 verified, firefox13 verified, fennec11+)

Details

(Whiteboard: startup-ux)

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

8 years ago
expected: if i have 3 tabs at startup, the initial UI should load with the tab button's value as "3", and the button should not change at all.

actual: if i have 3 tabs at startup, the initual UI loads and the tab button's value is "+". then, a few moments later it starts animatedly changing to numbers and finally ending up at "3".

it feels quite slow - the browser is visibly chunking it's way through it's startup stuff.

not animating the button, and preloading it's value with the correct number of tabs would resolve the problem.

(the browser might not actually be ready yet - but that's another problem)
This sounds like bug 706835, which should be in today's Nightly.
Reporter

Comment 2

8 years ago
The page, in this case, was about:firefox.
Actually, I think this is more of a problem with session restore being slow. I don't know the details of how that's implemented, but I imagine we don't have that tab count number until gecko starts up. Brian, is that the case?

Bug 706835 fixed the case where we were animating from 0->1 tabs, but it won't fix this problem. I guess we could do something to suppress the animations until session restore is done, but we'd still see the "3" appear late.
Blocks: 721008
Whiteboard: startup-ux
Yeah - we don't want to see the number animating up from 1 -- it should come in, static, at the number of tabs the user thinks he/she has.
Assignee

Comment 5

8 years ago
(In reply to Margaret Leibovic [:margaret] from comment #3)
> Actually, I think this is more of a problem with session restore being slow.
> I don't know the details of how that's implemented, but I imagine we don't
> have that tab count number until gecko starts up. Brian, is that the case?

The session store works by reading JSON containing an array of all previous tabs. For each tab, we call BrowserApp.addTab, which will fire a series of individual tab add messages to GeckoApp. We could improve this by batching the set of tabs in a single message and incrementing the tab counter only once.
We could send a Session:RestoreBegin / Session:RestoreEnd to Java and use that as a hint and Java can pause doing some UI changes.
Assignee

Updated

8 years ago
Assignee: nobody → bnicholson
The delay comes from the fact that we wait for Gecko to load to then restore tabs. Ideally, the Java side would immediately stub all restored tabs by directly reading the sessionstore file. Gecko would then "revive" the tabs once they're selected. This way we wouldn't need to wait for gecko to load to start restoring tabs.
tracking-fennec: --- → 11+
Priority: -- → P2
Assignee

Updated

8 years ago
Summary: perceptible delay between initial UI load and session being restored → Don't animate tab counter during session restore
Assignee

Comment 8

8 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #592885 - Flags: review?(mark.finkle)
Comment on attachment 592885 [details] [diff] [review]
patch

This looks fine, but can we refactor the updateTabs and updateTabsAndAnimate as we discussed on IRC?
Attachment #592885 - Flags: review?(mark.finkle) → review-
(In reply to Brian Nicholson (:bnicholson) from comment #5)
> (In reply to Margaret Leibovic [:margaret] from comment #3)
> > Actually, I think this is more of a problem with session restore being slow.
> > I don't know the details of how that's implemented, but I imagine we don't
> > have that tab count number until gecko starts up. Brian, is that the case?
> 
> The session store works by reading JSON containing an array of all previous
> tabs. For each tab, we call BrowserApp.addTab, which will fire a series of
> individual tab add messages to GeckoApp. We could improve this by batching
> the set of tabs in a single message and incrementing the tab counter only
> once.

Given that this bug is only about the tab counter animation now, I filed bug 722661 to track the solution proposed above.
OS: Mac OS X → Android
Hardware: x86 → All
Assignee

Comment 11

8 years ago
Posted patch patch v2Splinter Review
renamed methods
Attachment #592885 - Attachment is obsolete: true
Attachment #593218 - Flags: review?(mark.finkle)
Comment on attachment 593218 [details] [diff] [review]
patch v2


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>       // Be ready to handle any restore failures by making sure we have a valid tab opened
>       let restoreCleanup = {
>         observe: function(aSubject, aTopic, aData) {
>           Services.obs.removeObserver(restoreCleanup, "sessionstore-windows-restored");
>           if (aData == "fail") {
>             let params = { selected: restoreToFront };
>             BrowserApp.addTab("about:home", { showProgress: false });
>           }
>+          sendMessageToJava({
>+            gecko: {
>+              type: "Session:RestoreEnd"
>+            }
>+          });

* Add a blank line before the sendMessageToJava call
* "params" is not used in code above what you added. I assume we want to do:
              BrowserApp.addTab("about:home", { selected: restoreToFront, showProgress: false });

Can you open a bug on this and CC margaret on it too?
Attachment #593218 - Flags: review?(mark.finkle) → review+
Assignee

Comment 14

8 years ago
(In reply to Mark Finkle (:mfinkle) from comment #12)
> * "params" is not used in code above what you added. I assume we want to do:
>               BrowserApp.addTab("about:home", { selected: restoreToFront,
> showProgress: false });
> 
> Can you open a bug on this and CC margaret on it too?

Filed bug 723349.
Assignee

Comment 15

8 years ago
Comment on attachment 593218 [details] [diff] [review]
patch v2

[Approval Request Comment]
Removes tab counter animation during session restore. Low risk.
Attachment #593218 - Flags: approval-mozilla-beta?
Attachment #593218 - Flags: approval-mozilla-aurora?
Comment on attachment 593218 [details] [diff] [review]
patch v2

[Triage Comment]
Mobile only - approved for Aurora 12 (if necessary) and Beta 11.
Attachment #593218 - Flags: approval-mozilla-beta?
Attachment #593218 - Flags: approval-mozilla-beta+
Attachment #593218 - Flags: approval-mozilla-aurora?
Attachment #593218 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/9588a4623665
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Verified fixed on:

Firefox 13.0a1 (2012-02-07)
20120207031136
http://hg.mozilla.org/mozilla-central/rev/b077059c575a
Device: HTC Desire Z
OS: Android 2.3.3

Firefox 12.0a2 (2012-02-07)
20120207042017
http://hg.mozilla.org/releases/mozilla-aurora/rev/cd3f31d128b8
Device: HTC Desire Z
OS: Android 2.3.3

Firefox 11.0
20120206202409
http://hg.mozilla.org/releases/mozilla-beta/rev/1c0aba74d116
Device: HTC Desire Z
OS: Android 2.3.3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.