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)
Tracking
(firefox11 verified, firefox12 verified, firefox13 verified, fennec11+)
VERIFIED
FIXED
Firefox 13
People
(Reporter: dietrich, Assigned: bnicholson)
References
Details
(Whiteboard: startup-ux)
Attachments
(1 file, 1 obsolete file)
13.28 KB,
patch
|
mfinkle
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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)
Comment 1•12 years ago
|
||
This sounds like bug 706835, which should be in today's Nightly.
Reporter | ||
Comment 2•12 years ago
|
||
The page, in this case, was about:firefox.
Comment 3•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: startup-ux
Comment 4•12 years ago
|
||
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•12 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.
Comment 6•12 years ago
|
||
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•12 years ago
|
Assignee: nobody → bnicholson
Comment 7•12 years ago
|
||
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.
Updated•12 years ago
|
tracking-fennec: --- → 11+
Priority: -- → P2
Assignee | ||
Updated•12 years ago
|
Summary: perceptible delay between initial UI load and session being restored → Don't animate tab counter during session restore
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #592885 -
Flags: review?(mark.finkle)
Comment 9•12 years ago
|
||
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-
Comment 10•12 years ago
|
||
(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.
Updated•12 years ago
|
OS: Mac OS X → Android
Hardware: x86 → All
Assignee | ||
Comment 11•12 years ago
|
||
renamed methods
Attachment #592885 -
Attachment is obsolete: true
Attachment #593218 -
Flags: review?(mark.finkle)
Comment 12•12 years ago
|
||
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 13•12 years ago
|
||
Landed on inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/9588a4623665
Assignee | ||
Comment 14•12 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•12 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 16•12 years ago
|
||
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+
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9588a4623665
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Comment 20•12 years ago
|
||
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
Updated•12 years ago
|
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•