Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1358173008:
Add UMA stats for pinch-zoom behavior on Android (Closed)
DescriptionAdd UMA stats for pinch-zoom behavior on Android
This CL adds two UMA statistics: Viewport.DidPinchZoom and
Viewport.MaxPageScale. For each page load, DidPinchZoom tracks whether the user
changed the page scale(e.g. pinch-zoom, double-tap, etc). For those pages where
the user did change the scalee, MaxPageScale keeps track of the furthest the
user zoomed in. In both cases, we only record the stat if the user interacted
with the page; hence, the tracking is turned on when Blink receives an input
event.
BUG=534456
Committed: https://crrev.com/3c89821632ac5ab5a5199c4d4a8920cd35f7cbb5
Cr-Commit-Position: refs/heads/master@{#352119}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 28
Patch Set 4 : Rebase #Patch Set 5 : Review fixes #Patch Set 6 : Small build fix to previous patch #
Total comments: 3
Patch Set 7 : Rebase #
Messages
Total messages: 34 (8 generated)
bokan@chromium.org changed reviewers: + yoav@yoav.ws
Yoav, WDYT? I originally wanted to have it for Desktop platforms too but then we'd have to differentiate between touch-screen and non-touch-screen and after some thought I'm not sure how much value it would give us so I made it Android only. Additionally, I only track stats if the user interacts with the page in any way (Blink sees some kind of input). This was originally to prevent tracking all sorts of background renderers on desktop but has the nice benefit of not including useless navigation (e.g. the user just hitting back in history). On the flip side, if the user loads a page and then just closes it or goes back, we wont track that but I imagine that's probably rare.
I'm not sure I'm the most qualified to review this, since I'm not that familiar with these parts of the code. A few questions nevertheless :) https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2075: page()->frameHost().visualViewport().startTrackingPinchStats(); Would that mean that we start tracking pinch zoom regardless of the input event type? https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:4232: if (pageScaleDelta != 1) { What's the pageScaleDelta value in non-mobile zoom-out scenarios? I'd assume it's lower than 1 and would be counted as a scale change.
On 2015/09/25 08:46:59, Yoav Weiss wrote: > I'm not sure I'm the most qualified to review this, since I'm not that familiar > with these parts of the code. A few questions nevertheless :) > Np, just wanted to give you the first look. I'll add local experts after. > https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebViewImpl.cpp:2075: > page()->frameHost().visualViewport().startTrackingPinchStats(); > Would that mean that we start tracking pinch zoom regardless of the input event > type? > > https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebViewImpl.cpp:4232: if (pageScaleDelta != 1) { > What's the pageScaleDelta value in non-mobile zoom-out scenarios? I'd assume > it's lower than 1 and would be counted as a scale change.
https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2075: page()->frameHost().visualViewport().startTrackingPinchStats(); On 2015/09/25 08:46:59, Yoav Weiss wrote: > Would that mean that we start tracking pinch zoom regardless of the input event > type? Yes, I use it more as a signal that this is something the user can see and interact with. Though on Android I think that's pretty much just touches... https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:4232: if (pageScaleDelta != 1) { On 2015/09/25 08:46:59, Yoav Weiss wrote: > What's the pageScaleDelta value in non-mobile zoom-out scenarios? I'd assume > it's lower than 1 and would be counted as a scale change. This is the delta coming from the compositor. As such, it's only the *change* in page scale that occured on the compositor that Blink doesn't know about yet. The only changes from the compositor are due to pinch-zoom, double-tap to zoom animations, and zoom-to-legible text editing. The initial page scale that gets set on non-mobile pages is done from the Blink main thread and sent to the compositor.
On 2015/09/25 17:01:49, bokan wrote: > https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebViewImpl.cpp:2075: > page()->frameHost().visualViewport().startTrackingPinchStats(); > On 2015/09/25 08:46:59, Yoav Weiss wrote: > > Would that mean that we start tracking pinch zoom regardless of the input > event > > type? > > Yes, I use it more as a signal that this is something the user can see and > interact with. Though on Android I think that's pretty much just touches... > > https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebViewImpl.cpp:4232: if (pageScaleDelta != 1) { > On 2015/09/25 08:46:59, Yoav Weiss wrote: > > What's the pageScaleDelta value in non-mobile zoom-out scenarios? I'd assume > > it's lower than 1 and would be counted as a scale change. > > This is the delta coming from the compositor. As such, it's only the *change* in > page scale that occured on the compositor that Blink doesn't know about yet. The > only changes from the compositor are due to pinch-zoom, double-tap to zoom > animations, and zoom-to-legible text editing. The initial page scale that gets > set on non-mobile pages is done from the Blink main thread and sent to the > compositor. Makes sense!
bokan@chromium.org changed reviewers: + isherman@chromium.org, rbyers@chromium.org
+rbyers@ for Blink changes +isherman@ for histograms.xml https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48796: +<histogram name="Viewport.MaxPageScale" units="Percent"> Is it ok that the percent value for this metric will be greater than 100? (it maxes out at 500%)
On 2015/09/28 13:02:09, bokan wrote: > +rbyers@ for Blink changes > +isherman@ for histograms.xml > > https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:48796: +<histogram > name="Viewport.MaxPageScale" units="Percent"> > Is it ok that the percent value for this metric will be greater than 100? (it > maxes out at 500%) Sure. For page scale that makes total sense
On 2015/09/28 13:11:12, Yoav Weiss wrote: > On 2015/09/28 13:02:09, bokan wrote: > > +rbyers@ for Blink changes > > +isherman@ for histograms.xml > > > > > https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:48796: +<histogram > > name="Viewport.MaxPageScale" units="Percent"> > > Is it ok that the percent value for this metric will be greater than 100? (it > > maxes out at 500%) > > Sure. For page scale that makes total sense I mean for the actual UMA dashboard, I'm wondering if units="Percent" will clmap it between 0-100?
On 2015/09/28 13:12:39, bokan wrote: > On 2015/09/28 13:11:12, Yoav Weiss wrote: > > On 2015/09/28 13:02:09, bokan wrote: > > > +rbyers@ for Blink changes > > > +isherman@ for histograms.xml > > > > > > > > > https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... > > > tools/metrics/histograms/histograms.xml:48796: +<histogram > > > name="Viewport.MaxPageScale" units="Percent"> > > > Is it ok that the percent value for this metric will be greater than 100? > (it > > > maxes out at 500%) > > > > Sure. For page scale that makes total sense > > I mean for the actual UMA dashboard, I'm wondering if units="Percent" will clmap > it between 0-100? Oh, in that case I have no opinion.
https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:82: sendUMAMetrics(); Is there already an established pattern for renderer stats sent per page load? It seems like it's likely to be fraught with edge cases. Some small (un-biased) information loss (like tab discarding) is probably fine here. But if you weren't reporting a substantial fraction then I'd worry about the validity of the data. Maybe just do a quick manual sanity check that doing 20 page loads (navigating between real-world pages) reports this exactly 20 times? https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:663: return; Always sucks to add more #if OS(ANDROID (eg. makes testing harder, makes mobile emulation subtly different than real mobile). See below. https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:677: bool isNonMobile = I thought this logic (along with "am I on Android") was already encapsulated somewhere else ("shouldEnableDesktopWorkarounds" or some such function somewhere perhaps)? It would be nice to use a consistent definition, since the details are a little subtle. https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:689: m_maxPageScale = std::max(m_maxPageScale, m_scale); I assume this gets called only at GesturePinchEnd time (not for each scale change throughout the pinch gesture), right? Is that the notion of "max" you want? https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48791: + constant width) page views that had a user initiated page scale (e.g. nit: you should explicitly include somewhere that this is tracked only on mobile.
https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:82: sendUMAMetrics(); On 2015/09/28 18:43:58, Rick Byers wrote: > Is there already an established pattern for renderer stats sent per page load? > It seems like it's likely to be fraught with edge cases. Some small (un-biased) > information loss (like tab discarding) is probably fine here. But if you > weren't reporting a substantial fraction then I'd worry about the validity of > the data. > > Maybe just do a quick manual sanity check that doing 20 page loads (navigating > between real-world pages) reports this exactly 20 times? I copied this pattern from the Blink useCounter which reports metrics on Page destruction (VisualViewport is destroyed with the page) and on commit so we should have the same profile as that. I'd tried this earlier and it seemed to work as expected but I just double checked and it seems to only report when following links, not new navigations. I'll investigate a bit more. https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:677: bool isNonMobile = On 2015/09/28 18:43:58, Rick Byers wrote: > I thought this logic (along with "am I on Android") was already encapsulated > somewhere else ("shouldEnableDesktopWorkarounds" or some such function somewhere > perhaps)? It would be nice to use a consistent definition, since the details > are a little subtle. WebViewImpl::shouldDisableDesktopWorkarounds. It's slightly different than what we agreed on - in that case, the page is "mobile" iff the layout size == viewport size OR the author explicitly disabled zoom. That said, I think it would work as well. Again, Yoav: what do you think? https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:689: m_maxPageScale = std::max(m_maxPageScale, m_scale); On 2015/09/28 18:43:58, Rick Byers wrote: > I assume this gets called only at GesturePinchEnd time (not for each scale > change throughout the pinch gesture), right? Is that the notion of "max" you > want? Good point. I do think capturing the scale only at end is probably the right thing here but maybe Yoav can confirm.
https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:701: Platform::current()->histogramEnumeration("Viewport.MaxPageScale", zoomPercentage, 501); This allocates a histogram with 500 buckets, which is pretty wasteful of memory. Is it the case that any page scale between 1 and 500 is possible, or are there only a few that are actually possible? In either case, you might want to use a sparse histogram, which only allocates the used buckets on the client. If there are only a few, then you could also use a regular enumerated histogram, but one where you manually map the page scales onto a much smaller enum. https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48792: + pinch-zoom, double-tap). Please document when this is recorded. On page load? On page close? Multiple triggers? https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48796: +<histogram name="Viewport.MaxPageScale" units="Percent"> On 2015/09/28 13:02:09, bokan wrote: > Is it ok that the percent value for this metric will be greater than 100? (it > maxes out at 500%) Yes, that's fine. (I'd probably write units="%") https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48801: + which have had a page scale changing gesture. Please document when this is recorded -- at the time of each page scaling change gesture? https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54473: +<enum name="DidScalePageEnum" type="int"> nit: Please omit the "Enum" suffix in the enum name (to match the style used elsewhere in this file). In fact, the typical name for this enum would be "BooleanDidScalePage"
Review comments addressed. PTAL. Yoav, do the changes seem ok to you? https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:663: return; On 2015/09/28 18:43:58, Rick Byers wrote: > Always sucks to add more #if OS(ANDROID (eg. makes testing harder, makes mobile > emulation subtly different than real mobile). See below. Gone in latest patch. https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:677: bool isNonMobile = On 2015/09/29 20:33:20, bokan wrote: > On 2015/09/28 18:43:58, Rick Byers wrote: > > I thought this logic (along with "am I on Android") was already encapsulated > > somewhere else ("shouldEnableDesktopWorkarounds" or some such function > somewhere > > perhaps)? It would be nice to use a consistent definition, since the details > > are a little subtle. > > WebViewImpl::shouldDisableDesktopWorkarounds. It's slightly different than what > we agreed on - in that case, the page is "mobile" iff the layout size == > viewport size OR the author explicitly disabled zoom. That said, I think it > would work as well. Again, Yoav: what do you think? Latest patch replaces this with the shouldDisableDesktopWorkarounds call (I moved it from WebViewImpl to here). Yoav, does that sound ok? https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:701: Platform::current()->histogramEnumeration("Viewport.MaxPageScale", zoomPercentage, 501); On 2015/09/30 04:30:28, Ilya Sherman wrote: > This allocates a histogram with 500 buckets, which is pretty wasteful of memory. > Is it the case that any page scale between 1 and 500 is possible, or are there > only a few that are actually possible? In either case, you might want to use a > sparse histogram, which only allocates the used buckets on the client. If there > are only a few, then you could also use a regular enumerated histogram, but one > where you manually map the page scales onto a much smaller enum. Ok, I mapped the percentages into ~20 buckets comprising 25% ranges - that should be fine enough for our needs. https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48791: + constant width) page views that had a user initiated page scale (e.g. On 2015/09/28 18:43:58, Rick Byers wrote: > nit: you should explicitly include somewhere that this is tracked only on > mobile. Done. https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48792: + pinch-zoom, double-tap). On 2015/09/30 04:30:28, Ilya Sherman wrote: > Please document when this is recorded. On page load? On page close? Multiple > triggers? Done. https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48796: +<histogram name="Viewport.MaxPageScale" units="Percent"> On 2015/09/30 04:30:28, Ilya Sherman wrote: > On 2015/09/28 13:02:09, bokan wrote: > > Is it ok that the percent value for this metric will be greater than 100? (it > > maxes out at 500%) > > Yes, that's fine. (I'd probably write units="%") Changed this to an enum so that I can bucket the values. https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48801: + which have had a page scale changing gesture. On 2015/09/30 04:30:28, Ilya Sherman wrote: > Please document when this is recorded -- at the time of each page scaling change > gesture? Done. https://codereview.chromium.org/1358173008/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54473: +<enum name="DidScalePageEnum" type="int"> On 2015/09/30 04:30:28, Ilya Sherman wrote: > nit: Please omit the "Enum" suffix in the enum name (to match the style used > elsewhere in this file). In fact, the typical name for this enum would be > "BooleanDidScalePage" Done.
https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:82: sendUMAMetrics(); On 2015/09/29 20:33:20, bokan wrote: > On 2015/09/28 18:43:58, Rick Byers wrote: > > Is there already an established pattern for renderer stats sent per page load? > > > It seems like it's likely to be fraught with edge cases. Some small > (un-biased) > > information loss (like tab discarding) is probably fine here. But if you > > weren't reporting a substantial fraction then I'd worry about the validity of > > the data. > > > > Maybe just do a quick manual sanity check that doing 20 page loads (navigating > > between real-world pages) reports this exactly 20 times? > > I copied this pattern from the Blink useCounter which reports metrics on Page > destruction (VisualViewport is destroyed with the page) and on commit so we > should have the same profile as that. I'd tried this earlier and it seemed to > work as expected but I just double checked and it seems to only report when > following links, not new navigations. I'll investigate a bit more. Right, so this wont report stats for a page if the user navigates away with the URL bar. Looks like as soon as we navigate to a new URL, the old process is killed without getting a chance to do any cleanup and a new one is started up. This means we only get stats for navigation using links and history which seems unfortunate. Do we have any data on how often users navigate away using the URL bar vs links? I think we'll lose a pretty significant chunk of page loads this way (I'd expect ~10-25%). The only option I can see, since the renderer gets killed pretty ruthlessly, is something like what https://docs.google.com/document/u/2/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4... is suggesting - periodically passing the data to the Browser and letting it decide when to upload the metrics. That seems pretty heavy weight for this though. Thoughts?
https://codereview.chromium.org/1358173008/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1358173008/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48840: + Android only. So, does this record whether the new page is zoomed, or whether the prior page was not zoomed? If it records data about the prior page, what happens if the user just closes the tab?
https://codereview.chromium.org/1358173008/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1358173008/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48840: + Android only. On 2015/10/01 22:09:00, Ilya Sherman wrote: > So, does this record whether the new page is zoomed, or whether the prior page > was not zoomed? If it records data about the prior page, what happens if the > user just closes the tab? The old page, unfortunately there's a lot of ways we can lose this data - see the discussion in VisualViewport.cpp.
Ok, this LGTM if you and Yoav decide you want to just go with the subset of data we get (same as UseCounters, which we do find pretty useful in practice despite this limitation). https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1358173008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:82: sendUMAMetrics(); On 2015/10/01 20:58:15, bokan wrote: > On 2015/09/29 20:33:20, bokan wrote: > > On 2015/09/28 18:43:58, Rick Byers wrote: > > > Is there already an established pattern for renderer stats sent per page > load? > > > > > It seems like it's likely to be fraught with edge cases. Some small > > (un-biased) > > > information loss (like tab discarding) is probably fine here. But if you > > > weren't reporting a substantial fraction then I'd worry about the validity > of > > > the data. > > > > > > Maybe just do a quick manual sanity check that doing 20 page loads > (navigating > > > between real-world pages) reports this exactly 20 times? > > > > I copied this pattern from the Blink useCounter which reports metrics on Page > > destruction (VisualViewport is destroyed with the page) and on commit so we > > should have the same profile as that. I'd tried this earlier and it seemed to > > work as expected but I just double checked and it seems to only report when > > following links, not new navigations. I'll investigate a bit more. > > Right, so this wont report stats for a page if the user navigates away with the > URL bar. Looks like as soon as we navigate to a new URL, the old process is > killed without getting a chance to do any cleanup and a new one is started up. > > This means we only get stats for navigation using links and history which seems > unfortunate. Do we have any data on how often users navigate away using the URL > bar vs links? I think we'll lose a pretty significant chunk of page loads this > way (I'd expect ~10-25%). > > The only option I can see, since the renderer gets killed pretty ruthlessly, is > something like what > https://docs.google.com/document/u/2/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4... > is suggesting - periodically passing the data to the Browser and letting it > decide when to upload the metrics. That seems pretty heavy weight for this > though. > > Thoughts? Ugh, that's a pain. I didn't realize UseCounter data was this bad. Have you found a bug for that (beyond the page-load-time specific bug 382542)? Fixing that seems more important than this specific instance, and maybe there's some way to fix it in a general way. If there's not already a bug for that, could you file one? Does the same issue apply for cross-origin navigation via links (which I believe also tears down the old renderer pretty quickly)? If so then I could imagine whole classes of appy sites might not get logged at all (eg. when do you ever leave a facebook.com or gmail.com page in a way that WOULD report data?). But I agree that it's not worth doing something special case here. I think you should either leave it as-is and live with the inaccuracy, or change what you're looking at. Eg. what if we just recorded the scale at the end of every pinch gesture, perhaps that would be good enough? I guess you still wouldn't know what fraction of pages get a pinch at all.
https://codereview.chromium.org/1358173008/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1358173008/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:48840: + Android only. On 2015/10/01 22:10:24, bokan wrote: > On 2015/10/01 22:09:00, Ilya Sherman wrote: > > So, does this record whether the new page is zoomed, or whether the prior page > > was not zoomed? If it records data about the prior page, what happens if the > > user just closes the tab? > > The old page, unfortunately there's a lot of ways we can lose this data - see > the discussion in VisualViewport.cpp. I see. Please clarify that here in the histogram <summary>. Other than that, LGTM.
LGTM
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1358173008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1358173008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1358173008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1358173008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws, isherman@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1358173008/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1358173008/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1358173008/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3c89821632ac5ab5a5199c4d4a8920cd35f7cbb5 Cr-Commit-Position: refs/heads/master@{#352119} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
