Closed Bug 904172 Opened 11 years ago Closed 11 years ago

Refine thumbnail animation

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect, P1)

26 Branch
x86
macOS
defect

Tracking

(fennec26+)

RESOLVED FIXED
Firefox 26
Tracking Status
fennec 26+ ---

People

(Reporter: ibarlow, Assigned: sriram)

References

Details

Attachments

(2 files)

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
Priority: -- → P1
Attached 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)
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)
Sorry Sriram, lost the link to your build from yesterday. Would you mind posting it here?
Assignee: nobody → sriram
tracking-fennec: --- → ?
tracking-fennec: ? → 26+
Attached 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+
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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?
Blocks: 908225
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?
Depends on: 914872
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.