Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
707193004:
Plugin Power Saver: Collect size histogram on Flash Plugins users click. (Closed)
DescriptionPlugin Power Saver: Collect size histogram on Flash Plugins users click.
BUG=403800
Committed: https://crrev.com/8f72990be00c94350ef23da23da3ea8f73a04d8c
Cr-Commit-Position: refs/heads/master@{#303483}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 16
Patch Set 6 : #
Total comments: 17
Patch Set 7 : #Patch Set 8 : #
Total comments: 4
Patch Set 9 : #
Messages
Total messages: 21 (3 generated)
tommycli@chromium.org changed reviewers: + groby@chromium.org, thestig@chromium.org
groby/thestig: I structured the size histogram in the if (w < max_w && h < max_h) way - the same logic used in our heuristic. This made sense to me. If this makes sense to you guys also, and the size bins I made look good, let me know so I can push this review fwd to raymes/isherman.
tommycli@chromium.org changed reviewers: + isherman@chromium.org, raymes@chromium.org
isherman: What do you think of this histogram scheme? raymes: I know this adds more cruft to an already-crowded class. The cleanup CL is in progress and will come next week. (see https://codereview.chromium.org/707113002/)
I'm not clear what kind of action we can take based on this data, or what hypothesis we're testing. (I.e. what's the value of the data?) 1) We treat width and height as independent criteria in the heuristic, while the histogram couples them. As a result, e.g. a 970 x 250 banner item would fall into the 1024x768 bucket, which is misleading at best. 2) The buckets are incredibly coarse - the difference between 400x300 and 512x364 is an over 50% increase in visible data. 3) We assume currently that our cutoff point is _somewhere_ around 400x300 - I'm not sure why we would distinguish sizes way beyond that. My best guess at a good size metric right now is: * Hypothesis 1: both width and height are bimodal - almost all content will be either to the left or the right of the cutoff point. * Hypothesis 2: the cutoff point is at ~400x300. * Hypothesis 3: There's quite a bit of content that's very small and not necessarily user visible With that, here's how I'd structure the metrics: * Separate histograms for width/height, because we treat them as independent. * The histograms should probably span up to ~ 500x400, and we don't care much about the breakdown below 300x200 - if we can't see a clear bimodal distribution there, our cutoff is way out of bounds. * With the customary 50 buckets for a histogram, that gets us a resolution of 4 pixel per bucket - enough for a fine-grained decision. * We should have an additional histogram for small items - 1x1,5x5,10x10, the rest. This will give us a good idea how prevalent small items are and if they need to be treated separately. * We might want a histogram for aspect ratios. Rough guess it's extending from 1:4 - 4:1. Anything beyond that is unlikely to be essential content. Information we get from that if there's indeed a spike at 4:3 and 16:9. (And if there are other common ratios that we can use as a signal)
groby: See if you like these revised metrics better.
https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:193: UMA_HISTOGRAM_SPARSE_SLOWLY( \ I think we need cutoffs - beyond 3:1 or 4:1 it's becoming really uninteresting to sample more details. Also, the effective granularity for aspect ratios < 1 is worse than the other way around. So, how about this for the value: // flip ratio to always be >1 if (height > width) swapped = true; std::swap(width, height) } // Clamp ratio at 400, 401 is anything > 4:1 if (height) ratio = min(((width * 100) / height), 400) else ratio = 401; // Flag swapped values if (swapped) ratio = -ratio; Or, if you want to be concise: ratio = (min(width,height) ? min(max(width, height) / min(width,height), 400) : 401) * ((width < height) ? -1 : 1); // I'd rather you're not concise and spell this out in the recording function :) https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:420: "Plugin.FlashClickSizeAspectRatio"; Plugin.Flash.ClickSize.AspectRatio ? (And the same for the other metrics). We seem to usually treat names as a hierarchy of namespaces, so this seems a better name. https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:443: UMA_HISTOGRAM_COUNTS(kFlashClickSizeHeightHistogram, height); I believe these are exponential histograms. We want a linear one for all of them. Something like base::HistogramBase* histogram = base::LinearHistogram::FactoryGet( kFlashClickSizeHeightHistogram 200, // minimum height 400, // maximum height 50, // number of buckets. base::HistogramBase::kUmaTargetedHistogramFlag); histogram->Add(height); https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:915: if (IsFlashPlugin(module_.get())) { Curious: Why did this move? To do all recording at the same time? https://codereview.chromium.org/707193004/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/707193004/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:25090: + <summary>Aspect ratio of Flash plugins users click at least once.</summary> Should probably explain what the values mean. (I.e. it's aspect ratio times 100, and if you go with the suggestion in the cc file, what the sign means https://codereview.chromium.org/707193004/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:25103: +<histogram name="Plugin.FlashInstanceSize" enum="FlashInstanceSize"> Technically, the name should reflect this is about tiny content.
groby: See if you like this any better. Thanks! https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:193: UMA_HISTOGRAM_SPARSE_SLOWLY( \ On 2014/11/07 21:57:44, groby wrote: > I think we need cutoffs - beyond 3:1 or 4:1 it's becoming really uninteresting > to sample more details. Also, the effective granularity for aspect ratios < 1 is > worse than the other way around. > > So, how about this for the value: > > // flip ratio to always be >1 > if (height > width) > swapped = true; > std::swap(width, height) > } > > // Clamp ratio at 400, 401 is anything > 4:1 > if (height) > ratio = min(((width * 100) / height), 400) > else > ratio = 401; > > // Flag swapped values > if (swapped) > ratio = -ratio; > > Or, if you want to be concise: > ratio = (min(width,height) ? min(max(width, height) / min(width,height), 400) : > 401) * ((width < height) ? -1 : 1); // I'd rather you're not concise and spell > this out in the recording function :) > > I used the same aspect ratio encoding as here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... I was planning to consolidate the two macros in a followup CL. https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:420: "Plugin.FlashClickSizeAspectRatio"; On 2014/11/07 21:57:44, groby wrote: > Plugin.Flash.ClickSize.AspectRatio ? (And the same for the other metrics). We > seem to usually treat names as a hierarchy of namespaces, so this seems a better > name. Done. https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:443: UMA_HISTOGRAM_COUNTS(kFlashClickSizeHeightHistogram, height); On 2014/11/07 21:57:44, groby wrote: > I believe these are exponential histograms. We want a linear one for all of > them. Something like > > base::HistogramBase* histogram = > base::LinearHistogram::FactoryGet( > kFlashClickSizeHeightHistogram > 200, // minimum height > 400, // maximum height > 50, // number of buckets. > base::HistogramBase::kUmaTargetedHistogramFlag); > histogram->Add(height); > Done. I followed this example: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... Are they 'doing it wrong' then? https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:915: if (IsFlashPlugin(module_.get())) { On 2014/11/07 21:57:44, groby wrote: > Curious: Why did this move? To do all recording at the same time? Yes. And it had to be here because you can only get the bounds after the constructor finishes. https://codereview.chromium.org/707193004/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/707193004/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:25090: + <summary>Aspect ratio of Flash plugins users click at least once.</summary> On 2014/11/07 21:57:44, groby wrote: > Should probably explain what the values mean. (I.e. it's aspect ratio times 100, > and if you go with the suggestion in the cc file, what the sign means Done. https://codereview.chromium.org/707193004/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:25103: +<histogram name="Plugin.FlashInstanceSize" enum="FlashInstanceSize"> On 2014/11/07 21:57:44, groby wrote: > Technically, the name should reflect this is about tiny content. Done.
LGTM % width/height range. (And obviously, OWNER still required) https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:193: UMA_HISTOGRAM_SPARSE_SLOWLY( \ On 2014/11/07 22:46:37, tommycli wrote: > On 2014/11/07 21:57:44, groby wrote: > > I think we need cutoffs - beyond 3:1 or 4:1 it's becoming really > uninteresting > > to sample more details. Also, the effective granularity for aspect ratios < 1 > is > > worse than the other way around. > > > > So, how about this for the value: > > > > // flip ratio to always be >1 > > if (height > width) > > swapped = true; > > std::swap(width, height) > > } > > > > // Clamp ratio at 400, 401 is anything > 4:1 > > if (height) > > ratio = min(((width * 100) / height), 400) > > else > > ratio = 401; > > > > // Flag swapped values > > if (swapped) > > ratio = -ratio; > > > > Or, if you want to be concise: > > ratio = (min(width,height) ? min(max(width, height) / min(width,height), 400) > : > > 401) * ((width < height) ? -1 : 1); // I'd rather you're not concise and spell > > this out in the recording function :) > > > > > > I used the same aspect ratio encoding as here: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > I was planning to consolidate the two macros in a followup CL. These are video aspect ratios - they're rarely flipped (i.e. <1), so they don't need to tackle that problem. Essentially, they're expected to be 4:3/16:9 pretty much all the time. Hm. We _expect_ that to be the case as well, so we could indeed re-use that macro. If we're outside that range, the data will just be slightly less useful. Since it promotes re-use, let's re-use. https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:443: UMA_HISTOGRAM_COUNTS(kFlashClickSizeHeightHistogram, height); On 2014/11/07 22:46:37, tommycli wrote: > On 2014/11/07 21:57:44, groby wrote: > > I believe these are exponential histograms. We want a linear one for all of > > them. Something like > > > > base::HistogramBase* histogram = > > base::LinearHistogram::FactoryGet( > > kFlashClickSizeHeightHistogram > > 200, // minimum height > > 400, // maximum height > > 50, // number of buckets. > > base::HistogramBase::kUmaTargetedHistogramFlag); > > histogram->Add(height); > > > > Done. > > I followed this example: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > Are they 'doing it wrong' then? > They're not "doing it wrong", but they might be interested in different data. If you look at their width/height metrics, you'll clearly see the exponential bucketing, which might serve the needs they have. We're interested in getting a fairly exact reading of the cutoff point, so having things vanish in large buckets would not be helpful. (Also, their range is bigger than ours - we want a narrow sample around the cutoff point, they probably want an overview) https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:449: 0, // minimum width Min/max width should be ~ 300-500. I don't believe we care much outside of that, and restricting the range gives us finer resolution per bucket. https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:457: 0, // minimum height min/max height should probably be 200-400 https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... File content/renderer/pepper/plugin_power_saver_helper.cc (right): https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/plugin_power_saver_helper.cc:31: "Plugin.PowerSaver.PeripheralHeuristic"; If that name was already in TOT and generated UMA metrics, we now have two counters :) (Still, +1 to keep names consistent)
groby: thanks for review! https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:193: UMA_HISTOGRAM_SPARSE_SLOWLY( \ On 2014/11/07 23:02:20, groby wrote: > On 2014/11/07 22:46:37, tommycli wrote: > > On 2014/11/07 21:57:44, groby wrote: > > > I think we need cutoffs - beyond 3:1 or 4:1 it's becoming really > > uninteresting > > > to sample more details. Also, the effective granularity for aspect ratios < > 1 > > is > > > worse than the other way around. > > > > > > So, how about this for the value: > > > > > > // flip ratio to always be >1 > > > if (height > width) > > > swapped = true; > > > std::swap(width, height) > > > } > > > > > > // Clamp ratio at 400, 401 is anything > 4:1 > > > if (height) > > > ratio = min(((width * 100) / height), 400) > > > else > > > ratio = 401; > > > > > > // Flag swapped values > > > if (swapped) > > > ratio = -ratio; > > > > > > Or, if you want to be concise: > > > ratio = (min(width,height) ? min(max(width, height) / min(width,height), > 400) > > : > > > 401) * ((width < height) ? -1 : 1); // I'd rather you're not concise and > spell > > > this out in the recording function :) > > > > > > > > > > I used the same aspect ratio encoding as here: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > I was planning to consolidate the two macros in a followup CL. > > These are video aspect ratios - they're rarely flipped (i.e. <1), so they don't > need to tackle that problem. Essentially, they're expected to be 4:3/16:9 > pretty much all the time. Hm. > > We _expect_ that to be the case as well, so we could indeed re-use that macro. > If we're outside that range, the data will just be slightly less useful. Since > it promotes re-use, let's re-use. Done. https://codereview.chromium.org/707193004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_plugin_instance_impl.cc:443: UMA_HISTOGRAM_COUNTS(kFlashClickSizeHeightHistogram, height); On 2014/11/07 23:02:20, groby wrote: > On 2014/11/07 22:46:37, tommycli wrote: > > On 2014/11/07 21:57:44, groby wrote: > > > I believe these are exponential histograms. We want a linear one for all of > > > them. Something like > > > > > > base::HistogramBase* histogram = > > > base::LinearHistogram::FactoryGet( > > > kFlashClickSizeHeightHistogram > > > 200, // minimum height > > > 400, // maximum height > > > 50, // number of buckets. > > > base::HistogramBase::kUmaTargetedHistogramFlag); > > > histogram->Add(height); > > > > > > > Done. > > > > I followed this example: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > Are they 'doing it wrong' then? > > > > They're not "doing it wrong", but they might be interested in different data. If > you look at their width/height metrics, you'll clearly see the exponential > bucketing, which might serve the needs they have. > > We're interested in getting a fairly exact reading of the cutoff point, so > having things vanish in large buckets would not be helpful. (Also, their range > is bigger than ours - we want a narrow sample around the cutoff point, they > probably want an overview) Done. Good point. https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:449: 0, // minimum width On 2014/11/07 23:02:20, groby wrote: > Min/max width should be ~ 300-500. I don't believe we care much outside of that, > and restricting the range gives us finer resolution per bucket. I feel that if we're going to collect the histogram we might as well get a good sample of all the plugin instance sizes. At 1000 max and 100 buckets, that's a resolution of 10 px per bin. Is that sufficient for your experiment? https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:457: 0, // minimum height On 2014/11/07 23:02:20, groby wrote: > min/max height should probably be 200-400 See above. It's not a huge deal, but it seems nice to get the whole range.
https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:195: (height) ? ((width) * 100) / (height) : kInfiniteRatio); nit: Why define this as a macro rather than as a function, or just inlined? It seems to only be used in one place. https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:443: } Rather than repeating the UMA_HISTOGRAM_ENUMERATION macro three times, it would be better to just define a variable of type PluginFlashTinyContentSize, and set it via the if-stmts. https://codereview.chromium.org/707193004/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/707193004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25099: -<histogram name="Plugin.PowerSaverPeripheralHeuristic" Please mark the old histogram as <obsolete>. https://codereview.chromium.org/707193004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25111: -<histogram name="Plugin.PowerSaverUnthrottle" Please mark the old histogram as <obsolete>.
isherman: thanks! https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:195: (height) ? ((width) * 100) / (height) : kInfiniteRatio); On 2014/11/07 23:20:08, Ilya Sherman wrote: > nit: Why define this as a macro rather than as a function, or just inlined? It > seems to only be used in one place. It's from https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... and i plan to merge the two in a followup CL. https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:443: } On 2014/11/07 23:20:08, Ilya Sherman wrote: > Rather than repeating the UMA_HISTOGRAM_ENUMERATION macro three times, it would > be better to just define a variable of type PluginFlashTinyContentSize, and set > it via the if-stmts. Done. https://codereview.chromium.org/707193004/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/707193004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25099: -<histogram name="Plugin.PowerSaverPeripheralHeuristic" On 2014/11/07 23:20:08, Ilya Sherman wrote: > Please mark the old histogram as <obsolete>. Done. Wow the old name was just in ToT for one day. Is this going to be stuck in histograms.xml for all of eternity? https://codereview.chromium.org/707193004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25111: -<histogram name="Plugin.PowerSaverUnthrottle" On 2014/11/07 23:20:08, Ilya Sherman wrote: > Please mark the old histogram as <obsolete>. Done.
LGTM, thanks. https://codereview.chromium.org/707193004/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/707193004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25099: -<histogram name="Plugin.PowerSaverPeripheralHeuristic" On 2014/11/08 00:02:25, tommycli wrote: > On 2014/11/07 23:20:08, Ilya Sherman wrote: > > Please mark the old histogram as <obsolete>. > > Done. > > Wow the old name was just in ToT for one day. Is this going to be stuck in > histograms.xml for all of eternity? Oh, if the old name was only shipped in ToT for one day, then it's fine to do what you did before, and skip marking this as <obsolete>. I didn't realize the histogram was so fresh, sorry.
https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:449: 0, // minimum width On 2014/11/07 23:14:10, tommycli wrote: > On 2014/11/07 23:02:20, groby wrote: > > Min/max width should be ~ 300-500. I don't believe we care much outside of > that, > > and restricting the range gives us finer resolution per bucket. > > I feel that if we're going to collect the histogram we might as well get a good > sample of all the plugin instance sizes. > > At 1000 max and 100 buckets, that's a resolution of 10 px per bin. Is that > sufficient for your experiment? I'm happy to trade resolution for range *iff* there's anything we can do as a reaction to the data in the new range. "might as well" shouldn't be a reason, though :) And I don't see anything actionable we can gain from an extended range.
groby/isherman: thanks! https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:449: 0, // minimum width On 2014/11/08 00:21:36, groby wrote: > On 2014/11/07 23:14:10, tommycli wrote: > > On 2014/11/07 23:02:20, groby wrote: > > > Min/max width should be ~ 300-500. I don't believe we care much outside of > > that, > > > and restricting the range gives us finer resolution per bucket. > > > > I feel that if we're going to collect the histogram we might as well get a > good > > sample of all the plugin instance sizes. > > > > At 1000 max and 100 buckets, that's a resolution of 10 px per bin. Is that > > sufficient for your experiment? > > I'm happy to trade resolution for range *iff* there's anything we can do as a > reaction to the data in the new range. "might as well" shouldn't be a reason, > though :) > > And I don't see anything actionable we can gain from an extended range. Well alright I changed it since it's not that big of a deal. I still think perhaps another team might find the size data useful. https://codereview.chromium.org/707193004/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/707193004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25099: -<histogram name="Plugin.PowerSaverPeripheralHeuristic" On 2014/11/08 00:06:24, Ilya Sherman wrote: > On 2014/11/08 00:02:25, tommycli wrote: > > On 2014/11/07 23:20:08, Ilya Sherman wrote: > > > Please mark the old histogram as <obsolete>. > > > > Done. > > > > Wow the old name was just in ToT for one day. Is this going to be stuck in > > histograms.xml for all of eternity? > > Oh, if the old name was only shipped in ToT for one day, then it's fine to do > what you did before, and skip marking this as <obsolete>. I didn't realize the > histogram was so fresh, sorry. Done.
lgtm I would prefer this to be landed after the refactor but if you are in a hurry for some reason I'm ok landing. Let's just try to avoid more CLs till the refactor lands :) https://codereview.chromium.org/707193004/diff/140001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:402: enum PluginFlashTinyContentSize { You may want to make a comment about what this is for. In particular it's not immediately obvious what the units are. https://codereview.chromium.org/707193004/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:426: void RecordFlashSizeMetric(int width, int height) { nit: Comments for these helper functions would be helpful.
groby/isherman/raymes: Thanks! I'll CQ it now. https://codereview.chromium.org/707193004/diff/140001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/707193004/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:402: enum PluginFlashTinyContentSize { On 2014/11/10 02:54:30, raymes wrote: > You may want to make a comment about what this is for. In particular it's not > immediately obvious what the units are. Done. https://codereview.chromium.org/707193004/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:426: void RecordFlashSizeMetric(int width, int height) { On 2014/11/10 02:54:30, raymes wrote: > nit: Comments for these helper functions would be helpful. Done.
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/707193004/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8f72990be00c94350ef23da23da3ea8f73a04d8c Cr-Commit-Position: refs/heads/master@{#303483} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
