Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2096203002:
[TTS] Basic Tap Suppression functionality. (Closed)
|
Created:
4 years, 6 months ago by Donn Denman Modified:
4 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[TTS] Basic Tap Suppression functionality.
Adds a feature to suppress the Contextual Search UX when a Tap
occurs and various signals indicate that it's unlikely the user
will open the panel.
For this CL, the only two signals are:
1) Whether the user has Tapped N times without opening the panel.
2) Whether the previous Tap was in about the same location recently.
Signal #1 suppresses, signal #2 overrides the suppression by triggering.
Also minor cleanup to TapFarFromPreviousSuppression and
ContextualSearchTapState.
BUG=609918
Committed: https://crrev.com/efdef4bf3549ba781852f35b4b642a21d9878b3b
Cr-Commit-Position: refs/heads/master@{#403239}
Patch Set 1 #Patch Set 2 : Just removed a blank line. #
Total comments: 28
Patch Set 3 : Various cleanup responding to comments, added histograms to histograms.xml. #Patch Set 4 : Added compensation for Quick Answers and rebased. #
Total comments: 6
Messages
Total messages: 41 (14 generated)
donnd@chromium.org changed reviewers: + pedrosimonetti@chromium.org
Pedro, PTAL sometime Monday. Thanks!
twellington@google.com changed reviewers: + twellington@google.com
https://chromiumcodereview.appspot.com/2096203002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://chromiumcodereview.appspot.com/2096203002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:911: wasSearchContentViewSeen ? RESULTS_SEEN : RESULTS_NOT_SEEN, Will this ever get called when the panel was shown? It looks like we're only logging when suppression is enabled, but it might be beneficial to log counterfactual data as well. https://chromiumcodereview.appspot.com/2096203002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapFarFromPreviousSuppression.java (right): https://chromiumcodereview.appspot.com/2096203002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapFarFromPreviousSuppression.java:43: return mIsSelectionEmpty || wasTapCloseToPreviousTap(x, y); Shouldn't we return false (don't handle tap) if the selection is empty? https://chromiumcodereview.appspot.com/2096203002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java (right): https://chromiumcodereview.appspot.com/2096203002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:101: private boolean wasTapCloseToPreviousTap(ContextualSearchTapState tapState, int x, int y) { nit: can we combine this with the method in TapFarFromPreviousSuppression.java, maybe in the superclass w/ an extra param for the distance threshold? https://chromiumcodereview.appspot.com/2096203002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:105: // Close enough in time, also in space? The space is a 30dp square? Are we trying to detect if the user tapped on the same word? I think 30dp is probably good enough in height, but width depends on the word.. might be a good enough approximation for now. Will we use logging to determine if triggering on the second time makes sense? (e.g. high ctr means we should definitely trigger on a second close tap, low ctr means the user still wasn't trying to trigger us on the second tap).
lgtm Here are my comments. From an UX perspective, I don't like this approach very much because I think it's not consistent. It might work with a single tap, it might not. When it doesn't work, tapping again might work. But then if you use it with double tap (by using I mean opening the panel) it will start working with tap again. To make things worse, a regular (fast) double tap will not work either, cause it will select the word and show the pins, so we'll interpret it as a verbatim search (like long-press). So the message is something along the lines: try tapping. If it doesn't work, tap again, but not too fast and not too slow. Of course, we can improve this by not selecting the word (and showing the pins) upon a double tap, and I believe we should do that before experimenting with this approach with live users. For reference, this is the CL that added that behavior, and we should probably coordinate with the input folks to revert this behavior: https://codereview.chromium.org/1358263002 https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:353: new TapSuppressionHeuristics(mActivity, this, mLastTapState, x, y); We were trying to not expose the Activity and not passing it around unnecessarily, so I don't like how we're passing it here. It seems that what we really need in the TapSuppression class is the tap count. Can't we pass the count instead? On top of my head I would think we would get this information from the Policy, but I see that this class does not know about the Policy, which is unfortunate since its making decisions that depend on our "policies". Another suggestion is to get the Activity from the SelectionController.getActivity() method. This is not great either, since we would still expose the Activity, but it seems a little better IMO. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapState.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapState.java:34: float x() { Nit: To be consistent with how we name other getter methods in our codebase, shouldn't this be called getX() and getY()? https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapFarFromPreviousSuppression.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapFarFromPreviousSuppression.java:43: return mIsSelectionEmpty || wasTapCloseToPreviousTap(x, y); On 2016/06/27 18:44:01, twellington_google.com wrote: > Shouldn't we return false (don't handle tap) if the selection is empty? Tap recognition happens before any selection is issued. Empty selection here seems to be used as a proxy for determining when the previous tap was not valid/handled, given that a (unhandled) tap will cause the caret to be positioned where the used tap. And the caret is essentially a empty selection (selection with length zero). In either case, I think it's worth putting a comment here because it might not immediately obvious what this is doing. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:15: private static final int TIME_THRESHOLD_MILLISECONDS = 3000; The "second tap" is essentially a double tap and I would think 3 seconds is too much for that. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:16: private static final double TAP_TWICE_DISTANCE_SQUARED_DP = Math.pow(30, 2); The "twice" in the name here is confusing, and seems unnecessary. Is this a twice of what? Should we use the system's default value? https://developer.android.com/reference/android/view/ViewConfiguration.html#g... https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:70: if (mIsTapSuppressionEnabled) { This will be called even when was triggered by long press, isn't? If so, don't we want to filter these out? I would think the condition here would be: if (wasActivatedByTap && mIsTapSuppressionEnabled) https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:103: if (System.nanoTime() - tapState.tapTimeNanoseconds() The duration threshold does not belong in a method called wasTapCloseToPreviousTap(), as "duration" has nothing to do with "proximity". I'd suggest deleting this method and moving its content to shouldHandleSecondTap().
Theresa, PTAL. Pedro, feel free to review again! Thanks to both of you for the reviews. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:353: new TapSuppressionHeuristics(mActivity, this, mLastTapState, x, y); On 2016/06/27 22:14:29, pedrosimonetti wrote: > We were trying to not expose the Activity and not passing it around > unnecessarily, so I don't like how we're passing it here. > > It seems that what we really need in the TapSuppression class is the tap count. > Can't we pass the count instead? > > On top of my head I would think we would get this information from the Policy, > but I see that this class does not know about the Policy, which is unfortunate > since its making decisions that depend on our "policies". > > Another suggestion is to get the Activity from the > SelectionController.getActivity() method. This is not great either, since we > would still expose the Activity, but it seems a little better IMO. Passed in tap count instead of the Activity. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapState.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapState.java:34: float x() { On 2016/06/27 22:14:29, pedrosimonetti wrote: > Nit: To be consistent with how we name other getter methods in our codebase, > shouldn't this be called getX() and getY()? Done. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:887: public static void logTapSuppression(boolean wasSuppressed, boolean isSecondTap) { I decided we don't really need these -- we'll know lots of taps are suppressed just from looking at ResultsSeenByGesture. Removed. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:911: wasSearchContentViewSeen ? RESULTS_SEEN : RESULTS_NOT_SEEN, On 2016/06/27 18:44:01, twellington_google.com wrote: > Will this ever get called when the panel was shown? It looks like we're only > logging when suppression is enabled, but it might be beneficial to log > counterfactual data as well. Yes, this is called when the panel is shown for the first N times: Even though suppression is enabled, we show the panel multiple times before we actually suppress. Logging the counterfactual data is a good idea in theory, but because we have a configurable threshold for suppressing we'd need to configure the Finch control group for the counterfactual threshold we'd be evaluating. This seems overly complicated to me. We just want to have a few experiment arms at different settings and check the ResultsSeenByGesture for them to evaluate the new CTR. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapFarFromPreviousSuppression.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapFarFromPreviousSuppression.java:43: return mIsSelectionEmpty || wasTapCloseToPreviousTap(x, y); On 2016/06/27 18:44:01, twellington_google.com wrote: > Shouldn't we return false (don't handle tap) if the selection is empty? Decided to go back to the other logic (details in my other reply). Sorry for the confusion, I thought this would be simpler but there are issues. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapFarFromPreviousSuppression.java:43: return mIsSelectionEmpty || wasTapCloseToPreviousTap(x, y); On 2016/06/27 22:14:29, pedrosimonetti wrote: > On 2016/06/27 18:44:01, http://twellington_google.com wrote: > > Shouldn't we return false (don't handle tap) if the selection is empty? > > Tap recognition happens before any selection is issued. Empty selection here > seems to be used as a proxy for determining when the previous tap was not > valid/handled, given that a (unhandled) tap will cause the caret to be > positioned where the used tap. And the caret is essentially a empty selection > (selection with length zero). > > In either case, I think it's worth putting a comment here because it might not > immediately obvious what this is doing. Updated the comments to make this more clear. Using the selection should be clearer for the code, and better aligned with the purpose of this subfeature (as described in the new class comment). However this seems to be another case where the selection is not shown but the controller thinks it is. Reverted this class to the way it was before, modulo comments and name changes for accessors. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:15: private static final int TIME_THRESHOLD_MILLISECONDS = 3000; On 2016/06/27 22:14:29, pedrosimonetti wrote: > The "second tap" is essentially a double tap and I would think 3 seconds is too > much for that. I think we briefly talked with Zach about this and agreed 2 or 3 seconds seemed reasonable. I think this is different from double-tap, which is essentially a single gesture. This is basically a retry by the user. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:16: private static final double TAP_TWICE_DISTANCE_SQUARED_DP = Math.pow(30, 2); On 2016/06/27 22:14:29, pedrosimonetti wrote: > The "twice" in the name here is confusing, and seems unnecessary. > > Is this a twice of what? > > Should we use the system's default value? > https://developer.android.com/reference/android/view/ViewConfiguration.html#g... Renamed to TAP_RADIUS_SQUARED_DP. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:37: mIsSecondTap = previousTapState != null && previousTapState.wasSuppressed(); I think there were some cases where we think we're handling a second Tap but we still have not started suppressing based on count. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:70: if (mIsTapSuppressionEnabled) { On 2016/06/27 22:14:29, pedrosimonetti wrote: > This will be called even when was triggered by long press, isn't? > > If so, don't we want to filter these out? I would think the condition here would > be: > > if (wasActivatedByTap && mIsTapSuppressionEnabled) Done. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:101: private boolean wasTapCloseToPreviousTap(ContextualSearchTapState tapState, int x, int y) { On 2016/06/27 18:44:01, twellington_google.com wrote: > nit: can we combine this with the method in TapFarFromPreviousSuppression.java, > maybe in the superclass w/ an extra param for the distance threshold? I don't think it's worth combining since we're just doing some multiplication and a compare. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:103: if (System.nanoTime() - tapState.tapTimeNanoseconds() On 2016/06/27 22:14:29, pedrosimonetti wrote: > The duration threshold does not belong in a method called > wasTapCloseToPreviousTap(), as "duration" has nothing to do with "proximity". > > I'd suggest deleting this method and moving its content to > shouldHandleSecondTap(). Done. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:105: // Close enough in time, also in space? On 2016/06/27 18:44:01, twellington_google.com wrote: > Will we use logging to determine if triggering on the second time makes sense? > (e.g. high ctr means we should definitely trigger on a second close tap, low ctr > means the user still wasn't trying to trigger us on the second tap). Yes, that was my plan. https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:105: // Close enough in time, also in space? On 2016/06/27 18:44:01, twellington_google.com wrote: > The space is a 30dp square? Are we trying to detect if the user tapped on the > same word? I think 30dp is probably good enough in height, but width depends on > the word.. might be a good enough approximation for now. > > Will we use logging to determine if triggering on the second time makes sense? > (e.g. high ctr means we should definitely trigger on a second close tap, low ctr > means the user still wasn't trying to trigger us on the second tap). We're trying to determine if the user is tapping in roughly the same place, so we're implementing that as a 30 dp radius. Changed the code to make that more obvious.
twellington@chromium.org changed reviewers: + twellington@chromium.org
Overall looks good, just have a couple of questions :) https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:911: wasSearchContentViewSeen ? RESULTS_SEEN : RESULTS_NOT_SEEN, On 2016/06/28 23:39:08, Donn Denman wrote: > On 2016/06/27 18:44:01, http://twellington_google.com wrote: > > Will this ever get called when the panel was shown? It looks like we're only > > logging when suppression is enabled, but it might be beneficial to log > > counterfactual data as well. > > Yes, this is called when the panel is shown for the first N times: Even though > suppression is enabled, we show the panel multiple times before we actually > suppress. That makes sense, thanks. > Logging the counterfactual data is a good idea in theory, but because we have a > configurable threshold for suppressing we'd need to configure the Finch control > group for the counterfactual threshold we'd be evaluating. This seems overly > complicated to me. We just want to have a few experiment arms at different > settings and check the ResultsSeenByGesture for them to evaluate the new CTR. What will we use this histogram to determine? For users who use the feature, we'll have normal CTR, for users who don't use the feature, we'll have N counts in the not seen bucket (where N is the number we suppress at). Is that different than ResultsSeenByGesture for the groups where tap suppression is enabled? I think we need to be able to answer these two questions: 1. What impact does this have on overall CTR? We can determine this from ResultsSeenByGesture comparisons. 2. Did we lose any engagement (did we suppress when the user would have clicked through)? #2 I think is harder to measure (we need counter-factual data), but my instinct is any lost engagement will be really low so this doesn't seem like a big risk. We can always merge a small CL back if we need more data. https://codereview.chromium.org/2096203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java (right): https://codereview.chromium.org/2096203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:37: mIsSecondTap = previousTapState != null && previousTapState.wasSuppressed() Could the previous tap have been suppressed for a reason other than our tap suppression threshold? Do we care? https://codereview.chromium.org/2096203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:44: doSuppressTap = !shouldHandle; nit: combine with line above? https://codereview.chromium.org/2096203002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2096203002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50246: + Whether results were seen for a Tap during suppression when Tap Suppression nit: I think this description could be a bit more clear, e.g. Whether results were seen when a second tap triggered contextual search after the first tap was suppressed...
donnd@chromium.org changed reviewers: - twellington@google.com
Thanks for the review Theresa! https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:911: wasSearchContentViewSeen ? RESULTS_SEEN : RESULTS_NOT_SEEN, On 2016/06/29 01:51:57, Theresa Wellington wrote: > On 2016/06/28 23:39:08, Donn Denman wrote: > > On 2016/06/27 18:44:01, http://twellington_google.com wrote: > > > Will this ever get called when the panel was shown? It looks like we're only > > > logging when suppression is enabled, but it might be beneficial to log > > > counterfactual data as well. > > > > Yes, this is called when the panel is shown for the first N times: Even though > > suppression is enabled, we show the panel multiple times before we actually > > suppress. > > That makes sense, thanks. > > > Logging the counterfactual data is a good idea in theory, but because we have > a > > configurable threshold for suppressing we'd need to configure the Finch > control > > group for the counterfactual threshold we'd be evaluating. This seems overly > > complicated to me. We just want to have a few experiment arms at different > > settings and check the ResultsSeenByGesture for them to evaluate the new CTR. > > > What will we use this histogram to determine? For users who use the feature, > we'll have normal CTR, for users who don't use the feature, we'll have N counts > in the not seen bucket (where N is the number we suppress at). Is that > different than ResultsSeenByGesture for the groups where tap suppression is > enabled? > > I think we need to be able to answer these two questions: > 1. What impact does this have on overall CTR? We can determine this from > ResultsSeenByGesture comparisons. > 2. Did we lose any engagement (did we suppress when the user would have clicked > through)? > > #2 I think is harder to measure (we need counter-factual data), but my instinct > is any lost engagement will be really low so this doesn't seem like a big risk. > We can always merge a small CL back if we need more data. Actually these two histograms should split the overall CTR into those that happen before the count limit and those that happen after the count limit (on a second tap). Remember when we suppress we don't get either seen or not-seen (we should rename to panel opened/peeked or something like that to avoid confusion). Together they should sum to the overall CTR, which will help validate the metrics. Separately I think they are useful -- the CTR for the second tap should inform us whether folks are discovering that second tap and finding it useful. For #2 -- loss of engagement -- I had planned to just look at panel opens in response to Tap. I do think this could go down a lot, especially if people don't figure out they can long-press or tap twice and open to get the feature to come back. The feature has "gone away" in the past due to our outage, so they may just think it's happening again. We want to look at this usage to determine the best N threshold for suppression. https://codereview.chromium.org/2096203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java (right): https://codereview.chromium.org/2096203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:37: mIsSecondTap = previousTapState != null && previousTapState.wasSuppressed() On 2016/06/29 01:51:57, Theresa Wellington wrote: > Could the previous tap have been suppressed for a reason other than our tap > suppression threshold? Do we care? Yes, it could have for TapFarFromPreviousSuppression, but we don't care, as long as shouldHandleFirstTap returns true we can safely consider this a second tap. https://codereview.chromium.org/2096203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppression.java:44: doSuppressTap = !shouldHandle; On 2016/06/29 01:51:57, Theresa Wellington wrote: > nit: combine with line above? Done. https://codereview.chromium.org/2096203002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2096203002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50246: + Whether results were seen for a Tap during suppression when Tap Suppression On 2016/06/29 01:51:57, Theresa Wellington wrote: > nit: I think this description could be a bit more clear, e.g. Whether results > were seen when a second tap triggered contextual search after the first tap was > suppressed... Done.
lgtm https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java (right): https://codereview.chromium.org/2096203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchUma.java:911: wasSearchContentViewSeen ? RESULTS_SEEN : RESULTS_NOT_SEEN, On 2016/06/29 02:30:36, Donn Denman wrote: > On 2016/06/29 01:51:57, Theresa Wellington wrote: > > On 2016/06/28 23:39:08, Donn Denman wrote: > > > On 2016/06/27 18:44:01, http://twellington_google.com wrote: > > > > Will this ever get called when the panel was shown? It looks like we're > only > > > > logging when suppression is enabled, but it might be beneficial to log > > > > counterfactual data as well. > > > > > > Yes, this is called when the panel is shown for the first N times: Even > though > > > suppression is enabled, we show the panel multiple times before we actually > > > suppress. > > > > That makes sense, thanks. > > > > > Logging the counterfactual data is a good idea in theory, but because we > have > > a > > > configurable threshold for suppressing we'd need to configure the Finch > > control > > > group for the counterfactual threshold we'd be evaluating. This seems > overly > > > complicated to me. We just want to have a few experiment arms at different > > > settings and check the ResultsSeenByGesture for them to evaluate the new > CTR. > > > > > > What will we use this histogram to determine? For users who use the feature, > > we'll have normal CTR, for users who don't use the feature, we'll have N > counts > > in the not seen bucket (where N is the number we suppress at). Is that > > different than ResultsSeenByGesture for the groups where tap suppression is > > enabled? > > > > I think we need to be able to answer these two questions: > > 1. What impact does this have on overall CTR? We can determine this from > > ResultsSeenByGesture comparisons. > > 2. Did we lose any engagement (did we suppress when the user would have > clicked > > through)? > > > > #2 I think is harder to measure (we need counter-factual data), but my > instinct > > is any lost engagement will be really low so this doesn't seem like a big > risk. > > We can always merge a small CL back if we need more data. > > Actually these two histograms should split the overall CTR into those that > happen before the count limit and those that happen after the count limit (on a > second tap). Remember when we suppress we don't get either seen or not-seen (we > should rename to panel opened/peeked or something like that to avoid confusion). > Together they should sum to the overall CTR, which will help validate the > metrics. Separately I think they are useful -- the CTR for the second tap > should inform us whether folks are discovering that second tap and finding it > useful. I wasn't considering the second tap getting logged separately, makes sense now, thanks! > For #2 -- loss of engagement -- I had planned to just look at panel opens in > response to Tap. I do think this could go down a lot, especially if people > don't figure out they can long-press or tap twice and open to get the feature to > come back. The feature has "gone away" in the past due to our outage, so they > may just think it's happening again. We want to look at this usage to determine > the best N threshold for suppression. We'll just look at raw # of opens?
> We'll just look at raw # of opens? Yep, I think that should be good enough.
Thanks a lot Theresa!
donnd@chromium.org changed reviewers: + rkaplow@chromium.org
Robert, PTAL at Histograms.xml. THANKS!
lgtm
Robert, PTAL. Thanks!
donnd@chromium.org changed reviewers: + holte@chromium.org - rkaplow@chromium.org
Steven, can you pick up this review of histograms.xml? Moving Robert to cc since he may be busy.
donnd@chromium.org changed reviewers: + mpearson@chromium.org - holte@chromium.org
Mark, do you have time to look at histograms.xml in this CL? We're just adding two histograms to evaluate a tap suppression experiment for Contextual Search. If all you metrics folks are busy at the innovation week it can wait till next week! Steven and Robert moved to cc, looks like you are both at the metrics innovation week.
rkaplow@google.com changed reviewers: + rkaplow@google.com
lgtm sorry for delay. histograms lg
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/06/30 17:15:54, rkaplow1 wrote: > lgtm > > sorry for delay. histograms lg Thanks Robert!
donnd@chromium.org changed reviewers: - mpearson@chromium.org, rkaplow@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
donnd@chromium.org changed reviewers: + rkaplow@chromium.org
Adding Robert back as a reviewer just so presubmit check for OWNERS will pass.
Robert, can you stamp this LGTM again? Looks like your previous LGTM came from your google.com account and that might not work.
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [TTS] Basic Tap Suppression functionality. Adds a feature to suppress the Contextual Search UX when a Tap occurs and various signals indicate that it's unlikely the user will open the panel. For this CL, the only two signals are: 1) Whether the user has Tapped N times without opening the panel. 2) Whether the previous Tap was in about the same location recently. Signal #1 suppresses, signal #2 overrides the suppression by triggering. Also minor cleanup to TapFarFromPreviousSuppression and ContextualSearchTapState. BUG=609918 ========== to ========== [TTS] Basic Tap Suppression functionality. Adds a feature to suppress the Contextual Search UX when a Tap occurs and various signals indicate that it's unlikely the user will open the panel. For this CL, the only two signals are: 1) Whether the user has Tapped N times without opening the panel. 2) Whether the previous Tap was in about the same location recently. Signal #1 suppresses, signal #2 overrides the suppression by triggering. Also minor cleanup to TapFarFromPreviousSuppression and ContextualSearchTapState. BUG=609918 Committed: https://crrev.com/efdef4bf3549ba781852f35b4b642a21d9878b3b Cr-Commit-Position: refs/heads/master@{#403239} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/efdef4bf3549ba781852f35b4b642a21d9878b3b Cr-Commit-Position: refs/heads/master@{#403239}
Message was sent while issue was closed.
On 2016/06/30 18:55:30, rkaplow wrote: > lgtm THANKS Robert, that worked! |
