Migrate AnimatorUpdateListeners to lambdas |
||||
Issue descriptionAccording to the https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md#Lambdas-and-Method-References lambdas are allowed and preferred in various scenarios over listeners. https://chromium-review.googlesource.com/c/chromium/src/+/1244440 is migrating AnimatorUpdateListener usage to the lambdas.
,
Sep 26
I'm ok with this, but for consistency, could you try to update other listeners in the compositor/ repository as well?
,
Sep 26
yes, when they have one method - https://chromium-review.googlesource.com/c/chromium/src/+/1244242, more if I will find them and when I won't be overloaded with other changes which I had to split after https://chromium-review.googlesource.com/c/chromium/src/+/1234239 no, when they have more than one method
,
Sep 28
The style guide says: "Use them only where the cost of an extra class & method definition is justified." I'd like to better understand how this applies to the changes we're discussing. We're already creating inner classes in these cases, so there's no extra cost, right?
,
Sep 28
,
Sep 28
Yeah, these are all already anonymous inner classes so this should be more or less a noop.
,
Sep 28
Yes, that warning can be applied to anonymous inner classes as well, which is a bit confusing. In general, I think lambdas are more readable when obvious class and method names are stripped off, and we can focus on the core functionality. At Google, outside of Chrome team, I've noticed that shortened syntactic forms are preferred and regarded as more readable - as long as they are backed by clear method names, right amount of comments, and unit tests such that we can abstract the functionalities better at a higher level. Just my two cents.
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9ee32f1efa92c4baac2f896aab79302fc241feb0 commit 9ee32f1efa92c4baac2f896aab79302fc241feb0 Author: Marcin Wiacek <marcin@mwiacek.com> Date: Tue Oct 02 00:07:47 2018 Migrating creating AnimatorUpdateListeners to lambdas According to various sources lambdas can provide profits in some scenarios. According to the https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md#Lambdas-and-Method-References lambdas are allowed also in Chrome, patch is migrating AnimatorUpdateListener usage to them. BUG= 889613 Change-Id: I7d4af5f34e83abe03bd998359e4c0b3cc149e3f0 Reviewed-on: https://chromium-review.googlesource.com/1244440 Commit-Queue: Marcin WiÄ…cek <marcin@mwiacek.com> Reviewed-by: Changwan Ryu <changwan@chromium.org> Reviewed-by: Matthew Jones <mdjones@chromium.org> Reviewed-by: Donn Denman <donnd@chromium.org> Cr-Commit-Position: refs/heads/master@{#595651} [modify] https://crrev.com/9ee32f1efa92c4baac2f896aab79302fc241feb0/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelAnimation.java [modify] https://crrev.com/9ee32f1efa92c4baac2f896aab79302fc241feb0/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchBarBannerControl.java [modify] https://crrev.com/9ee32f1efa92c4baac2f896aab79302fc241feb0/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchBarControl.java [modify] https://crrev.com/9ee32f1efa92c4baac2f896aab79302fc241feb0/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchCaptionControl.java [modify] https://crrev.com/9ee32f1efa92c4baac2f896aab79302fc241feb0/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchImageControl.java [modify] https://crrev.com/9ee32f1efa92c4baac2f896aab79302fc241feb0/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPromoControl.java [modify] https://crrev.com/9ee32f1efa92c4baac2f896aab79302fc241feb0/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ToolbarSwipeLayout.java
,
Oct 2
|
||||
►
Sign in to add a comment |
||||
Comment 1 by mar...@mwiacek.com
, Sep 26