Refine thumbnail animation

RESOLVED FIXED in Firefox 26

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ibarlow, Assigned: sriram)

Tracking

(Depends on 1 bug, Blocks 1 bug)

26 Branch
Firefox 26
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec26+)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
The new thumbnail animation in Fig is nice, but it should be a little more subtle. This is the bug to get the timing and feel to where we want it. 

My initial suggestion would be to 
* Keep the timing as is for now
* Reduce the amount of zoom in the animation - I think it goes from 50%->100% now. Maybe it should be 80%->100%
* Add a subtle fade-in while zooming
(Reporter)

Updated

6 years ago
Priority: -- → P1
(Assignee)

Comment 1

6 years ago
Posted patch PatchSplinter Review
Made changes as per the bug.

However, the animation doesn't look so good now. We are increasing the size from 80% to 100% in 250ms. We used the same time for 50% to 100% which felt very dynamic. This one feels very very slow and goes on to feel like stuttering.
Attachment #789703 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(ibarlow)
(Reporter)

Comment 2

6 years ago
Maybe try half the time then, and post a build?
Flags: needinfo?(ibarlow)
Comment on attachment 789703 [details] [diff] [review]
Patch

Dropping review flag until a test build is approved by ibarlow.
Attachment #789703 - Flags: review?(lucasr.at.mozilla)
(Reporter)

Comment 4

6 years ago
Sorry Sriram, lost the link to your build from yesterday. Would you mind posting it here?
Assignee: nobody → sriram
tracking-fennec: --- → ?
tracking-fennec: ? → 26+
(Assignee)

Comment 5

6 years ago
Posted patch PatchSplinter Review
This patch would save more than 200ms. Basically, from my logging, I figured out:
1. Waiting for the cursor to do a measure pass on the GridView will cause re-layout and re-measure of the ListView. So, thats a bit wrong. This also reflects the widely used rule in Android community: "Don't use wrap_content with ListView". (ummmm). http://www.youtube.com/watch?v=wDBM6wVEO70&t=40m45s
We don't use the wrap_content, but we set a minimum of (0,0) and then calculates when the need comes, causing a re-layout and re-measure.

2. Calculating the width and height of the GridView everytime someone asks it is pretty bad! ListView was returning the result of measure in 1-2ms. GridView took 25ms for every call. So I felt like caching.

3. Using an animation where each item comes one by one causes layout pass to be done for every single view coming in.

This patches measures the width and height, thinking that there will mMaxBookmarks entries, only once. These values are cached. When the device rotates, the values are re-calculated. From every other occasion, the cached value is returned. This saves us 23ms (on the average) for every call. (I can hear a Wow!)

This patch also removes the animation. The animation used here caused re-layout. We need to move to fading in, which would cause all the bookmarks to come at once, and then fade from default to a thumbnail image. That would be in a separate patch. This saves the layout in every call -- which was about 60-100ms (Wow! Woww!).

Visually bookmarks page feels faster. Will wait for Eideticker to see if we are really faster.
Attachment #801030 - Flags: review?(mark.finkle)
Comment on attachment 801030 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/home/TopBookmarksView.java b/mobile/android/base/home/TopBookmarksView.java

>     protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {

>         final int measuredWidth = getMeasuredWidth();
>+        if (measuredWidth == mMeasuredWidth) {
>+            setMeasuredDimension(mMeasuredWidth, mMeasuredHeight);

Add a comment inside the if block about just using the cached values since the width is the same

>+        // Get the first child from the adapter.
>+        final View child = new TopBookmarkItemView(getContext());
> 
>+        // Set a default LayoutParams on the child, if it doesn't have one on its own.
>+        AbsListView.LayoutParams params = (AbsListView.LayoutParams) child.getLayoutParams();

Is there a chance the child is null? We do a null check on it in the current code.

Make sure you file the followup bug to fade in the bookmarks. I'm sure Ian will want that feature.
Attachment #801030 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 7

6 years ago
The null check is not needed here. The earlier version was looking at the adapter. We don't use adapter now.

https://hg.mozilla.org/integration/fx-team/rev/bbddba8cc6e6
https://hg.mozilla.org/mozilla-central/rev/bbddba8cc6e6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
(Assignee)

Comment 9

6 years ago
Comment on attachment 801030 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Dynamic toolbars
User impact if declined: Password doorhangers won't be shown if the dynamic toolbar is hidden.
Testing completed (on m-c, etc.): Landed in m-c on 09/09.
Risk to taking this patch (and alternatives if risky): Very very low. We set a default size for the popups.
String or IDL/UUID changes made by this patch: Zilch.
Attachment #801030 - Flags: approval-mozilla-beta?
Attachment #801030 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Blocks: 908225
(Assignee)

Comment 10

6 years ago
Comment on attachment 801030 [details] [diff] [review]
Patch

Supposed to be another bug. Removing uplift approval requests.
Attachment #801030 - Flags: approval-mozilla-beta?
Attachment #801030 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Depends on: 914872

Updated

6 years ago
Duplicate of this bug: 910730
You need to log in before you can comment on or make changes to this bug.