Closed Bug 1329145 Opened 7 years ago Closed 7 years ago

Custom tabs: support custom enter and exit animation

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: jcheng, Assigned: walkingice)

References

Details

Attachments

(3 files)

Bug 1329145 - Part 1: re-indent code style
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1329145 - Part 2: To apply custom animation if any
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1329145 - Part3: add test cases
59 bytes, text/x-review-board-request
sebastian
: review+
Details 
as a 3rd party app developer, I want to be able to customize the enter and exit animation when I make use of custom tabs
Assignee: nobody → walkingice0204
Attachment #8831030 - Flags: review?(s.kaspari)
Attachment #8831031 - Flags: review?(s.kaspari)
Attachment #8831032 - Flags: review?(s.kaspari)
Comment on attachment 8831030 [details]
Bug 1329145 - Part 1: re-indent code style

https://reviewboard.mozilla.org/r/107682/#review109232
Attachment #8831030 - Flags: review?(s.kaspari) → review+
Comment on attachment 8831031 [details]
Bug 1329145 - Part 2: To apply custom animation if any

https://reviewboard.mozilla.org/r/107684/#review109234

Some concerns but overall still r+.

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:48
(Diff revision 1)
>      private boolean useDomainTitle = true;
>  
>      private int toolbarColor;
>      private String toolbarTitle;
>  
> +    private Bundle animationBundle;

Do we need to referene this from the activity? Can't we get it from the Intent when needed?

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:111
(Diff revision 1)
>      @Override
> +    public String getPackageName() {
> +        if (isFinishing() && BundleUtil.hasExitAnimation(animationBundle)) {
> +            // The caller specify its own customized animation
> +            // Use its package name to retrieve animation resource
> +            return BundleUtil.getPackageName(animationBundle);
> +        } else {
> +            return super.getPackageName();
> +        }
> +    }

Why do we need to override this (can you add a comment)?

I'm a bit afraid that this will lead to problems down the road (e.g. we pass this context to another class or helper method and it wants to read the name of the package it's in). Is there a way to avoid this?

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:224
(Diff revision 1)
>              window.addFlags(WindowManager.LayoutParams.FLAG_DRAWS_SYSTEM_BAR_BACKGROUNDS);
>              window.setStatusBarColor(ColorUtil.darken(toolbarColor, 0.25));
>          }
>      }
> +
> +    private static class BundleUtil {

Let's move this into a separate class in the same package. We are starting to build a completely new activity here. Let's not make the same mistakes we did with BrowserApp and let it grow into a beast. :)

Also: If you think we should adapt some better architecture in Fennec then this new activity is a good place to start and show better approaches. :)
Attachment #8831031 - Flags: review?(s.kaspari) → review+
Comment on attachment 8831032 [details]
Bug 1329145 - Part3: add test cases

https://reviewboard.mozilla.org/r/107686/#review109236

Awesome! <3 tests
Attachment #8831032 - Flags: review?(s.kaspari) → review+
Comment on attachment 8831031 [details]
Bug 1329145 - Part 2: To apply custom animation if any

https://reviewboard.mozilla.org/r/107684/#review109234

> Do we need to referene this from the activity? Can't we get it from the Intent when needed?

Done, use `getIntent` to make less references.

> Why do we need to override this (can you add a comment)?
> 
> I'm a bit afraid that this will lead to problems down the road (e.g. we pass this context to another class or helper method and it wants to read the name of the package it's in). Is there a way to avoid this?

after peeping AOSP source code, method `overridePendingTransition` will call getPackageName directly, to get animation resource from specific package. We should change package name to get 3rd-party-app custom animation resource.

I did some modification to here, added a boolean flag in `finish` to make sure only return 3rd party package name if conditions satisfied.

https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/app/Activity.java#4952

> Let's move this into a separate class in the same package. We are starting to build a completely new activity here. Let's not make the same mistakes we did with BrowserApp and let it grow into a beast. :)
> 
> Also: If you think we should adapt some better architecture in Fennec then this new activity is a good place to start and show better approaches. :)

Done, I moved it into a standalone class and also rename it to IntentUtil. Because I want to add more code to it, as well as unit-test.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/98a7d9384109
Part 1: re-indent code style r=sebastian
https://hg.mozilla.org/integration/autoland/rev/e627854c8ae5
Part 2: To apply custom animation if any r=sebastian
https://hg.mozilla.org/integration/autoland/rev/f10e4c476899
Part3: add test cases r=sebastian
Keywords: checkin-needed
Verified as fixed in build 55.0a1 (2017-05-17).
Devices: Nexus 9 (Android 7.1.1) and HTC Desire 820 (Android 6.0.1).
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

Creator:
Created:
Updated:
Size: