New issue
Advanced search Search tips

Issue 889613 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Feature



Sign in to add a comment

Migrate AnimatorUpdateListeners to lambdas

Project Member Reported by mar...@mwiacek.com, Sep 26

Issue description

According 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.


 
mdjones@, changwan@, could you provide your comment to this change please?
I'm ok with this, but for consistency, could you try to update other listeners in the compositor/ repository as well?
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
Owner: mar...@mwiacek.com
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?

Status: Assigned (was: Unconfirmed)
Yeah, these are all already anonymous inner classes so this should be more or less a noop.
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.

Project Member

Comment 8 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment