Contextual Search UI changes for custom tabs

Contextual Search UI changes for custom tabs

For custom tabs, in the maximized state a tap on the search panel should not promote to a new tab. Also, a close button has been added in the maximized state.

BUG=499502
Committed: https://crrev.com/38bbc5a1475b346d0bd3164712737dd96d3636a2
Cr-Commit-Position: refs/heads/master@{#337948}

[Theresa, 2015-07-01 18:29:08]

twellington@chromium.org changed reviewers:
+ pedrosimonetti@chromium.org

[Theresa, 2015-07-01 18:29:09]

I think a method in FeatureUtilities.java (or even a separate
ContextualSearchFeatures.java) is cleaner than passing isCustomTab around in
constructors.

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(left):

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:10:
import
org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchPanel.StateChangeReason;
This got added in my last CL and it shouldn't have; since this class declares
the StateChangeReason enum, this import is unncessary.

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1040:
mSearchIconOpacity = searchIconOpacity;
I'll remove this bit (hiding the search icon)

[pedro (no code reviews), 2015-07-01 19:28:53]

Thanks Theresa, I agree that having a method in FeatureUtilities to check for
custom tabs is indeed better.

Feel free to ping me if you have more questions with the remaining
implementation and I will be glad to help.

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:251:
if (!mIsCustomTab) {
I agree that it would be better having this logic elsewhere, than piping it all
the way through the system.

Also, the Panel shouldn't know about custom tabs. It should know whether certain
UI elements/features are present. As we discussed, I'm planning on adding a
ContextualSearchPanelFeatures class, and we should include two methods in it:

ContextualSearchPanelFeatures.isSearchTermRefiningAvailable()
ContextualSearchPanelFeatures.isCloseButtonAvailable()

I'm gonna include a placeholder for those methods and my CL, and you can later
change it and check for custom tabs in there.

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:253:
} else if (isTouchOnCloseButton(x, y)) {
Nit: For consistency reasons with other similar methods, I would call this
isCoordinateInsideCloseButton.

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1040:
mSearchIconOpacity = searchIconOpacity;
On 2015/07/01 18:29:09, Theresa Wellington wrote:
> I'll remove this bit (hiding the search icon)

Ok

[Theresa, 2015-07-06 20:28:32]

ptal

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1219133003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:253:
} else if (isTouchOnCloseButton(x, y)) {
On 2015/07/01 19:28:53, pedrosimonetti wrote:
> Nit: For consistency reasons with other similar methods, I would call this
> isCoordinateInsideCloseButton.

Done.

[Theresa, 2015-07-07 23:18:55]

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:246:
if (!isCloseIconVisible()) {
Check ContextualSearchPanelFeatures.isSearchTermRefiningAvailable() instead.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:248:
} else if (isCoordinateInsideCloseButton(x, y)) {
Check ContextualSearchPanelFeatures.isCloseButtonAvailable() in an if statement
(rather than an else if).

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1113:
float searchIconOpacity = MathUtils.interpolate(
Add isSearchIconVisible() and pass along icon visibility to cc layers.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1123:
percentage);
Need to make the arrow fade out all the way before having the close icon fade
in.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1127:
if (isCloseIconVisible()) {
Change opacity regardless of visibility (since cc layer checks for visibility)?

[Theresa, 2015-07-08 00:29:16]

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1113:
float searchIconOpacity = MathUtils.interpolate(
On 2015/07/07 23:18:55, Theresa Wellington wrote:
> Add isSearchIconVisible() and pass along icon visibility to cc layers.

Done.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1127:
if (isCloseIconVisible()) {
On 2015/07/07 23:18:55, Theresa Wellington wrote:
> Change opacity regardless of visibility (since cc layer checks for
visibility)?

Done.

[pedro (no code reviews), 2015-07-08 00:43:47]

Sorry for the delay! I had to prioritize a release blocker fix for M44.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:246:
if (!isCloseIconVisible()) {
On 2015/07/07 23:18:54, Theresa Wellington wrote:
> Check ContextualSearchPanelFeatures.isSearchTermRefiningAvailable() instead.

Yes.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:248:
} else if (isCoordinateInsideCloseButton(x, y)) {
On 2015/07/07 23:18:54, Theresa Wellington wrote:
> Check ContextualSearchPanelFeatures.isCloseButtonAvailable() in an if
statement
> (rather than an else if).

Exactly.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:487:
public float getSearchBarWidth() {
We don't need this new method. Instead, we can simply call getWidth(), which
returns the width of the Panel, given that the Bar has the same width as the
Panel.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:574:
return FeatureUtilities.getCustomTabVisible();
Yes!

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:587:
@SuppressWarnings("deprecation")
Why is there a @SuppressWarnings here?

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:588:
public float getCloseIconWidth() {
This should probably called getCloseIconDimension() since it's used for both
width and height.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:607:
if (ViewCompat.getLayoutDirection(mContainerView) == View.LAYOUT_DIRECTION_RTL)
{
Newton or Aurimas might know better, but I think we should use
LocalizationUtils.isLayoutRtl() instead.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1113:
float searchIconOpacity = MathUtils.interpolate(
On 2015/07/07 23:18:55, Theresa Wellington wrote:
> Add isSearchIconVisible() and pass along icon visibility to cc layers.

My CL is already doing this.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1127:
if (isCloseIconVisible()) {
On 2015/07/07 23:18:55, Theresa Wellington wrote:
> Change opacity regardless of visibility (since cc layer checks for
visibility)?

Good point. You can remove the check here and let the contextual_search_layer.cc
decide whether to render it.

https://codereview.chromium.org/1219133003/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1219133003/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:240: }
Add else block that removes the layer, if not present. See the Arrow Icon code
below.

This way we don't keep the layer in the hierarchy when not needed (which could
potentially slow down the compositor).

[Theresa, 2015-07-08 16:03:58]

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:487:
public float getSearchBarWidth() {
On 2015/07/08 00:43:47, pedrosimonetti wrote:
> We don't need this new method. Instead, we can simply call getWidth(), which
> returns the width of the Panel, given that the Bar has the same width as the
> Panel.

Done.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:587:
@SuppressWarnings("deprecation")
On 2015/07/08 00:43:47, pedrosimonetti wrote:
> Why is there a @SuppressWarnings here?

getDrawable(int id) was deprecated in API level 22 in favor of getDrawable(int,
theme), which wasn't added until API level 21. Our min API level is 16, so we
can't use getDrawable(int, theme). I changed this to
ApiCompatibilityUtils.getDrawable(Resources res, int id), which seems to be
preferred way in Chromium of dealing with the getDrawable deprecation.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:588:
public float getCloseIconWidth() {
On 2015/07/08 00:43:47, pedrosimonetti wrote:
> This should probably called getCloseIconDimension() since it's used for both
> width and height.

Done.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:607:
if (ViewCompat.getLayoutDirection(mContainerView) == View.LAYOUT_DIRECTION_RTL)
{
On 2015/07/08 00:43:47, pedrosimonetti wrote:
> Newton or Aurimas might know better, but I think we should use
> LocalizationUtils.isLayoutRtl() instead.

Done.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1113:
float searchIconOpacity = MathUtils.interpolate(
On 2015/07/08 00:43:47, pedrosimonetti wrote:
> On 2015/07/07 23:18:55, Theresa Wellington wrote:
> > Add isSearchIconVisible() and pass along icon visibility to cc layers.
> 
> My CL is already doing this.

Acknowledged. I'll leave it alone then.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1123:
percentage);
On 2015/07/07 23:18:55, Theresa Wellington wrote:
> Need to make the arrow fade out all the way before having the close icon fade
> in.

Done.

https://codereview.chromium.org/1219133003/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1219133003/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:240: }
On 2015/07/08 00:43:47, pedrosimonetti wrote:
> Add else block that removes the layer, if not present. See the Arrow Icon code
> below.
> 
> This way we don't keep the layer in the hierarchy when not needed (which could
> potentially slow down the compositor).

Done.

[pedro (no code reviews), 2015-07-08 20:15:06]

Theresa, please sync and rebase now that my change was submitted. Posted some
more comments, mostly nits.

Will take a final look once you rebase it and make the changes to use the
ContextualSearchPanelFeatures class, as we discussed.

Overall, the CL is already looking pretty good.

https://codereview.chromium.org/1219133003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1112:
// Determine arrow and close icon opacities.
Nit: Add comment explaining what this is doing, and why we are doing it (to
prevent both icons to be momentarily displayed on top of each other).

https://codereview.chromium.org/1219133003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1118:
arrowIconPercentage = percentage * 2;
Two observations:

1) I think that dividing by 0.5 makes the code more readable, even though
multiplying by two is mathematically equivalent, since it makes it clearer that
we're dealing with a ratio/percentage, related to half of the progress from
expanded to maximize.

2) We should cap the percentage to [0, 0.5] (for the arrow icon) or [0.5, 1]
(for the close icon), to make sure the resulting value is withing the range [0,
1].

The resulting code would be something like:
arrowIconPercentage = Math.min(percentage, .5f) / .5f;
closeIconPercentage = Math.max(percentage - .5f, .5f) / .5f;

https://codereview.chromium.org/1219133003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1119:
closeIconPercentage = (float) ((percentage - 0.5) * 2.0);
Remember to use float type suffix:
  0.5 --> .5f
  2.0 --> 2.f

https://codereview.chromium.org/1219133003/diff/80001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1219133003/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:154: -
search_icon_resource->size.height() / 2;
Nit: Differently from Java, the operator should be placed in the previous line.

[Theresa, 2015-07-08 21:04:49]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2015-07-08 21:04:59]

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:246:
if (!isCloseIconVisible()) {
On 2015/07/08 00:43:46, pedrosimonetti wrote:
> On 2015/07/07 23:18:54, Theresa Wellington wrote:
> > Check ContextualSearchPanelFeatures.isSearchTermRefiningAvailable() instead.
> 
> Yes.

Done.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:248:
} else if (isCoordinateInsideCloseButton(x, y)) {
On 2015/07/08 00:43:47, pedrosimonetti wrote:
> On 2015/07/07 23:18:54, Theresa Wellington wrote:
> > Check ContextualSearchPanelFeatures.isCloseButtonAvailable() in an if
> statement
> > (rather than an else if).
> 
> Exactly.

Done.

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:574:
return FeatureUtilities.getCustomTabVisible();
On 2015/07/08 00:43:47, pedrosimonetti wrote:
> Yes!

Done.

https://codereview.chromium.org/1219133003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1112:
// Determine arrow and close icon opacities.
On 2015/07/08 20:15:05, pedrosimonetti wrote:
> Nit: Add comment explaining what this is doing, and why we are doing it (to
> prevent both icons to be momentarily displayed on top of each other).

Done. I had the explanation a little lower down, but it makes more sense here.

https://codereview.chromium.org/1219133003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1118:
arrowIconPercentage = percentage * 2;
On 2015/07/08 20:15:05, pedrosimonetti wrote:
> Two observations:
> 
> 1) I think that dividing by 0.5 makes the code more readable, even though
> multiplying by two is mathematically equivalent, since it makes it clearer
that
> we're dealing with a ratio/percentage, related to half of the progress from
> expanded to maximize.
> 
> 2) We should cap the percentage to [0, 0.5] (for the arrow icon) or [0.5, 1]
> (for the close icon), to make sure the resulting value is withing the range
[0,
> 1].
> 
> The resulting code would be something like:
> arrowIconPercentage = Math.min(percentage, .5f) / .5f;
> closeIconPercentage = Math.max(percentage - .5f, .5f) / .5f;

Done. For closeIconPercentage, the line is a little different. We want a value
of [0, .5] to be divided by .5f. To avoid negative values, we should do the max
of (percentage - .5f, 0.f).

https://codereview.chromium.org/1219133003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1119:
closeIconPercentage = (float) ((percentage - 0.5) * 2.0);
On 2015/07/08 20:15:05, pedrosimonetti wrote:
> Remember to use float type suffix:
>   0.5 --> .5f
>   2.0 --> 2.f

Done.

https://codereview.chromium.org/1219133003/diff/80001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1219133003/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:154: -
search_icon_resource->size.height() / 2;
On 2015/07/08 20:15:05, pedrosimonetti wrote:
> Nit: Differently from Java, the operator should be placed in the previous
line.

Done.

[Theresa, 2015-07-08 21:06:01]

twellington@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Theresa, 2015-07-08 21:06:02]

+dtrainor for OWNERS review

[commit-bot: I haz the power, 2015-07-08 21:06:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1219133003/100001

[commit-bot: I haz the power, 2015-07-08 21:06:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-08 21:06:40]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[pedro (no code reviews), 2015-07-08 21:21:19]

lgtm

Just a couple more nits. Code LGTM. You will need an owners approval though.

https://codereview.chromium.org/1219133003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:578:
public boolean isCloseIconVisible() {
Nit: We don't need to add this new method. We can simply use
ContextualSearchPanelFeatures.isCloseButtonAvailable() directly.

https://codereview.chromium.org/1219133003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1117:
percentage);
Here we should probably use the same percentage as the arrow icon, so they both
fade at the same time. For this reason, we could call the local variable
fadingElementPercentage, instead of arrowIconPercentage.

[Theresa, 2015-07-08 21:43:39]

https://codereview.chromium.org/1219133003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java
(right):

https://codereview.chromium.org/1219133003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:578:
public boolean isCloseIconVisible() {
On 2015/07/08 21:21:19, pedrosimonetti wrote:
> Nit: We don't need to add this new method. We can simply use
> ContextualSearchPanelFeatures.isCloseButtonAvailable() directly.

Done.

https://codereview.chromium.org/1219133003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1117:
percentage);
On 2015/07/08 21:21:19, pedrosimonetti wrote:
> Here we should probably use the same percentage as the arrow icon, so they
both
> fade at the same time. For this reason, we could call the local variable
> fadingElementPercentage, instead of arrowIconPercentage.

Done.

[David Trainor- moved to gerrit, 2015-07-08 22:43:03]

lgtm

[Theresa, 2015-07-09 00:05:29]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2015-07-09 00:05:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
pedrosimonetti@chromium.org
Link to the patchset: https://codereview.chromium.org/1219133003/#ps120001
(title: "Use fadingInPercentage and fadingOutPercentage, get rid of
PanelBase#isCloseIconVisible()")

[commit-bot: I haz the power, 2015-07-09 00:05:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1219133003/120001

[commit-bot: I haz the power, 2015-07-09 00:12:09]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-09 00:13:11]

Patchset 7 (id:??) landed as
https://crrev.com/38bbc5a1475b346d0bd3164712737dd96d3636a2
Cr-Commit-Position: refs/heads/master@{#337948}