Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
53283003:
Added UMA stat for tracking accidental navigations on double tap. (Closed)
|
Created:
7 years, 1 month ago by bokan Modified:
7 years, 1 month ago Reviewers:
WRONG-USE-chromium, jakearchibald, jdduke (slow), jar (doing other things), bulach, Rick Byers, Ted C, Alexei Svitkine (slow) CC:
chromium-reviews, jar (doing other things), jam, joi+watch-content_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, Ilya Sherman, aelias_OOO_until_Jul13 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded UMA stat for tracking accidental navigations on double tap.
Adding UMA stat to see the effect of the --disable-click-delay flag
on accidental navigations caused by double tap.
Added two UMA enum histograms, one for when click delays are enabled
and one for disabled. The stats track the first user action within
5 seconds of a double tap gesture. The three possibilities are
"navigate back", "stop navigation", and "no action". No action means
we didn't navigate back or stop load within the 5 seconds.
Note, this only tracks navigations within the same ContentView, if a
navigation opens a new tab, the back/stop for those wont be recorded.
However, in practice this shouldn't make a big difference.
BUG=312735
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233676
Patch Set 1 #
Total comments: 14
Patch Set 2 : #Patch Set 3 : #
Total comments: 13
Patch Set 4 : #
Total comments: 9
Patch Set 5 : #
Total comments: 28
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 1
Patch Set 9 : #Patch Set 10 : Made nested classes static #
Total comments: 39
Patch Set 11 : #
Total comments: 1
Patch Set 12 : #
Messages
Total messages: 34 (0 generated)
I've added the UMA stats as I think the bug describes them. I've played with it a bit (with output to the log) and it seems reasonable. If this looks ok I can loop in OWNERS.
Just curious, what kind of threshold are we looking for to deem the accidental navigation (un)acceptable?
On 2013/10/30 21:38:04, jdduke wrote: > Just curious, what kind of threshold are we looking for to deem the accidental > navigation (un)acceptable? It was brought up in the e-mail thread. I think Jake mentioned a >5% increase in the stat without delay would be concerning.
On 2013/10/30 21:45:03, bokan wrote: > On 2013/10/30 21:38:04, jdduke wrote: > > Just curious, what kind of threshold are we looking for to deem the accidental > > navigation (un)acceptable? > > It was brought up in the e-mail thread. I think Jake mentioned a >5% increase in > the stat without delay would be concerning. Got it, thanks.
On 2013/10/30 21:47:37, jdduke wrote: > On 2013/10/30 21:45:03, bokan wrote: > > On 2013/10/30 21:38:04, jdduke wrote: > > > Just curious, what kind of threshold are we looking for to deem the > accidental > > > navigation (un)acceptable? > > > > It was brought up in the e-mail thread. I think Jake mentioned a >5% increase > in > > the stat without delay would be concerning. > > Got it, thanks. I don't think we can really say until we know what the baseline is. I think Jake's "+5%" number was based on my example of a 2% baseline (so 2% to 7% would be cause for concern). I wonder if we should also record a count of taps so that we can show the other side of the trade off.
On 2013/10/31 16:58:52, Rick Byers wrote: > On 2013/10/30 21:47:37, jdduke wrote: > > On 2013/10/30 21:45:03, bokan wrote: > > > On 2013/10/30 21:38:04, jdduke wrote: > > > > Just curious, what kind of threshold are we looking for to deem the > > accidental > > > > navigation (un)acceptable? > > > > > > It was brought up in the e-mail thread. I think Jake mentioned a >5% > increase > > in > > > the stat without delay would be concerning. > > > > Got it, thanks. > > I don't think we can really say until we know what the baseline is. I think > Jake's "+5%" number was based on my example of a 2% baseline (so 2% to 7% would > be cause for concern). > > I wonder if we should also record a count of taps so that we can show the other > side of the trade off. Hmm, what do you mean? No one will be using double tap to intentionally navigate to a new page so what would this give us?
On 2013/10/31 17:07:21, bokan wrote: > On 2013/10/31 16:58:52, Rick Byers wrote: > > On 2013/10/30 21:47:37, jdduke wrote: > > > On 2013/10/30 21:45:03, bokan wrote: > > > > On 2013/10/30 21:38:04, jdduke wrote: > > > > > Just curious, what kind of threshold are we looking for to deem the > > > accidental > > > > > navigation (un)acceptable? > > > > > > > > It was brought up in the e-mail thread. I think Jake mentioned a >5% > > increase > > > in > > > > the stat without delay would be concerning. > > > > > > Got it, thanks. > > > > I don't think we can really say until we know what the baseline is. I think > > Jake's "+5%" number was based on my example of a 2% baseline (so 2% to 7% > would > > be cause for concern). > > > > I wonder if we should also record a count of taps so that we can show the > other > > side of the trade off. > > Hmm, what do you mean? No one will be using double tap to intentionally navigate > to a new page so what would this give us? Does the tap down ack disposition from the renderer provide us with anything meaningful? If so, perhaps a better stat is the relative number of "consumed" tap downs on double tap.
On 2013/10/31 17:11:13, jdduke wrote: > On 2013/10/31 17:07:21, bokan wrote: > > On 2013/10/31 16:58:52, Rick Byers wrote: > > > On 2013/10/30 21:47:37, jdduke wrote: > > > > On 2013/10/30 21:45:03, bokan wrote: > > > > > On 2013/10/30 21:38:04, jdduke wrote: > > > > > > Just curious, what kind of threshold are we looking for to deem the > > > > accidental > > > > > > navigation (un)acceptable? > > > > > > > > > > It was brought up in the e-mail thread. I think Jake mentioned a >5% > > > increase > > > > in > > > > > the stat without delay would be concerning. > > > > > > > > Got it, thanks. > > > > > > I don't think we can really say until we know what the baseline is. I think > > > Jake's "+5%" number was based on my example of a 2% baseline (so 2% to 7% > > would > > > be cause for concern). > > > > > > I wonder if we should also record a count of taps so that we can show the > > other > > > side of the trade off. > > > > Hmm, what do you mean? No one will be using double tap to intentionally > navigate > > to a new page so what would this give us? It tells us the false-positive rate of our metric. Presumably it's at least occasionally possible that someone will zoom then immediately go back. Hopefully you're right and it'll be essentially zero, but we still need to know the baseline in order to talk meaningfully about the significance of any specific result. > Does the tap down ack disposition from the renderer provide us with anything > meaningful? If so, perhaps a better stat is the relative number of "consumed" > tap downs on double tap. tapDown, no. But the tap disposition should be at least partly useful in theory. Using that occurred to me as well, but I'm concerned that there will be many cases where it's wrong. Eg. I don't think people are in the habit of calling preventDefault from their JS click handlers (as it has little/no effect). We could focus primarily on the case of link navigation if we thought that was sufficient (although there's likely to be some false positive rate we won't be able to see in this approach - people accidentally double tapping when they intended to tap).
Looks good, just a few suggestions. https://codereview.chromium.org/53283003/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right): https://codereview.chromium.org/53283003/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:225: mContentViewCore.updateActionAfterDoubleTapUMA( since this is pretty general (has nothing to do with double tap here) perhaps it should have a more general name - like 'reportAction'. https://codereview.chromium.org/53283003/diff/1/content/browser/android/conte... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/1/content/browser/android/conte... content/browser/android/content_view_core_impl.cc:1624: // accidental navigations after a double tap nit - missing . https://codereview.chromium.org/53283003/diff/1/content/browser/android/conte... content/browser/android/content_view_core_impl.cc:1627: type, 3); ugh - hard-coding this 3 is a little unfortunate, but I see the java/native boundary makes it awkward to do the normal 'MAX' enum thing. How about just adding a comment to the enum definition saying to change this line if more elements are added. https://codereview.chromium.org/53283003/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/53283003/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:459: private static final long ACTION_AFTER_DOUBLE_TAP_WINDOW = 5000; add an _MS suffix to make it clear this is milliseconds. https://codereview.chromium.org/53283003/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:780: mHasClickDelay = !CommandLine.getInstance().hasSwitch(CommandLine.DISABLE_CLICK_DELAY); If you move the logic in this file into ContentViewGestureHandler (and plumb the reportAction... method form here into it), then you could avoid checking this command line again. Would be slightly cleaner I think. https://codereview.chromium.org/53283003/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1360: updateDoubleTapUmaTimer(timeMs); what exactly is this timeMs here - the original event timestamp? Probably better to use the current wall clock time here. Eg. if there was a lot of main thread jank than the double tap won't have actually happened until awhile after the event timestamp. https://codereview.chromium.org/53283003/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1372: mLastDoubleTapTime = timeMs; If we didn't report an UMA above but mLastDoubleTapTime is set, then we should be reporting a NO_ACTION metric here. Otherwise a constant stream of double tap events wouldn't report the metric at all...
Addressed all of Rick's feedback. I made the "Action Reporting" more generic and moved it into the GestureHandler. I also added a metric for "Delayed Taps" which will give us an idea of how often users are sending a tap down that's delayed (i.e. flag is off and page is non-mobile). https://codereview.chromium.org/53283003/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right): https://codereview.chromium.org/53283003/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:225: mContentViewCore.updateActionAfterDoubleTapUMA( On 2013/10/31 17:53:10, Rick Byers wrote: > since this is pretty general (has nothing to do with double tap here) perhaps it > should have a more general name - like 'reportAction'. Changed to reportActionForUMA https://codereview.chromium.org/53283003/diff/1/content/browser/android/conte... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/1/content/browser/android/conte... content/browser/android/content_view_core_impl.cc:1624: // accidental navigations after a double tap On 2013/10/31 17:53:10, Rick Byers wrote: > nit - missing . Done. https://codereview.chromium.org/53283003/diff/1/content/browser/android/conte... content/browser/android/content_view_core_impl.cc:1627: type, 3); On 2013/10/31 17:53:10, Rick Byers wrote: > ugh - hard-coding this 3 is a little unfortunate, but I see the java/native > boundary makes it awkward to do the normal 'MAX' enum thing. How about just > adding a comment to the enum definition saying to change this line if more > elements are added. It's already there :). But I've gone further and just made the count a param into this function so updating the enum will be enough. https://codereview.chromium.org/53283003/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/53283003/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:459: private static final long ACTION_AFTER_DOUBLE_TAP_WINDOW = 5000; On 2013/10/31 17:53:10, Rick Byers wrote: > add an _MS suffix to make it clear this is milliseconds. Done. https://codereview.chromium.org/53283003/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:780: mHasClickDelay = !CommandLine.getInstance().hasSwitch(CommandLine.DISABLE_CLICK_DELAY); On 2013/10/31 17:53:10, Rick Byers wrote: > If you move the logic in this file into ContentViewGestureHandler (and plumb the > reportAction... method form here into it), then you could avoid checking this > command line again. Would be slightly cleaner I think. Done. https://codereview.chromium.org/53283003/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1360: updateDoubleTapUmaTimer(timeMs); On 2013/10/31 17:53:10, Rick Byers wrote: > what exactly is this timeMs here - the original event timestamp? Probably > better to use the current wall clock time here. Eg. if there was a lot of main > thread jank than the double tap won't have actually happened until awhile after > the event timestamp. It was supposed to be the current time. I've changed to use the current clock time. https://codereview.chromium.org/53283003/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1372: mLastDoubleTapTime = timeMs; On 2013/10/31 17:53:10, Rick Byers wrote: > If we didn't report an UMA above but mLastDoubleTapTime is set, then we should > be reporting a NO_ACTION metric here. Otherwise a constant stream of double tap > events wouldn't report the metric at all... Done. https://codereview.chromium.org/53283003/diff/180001/tools/metrics/actions/ch... File tools/metrics/actions/chromeactions.txt (right): https://codereview.chromium.org/53283003/diff/180001/tools/metrics/actions/ch... tools/metrics/actions/chromeactions.txt:1664: 0x91586aefc4a53dc7 VirtualKeyboardLoaded extract_actions.py --hash added this but it's not related to my CL...what should I do about this?
Thanks David! A few more things to discuss... https://codereview.chromium.org/53283003/diff/180001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/180001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1627: content::RecordAction(content::UserMetricsAction("DelayedGestureTap")); This is OK (I think it gives us the main measure we want), but I was actually thinking we'd use a histogram with two values - delayed or not delayed. That way we can also chart what fraction of taps are delayed over time (as a result of all our efforts to delay fewer taps). Sorry I wasn't clear about that. What do you think - worth changing? https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:462: public static class UMAUserAction { Can't you use a Java enum type here? Looks like we do elsewhere. https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:470: // Not passed to UMA stats - used internally only Seems kind of hacky to include these in the enum... https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1414: public void sendUserActionUMA(int actionType, boolean clickDelayEnabled) { Is it not possible/easy to call into native directly form ContentGestureHandler? I didn't realize we'd have to call back into this class like this - sorry... https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1288: if (actionType == ContentViewCore.UMAUserAction.DOUBLE_TAP) { I think it would be cleaner to just handle this case in a separate function. Then you wouldn't need to "pollute" your enum with this special internal-only value. https://codereview.chromium.org/53283003/diff/180001/tools/metrics/actions/ch... File tools/metrics/actions/chromeactions.txt (right): https://codereview.chromium.org/53283003/diff/180001/tools/metrics/actions/ch... tools/metrics/actions/chromeactions.txt:1664: 0x91586aefc4a53dc7 VirtualKeyboardLoaded On 2013/10/31 22:31:59, bokan wrote: > extract_actions.py --hash added this but it's not related to my CL...what should > I do about this? I'd remove it, just to avoid polluting your CL (although of course someone should be adding it - maybe ping whoever added it without running extract_actions...
https://codereview.chromium.org/53283003/diff/180001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/180001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1627: content::RecordAction(content::UserMetricsAction("DelayedGestureTap")); On 2013/11/01 14:45:09, Rick Byers wrote: > This is OK (I think it gives us the main measure we want), but I was actually > thinking we'd use a histogram with two values - delayed or not delayed. That > way we can also chart what fraction of taps are delayed over time (as a result > of all our efforts to delay fewer taps). Sorry I wasn't clear about that. What > do you think - worth changing? Yup, makes sense and shouldn't be hard. https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:462: public static class UMAUserAction { On 2013/11/01 14:45:09, Rick Byers wrote: > Can't you use a Java enum type here? Looks like we do elsewhere. Changed. https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:470: // Not passed to UMA stats - used internally only On 2013/11/01 14:45:09, Rick Byers wrote: > Seems kind of hacky to include these in the enum... I cleaned it up. PTAL https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1414: public void sendUserActionUMA(int actionType, boolean clickDelayEnabled) { On 2013/11/01 14:45:09, Rick Byers wrote: > Is it not possible/easy to call into native directly form ContentGestureHandler? > I didn't realize we'd have to call back into this class like this - sorry... I don't think it has a binding class, there's an interface (MotionEventDelegate) in GestureHandler this class implements that has a comment saying that's how GestureHandler calls into native. I just added this method to that interface. https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/53283003/diff/180001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1288: if (actionType == ContentViewCore.UMAUserAction.DOUBLE_TAP) { On 2013/11/01 14:45:09, Rick Byers wrote: > I think it would be cleaner to just handle this case in a separate function. > Then you wouldn't need to "pollute" your enum with this special internal-only > value. Done. https://codereview.chromium.org/53283003/diff/180001/tools/metrics/actions/ch... File tools/metrics/actions/chromeactions.txt (right): https://codereview.chromium.org/53283003/diff/180001/tools/metrics/actions/ch... tools/metrics/actions/chromeactions.txt:1664: 0x91586aefc4a53dc7 VirtualKeyboardLoaded On 2013/11/01 14:45:09, Rick Byers wrote: > On 2013/10/31 22:31:59, bokan wrote: > > extract_actions.py --hash added this but it's not related to my CL...what > should > > I do about this? > > I'd remove it, just to avoid polluting your CL (although of course someone > should be adding it - maybe ping whoever added it without running > extract_actions... Since the tap is now a histogram this file goes away.
Thanks, LGTM with nits! https://codereview.chromium.org/53283003/diff/260001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/260001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1629: UMA_HISTOGRAM_ENUMERATION("UserInput.SingleTapType", Nit: rather than create a new UserInput top level, it's probably nicer to re-use either 'Touchscreen' or 'Event'. https://codereview.chromium.org/53283003/diff/260001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/53283003/diff/260001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:342: * Send action after dobule tap for UMA stat tracking nit - missing period here and below. https://codereview.chromium.org/53283003/diff/260001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:342: * Send action after dobule tap for UMA stat tracking nit - Looks like convention is to add javadoc comments for parameters, please add here and below. https://codereview.chromium.org/53283003/diff/260001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:548: mMotionEventDelegate.sendSingleTapUMA( nit - don't repeat the call to the function, just make the parameter be conditional (inline or with a local variable). Eg: isDoubleTapDisabled || mDisableClickDelay ? UNDELAYED_TAP : DELAYED_TAP;
Adding bulach@ and jar@ for OWNERS https://codereview.chromium.org/53283003/diff/260001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/260001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1629: UMA_HISTOGRAM_ENUMERATION("UserInput.SingleTapType", On 2013/11/01 19:27:03, Rick Byers wrote: > Nit: rather than create a new UserInput top level, it's probably nicer to re-use > either 'Touchscreen' or 'Event'. Changed these to Event - Touchscreen doesn't seem to exist in histogram.xml https://codereview.chromium.org/53283003/diff/260001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/53283003/diff/260001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:342: * Send action after dobule tap for UMA stat tracking On 2013/11/01 19:27:03, Rick Byers wrote: > nit - missing period here and below. Done. https://codereview.chromium.org/53283003/diff/260001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:342: * Send action after dobule tap for UMA stat tracking On 2013/11/01 19:27:03, Rick Byers wrote: > nit - Looks like convention is to add javadoc comments for parameters, please > add here and below. Done. https://codereview.chromium.org/53283003/diff/260001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:548: mMotionEventDelegate.sendSingleTapUMA( On 2013/11/01 19:27:03, Rick Byers wrote: > nit - don't repeat the call to the function, just make the parameter be > conditional (inline or with a local variable). Eg: > isDoubleTapDisabled || mDisableClickDelay ? UNDELAYED_TAP : DELAYED_TAP; Done.
https://codereview.chromium.org/53283003/diff/260001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/260001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1629: UMA_HISTOGRAM_ENUMERATION("UserInput.SingleTapType", On 2013/11/01 19:54:02, bokan wrote: > On 2013/11/01 19:27:03, Rick Byers wrote: > > Nit: rather than create a new UserInput top level, it's probably nicer to > re-use > > either 'Touchscreen' or 'Event'. > Changed these to Event - Touchscreen doesn't seem to exist in histogram.xml Ah, this is another one that's still stuck in the old google-internal histograms.xml file (https://chromegw/viewvc/chrome-internal/trunk/tools/histograms/histograms.xml...). It should probably get moved to the public one. You can go ahead and use it if you like (does seem a little more appropriate than Event I think). Eg. see usage of it here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...
https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:551: isDoubleTapDisabled() || mDisableClickDelay ? I'm not sure it makes complete sense to group |isDoubleTapDisabled()| and |mDisableClickDelay| together. If |isDoubleTapDisabled()| is true, double-taps become two distinct taps. There will never be a corresponding DOUBLE_TAP gesture, so it really shouldn't contribute to the "accidental" tap statistic. I wonder if three categories would improve the situation? UNDELAYED, ARTIFICIALLY_UNDELAYED, and DELAYED, where UNDELAYED = isDoubleTapDisabled(), and ARTIFICIALLY_UNDELAYED = mDisableClickDelay && !IsDoubleTapDisabled() In that case I wonder if we should also be tracking GESTURE_SINGLE_TAP_UP...
+cc Alexei for histograms.xml review
https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1625: JNIEnv* env, Nit: move this to the line above and align the other args, same for the function below. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1630: type, count); Nit: These can fit on the previous line. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1637: bool hasDelay, Nit: has_delay https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1643: if(hasDelay) { Nit: Space after if https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1645: type, count); Nit: Can these fit on the previous line? If not, this needs to indent 2 more (same for below). https://codereview.chromium.org/53283003/diff/330001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/53283003/diff/330001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:3792: + <summary>The number of gesture taps that are delayed vs undelayed</summary> This description really needs more context. When is it logged? What does delayed vs undelayed mean?
https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1625: JNIEnv* env, On 2013/11/02 00:12:39, Alexei Svitkine wrote: > Nit: move this to the line above and align the other args, same for the function > below. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1630: type, count); On 2013/11/02 00:12:39, Alexei Svitkine wrote: > Nit: These can fit on the previous line. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1637: bool hasDelay, On 2013/11/02 00:12:39, Alexei Svitkine wrote: > Nit: has_delay https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1643: if(hasDelay) { On 2013/11/02 00:12:39, Alexei Svitkine wrote: > Nit: Space after if https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1645: type, count); On 2013/11/02 00:12:39, Alexei Svitkine wrote: > Nit: Can these fit on the previous line? If not, this needs to indent 2 more > (same for below). https://codereview.chromium.org/53283003/diff/330001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/53283003/diff/330001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:3792: + <summary>The number of gesture taps that are delayed vs undelayed</summary> On 2013/11/02 00:12:39, Alexei Svitkine wrote: > This description really needs more context. When is it logged? What does delayed > vs undelayed mean?
(Sorry for the spam, rietveld gave me some errors when sending comments...)
https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:551: isDoubleTapDisabled() || mDisableClickDelay ? On 2013/11/01 20:17:46, jdduke wrote: > > I'm not sure it makes complete sense to group |isDoubleTapDisabled()| and > |mDisableClickDelay| together. If |isDoubleTapDisabled()| is true, double-taps > become two distinct taps. There will never be a corresponding DOUBLE_TAP > gesture, so it really shouldn't contribute to the "accidental" tap statistic. The idea with this metric is completely separate from the "accidental tap statistic" stuff we're doing elsewhere. The intention here is to provide some measure of the potential amount of time to be gained by disabling the click delay (the other side of the cost/benefit trade-off). > I wonder if three categories would improve the situation? UNDELAYED, > ARTIFICIALLY_UNDELAYED, and DELAYED, where > > UNDELAYED = isDoubleTapDisabled(), and > ARTIFICIALLY_UNDELAYED = mDisableClickDelay && !IsDoubleTapDisabled() Yes, thinking about it more, I agree we probably don't want to lump the two undelayed cases together (since we're trying to evaluate the benefit of disabling the click delay). Actually I think it would be fine to ignore mDisableClickDelay entirely here and just report taps what would normally be delayed vs. those that wouldn't (so the 'experiment' with the flag is irrelevant for this measure). That would be simplest. There's no harm in having all 3 states if you think there's value in capturing the extra data though. > In that case I wonder if we should also be tracking GESTURE_SINGLE_TAP_UP... Ah yes, it looks like we should be counting that case too. It sounds like it's a pretty rare edge case though, right? If it's not going to move the statistic much then it's probably not worth worrying about.
drive-by: https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.h:227: int type, jint rather than int https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.h:228: bool hasDelay, nit: has_delay https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:462: public enum UMAActionAfterDoubleTap { can't have enums on android java, it's really frowned upon due to its overhead.. please use static final int constants for the values (yeah, loosing typesafety is a known-issue :)
https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:462: public enum UMAActionAfterDoubleTap { On 2013/11/04 18:22:31, bulach wrote: > can't have enums on android java, it's really frowned upon due to its overhead.. > please use static final int constants for the values (yeah, loosing typesafety > is a known-issue :) Sorry, my bad - I asked bokan to switch to using real enums for the type safety! I searched and found some other examples (eg. org.chromium.android_webview.LayoutAlgorithm and org.chromium.chrome.browser.Type).
Thanks for feedback all, ptal. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1625: JNIEnv* env, On 2013/11/02 00:12:39, Alexei Svitkine wrote: > Nit: move this to the line above and align the other args, same for the function > below. Done. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1630: type, count); On 2013/11/02 00:12:39, Alexei Svitkine wrote: > Nit: These can fit on the previous line. Done. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1637: bool hasDelay, On 2013/11/02 00:12:39, Alexei Svitkine wrote: > Nit: has_delay Done. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1643: if(hasDelay) { On 2013/11/02 00:12:39, Alexei Svitkine wrote: > Nit: Space after if Done. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1645: type, count); On 2013/11/02 00:12:39, Alexei Svitkine wrote: > Nit: Can these fit on the previous line? If not, this needs to indent 2 more > (same for below). Done. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.h:227: int type, On 2013/11/04 18:22:31, bulach wrote: > jint rather than int Done. https://codereview.chromium.org/53283003/diff/330001/content/browser/android/... content/browser/android/content_view_core_impl.h:228: bool hasDelay, On 2013/11/04 18:22:31, bulach wrote: > nit: has_delay Done. And switched to jboolean https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:462: public enum UMAActionAfterDoubleTap { On 2013/11/04 18:22:31, bulach wrote: > can't have enums on android java, it's really frowned upon due to its overhead.. > please use static final int constants for the values (yeah, loosing typesafety > is a known-issue :) Done. https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/53283003/diff/330001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:551: isDoubleTapDisabled() || mDisableClickDelay ? On 2013/11/02 12:58:45, rbyers wrote: > On 2013/11/01 20:17:46, jdduke wrote: > > > > I'm not sure it makes complete sense to group |isDoubleTapDisabled()| and > > |mDisableClickDelay| together. If |isDoubleTapDisabled()| is true, double-taps > > become two distinct taps. There will never be a corresponding DOUBLE_TAP > > gesture, so it really shouldn't contribute to the "accidental" tap statistic. > > The idea with this metric is completely separate from the "accidental tap > statistic" stuff we're doing elsewhere. The intention here is to provide some > measure of the potential amount of time to be gained by disabling the click > delay (the other side of the cost/benefit trade-off). > > > I wonder if three categories would improve the situation? UNDELAYED, > > ARTIFICIALLY_UNDELAYED, and DELAYED, where > > > > UNDELAYED = isDoubleTapDisabled(), and > > ARTIFICIALLY_UNDELAYED = mDisableClickDelay && !IsDoubleTapDisabled() > > Yes, thinking about it more, I agree we probably don't want to lump the two > undelayed cases together (since we're trying to evaluate the benefit of > disabling the click delay). Actually I think it would be fine to ignore > mDisableClickDelay entirely here and just report taps what would normally be > delayed vs. those that wouldn't (so the 'experiment' with the flag is irrelevant > for this measure). That would be simplest. There's no harm in having all 3 > states if you think there's value in capturing the extra data though. > > > In that case I wonder if we should also be tracking GESTURE_SINGLE_TAP_UP... > > Ah yes, it looks like we should be counting that case too. It sounds like it's > a pretty rare edge case though, right? If it's not going to move the statistic > much then it's probably not worth worrying about. Agreed. Since this statistic is unrelated to accidentally double-tapping, it doesn't make sense to take into account an experiment related to it. I've changed it so that UNDELAYED_TAP == isDoubleTapDisabled() and DELAYED_TAP otherwise. This will give use tap statistics regardless of the disable click delay flag. Gesture single tap up looks less common, but it's easy enough to increment the counter there too so I added it. https://codereview.chromium.org/53283003/diff/330001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/53283003/diff/330001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:3792: + <summary>The number of gesture taps that are delayed vs undelayed</summary> On 2013/11/02 00:12:39, Alexei Svitkine wrote: > This description really needs more context. When is it logged? What does delayed > vs undelayed mean? Added context for all three histograms.
asvitkine@: ptal at histograms.xml bulach@: ptal at the rest Thanks
LGTM with a nit https://codereview.chromium.org/53283003/diff/570001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/570001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1642: count); Nit: Align |count|, here and below.
+tedchoc@ for OWNER in */android
Mainly styling java nits, but the main question I have is about the method placements in TabBase. https://codereview.chromium.org/53283003/diff/690001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right): https://codereview.chromium.org/53283003/diff/690001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:228: mContentViewCore.reportActionAfterDoubleTapUMA( why is this in Tab instead of just in ContentViewCore#goBack? https://codereview.chromium.org/53283003/diff/690001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:229: ContentViewCore.UMAActionAfterDoubleTap.NAVIGATE_BACK); this should be +4 indent https://codereview.chromium.org/53283003/diff/690001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:344: ContentViewCore.UMAActionAfterDoubleTap.NAVIGATE_STOP); same question and style nit as above https://codereview.chromium.org/53283003/diff/690001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/690001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1625: jobject obj, I can't say I'm a big fan of this living within ContentViewCore. I'd rather the gesture handler track this themselves. Unfortunately, there are no native components nor any upstream UMA dumping grounds exposed in java, so I guess this is the best we've got. https://codereview.chromium.org/53283003/diff/690001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1641: UMA_HISTOGRAM_ENUMERATION("Event.ActionAfterDoubleTapWithDelay", type, -2 indent https://codereview.chromium.org/53283003/diff/690001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1644: UMA_HISTOGRAM_ENUMERATION("Event.ActionAfterDoubleTapNoDelay", type, same https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1338: public void reportActionAfterDoubleTapUMA(int type) { based on my other comment in TabBase, hopefully this can become private https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1414: nativeSendSingleTapUma( probably worth a check like: if (mNativeContentViewCore == 0) return; There are some weird lifetime issues with WebView, so probably best to be consistent and handle things dying underneath us. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:348: boolean clickDelayEnabled); move up on the previous line https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1175: reportDoubleTap(); in java, braces are always required unless the condition and statement can fit on a single line (which they could here). if (type == GESTURE_DOUBLE_TAP) reportDoubleTap(); https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1302: public void reportDoubleTap() { all public methods should have javadoc...that being said it doesn't look like this is used outside of this class so maybe it can be private? https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1305: if (mLastDoubleTapTimeMs > 0) braces required https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1308: !mDisableClickDelay); with a line length of 100 chars, does this fit on the previous line? https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1313: public void reportActionAfterDoubleTapUMA(int type) { javadoc https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1316: if (mLastDoubleTapTimeMs == 0) put return on same line https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1331: if (mLastDoubleTapTimeMs == 0) return on this line https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1336: >= ACTION_AFTER_DOUBLE_TAP_WINDOW_MS) { does this not fit under a 100 chars if on the previous line? https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1340: !mDisableClickDelay); put on previous line https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1347: private long mLastDoubleTapTimeMs = 0; 0 is the default so it's not necessary to specify it. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1348: private static final long ACTION_AFTER_DOUBLE_TAP_WINDOW_MS = 5000; local variables at the top of the file (constants going above local variables)
Thanks for the feedback, I've addressed all your comments. (Sorry about the style, first CL in Java) https://codereview.chromium.org/53283003/diff/690001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right): https://codereview.chromium.org/53283003/diff/690001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:228: mContentViewCore.reportActionAfterDoubleTapUMA( On 2013/11/07 04:26:08, Ted C wrote: > why is this in Tab instead of just in ContentViewCore#goBack? Done. https://codereview.chromium.org/53283003/diff/690001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:229: ContentViewCore.UMAActionAfterDoubleTap.NAVIGATE_BACK); On 2013/11/07 04:26:08, Ted C wrote: > this should be +4 indent Done. https://codereview.chromium.org/53283003/diff/690001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:344: ContentViewCore.UMAActionAfterDoubleTap.NAVIGATE_STOP); On 2013/11/07 04:26:08, Ted C wrote: > same question and style nit as above Done. https://codereview.chromium.org/53283003/diff/690001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/53283003/diff/690001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1641: UMA_HISTOGRAM_ENUMERATION("Event.ActionAfterDoubleTapWithDelay", type, On 2013/11/07 04:26:08, Ted C wrote: > -2 indent Done. https://codereview.chromium.org/53283003/diff/690001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1644: UMA_HISTOGRAM_ENUMERATION("Event.ActionAfterDoubleTapNoDelay", type, On 2013/11/07 04:26:08, Ted C wrote: > same Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1338: public void reportActionAfterDoubleTapUMA(int type) { On 2013/11/07 04:26:08, Ted C wrote: > based on my other comment in TabBase, hopefully this can become private Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1414: nativeSendSingleTapUma( On 2013/11/07 04:26:08, Ted C wrote: > probably worth a check like: > if (mNativeContentViewCore == 0) return; > > There are some weird lifetime issues with WebView, so probably best to be > consistent and handle things dying underneath us. Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:348: boolean clickDelayEnabled); On 2013/11/07 04:26:08, Ted C wrote: > move up on the previous line Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1175: reportDoubleTap(); On 2013/11/07 04:26:08, Ted C wrote: > in java, braces are always required unless the condition and statement can fit > on a single line (which they could here). > > if (type == GESTURE_DOUBLE_TAP) reportDoubleTap(); Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1302: public void reportDoubleTap() { On 2013/11/07 04:26:08, Ted C wrote: > all public methods should have javadoc...that being said it doesn't look like > this is used outside of this class so maybe it can be private? Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1305: if (mLastDoubleTapTimeMs > 0) On 2013/11/07 04:26:08, Ted C wrote: > braces required Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1308: !mDisableClickDelay); On 2013/11/07 04:26:08, Ted C wrote: > with a line length of 100 chars, does this fit on the previous line? Yup, done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1313: public void reportActionAfterDoubleTapUMA(int type) { On 2013/11/07 04:26:08, Ted C wrote: > javadoc Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1316: if (mLastDoubleTapTimeMs == 0) On 2013/11/07 04:26:08, Ted C wrote: > put return on same line Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1331: if (mLastDoubleTapTimeMs == 0) On 2013/11/07 04:26:08, Ted C wrote: > return on this line Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1336: >= ACTION_AFTER_DOUBLE_TAP_WINDOW_MS) { On 2013/11/07 04:26:08, Ted C wrote: > does this not fit under a 100 chars if on the previous line? It does, I didn't realize the line length in Java was 100. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1340: !mDisableClickDelay); On 2013/11/07 04:26:08, Ted C wrote: > put on previous line Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1347: private long mLastDoubleTapTimeMs = 0; On 2013/11/07 04:26:08, Ted C wrote: > 0 is the default so it's not necessary to specify it. Done. https://codereview.chromium.org/53283003/diff/690001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1348: private static final long ACTION_AFTER_DOUBLE_TAP_WINDOW_MS = 5000; On 2013/11/07 04:26:08, Ted C wrote: > local variables at the top of the file (constants going above local variables) Done.
lgtm https://codereview.chromium.org/53283003/diff/810001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/53283003/diff/810001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1323: defined in ContentViewCore. need a * at the beginning of this comment line
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bokan@chromium.org/53283003/900001
Message was sent while issue was closed.
Change committed as 233676 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
