Closed
Bug 1261044
Opened 8 years ago
Closed 8 years ago
Simplify/Replace AnimatorProxy class
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: sebastian, Assigned: Aallam.Mouaad, Mentored)
References
Details
Attachments
(1 file, 3 obsolete files)
15.20 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
AnimatorProxy was a helper class to use the different animation frameworks through a unified interface. https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/animation/AnimatorProxy.java Nowadays there's only one implementation left: AnimatorProxyPostHC. So we should be able to move this code into a helper class that doesn't need to support multiple implementations.
Assignee | ||
Comment 1•8 years ago
|
||
Hello Sebastian! :) Where the new class should be created (i suppose same package) ?
Flags: needinfo?(s.kaspari)
Reporter | ||
Comment 2•8 years ago
|
||
Yeah, the same package (animation) sounds reasonable. Would you like to work on that?
Flags: needinfo?(s.kaspari)
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #2) > Yeah, the same package (animation) sounds reasonable. Would you like to work > on that? Yes, i will try do it; i will name the new class AnimatorProxyPostHC then.
Reporter | ||
Comment 4•8 years ago
|
||
I think we can go with something more generic like AnimatorUtils.
Assignee | ||
Comment 5•8 years ago
|
||
The class should keep the interface AnimatorProxyImpl ?
Flags: needinfo?(s.kaspari)
Assignee | ||
Comment 6•8 years ago
|
||
To not stay and starting line i did a simple first intuition changes. I looked into the current implementation of the class, and i tried to adapt the class to them, i have tested this on my device and it's working, maybe the code need more adjustments, please tell me what you think and what i need to add/remove :) Note: i kept the same name of the class for the moment just to avoid doing a full build (the test i did was with artifact mode).
Attachment #8738323 -
Flags: review?(s.kaspari)
Reporter | ||
Updated•8 years ago
|
Attachment #8738323 -
Attachment is patch: true
Attachment #8738323 -
Attachment mime type: text/x-patch → text/plain
Flags: needinfo?(s.kaspari)
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → Aallam.Mouaad
Status: NEW → ASSIGNED
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8738323 [details] [diff] [review] Simple first intuition changes Review of attachment 8738323 [details] [diff] [review]: ----------------------------------------------------------------- I just saw that we have a class called ViewHelper that uses AnimatorProxy: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/animation/ViewHelper.java Let's get rid of AnimatorProxy completely and move the functionality to ViewHelper. All calls to AnimatorProxy should be redirected to ViewHelper. Instead of using a refrence to a view let's make the method static and add the view to the parameter list (like the other methods in ViewHelper).
Attachment #8738323 -
Flags: review?(s.kaspari) → feedback+
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Sebastian Kaspari (:sebastian) from comment #7) > I just saw that we have a class called ViewHelper that uses AnimatorProxy: > https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/ > mozilla/gecko/animation/ViewHelper.java it's used here too: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/animation/PropertyAnimator.java#79 i think we will need to modify this class too (to get rid of the 'create' method)
Assignee | ||
Comment 9•8 years ago
|
||
Here you go :D ! in this patch : - Deleting the class AnimatorProxy - Updating the class ViewHelper (making the old methods static, and adding new methods from AnimatorProxy) - Updating the class PropertyAnimator (from using AnimatorProxy to use ViewHelper) - Updating the file moz.build (removing the class AnimatorProxy from it) I built this with artifact (with both Android Studio and mach) and it worked for me like a charm ! :D
Attachment #8738323 -
Attachment is obsolete: true
Attachment #8739624 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 10•8 years ago
|
||
> attachment 8739624 [details] [diff] [review] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e1d552b249b&selectedJob=19219353
Reporter | ||
Comment 11•8 years ago
|
||
Comment on attachment 8739624 [details] [diff] [review] bug1261044.diff Review of attachment 8739624 [details] [diff] [review]: ----------------------------------------------------------------- This looks already quite good. Can you look at the failed lint task? Running "mach gradle lint" should create a report. It seems like your change introduced and error. ::: mobile/android/base/java/org/mozilla/gecko/animation/ViewHelper.java @@ +11,5 @@ > private ViewHelper() { > } > > public static float getTranslationX(View view) { > + if (view != null) NIT: Please add braces here, even to single line if statements.
Attachment #8739624 -
Flags: review?(s.kaspari) → feedback+
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf4c2cafebc4 Added: Braces for 'if' statements
Attachment #8739624 -
Attachment is obsolete: true
Attachment #8740595 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cacd6cb0235 For the previous patch try showed a "checkstyle" error, i found out later that the error is due to one missing 'tab' missing at the closing bracets (due to the fact that i edited the java file with |nano|, the missing tab wasn't visible on |nano| somehow!).
Attachment #8740595 -
Attachment is obsolete: true
Attachment #8740595 -
Flags: review?(s.kaspari)
Attachment #8740710 -
Flags: review?(s.kaspari)
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8740710 [details] [diff] [review] Fix updated Review of attachment 8740710 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. :) I'll land the patch.
Attachment #8740710 -
Flags: review?(s.kaspari) → review+
Reporter | ||
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d5ee38a7a2f7d2237e994a556462444c99590fcf Bug 1261044 - Move AnimatorProxy functionality into ViewHelper. r=sebastian
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d5ee38a7a2f7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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
•