[ContextualSearch] Adds long press promo for Contextual Search.

[ContextualSearch] Adds long press promo for Contextual Search.

- Long Press Promo with custom appearance animation.

- Individual Promo Control based on new ContextualSearchInflater class.

- Initial refactoring to allow custom Controls to be rendered
  without requiring changes to the Panel implementation.

- Fixes division by zero bug in Panel progress calculation.

- Removes unnecessary Layer properties.

- Removes duplicate code in ContextualSearchManager.

BUG=511475
Committed: https://crrev.com/4dca47ac6200ace8661a51d4638f98a085d3cde8
Cr-Commit-Position: refs/heads/master@{#352404}

[pedro (no code reviews), 2015-10-02 10:33:40]

pedrosimonetti@chromium.org changed reviewers:
+ aurimas@chromium.org, dtrainor@chromium.org

[pedro (no code reviews), 2015-10-02 10:33:40]

dtrainor@ and aurimas@ please take a look at this change. I still need to add
JavaDoc for the PeekPromoControl class, but otherwise this CL is pretty much
done.

Others, I'm including you as a cc. No need to review the change. Comments are
welcome though.

[pedro (no code reviews), 2015-10-02 10:35:53]

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:153:
color_utils::AlphaBlend(kPeekPromoRippleBackgroundColor,
Question to reviewers: should I be concerned about the cost of calling
AlphaBlend() on every frame? I could cache the blended value by comparing if
search_peek_promo_ripple_opacity (which drives the blend) has changed since the
last frame.

[Theresa, 2015-10-02 17:34:44]

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

[Theresa, 2015-10-02 17:34:45]

https://codereview.chromium.org/1379183003/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/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:237:
new Handler().postDelayed(new Runnable() {
What does using postDelayed achieve if the delay is 0?

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java
(right):

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:43:
int peekPromoTextResourceId = R.id.contextual_search_peek_promo;
nit: for consistency I would inline these below (on line 95/96) like we do for
other R.id/R.drawables that are passed directly.

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(left):

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1095:
public void onContentViewVisibilityChanged(boolean isVisible) {
Just to confirm, this code is duplicate with
ContextualSearchManager#onVisibilityChanged, correct? In a quick code search, it
looks like this method (onContentViewVisibilityChanged) never actually gets
called, is that correct?

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:383: <message
name="IDS_CONTEXTUAL_SEARCH_PEEK_PROMO_NEW" desc="A button to confirm and
dismiss opt out promo">
The description for this string and the one below need to be updated.

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:138: // Peek
Promo Container
nit: add a //-------------- line above this line? and below for "peek promo
ripple" and "peek promo text"? Or was this style intentional?

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:199:
peek_promo_text_->SetUIResourceId(
Does the "NEW" button and promo text get reversed in RTL?

[mdjones, 2015-10-02 19:53:14]

mdjones@chromium.org changed reviewers:
+ mdjones@chromium.org

[mdjones, 2015-10-02 19:53:15]

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(left):

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1095:
public void onContentViewVisibilityChanged(boolean isVisible) {
On 2015/10/02 17:34:45, Theresa Wellington wrote:
> Just to confirm, this code is duplicate with
> ContextualSearchManager#onVisibilityChanged, correct? In a quick code search,
it
> looks like this method (onContentViewVisibilityChanged) never actually gets
> called, is that correct?

Yeah, this is dead code I forgot to remove in part of the refactor.

[pedro (no code reviews), 2015-10-02 20:29:09]

PTAL

https://codereview.chromium.org/1379183003/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/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:237:
new Handler().postDelayed(new Runnable() {
On 2015/10/02 17:34:45, Theresa Wellington wrote:
> What does using postDelayed achieve if the delay is 0?

postDelayed with delay 0 lets the CPU "breathe for a moment" before executing an
expensive task (giving some cycles to complete pending tasks).

I was using a delay because I was inflating the promo just before animating, but
now I'm inflating it before, when we first show it. So this code is not
necessary anymore.

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java
(right):

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:43:
int peekPromoTextResourceId = R.id.contextual_search_peek_promo;
On 2015/10/02 17:34:45, Theresa Wellington wrote:
> nit: for consistency I would inline these below (on line 95/96) like we do for
> other R.id/R.drawables that are passed directly.

Good point.

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(left):

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1095:
public void onContentViewVisibilityChanged(boolean isVisible) {
On 2015/10/02 17:34:45, Theresa Wellington wrote:
> Just to confirm, this code is duplicate with
> ContextualSearchManager#onVisibilityChanged, correct? In a quick code search,
it
> looks like this method (onContentViewVisibilityChanged) never actually gets
> called, is that correct?

Correct.

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1095:
public void onContentViewVisibilityChanged(boolean isVisible) {
On 2015/10/02 19:53:15, mdjones wrote:
> On 2015/10/02 17:34:45, Theresa Wellington wrote:
> > Just to confirm, this code is duplicate with
> > ContextualSearchManager#onVisibilityChanged, correct? In a quick code
search,
> it
> > looks like this method (onContentViewVisibilityChanged) never actually gets
> > called, is that correct?
> 
> Yeah, this is dead code I forgot to remove in part of the refactor.

Acknowledged.

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:383: <message
name="IDS_CONTEXTUAL_SEARCH_PEEK_PROMO_NEW" desc="A button to confirm and
dismiss opt out promo">
On 2015/10/02 17:34:45, Theresa Wellington wrote:
> The description for this string and the one below need to be updated.

Oops.

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:138: // Peek
Promo Container
On 2015/10/02 17:34:45, Theresa Wellington wrote:
> nit: add a //-------------- line above this line? and below for "peek promo
> ripple" and "peek promo text"? Or was this style intentional?

It was intentional, supposed to be more subtle than the separator above, since
it's a sub section. But it looks inconsistent. Adding the line above the sub
sections. They are narrower than the main sections, so you can still visually
tell which one is which.

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:199:
peek_promo_text_->SetUIResourceId(
On 2015/10/02 17:34:45, Theresa Wellington wrote:
> Does the "NEW" button and promo text get reversed in RTL? 

Yes.

[David Trainor- moved to gerrit, 2015-10-02 20:49:20]

I'm fine with this.  lgtm.  But wait for other reviewers!

https://codereview.chromium.org/1379183003/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/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:237:
new Handler().postDelayed(new Runnable() {
On 2015/10/02 20:29:08, pedrosimonetti wrote:
> On 2015/10/02 17:34:45, Theresa Wellington wrote:
> > What does using postDelayed achieve if the delay is 0?
> 
> postDelayed with delay 0 lets the CPU "breathe for a moment" before executing
an
> expensive task (giving some cycles to complete pending tasks).
> 
> I was using a delay because I was inflating the promo just before animating,
but
> now I'm inflating it before, when we first show it. So this code is not
> necessary anymore.

I think post() calls postDelayed(0).  You could probably just use post().

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

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPeekPromoControl.java:32:
private final float mPaddingPx;
can we group these finals together.  and move the static final up top?

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPeekPromoControl.java:50:
Context context,
fix indent.

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPeekPromoControl.java:67:
void show() {
moar javadocz

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:178:
public void registerPeekPromoSeen() {
javadoc

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:153:
color_utils::AlphaBlend(kPeekPromoRippleBackgroundColor,
On 2015/10/02 10:35:53, pedrosimonetti wrote:
> Question to reviewers: should I be concerned about the cost of calling
> AlphaBlend() on every frame? I could cache the blended value by comparing if
> search_peek_promo_ripple_opacity (which drives the blend) has changed since
the
> last frame.

IIRC this isn't very expensive right?  It just normalizes/blends the 3 color
components?

[Theresa, 2015-10-02 20:59:59]

lgtm too - thank you for answering all my questions

[pedro (no code reviews), 2015-10-02 22:15:57]

Aurimas, could you please a look at this change?

Donn, I made changes in the ContextualSearchPolicy. I found documentation saying
it should only show for those who never opened the Panel (the issue we weren't
sure about), so I ended up using the existing getPromoOpenCount() and we will
now effectively only show it for those who never opened the Panel. I'm also now
hiding the Promo as soon as you finish opening the Panel, based on our
conversation. Could you take a look at the changes in the Policy changes? Feel
free to ping me if you want to chat about it.

https://codereview.chromium.org/1379183003/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/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:237:
new Handler().postDelayed(new Runnable() {
On 2015/10/02 20:49:20, David Trainor wrote:
> On 2015/10/02 20:29:08, pedrosimonetti wrote:
> > On 2015/10/02 17:34:45, Theresa Wellington wrote:
> > > What does using postDelayed achieve if the delay is 0?
> > 
> > postDelayed with delay 0 lets the CPU "breathe for a moment" before
executing
> an
> > expensive task (giving some cycles to complete pending tasks).
> > 
> > I was using a delay because I was inflating the promo just before animating,
> but
> > now I'm inflating it before, when we first show it. So this code is not
> > necessary anymore.
> 
> I think post() calls postDelayed(0).  You could probably just use post().

That's a good point. I now recall I was concerned that we were also doing
another animation at the same time (search term resolution), so delaying it
sounded a good idea. But we are only supposed to show the animation on long
press, and in this case we don't animate the resolution, so there's nothing else
going on at this moment. So I will simply call the method directly.

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

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPeekPromoControl.java:32:
private final float mPaddingPx;
On 2015/10/02 20:49:20, David Trainor wrote:
> can we group these finals together.  and move the static final up top?

Done.

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPeekPromoControl.java:50:
Context context,
On 2015/10/02 20:49:20, David Trainor wrote:
> fix indent.

Oops.

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPeekPromoControl.java:67:
void show() {
On 2015/10/02 20:49:20, David Trainor wrote:
> moar javadocz

Done.

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1379183003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:178:
public void registerPeekPromoSeen() {
On 2015/10/02 20:49:20, David Trainor wrote:
> javadoc

Done.

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/1379183003/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:153:
color_utils::AlphaBlend(kPeekPromoRippleBackgroundColor,
On 2015/10/02 20:49:20, David Trainor wrote:
> On 2015/10/02 10:35:53, pedrosimonetti wrote:
> > Question to reviewers: should I be concerned about the cost of calling
> > AlphaBlend() on every frame? I could cache the blended value by comparing if
> > search_peek_promo_ripple_opacity (which drives the blend) has changed since
> the
> > last frame.
> 
> IIRC this isn't very expensive right?  It just normalizes/blends the 3 color
> components?

Yeah, pretty much. Then it converts back to back to SkColor:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/color_utils...

I will keep the code how it is then.

[aurimas (slooooooooow), 2015-10-02 22:58:37]

I really think we should separate the JNI call for all the promo panel
visibility into a separate call, otherwise we will be paying the overhead of
calculating all the values and passing them via JNI 99.9% of the time without
showing the promo. I prefer you did not land it as it is.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/res...
File
chrome/android/java/res/drawable/contextual_search_peek_promo_new_background.xml
(right):

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/res...
chrome/android/java/res/drawable/contextual_search_peek_promo_new_background.xml:1:
<?xml version="1.0" encoding="utf-8"?>
Missing Chromium copyright notice.

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

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPeekPromoControl.java:225:
public void setProperty(AnimationType prop, float val) {
Make variable names proper words: type and value.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:154:
* @return Whether the Peek promo is available to be shown in any panel.
Any panel? It should probably say "to be shown in the contextual search panel"

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:177:
final Date expirationDate =
ContextualSearchFieldTrial.getPeekPromoExpirationDate();
Why do we need this? Can't we just update finch configs to disable this promo on
the server side?

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:187:
public void registerPeekPromoSeen() {
Add javadoc.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:190:

Remove empty line.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:384: New
Are we worried this will translate poorly in certain languages? As it will be a
long string in German. For example, in Lithuanian it will be "Naujiena". Would
that still look ok?

Additionally "a short word" in description does not mean anything to
translators, but you can specify a character limit and then they will abbreviate
it.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:386: <message
name="IDS_CONTEXTUAL_SEARCH_PEEK_PROMO_TEXT" desc="A promo message introducing
the Touch to Search feature">
Promo is a very informal word, I would use "promotional"

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

https://codereview.chromium.org/1379183003/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:48: int
peek_promo_ripple_resource_id,
I am really not happy with how big our method interfaces have gotten... we need
to work on possibly removing some unneeded parameter that only need to be set
during initialization, and not on very update.

This is particularly bad for promo that will only be shown a few times, but then
we will keep passing all these parameters. Should we split the update of the
promo into a separate JNI call?

[pedro (no code reviews), 2015-10-03 00:11:27]

I agree that there is room for improvement but I don't think things are as bad
as you described.

We are not calculating all the values when the Promo is not shown. When we call
getPeekPromoControl() from ContextualSearchManager, it will create a class and
cache it for as long the Panel keeps opened. If the Promo is not available that
class will be just a dummy one, and it will not do anything. I will not inflate
the View neither calculate interpolations. I will just return the default values
that the Compositor needs to know.

The extra cost with this CL is 6 new more parameters (considering that I'm
removing two that we don't need anymore) in the JNI call. This is still better
than making two JNI calls per frame in my opinion. We could improve by doing the
separation of initialization values versus rendering update values, but I think
this is outside the scope of this CL. This is also something Matt, David and I
are discussing with regards to our ongoing refactoring.

I would like to hear the opinion of other about this. I would vote for
submitting this change as is, since I'm already making preventions to make sure
this doesn't substantially impact those who are not seeing it.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/res...
File
chrome/android/java/res/drawable/contextual_search_peek_promo_new_background.xml
(right):

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/res...
chrome/android/java/res/drawable/contextual_search_peek_promo_new_background.xml:1:
<?xml version="1.0" encoding="utf-8"?>
On 2015/10/02 22:58:37, aurimas wrote:
> Missing Chromium copyright notice.

Done.

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

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPeekPromoControl.java:182:
if (!isVisible()) return;
Note that nothing will be calculated when the promo is not visible.

When we call getPeekPromoControl() from ContextualSearchManager, it will create
a class and cache it for as long the Panel keeps opened. If the Promo is not
available that class will be just a dummy one, and it will not do anything. I
will not inflate the View neither calculate interpolations. I will just return
the default values that the Compositor needs to know.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPeekPromoControl.java:225:
public void setProperty(AnimationType prop, float val) {
On 2015/10/02 22:58:37, aurimas wrote:
> Make variable names proper words: type and value.

Done.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:154:
* @return Whether the Peek promo is available to be shown in any panel.
On 2015/10/02 22:58:37, aurimas wrote:
> Any panel? It should probably say "to be shown in the contextual search panel"

Reworked the phrase.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:177:
final Date expirationDate =
ContextualSearchFieldTrial.getPeekPromoExpirationDate();
On 2015/10/02 22:58:37, aurimas wrote:
> Why do we need this? Can't we just update finch configs to disable this promo
on
> the server side?

We don't know when the user will get a new Finch config, since it requires a
restart. If we set an expiration date, then its guaranteed that all users who
were still seeing the promo will stop seeing it on that exact date.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:187:
public void registerPeekPromoSeen() {
On 2015/10/02 22:58:37, aurimas wrote:
> Add javadoc.

Done.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:190:

On 2015/10/02 22:58:37, aurimas wrote:
> Remove empty line.

Done.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:384: New
On 2015/10/02 22:58:37, aurimas wrote:
> Are we worried this will translate poorly in certain languages? As it will be
a
> long string in German. For example, in Lithuanian it will be "Naujiena". Would
> that still look ok?
> 
> Additionally "a short word" in description does not mean anything to
> translators, but you can specify a character limit and then they will
abbreviate
> it.

I just didn't know how to explain to the translater what this represents. I've
changed the phrasing. Let me know if it sounds better. I'm also open to
suggestions.

Also flipped the order, because it refers the promo message.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:386: <message
name="IDS_CONTEXTUAL_SEARCH_PEEK_PROMO_TEXT" desc="A promo message introducing
the Touch to Search feature">
On 2015/10/02 22:58:37, aurimas wrote:
> Promo is a very informal word, I would use "promotional"

Done.

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

https://codereview.chromium.org/1379183003/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:48: int
peek_promo_ripple_resource_id,
On 2015/10/02 22:58:37, aurimas wrote:
> I am really not happy with how big our method interfaces have gotten... we
need
> to work on possibly removing some unneeded parameter that only need to be set
> during initialization, and not on very update.
> 
> This is particularly bad for promo that will only be shown a few times, but
then
> we will keep passing all these parameters. Should we split the update of the
> promo into a separate JNI call?

See the message in my reply.

[aurimas (slooooooooow), 2015-10-03 00:54:03]

I don't see any tests for the feature. Can you add at least a basic one that
shows a promo so we know it is not crashing? You should also open a bug and
follow up with more tests.

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1379183003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:177:
final Date expirationDate =
ContextualSearchFieldTrial.getPeekPromoExpirationDate();
On 2015/10/03 at 00:11:27, pedrosimonetti wrote:
> On 2015/10/02 22:58:37, aurimas wrote:
> > Why do we need this? Can't we just update finch configs to disable this
promo on
> > the server side?
> 
> We don't know when the user will get a new Finch config, since it requires a
restart. If we set an expiration date, then its guaranteed that all users who
were still seeing the promo will stop seeing it on that exact date.

I still don't understand why we need this capability? Finch configs only take up
to a week to roll out.

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/contextual_search_peek_promo.xml (right):

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/contextual_search_peek_promo.xml:15:
android:textSize="14sp">
Does android:textSize on a linear layout set the text size for both the TextView
inside of it? I've never seen it used that way.

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

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:882:
assert mPeekPromoControl != null;
This seems like a silly check. We just set the value of the mPeekPromoControl
using new constructor that never returns null.

https://codereview.chromium.org/1379183003/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/1379183003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1045:
startSize == 0.f && endSize == 0.f ? 0.f :
: should be on the new line.

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:176:
// 5) The promo has not expired according to the expiration date defined by
Finch..
Remove the extra period at the end of this comment.

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:386: <message
name="IDS_CONTEXTUAL_SEARCH_PEEK_PROMO_NEW" desc="A single word that gets placed
next to the promotional message, highlight the new Touch To Search feature.">
Remove "single", it could be multiple in some languages.

Also should it say "the promotional message to highlight the new Touch to Search
feature."?

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

https://codereview.chromium.org/1379183003/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:130:
ui::ResourceManager::Resource* search_peek_promo_resource =
Should this be called search_peek_promo_text_resource?

https://codereview.chromium.org/1379183003/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:156: 63 *
search_peek_promo_ripple_opacity));
Your comment says 25% blend, but then you multiply by 63, what is up with that?

https://codereview.chromium.org/1379183003/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:193:
transform.RotateAboutZAxis(ripple_rotation);
How expensive are these rotations? Does it work ok on less powerful devices?

https://codereview.chromium.org/1379183003/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:201: if
(search_peek_promo_resource) {
If this fails should we be hiding entire promo?

[pedro (no code reviews), 2015-10-05 02:13:21]

Please take another look.

I've addressed comments and added a test.

I've also re-examined the whole CL to check for potential performance problems.
Haven't found any. While on that I realized I wasn't properly updating layout
data on the Java side and fixed that.

I did find something that can be improved in the old promo though. I'm preparing
another CL for that though.

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/contextual_search_peek_promo.xml (right):

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/contextual_search_peek_promo.xml:15:
android:textSize="14sp">
On 2015/10/03 00:54:02, aurimas wrote:
> Does android:textSize on a linear layout set the text size for both the
TextView
> inside of it? I've never seen it used that way.

You're right. Moved declaration to each TextView.

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

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:882:
assert mPeekPromoControl != null;
On 2015/10/03 00:54:02, aurimas wrote:
> This seems like a silly check. We just set the value of the mPeekPromoControl
> using new constructor that never returns null.

Done.

https://codereview.chromium.org/1379183003/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/1379183003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1045:
startSize == 0.f && endSize == 0.f ? 0.f :
On 2015/10/03 00:54:02, aurimas wrote:
> : should be on the new line.

Done.

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:176:
// 5) The promo has not expired according to the expiration date defined by
Finch..
On 2015/10/03 00:54:02, aurimas wrote:
> Remove the extra period at the end of this comment.

Code has been removed.

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1379183003/diff/80001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:386: <message
name="IDS_CONTEXTUAL_SEARCH_PEEK_PROMO_NEW" desc="A single word that gets placed
next to the promotional message, highlight the new Touch To Search feature.">
On 2015/10/03 00:54:02, aurimas wrote:
> Remove "single", it could be multiple in some languages.
> 
> Also should it say "the promotional message to highlight the new Touch to
Search
> feature."?

Done.

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

https://codereview.chromium.org/1379183003/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:130:
ui::ResourceManager::Resource* search_peek_promo_resource =
On 2015/10/03 00:54:02, aurimas wrote:
> Should this be called search_peek_promo_text_resource?

For consistency, it should be peek_promo_text_resource.

https://codereview.chromium.org/1379183003/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:156: 63 *
search_peek_promo_ripple_opacity));
On 2015/10/03 00:54:02, aurimas wrote:
> Your comment says 25% blend, but then you multiply by 63, what is up with
that?

My comment was confusing. Maximum alpha is 255, so 25% of that is approximately
63, the maximum value we will use in the blend.

I am using constants now, with improved comments. Things are more readable.

https://codereview.chromium.org/1379183003/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:193:
transform.RotateAboutZAxis(ripple_rotation);
On 2015/10/03 00:54:02, aurimas wrote:
> How expensive are these rotations? Does it work ok on less powerful devices?

I don't think the rotation is expensive. We use the rotation for the arrow as
well and they work on less powerful devices.

https://codereview.chromium.org/1379183003/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:201: if
(search_peek_promo_resource) {
On 2015/10/03 00:54:02, aurimas wrote:
> If this fails should we be hiding entire promo?

No, we should not.

Like we do for the rest of the dynamic resources, we need to check because since
they are dynamically generated, the first couple of frames might happen before
the resource is available. But we know because of the search_peek_promo_visible
that the promo should be there, so we should draw everything we can while we
wait for the resource.

[aurimas (slooooooooow), 2015-10-05 03:08:57]

This patch is looking good now -> LGTM given that there will be 2 follow-ups:
1. Fix the test TODO to make sure that the promo does not come up again.
2. Refactor to draw the promo via a separate JNI call.
  - arguments for this being that panel shows will never be showing this promo,
so making the users that do see it pay the price of drawing it using 2 JNI calls
seems reasonable.

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

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:872:
private ContextualSearchPeekPromoControl mPeekPromoControl;
In Chromium code we always put class level variables at the top of the file.

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

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1063:
// NOTE(pedrosimonetti): Handle special case from PanelState.UNDEFINED
How does one go from UNDEFINED to CLOSED?

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java
(right):

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:205:
float start, float end, long duration, long startTime);
This is C++ style parameter line wrapping. In java we wrap the new line with +8
spaces instead of aligning with the previous line.

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchEventFilterTest.java
(right):

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchEventFilterTest.java:159:
protected float getPeekPromoHeight() {
Comment says that we override these methods because they rely on native methods.
This does not seem to rely on anything native, am I missing something?

[Donn Denman, 2015-10-05 17:16:29]

donnd@chromium.org changed reviewers:
+ donnd@chromium.org

[Donn Denman, 2015-10-05 17:16:31]

LGTM, just a few nits (sorry to be annoying)!

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFieldTrial.java
(right):

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFieldTrial.java:238:
* @return Whether the Peek Promo is forcefully enabled (used for testing).
nit: s forcefully/forcibly

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFieldTrial.java:248:
if (sIsPeekPromoEnabled != null) return sIsPeekPromoEnabled;
nit: use the normal pattern for this -- ie change the sense of this compare and
then we can just have one return at the end.

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFieldTrial.java:258:
if (sPeekPromoMaxCount > 0) return sPeekPromoMaxCount;
nit: use single-return here too.

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1977:
// TODO(pedrosimonetti): investigate why calling longPressNode() doesn't make
the bar peek.
I have a hunch you need to add another gesture here to dismiss the selection --
should still be selected after dismissing the panel?  I'm also OK with doing
this as a follow-up.

[pedro (no code reviews), 2015-10-05 19:07:13]

> This patch is looking good now -> LGTM given that there will be 2 follow-ups:
> 1. Fix the test TODO to make sure that the promo does not come up again.

The promo never comes back again, otherwise I wouldn't submit this change. That
comment was saying that the Panel was not peeking, because I was forgetting that
the long press selection persists after closing the Panel and therefore should
be cleared before trying to select it again. In either case, the test is doing
the right thing now.

> 2. Refactor to draw the promo via a separate JNI call.
>   - arguments for this being that panel shows will never be showing this
promo,
> so making the users that do see it pay the price of drawing it using 2 JNI
calls
> seems reasonable.

Making extra JNI calls is the very opposite of what we want to do, unless your
goal is to make things slower. The extra cost of sending a handful more of
parameters in negligible compared to the cost of calling another JNI per frame.
Making the first-time experience of our users two times worse when we're not
impacting the experience of those who don't see the promo does not making any
sense to me.

Also, we should evaluate not just the performance impact of a change, but also
its impact on readability, maintainability, extensibility, memory consumption,
an so on.

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

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:872:
private ContextualSearchPeekPromoControl mPeekPromoControl;
On 2015/10/05 03:08:57, aurimas wrote:
> In Chromium code we always put class level variables at the top of the file.

We are moving a lot of code to and from Panel related classes. I will be easier
to maintain the code if we keep things that pretty much self contained in its
own section of code. The guidelines allow this, as it says that "there is no
single correct recipe for how to [order members of a class]. Different classes
may order their members differently".
https://engdoc.corp.google.com/eng/doc/devguide/java/styleguide.shtml?cl=head...

If we decide to follow this pattern, we should do it in all of our code, not
just here. For now, I'll keep being consistent with how this class is currently
ordered.

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

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:1063:
// NOTE(pedrosimonetti): Handle special case from PanelState.UNDEFINED
On 2015/10/05 03:08:57, aurimas wrote:
> How does one go from UNDEFINED to CLOSED?

When the Panel starts the peek animation, or when it completes the close
animation.

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java
(right):

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelDelegate.java:205:
float start, float end, long duration, long startTime);
On 2015/10/05 03:08:57, aurimas wrote:
> This is C++ style parameter line wrapping. In java we wrap the new line with
+8
> spaces instead of aligning with the previous line.

Done.

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFieldTrial.java
(right):

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFieldTrial.java:238:
* @return Whether the Peek Promo is forcefully enabled (used for testing).
On 2015/10/05 17:16:30, Donn Denman wrote:
> nit: s forcefully/forcibly 

Done.

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFieldTrial.java:248:
if (sIsPeekPromoEnabled != null) return sIsPeekPromoEnabled;
On 2015/10/05 17:16:30, Donn Denman wrote:
> nit: use the normal pattern for this -- ie change the sense of this compare
and
> then we can just have one return at the end.

Done.

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFieldTrial.java:258:
if (sPeekPromoMaxCount > 0) return sPeekPromoMaxCount;
On 2015/10/05 17:16:30, Donn Denman wrote:
> nit: use single-return here too.

Done.

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchEventFilterTest.java
(right):

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchEventFilterTest.java:159:
protected float getPeekPromoHeight() {
On 2015/10/05 03:08:57, aurimas wrote:
> Comment says that we override these methods because they rely on native
methods.
> This does not seem to rely on anything native, am I missing something?

You're wrong. You're missing the fact that the comment you're referring is about
the MockOverlayPanelContent class and this is a method of
MockContextualSearchPanel.

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1977:
// TODO(pedrosimonetti): investigate why calling longPressNode() doesn't make
the bar peek.
On 2015/10/05 17:16:31, Donn Denman wrote:
> I have a hunch you need to add another gesture here to dismiss the selection
--
> should still be selected after dismissing the panel?  I'm also OK with doing
> this as a follow-up.

Good point.

[pedro (no code reviews), 2015-10-05 19:07:31]

The CQ bit was checked by pedrosimonetti@chromium.org

[pedro (no code reviews), 2015-10-05 19:07:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dtrainor@chromium.org, aurimas@chromium.org,
donnd@chromium.org
Link to the patchset: https://codereview.chromium.org/1379183003/#ps270001
(title: "Addressing more comments")

[commit-bot: I haz the power, 2015-10-05 19:10:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1379183003/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1379183003/270001

[aurimas (slooooooooow), 2015-10-05 19:56:45]

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

https://codereview.chromium.org/1379183003/diff/250001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:872:
private ContextualSearchPeekPromoControl mPeekPromoControl;
On 2015/10/05 at 19:07:12, pedrosimonetti wrote:
> On 2015/10/05 03:08:57, aurimas wrote:
> > In Chromium code we always put class level variables at the top of the file.
> 
> We are moving a lot of code to and from Panel related classes. I will be
easier to maintain the code if we keep things that pretty much self contained in
its own section of code. The guidelines allow this, as it says that "there is no
single correct recipe for how to [order members of a class]. Different classes
may order their members differently".
>
https://engdoc.corp.google.com/eng/doc/devguide/java/styleguide.shtml?cl=head...
> 
> If we decide to follow this pattern, we should do it in all of our code, not
just here. For now, I'll keep being consistent with how this class is currently
ordered.

Just FYI Chromium does not use Google's java style guide, instead we use Android
Open Source style guide. See
https://www.chromium.org/developers/coding-style/java

[commit-bot: I haz the power, 2015-10-05 20:18:31]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2015-10-05 20:20:30]

Patchset 15 (id:??) landed as
https://crrev.com/4dca47ac6200ace8661a51d4638f98a085d3cde8
Cr-Commit-Position: refs/heads/master@{#352404}