Closed Bug 719479 Opened 12 years ago Closed 12 years ago

Don't animate tab counter during session restore

Categories

(Firefox for Android Graveyard :: General, defect, P2)

All
Android
defect

Tracking

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

VERIFIED FIXED
Firefox 13
Tracking Status
firefox11 --- verified
firefox12 --- verified
firefox13 --- verified
fennec 11+ ---

People

(Reporter: dietrich, Assigned: bnicholson)

References

Details

(Whiteboard: startup-ux)

Attachments

(1 file, 1 obsolete file)

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.
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.
(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: 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
Summary: perceptible delay between initial UI load and session being restored → Don't animate tab counter during session restore
Attached 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
Attached 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+
(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.
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: 12 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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: