 Chromium Code Reviews
 Chromium Code Reviews Issue 1050163004:
  [Contextual Search] Implements Opt-out promo.  (Closed) 
  Base URL: https://chromium.googlesource.com/chromium/src.git@master
    
  
    Issue 1050163004:
  [Contextual Search] Implements Opt-out promo.  (Closed) 
  Base URL: https://chromium.googlesource.com/chromium/src.git@master| Index: chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java | 
| diff --git a/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java b/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java | 
| index 33f83bb7b26b807ca2c90bc1d9dc7e17b82fa2b5..29571805a097d3e32ee1faaffef68dd0876892d3 100644 | 
| --- a/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java | 
| +++ b/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java | 
| @@ -5,12 +5,26 @@ | 
| package org.chromium.chrome.browser.compositor.bottombar.contextualsearch; | 
| import android.content.Context; | 
| +import android.os.Handler; | 
| +import android.text.method.LinkMovementMethod; | 
| +import android.text.style.ClickableSpan; | 
| +import android.view.LayoutInflater; | 
| +import android.view.View; | 
| +import android.view.View.MeasureSpec; | 
| +import android.view.ViewGroup; | 
| +import android.view.ViewGroup.MarginLayoutParams; | 
| +import android.widget.TextView; | 
| import org.chromium.base.VisibleForTesting; | 
| import org.chromium.chrome.R; | 
| import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchPanel.PanelState; | 
| import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchPanel.StateChangeReason; | 
| import org.chromium.chrome.browser.util.MathUtils; | 
| +import org.chromium.chrome.browser.widget.ContextualSearchControl; | 
| +import org.chromium.chrome.browser.widget.ContextualSearchOptOutPromo; | 
| +import org.chromium.ui.resources.dynamics.DynamicResourceLoader; | 
| +import org.chromium.ui.text.SpanApplier; | 
| +import org.chromium.ui.text.SpanApplier.SpanInfo; | 
| /** | 
| * Base abstract class for the Contextual Search Panel. | 
| @@ -165,6 +179,11 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| */ | 
| private boolean mIsShowing; | 
| + /** | 
| + * The current context. | 
| + */ | 
| + private Context mContext; | 
| 
Donn Denman
2015/04/02 19:45:00
Looks like you can make this final.
 
pedro (no code reviews)
2015/04/03 21:01:01
Done.
 | 
| + | 
| // ============================================================================================ | 
| // Constructor | 
| // ============================================================================================ | 
| @@ -173,6 +192,8 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| * @param context The current Android {@link Context}. | 
| */ | 
| public ContextualSearchPanelBase(Context context) { | 
| + mContext = context; | 
| + | 
| mPxToDp = 1.f / context.getResources().getDisplayMetrics().density; | 
| mSearchBarHeightPeeking = context.getResources().getDimension( | 
| @@ -353,6 +374,10 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| private boolean mIsSearchBarBorderVisible; | 
| private float mSearchBarBorderY; | 
| private float mSearchBarBorderHeight; | 
| + | 
| + boolean mSearchBarShadowVisible = false; | 
| + float mSearchBarShadowOpacity = 0.f; | 
| + | 
| private float mSearchProviderIconOpacity; | 
| private float mSearchIconPaddingLeft; | 
| private float mSearchIconOpacity; | 
| @@ -400,6 +425,20 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| } | 
| /** | 
| + * @return Whether the Search Bar shadow is visible. | 
| + */ | 
| + public boolean getSearchBarShadowVisible() { | 
| + return mSearchBarShadowVisible; | 
| + } | 
| + | 
| + /** | 
| + * @return The opacity of the Search Bar shadow. | 
| + */ | 
| + public float getSearchBarShadowOpacity() { | 
| + return mSearchBarShadowOpacity; | 
| + } | 
| + | 
| + /** | 
| * @return The opacity of the search provider's icon. | 
| */ | 
| public float getSearchProviderIconOpacity() { | 
| @@ -504,10 +543,68 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| // Promo states | 
| // -------------------------------------------------------------------------------------------- | 
| + private boolean mPromoVisible = false; | 
| + private float mPromoContentHeightPx = 0.f; | 
| + private float mPromoHeightPx; | 
| + private float mPromoOpacity; | 
| + | 
| + /** | 
| + * @param visible Whether the promo will be set to visible. | 
| + */ | 
| + protected void setPromoVisible(boolean visible) { | 
| + mPromoVisible = visible; | 
| + } | 
| + | 
| + /** | 
| + * @return Whether the promo is visible. | 
| + */ | 
| + public boolean getPromoVisible() { | 
| + return mPromoVisible; | 
| + } | 
| + | 
| + /** | 
| + * Sets the height of the promo content. | 
| + */ | 
| + public void setPromoContentHeightPx(float heightPx) { | 
| + mPromoContentHeightPx = heightPx; | 
| + } | 
| + | 
| + /** | 
| + * @return Height of the promo in dps. | 
| + */ | 
| + public float getPromoHeight() { | 
| + return mPromoHeightPx * mPxToDp; | 
| + } | 
| + | 
| + /** | 
| + * @return Height of the promo in pixels. | 
| + */ | 
| + public float getPromoHeightPx() { | 
| + return mPromoHeightPx; | 
| + } | 
| + | 
| + /** | 
| + * @return The opacity of the promo. | 
| + */ | 
| + public float getPromoOpacity() { | 
| + return mPromoOpacity; | 
| + } | 
| + | 
| + /** | 
| + * @return Y coordinate of the promo in pixels. | 
| + */ | 
| + protected float getPromoYPx() { | 
| + return Math.round((getOffsetY() + getSearchBarHeight()) / mPxToDp); | 
| 
Donn Denman
2015/04/02 19:45:00
Since you're returning a float, maybe you don't ne
 
pedro (no code reviews)
2015/04/03 21:01:02
We need to round it because it represents pixel va
 | 
| + } | 
| + | 
| + // -------------------------------------------------------------------------------------------- | 
| + // Opt In Promo states | 
| + // -------------------------------------------------------------------------------------------- | 
| + | 
| private float mPromoContentHeight; | 
| private boolean mShouldHidePromoHeader; | 
| - /** | 
| + /** | 
| * Sets the height of the promo content. | 
| */ | 
| protected void setPromoContentHeight(float height) { | 
| @@ -634,6 +731,9 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| if (state == PanelState.CLOSED) { | 
| mIsShowing = false; | 
| + destroyContextualSearchControl(); | 
| + } else if (state == PanelState.EXPANDED) { | 
| + showPromoViewAtYPosition(getPromoYPx()); | 
| } | 
| } | 
| @@ -643,6 +743,10 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| * @param height The height of the panel in dps. | 
| */ | 
| protected void setPanelHeight(float height) { | 
| + if (height != getPanelHeightFromState(PanelState.MAXIMIZED)) { | 
| 
Donn Denman
2015/04/02 19:45:00
Shouldn't this be EXPANDED, since the promo only s
 
David Trainor- moved to gerrit
2015/04/02 20:38:23
Can you explain this a bit?  Maybe I'm mixing up p
 
pedro (no code reviews)
2015/04/03 21:01:01
Added comments explaining what is going on.
 
pedro (no code reviews)
2015/04/03 21:01:02
You're right. It should be EXPANDED. Thanks for ca
 | 
| + hidePromoView(); | 
| + } | 
| + | 
| updatePanelForHeight(height); | 
| } | 
| @@ -750,6 +854,9 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| * @param percentage The completion percentage. | 
| */ | 
| private void updatePanelForCloseOrPeek(float percentage) { | 
| + // Update the opt out promo. | 
| + updatePromo(1.f); | 
| + | 
| // Base page offset. | 
| mBasePageY = 0.f; | 
| @@ -782,6 +889,9 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| * @param percentage The completion percentage. | 
| */ | 
| private void updatePanelForExpansion(float percentage) { | 
| + // Update the opt out promo. | 
| + updatePromo(1.f); | 
| + | 
| // Base page offset. | 
| float baseBaseY = MathUtils.interpolate( | 
| 0.f, | 
| @@ -834,6 +944,9 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| * @param percentage The completion percentage. | 
| */ | 
| private void updatePanelForMaximization(float percentage) { | 
| + // Update the opt out promo. | 
| + updatePromo(1.f - percentage); | 
| + | 
| // Base page offset. | 
| mBasePageY = getBasePageTargetY(); | 
| @@ -928,6 +1041,24 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| mProgressBarY = searchBarHeight - PROGRESS_BAR_HEIGHT_DP + 1; | 
| } | 
| + /** | 
| + * Updates the UI state for Opt Out Promo. | 
| + * | 
| + * @param visibilityPercentage The visibility percentage of the Promo. A visibility | 
| + * of 0 means the Promo is not visible. A visibility of 1 means the Promo is fully | 
| + * visible. And visibility between 0 and 1 means the Promo is partially visible. | 
| + */ | 
| + private void updatePromo(float visibilityPercentage) { | 
| + // TODO(pedrosimonetti): DO NOT SUBMIT! Only update Promo when enabled. We need | 
| 
David Trainor- moved to gerrit
2015/04/02 20:38:23
Ahhh!  Just reminding you about these comments :).
 
pedro (no code reviews)
2015/04/03 21:01:01
Done.
 | 
| + // to check for the Finch flag here! | 
| + // TODO(pedrosimonetti): DO NOT SUBMIT! Find where to initialize this! | 
| + mPromoVisible = true; | 
| + | 
| + mPromoHeightPx = Math.round(MathUtils.clamp(visibilityPercentage * mPromoContentHeightPx, | 
| + 0.f, mPromoContentHeightPx)); | 
| + mPromoOpacity = visibilityPercentage; | 
| + } | 
| + | 
| // ============================================================================================ | 
| // Base Page Offset | 
| // ============================================================================================ | 
| @@ -1003,7 +1134,186 @@ abstract class ContextualSearchPanelBase extends ContextualSearchPanelStateHandl | 
| } | 
| // ============================================================================================ | 
| - // Promo | 
| + // Resource Loader | 
| + // ============================================================================================ | 
| + | 
| + private ViewGroup mContainerView; | 
| + private DynamicResourceLoader mResourceLoader; | 
| + | 
| + /** | 
| + * @param resourceLoader The {@link DynamicResourceLoader} to register and unregister the view. | 
| + */ | 
| + public void setDynamicResourceLoader(DynamicResourceLoader resourceLoader) { | 
| + mResourceLoader = resourceLoader; | 
| + | 
| + if (mControl != null) { | 
| + mResourceLoader.registerResource(R.id.contextual_search_view, | 
| + mControl.getResourceAdapter()); | 
| + } | 
| + | 
| + // TODO(pedrosimonetti): DO NOT SUBMIT! Move this to the right place. | 
| 
David Trainor- moved to gerrit
2015/04/02 20:38:23
This looks okay to me.  Should this move?
 
pedro (no code reviews)
2015/04/03 21:01:01
Yes, this is in the right place now. I forgot to r
 | 
| + if (mSearchPromo != null) { | 
| + mResourceLoader.registerResource(R.id.contextual_search_opt_out_promo, | 
| + mSearchPromo.getResourceAdapter()); | 
| + } | 
| + } | 
| + | 
| + public void setContainerView(ViewGroup container) { | 
| 
David Trainor- moved to gerrit
2015/04/02 20:38:23
Javadoc
 
pedro (no code reviews)
2015/04/03 21:01:01
Done.
 | 
| + mContainerView = container; | 
| + } | 
| + | 
| + // ============================================================================================ | 
| + // ContextualSearchControl | 
| 
Donn Denman
2015/04/02 19:44:58
Nit: I would remove this divider and just merge th
 
pedro (no code reviews)
2015/04/03 21:01:02
Done.
 | 
| + // ============================================================================================ | 
| + | 
| + // TODO(pedrosimonetti): rename this to something more generic (e.g. BottomBarTextView). | 
| + | 
| + private ContextualSearchControl mControl; | 
| 
Donn Denman
2015/04/02 19:44:59
... and move this up.
 
pedro (no code reviews)
2015/04/03 21:01:01
Done.
 | 
| + | 
| + /** | 
| + * Inflates the Contextual Search control, if needed. | 
| 
Donn Denman
2015/04/02 19:44:58
Add comment that it sets/leaves the control invisi
 
pedro (no code reviews)
2015/04/03 21:01:01
Done.
 | 
| + */ | 
| + protected ContextualSearchControl getContextualSearchControl() { | 
| + assert mContainerView != null; | 
| + | 
| + if (mControl == null) { | 
| + LayoutInflater.from(mContext).inflate(R.layout.contextual_search_view, mContainerView); | 
| + mControl = (ContextualSearchControl) | 
| + mContainerView.findViewById(R.id.contextual_search_view); | 
| + if (mResourceLoader != null) { | 
| + mResourceLoader.registerResource(R.id.contextual_search_view, | 
| + mControl.getResourceAdapter()); | 
| + } | 
| + | 
| + // TODO(pedrosimonetti): DO NOT SUBMIT! Move this to the right place. | 
| + // TODO(pedrosimonetti): confirm with dtrainor@ when to inflate it | 
| + getContextualSearchOptOutPromo(); | 
| + } | 
| + | 
| + assert mControl != null; | 
| + // TODO(pedrosimonetti): For now, we're still relying on a Android View | 
| + // to render the text that appears in the Search Bar. The View will be | 
| + // invisible and will not capture events. Consider rendering the text | 
| + // in the Compositor and get rid of the View entirely. | 
| + mControl.setVisibility(View.INVISIBLE); | 
| + return mControl; | 
| + } | 
| + | 
| + protected void destroyContextualSearchControl() { | 
| + if (mControl != null) { | 
| + ((ViewGroup) mControl.getParent()).removeView(mControl); | 
| + mControl = null; | 
| + if (mResourceLoader != null) { | 
| + mResourceLoader.unregisterResource(R.id.contextual_search_view); | 
| + } | 
| + } | 
| + } | 
| + | 
| + // ============================================================================================ | 
| + // Opt Out Promo | 
| + // ============================================================================================ | 
| + | 
| + public void showPromoViewAtYPosition(float y) { | 
| + if (mSearchPromo == null) return; | 
| + | 
| + // TODO(pedrosimonetti): Confirm if this is the way to do it! | 
| + if (mSearchPromo.getLayoutParams() instanceof MarginLayoutParams) { | 
| 
David Trainor- moved to gerrit
2015/04/02 20:38:24
You don't need this instanceof check if you're the
 
pedro (no code reviews)
2015/04/03 21:01:01
Right. As you suggested, I'm using setTranslationY
 | 
| + ((MarginLayoutParams) mSearchPromo.getLayoutParams()).topMargin = (int) y; | 
| + } | 
| + | 
| + mSearchPromo.setVisibility(View.VISIBLE); | 
| + mSearchPromo.getParent().requestLayout(); | 
| 
David Trainor- moved to gerrit
2015/04/02 20:38:23
Why requestLayout/why getParent?  Same below?
 
pedro (no code reviews)
2015/04/03 21:01:02
We need to call requestLayout, otherwise the Promo
 | 
| + } | 
| + | 
| + public void hidePromoView() { | 
| + if (mSearchPromo == null) return; | 
| + | 
| + mSearchPromo.setVisibility(View.INVISIBLE); | 
| + mSearchPromo.getParent().requestLayout(); | 
| 
Donn Denman
2015/04/02 19:45:00
Do we still need this, given we'll do the same in
 
pedro (no code reviews)
2015/04/03 21:01:01
This is not necessary anymore.
 | 
| + new Handler().post(new Runnable() { | 
| 
Donn Denman
2015/04/02 19:44:59
Add a comment why this needs to be done in a runna
 
pedro (no code reviews)
2015/04/03 21:01:02
Thanks for pointing this out. The Runnable is not
 | 
| + @Override | 
| + public void run() { | 
| + mSearchPromo.getParent().requestLayout(); | 
| + } | 
| + }); | 
| + } | 
| + | 
| + // ============================================================================================ | 
| + // Opt Out Promo View | 
| + // ============================================================================================ | 
| + | 
| + // TODO(pedrosimonetti): Try to position the Promo using setTranslationY() instead of Margin | 
| + // TODO(pedrosimonetti): Try to substitute the outer Promo View to FrameLayout instead of | 
| + // LinearLayout | 
| + // TODO(pedrosimonetti): Consider maybe adding a 9.patch to avoid the hacky nested layouts in | 
| + // order to have the transparent gap at the top of the Promo View. | 
| + private ContextualSearchOptOutPromo mSearchPromo; | 
| 
Donn Denman
2015/04/02 19:44:59
nit: add a blank line after.
 
pedro (no code reviews)
2015/04/03 21:01:01
Done.
 | 
| + private ContextualSearchOptOutPromo getContextualSearchOptOutPromo() { | 
| + assert mContainerView != null; | 
| + if (mSearchPromo == null) { | 
| + LayoutInflater.from(mContext).inflate( | 
| + R.layout.contextual_search_opt_out_promo, mContainerView); | 
| + mSearchPromo = (ContextualSearchOptOutPromo) | 
| + mContainerView.findViewById(R.id.contextual_search_opt_out_promo); | 
| + if (mResourceLoader != null) { | 
| + mResourceLoader.registerResource(R.id.contextual_search_opt_out_promo, | 
| + mSearchPromo.getResourceAdapter()); | 
| + } | 
| + | 
| + // Fill in opt-out text with Settings link | 
| + TextView optOutText = | 
| + (TextView) mSearchPromo.findViewById(R.id.contextual_search_opt_out_text); | 
| + | 
| + ClickableSpan settingsLink = new ClickableSpan() { | 
| + @Override | 
| + public void onClick(View view) { | 
| + // TODO(pedrosimonetti): handle click! | 
| + } | 
| + | 
| + // Change link formatting to use our blue control color and no underline | 
| + @Override | 
| + public void updateDrawState(android.text.TextPaint textPaint) { | 
| + textPaint.setColor(mContext.getResources().getColor( | 
| + R.color.light_active_color)); | 
| + textPaint.setUnderlineText(false); | 
| + } | 
| + }; | 
| + | 
| + optOutText.setText(SpanApplier.applySpans( | 
| + mContext.getString(R.string.contextual_search_short_description), | 
| + new SpanInfo("<link>", "</link>", settingsLink))); | 
| + optOutText.setMovementMethod(LinkMovementMethod.getInstance()); | 
| + | 
| + // TODO(pedrosimonetti): handle button clicks! | 
| + | 
| + // TODO(pedrosimonetti): document! | 
| + mSearchPromo.setVisibility(View.INVISIBLE); | 
| + | 
| + // TODO(pedrosimonetti): find a better place to put this! | 
| + int widthMeasureSpec = | 
| + MeasureSpec.makeMeasureSpec(mContainerView.getWidth(), MeasureSpec.EXACTLY); | 
| + int heightMeasureSpec = MeasureSpec.makeMeasureSpec(0, MeasureSpec.UNSPECIFIED); | 
| + mSearchPromo.measure(widthMeasureSpec, heightMeasureSpec); | 
| + | 
| + setPromoContentHeightPx(mSearchPromo.getMeasuredHeight()); | 
| + } | 
| + assert mSearchPromo != null; | 
| + return mSearchPromo; | 
| + } | 
| + | 
| + // TODO(pedrosimonetti): DO NOT SUBMIT! call this! | 
| 
David Trainor- moved to gerrit
2015/04/02 20:38:23
?
 
pedro (no code reviews)
2015/04/03 21:01:02
This is being properly called now.
 | 
| + protected void destroySearchPromo() { | 
| + if (mSearchPromo != null) { | 
| + ((ViewGroup) mSearchPromo.getParent()).removeView(mSearchPromo); | 
| + mSearchPromo = null; | 
| 
David Trainor- moved to gerrit
2015/04/02 20:38:23
Does the promo go away if I dismiss/reload the con
 
pedro (no code reviews)
2015/04/03 21:01:01
For now, I'm destroying the View whenever the Pane
 | 
| + if (mResourceLoader != null) { | 
| + mResourceLoader.unregisterResource(R.id.contextual_search_view); | 
| + } | 
| + } | 
| + } | 
| + | 
| + // ============================================================================================ | 
| + // Opt In Promo | 
| // ============================================================================================ | 
| /** |