Index: chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java |
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java b/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java |
index 6234318ec73fae0d255163bce462adb6a5ece342..19ee9271e1b6f71ebeae27f1c6b8af188d9be3a4 100644 |
--- a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java |
+++ b/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java |
@@ -10,10 +10,15 @@ import org.chromium.base.VisibleForTesting; |
import org.chromium.chrome.browser.ChromeActivity; |
import org.chromium.chrome.browser.compositor.bottombar.OverlayPanel; |
import org.chromium.chrome.browser.contextualsearch.ContextualSearchBlacklist.BlacklistReason; |
+import org.chromium.chrome.browser.contextualsearch.action.ResolvedSearchAction; |
+import org.chromium.chrome.browser.contextualsearch.action.SearchAction; |
+import org.chromium.chrome.browser.contextualsearch.action.SearchActionListener; |
+import org.chromium.chrome.browser.contextualsearch.gesture.SearchGestureHost; |
import org.chromium.chrome.browser.preferences.ChromePreferenceManager; |
import org.chromium.chrome.browser.tab.Tab; |
import org.chromium.content.browser.ContentViewCore; |
import org.chromium.content_public.browser.GestureStateListener; |
+import org.chromium.content_public.browser.WebContents; |
import org.chromium.ui.touch_selection.SelectionEventType; |
import java.util.regex.Matcher; |
@@ -22,7 +27,7 @@ import java.util.regex.Pattern; |
/** |
* Controls selection gesture interaction for Contextual Search. |
*/ |
-public class ContextualSearchSelectionController { |
+public class ContextualSearchSelectionController implements SearchGestureHost { |
Theresa
2016/08/16 15:41:49
Longer term this class will become SearchGestureCo
Donn Denman
2016/08/17 04:35:21
I believe this class becomes the SearchGestureReco
pedro (no code reviews)
2016/08/22 20:54:17
Yes, this class is doing lots of things, and ideal
Donn Denman
2016/08/23 23:21:46
Acknowledged.
|
/** |
* The type of selection made by the user. |
@@ -79,10 +84,19 @@ public class ContextualSearchSelectionController { |
// The time of the most last scroll activity, or 0 if none. |
private long mLastScrollTimeNs; |
+ // When the last tap gesture happened. |
+ private long mTapTimeNanoseconds; |
+ |
// Tracks whether a Context Menu has just been shown and the UX has been dismissed. |
// The selection may be unreliable until the next reset. See crbug.com/628436. |
private boolean mIsContextMenuShown; |
+ // Set to true when we have identified that a pending tap has not been handled by Blink. |
+ private boolean mHasIdentifiedUnhandledTap; |
+ |
+ // The current Search action. |
+ private SearchAction mSearchAction; |
+ |
private class ContextualSearchGestureStateListener extends GestureStateListener { |
@Override |
public void onScrollStarted(int scrollOffsetY, int scrollExtentY) { |
@@ -113,6 +127,8 @@ public class ContextualSearchSelectionController { |
// handles (have a long-press selection) and the tap was consumed. |
if (!(consumed && mSelectionType == SelectionType.LONG_PRESS)) { |
scheduleInvalidTapNotification(); |
+ |
+ if (mHasIdentifiedUnhandledTap) createSearchAction(); |
Theresa
2016/08/16 15:41:49
This is happening before the selection is establis
Donn Denman
2016/08/17 04:35:21
That's right.
pedro (no code reviews)
2016/08/22 20:54:17
Not sure this is the right place to call createAct
Donn Denman
2016/08/23 23:21:46
In this initial integration of the refactoring we'
pedro (no code reviews)
2016/08/31 22:38:22
I think it should also be destroyed when a long pr
|
} |
} |
} |
@@ -338,6 +354,7 @@ public class ContextualSearchSelectionController { |
mLastTapState = null; |
mLastScrollTimeNs = 0; |
mIsContextMenuShown = false; |
+ mHasIdentifiedUnhandledTap = false; |
Theresa
2016/08/16 15:41:49
nit: reset mTapTimeNanos here too?
Donn Denman
2016/08/17 04:35:21
Good idea, done.
|
} |
/** |
@@ -360,6 +377,8 @@ public class ContextualSearchSelectionController { |
/** |
* Handles an unhandled tap gesture. |
+ * @param x The x coordinate. |
+ * @param y The y coordinate. |
*/ |
void handleShowUnhandledTapUIIfNeeded(int x, int y) { |
mWasTapGestureDetected = false; |
@@ -367,37 +386,16 @@ public class ContextualSearchSelectionController { |
// TODO(donnd): refactor to avoid needing a new handler API method as suggested by Pedro. |
if (mSelectionType != SelectionType.LONG_PRESS) { |
mWasTapGestureDetected = true; |
- long tapTimeNanoseconds = System.nanoTime(); |
- // TODO(donnd): add a policy method to get adjusted tap count. |
- ChromePreferenceManager prefs = ChromePreferenceManager.getInstance(mActivity); |
- int adjustedTapsSinceOpen = prefs.getContextualSearchTapCount() |
- - prefs.getContextualSearchTapQuickAnswerCount(); |
- TapSuppressionHeuristics tapHeuristics = |
- new TapSuppressionHeuristics(this, mLastTapState, x, y, adjustedTapsSinceOpen); |
- // TODO(donnd): Move to be called when the panel closes to work with states that change. |
- tapHeuristics.logConditionState(); |
- // Tell the manager what it needs in order to log metrics on whether the tap would have |
- // been suppressed if each of the heuristics were satisfied. |
- mHandler.handleMetricsForWouldSuppressTap(tapHeuristics); |
+ mTapTimeNanoseconds = System.nanoTime(); |
+ |
+ // NOTE(donnd): We first acknowledge that a unhandled tap was identified, but |
+ // we don't do anything now. Instead we'll wait for the onSingleTap() event to fire, |
+ // and only do something when an unhandled tap was identified. onSingleTap() will |
+ // always get fired, as opposed to showUnhandledTapUIIfNeeded(). |
+ mHasIdentifiedUnhandledTap = true; |
+ |
mX = x; |
mY = y; |
- boolean shouldSuppressTap = tapHeuristics.shouldSuppressTap(); |
- if (shouldSuppressTap) { |
- mHandler.handleSuppressedTap(); |
- } else { |
- // TODO(donnd): Find a better way to determine that a navigation will be triggered |
- // by the tap, or merge with other time-consuming actions like gathering surrounding |
- // text or detecting page mutations. |
- new Handler().postDelayed(new Runnable() { |
- @Override |
- public void run() { |
- mHandler.handleValidTap(); |
- } |
- }, TAP_NAVIGATION_DETECTION_DELAY); |
- } |
- // Remember the tap state for subsequent tap evaluation. |
- mLastTapState = |
- new ContextualSearchTapState(x, y, tapTimeNanoseconds, shouldSuppressTap); |
} else { |
// Long press; reset last tap state. |
mLastTapState = null; |
@@ -406,6 +404,99 @@ public class ContextualSearchSelectionController { |
} |
/** |
+ * Processes a {@link SearchAction}. |
+ * This should be called when the associated {@code SearchAction} has built its context (by |
+ * gathering surrounding text if needed, etc). |
+ * @param searchAction The {@link SearchAction} for this Tap gesture. |
+ * @param x The x coordinate. |
+ * @param y The y coordinate. |
+ */ |
+ void processSearchAction(SearchAction searchAction, int x, int y) { |
+ // TODO(donnd): use the supplied searchAction! |
Theresa
2016/08/16 15:41:49
The plan is to move these suppressions into the se
Donn Denman
2016/08/17 04:35:21
That's right.
|
+ // TODO(donnd): add a policy method to get adjusted tap count. |
+ ChromePreferenceManager prefs = ChromePreferenceManager.getInstance(mActivity); |
+ int adjustedTapsSinceOpen = prefs.getContextualSearchTapCount() |
+ - prefs.getContextualSearchTapQuickAnswerCount(); |
+ TapSuppressionHeuristics tapHeuristics = new TapSuppressionHeuristics( |
+ this, mLastTapState, x, y, adjustedTapsSinceOpen); |
+ // TODO(donnd): Move to be called when the panel closes to work with states that change. |
+ tapHeuristics.logConditionState(); |
+ // Tell the manager what it needs in order to log metrics on whether the tap would have |
+ // been suppressed if each of the heuristics were satisfied. |
+ mHandler.handleMetricsForWouldSuppressTap(tapHeuristics); |
+ |
+ boolean shouldSuppressTap = tapHeuristics.shouldSuppressTap(); |
+ if (shouldSuppressTap) { |
+ mHandler.handleSuppressedTap(); |
pedro (no code reviews)
2016/08/22 20:54:17
Should a suppressed gesture cause the action to be
Donn Denman
2016/08/23 23:21:46
I think it's simplest to just destroy the search a
|
+ } else { |
+ // TODO(donnd): Find a better way to determine that a navigation will be triggered |
+ // by the tap, or merge with other time-consuming actions like gathering surrounding |
+ // text or detecting page mutations. |
+ new Handler().postDelayed(new Runnable() { |
+ @Override |
+ public void run() { |
+ mHandler.handleValidTap(); |
+ } |
+ }, TAP_NAVIGATION_DETECTION_DELAY); |
+ } |
+ // Remember the tap state for subsequent tap evaluation. |
+ mLastTapState = new ContextualSearchTapState(x, y, mTapTimeNanoseconds, shouldSuppressTap); |
+ } |
+ |
+ /** |
+ * Creates the current {@link SearchAction}. |
+ */ |
+ void createSearchAction() { |
+ System.out.println("ctxs createSearchAction"); |
+ mSearchAction = new ResolvedSearchAction(new SearchActionListener() { |
+ |
+ @Override |
+ protected void onContextReady(SearchAction action) { |
+ System.out.println("ctxs onContextReady"); |
+ processSearchAction(action, (int) mX, (int) mY); |
+ } |
+ |
+ @Override |
+ protected void onActionAccepted(SearchAction action) { |
+ System.out.println("ctxs onActionAccepted"); |
+ destroySearchAction(); |
pedro (no code reviews)
2016/08/22 20:54:17
Why are we destroying the action here?
Donn Denman
2016/08/23 23:21:46
I guess we don't need to destroy the action, so re
|
+ } |
+ }); |
+ |
+ mSearchAction.extractContext(this); |
+ } |
+ |
+ /** |
+ * Destroys the current {@link SearchAction}. |
+ */ |
+ private void destroySearchAction() { |
+ System.out.println("ctxs destroySearchAction"); |
+ if (mSearchAction == null) return; |
+ |
+ mSearchAction.destroyAction(); |
+ mSearchAction = null; |
+ } |
+ |
+ // ============================================================================================ |
+ // SearchGestureHost |
+ // ============================================================================================ |
+ |
+ @Override |
+ public WebContents getTabWebContents() { |
+ ContentViewCore baseCvc = getBaseContentView(); |
Theresa
2016/08/16 15:41:49
I believe we're trying to remove CVC dependencies
Donn Denman
2016/08/17 04:35:21
Reworked this to not use a CVC, and added a TODO (
pedro (no code reviews)
2016/08/22 20:54:17
I'd call this baseContentViewCore or simply conten
Donn Denman
2016/08/23 23:21:46
Done -- code changed to not use CVC at all.
|
+ if (baseCvc == null) { |
+ return null; |
+ } |
+ return baseCvc.getWebContents(); |
+ } |
+ |
+ @Override |
+ public void dismissGesture() { |
+ System.out.println("ctxs dismissGesture"); |
+ destroySearchAction(); |
+ } |
+ |
+ /** |
* @return The Base Page's {@link ContentViewCore}, or {@code null} if there is no current tab. |
*/ |
ContentViewCore getBaseContentView() { |
Donn Denman
2016/08/17 04:35:21
Added a TODO here to stop using CVC.
|