|
|
Created:
7 years, 10 months ago by SteveT Modified:
7 years, 10 months ago CC:
chromium-reviews, MAD, Ilya Sherman, jar (doing other things) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionLog the freshness of the Variations seed in a histogram.
We do this by storing a pref of the last successful (200 or 304) fetch and comparing it to the next one.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184452
Patch Set 1 #
Total comments: 6
Patch Set 2 : use UMA_HISTOGRAMS_CUSTOM_TIMES #Patch Set 3 : use custom_counts #
Total comments: 6
Patch Set 4 : compare to startup #
Total comments: 11
Patch Set 5 : asvit nit #
Total comments: 2
Patch Set 6 : handle 200s if valid #
Total comments: 2
Patch Set 7 : Move log call #Patch Set 8 : local_state_ crash fix #Patch Set 9 : check comment #
Messages
Total messages: 28 (0 generated)
PTAL. I am mostly concerned about how sane UMA_HISTOGRAMS_COUNTS_10000 is.
https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", You sure you don't want UMA_HISTOGRAM_CUSTOM_TIMES?
Question for you! https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", Hmm, it's a little hard to parse what UMA_HISTOGRAM_CUSTOM_TIMES does from histograms.h. What's the time unit that it takes? Some uses of it suggest it takes the raw unit of TimeDelta (which is something super granular like microseconds or something?).
https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", On 2013/02/22 00:38:37, SteveT wrote: > Hmm, it's a little hard to parse what UMA_HISTOGRAM_CUSTOM_TIMES does from > histograms.h. > > What's the time unit that it takes? Some uses of it suggest it takes the raw > unit of TimeDelta (which is something super granular like microseconds or > something?). You give it bounds for the highest granularity of time that you care about (e.g. half an hour) and the lowest granularity (e.g. 7 days), and the number of buckets to use in the histogram. Then you pass in a TimeDelta object as the sample, and it does the conversion for you. So, it's essentially equivalent to UMA_HISTOGRAM_CUSTOM_COUNTS, except that it will do consistent time conversions for you.
Question for you in the first patch regarding the second patch. :) https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", So in my latest patch, I used UMA_HISTOGRAM_CUSTOM_TIMES as you suggested (with highest granularity = 1 minute, lowest = 7 days). Now I'm guessing crazy big values that I don't understand. Are they in TimeDelta raw units? Does the dashboard make them more readable in any way (ie - provide units in the UI)?
https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", On 2013/02/22 01:01:02, SteveT wrote: > So in my latest patch, I used UMA_HISTOGRAM_CUSTOM_TIMES as you suggested (with > highest granularity = 1 minute, lowest = 7 days). Now I'm guessing crazy big > values that I don't understand. Are they in TimeDelta raw units? Does the > dashboard make them more readable in any way (ie - provide units in the UI)? Oh, I see what you mean. You're right, the units on the dashboard would end up in milliseconds, which could be hard to read. Sounds like a nice feature improvement to the dashboard; but for now I guess I'd recommend something like the following: UMA_HISTOGRAM_CUSTOM_COUNTS( "Variations.SeedFreshness", 1, TimeDelta::FromDays(7).InMinutes(), 50, delta.InMinutes()); But, if you think COUNTS_10000 is clearer, that's ok too. Sorry for the misdirection!
Cool, thanks. PTAL. https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", Yeah, if the dashboard had a way to select the time units for TIME histograms, this would be fine. But since we're dealing with potentially long periods, doing the units in minutes is way easier to read, methinks. I like your compromise. Easier to understand than _10000.
LGTM, thanks. https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:295: int64 internal_now = base::Time::Now().ToInternalValue(); nit: I'd recommend writing this as base::Time now = base::Time::Now(); and converting to the internal representation down on line 309. https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:302: base::TimeDelta::FromInternalValue(internal_delta); nit: I'd write this as base::TimeDelta delta = now - base::Time::FromInternalValue(last_fetch_time_internal); In general, it's preferable to perform calculations with base::Time and base::TimeDelta objects than with their internal values.
https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedFreshness", delta.InMinutes(), Doing this here will not give us as much info as doing it in the way we discussed - where we log it at startup when we create field trials. It's not really relevant if in a given 16 hour Chrome session, the browser downloaded the seed 4 time without using it. In fact, we can already derive such info from Variations.SeedFetchResponseCode. For example, consider the hypothetical scenario where we modify the code to ping every 10 minutes after startup for the first 5 hours, and then every 5 hours otherwise. Suppose that 99% of users restart Chrome only after 6 hours has passed. In the current CL's implementation, the metric would indicate a very much improved freshness of the seed (since all the successful attempts during the first 5 hours would record the seed is only 10 minutes old). Except this would be misleading because it won't actually improve freshness in practice if all users are only restarting after 6 hours. We really need to record the freshness at startup, when the seed is being applied to paint an accurate picture here.
Back to Alexei (Ilya's comments not addressed yet). https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedFreshness", delta.InMinutes(), I see, so what we're measuring here is "how recently was the seed successfully updated" where as what you're suggesting we measure is "how recently was the seed APPLIED"? But since the seed is applied on every startup, we end up just getting a measure of how long a Chrome session is, right? Unless you mean to measure how often a *new* seed is applied (after a 200), in which case we're sort of just measuring how often developers update configurations. That doesn't seem useful, either. Could you clarify what you mean by logging at startup? Are we still comparing it to the time recorded here?
On 2013/02/22 15:17:54, SteveT wrote: > Back to Alexei (Ilya's comments not addressed yet). > > https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... > File chrome/browser/metrics/variations/variations_service.cc (right): > > https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... > chrome/browser/metrics/variations/variations_service.cc:304: > UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedFreshness", delta.InMinutes(), > I see, so what we're measuring here is "how recently was the seed successfully > updated" where as what you're suggesting we measure is "how recently was the > seed APPLIED"? > > But since the seed is applied on every startup, we end up just getting a measure > of how long a Chrome session is, right? Unless you mean to measure how often a > *new* seed is applied (after a 200), in which case we're sort of just measuring > how often developers update configurations. That doesn't seem useful, either. No, neither of those - you're confused. > Could you clarify what you mean by logging at startup? Are we still comparing it > to the time recorded here? Save the time of the success/304 in prefs as you do now in this CL. At startup (when trials are applied), do HISTOGRAM(Time::Now() - last_200_or_304_response_time_from_prefs) Yes, we will get one histogram entry per Chrome startup, but that's not the point. The point is to answer the question "How fresh is the seed that gets used by Chrome", where "used" means applying field trials from it on startup and freshness depends on last_200_or_304_response_time_from_prefs (how long ago did we get a new seed (200) or confirmed that the seed we have is current (304)).
On 2013/02/22 15:25:39, Alexei Svitkine wrote: > On 2013/02/22 15:17:54, SteveT wrote: > > Back to Alexei (Ilya's comments not addressed yet). > > > > > https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... > > File chrome/browser/metrics/variations/variations_service.cc (right): > > > > > https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... > > chrome/browser/metrics/variations/variations_service.cc:304: > > UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedFreshness", delta.InMinutes(), > > I see, so what we're measuring here is "how recently was the seed successfully > > updated" where as what you're suggesting we measure is "how recently was the > > seed APPLIED"? > > > > But since the seed is applied on every startup, we end up just getting a > measure > > of how long a Chrome session is, right? Unless you mean to measure how often a > > *new* seed is applied (after a 200), in which case we're sort of just > measuring > > how often developers update configurations. That doesn't seem useful, either. > > No, neither of those - you're confused. > > > Could you clarify what you mean by logging at startup? Are we still comparing > it > > to the time recorded here? > > Save the time of the success/304 in prefs as you do now in this CL. > > At startup (when trials are applied), do > HISTOGRAM(Time::Now() - last_200_or_304_response_time_from_prefs) > > Yes, we will get one histogram entry per Chrome startup, but that's not the > point. > > The point is to answer the question "How fresh is the seed that gets used by > Chrome", where "used" means applying field trials from it on startup and > freshness depends on last_200_or_304_response_time_from_prefs (how long ago did > we get a new seed (200) or confirmed that the seed we have is current (304)). Yeah, that makes more sense. Another way to interpret that is "the time between the next browser startup and the last seed download". So if the period is long, it means that they've sat around for a while without getting a new seed (so if it's ever longer than our 5h period, it means that Chrome was not running). Trying to figure out how this will help us figure out the mobile implementation... I guess we simply want to ensure that changing the way we schedule a seed request does NOT increase this value by a large margin, correct? (with whatever scheduling scheme we end up implementing for mobile)
On 2013/02/22 15:33:30, SteveT wrote: > On 2013/02/22 15:25:39, Alexei Svitkine wrote: > > On 2013/02/22 15:17:54, SteveT wrote: > > > Back to Alexei (Ilya's comments not addressed yet). > > > > > > > > > https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... > > > File chrome/browser/metrics/variations/variations_service.cc (right): > > > > > > > > > https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... > > > chrome/browser/metrics/variations/variations_service.cc:304: > > > UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedFreshness", delta.InMinutes(), > > > I see, so what we're measuring here is "how recently was the seed > successfully > > > updated" where as what you're suggesting we measure is "how recently was the > > > seed APPLIED"? > > > > > > But since the seed is applied on every startup, we end up just getting a > > measure > > > of how long a Chrome session is, right? Unless you mean to measure how often > a > > > *new* seed is applied (after a 200), in which case we're sort of just > > measuring > > > how often developers update configurations. That doesn't seem useful, > either. > > > > No, neither of those - you're confused. > > > > > Could you clarify what you mean by logging at startup? Are we still > comparing > > it > > > to the time recorded here? > > > > Save the time of the success/304 in prefs as you do now in this CL. > > > > At startup (when trials are applied), do > > HISTOGRAM(Time::Now() - last_200_or_304_response_time_from_prefs) > > > > Yes, we will get one histogram entry per Chrome startup, but that's not the > > point. > > > > The point is to answer the question "How fresh is the seed that gets used by > > Chrome", where "used" means applying field trials from it on startup and > > freshness depends on last_200_or_304_response_time_from_prefs (how long ago > did > > we get a new seed (200) or confirmed that the seed we have is current (304)). > > Yeah, that makes more sense. Another way to interpret that is "the time between > the next browser startup and the last seed download". So if the period is long, > it means that they've sat around for a while without getting a new seed (so if > it's ever longer than our 5h period, it means that Chrome was not running). > > Trying to figure out how this will help us figure out the mobile > implementation... I guess we simply want to ensure that changing the way we > schedule a seed request does NOT increase this value by a large margin, correct? > (with whatever scheduling scheme we end up implementing for mobile) Right. Additionally, it will help us evaluate the effect of changes we make to the desktop implementation on this histogram (the value of which we want to minimize). It will help us answer questions like: 1. How much of an effect does the AU seed fetch give us? (once we make that change) 2. How much do we lose if we fetch the seed 10 minutes after startup instead of on startup. 3. How much do we lose if we fetch every 5 hours per prefs instead of always on startup as we do now.
Made the suggested changes (and incorporated Ilya's suggestions). Back to you. https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:295: int64 internal_now = base::Time::Now().ToInternalValue(); On 2013/02/22 02:05:05, Ilya Sherman wrote: > nit: I'd recommend writing this as > > base::Time now = base::Time::Now(); > > and converting to the internal representation down on line 309. Done. https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:302: base::TimeDelta::FromInternalValue(internal_delta); On 2013/02/22 02:05:05, Ilya Sherman wrote: > nit: I'd write this as > > base::TimeDelta delta = > now - base::Time::FromInternalValue(last_fetch_time_internal); > > In general, it's preferable to perform calculations with base::Time and > base::TimeDelta objects than with their internal values. Done.
https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:182: base::Time now = base::Time::Now(); Move this into the if statement. Make it const. https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:183: int64 last_fetch_time_internal = Nit: const. https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:186: DCHECK_GE(now.ToInternalValue(), last_fetch_time_internal); This may happen if the user changed his system time between, which is something that could happen (and can't really be avoided). Don't DCHECK on it. I think we can just log these anyway - so that we have the info about them in the logs (assuming the histogram supports negative values). https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:187: base::TimeDelta delta = Nit: const. https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:191: 1, base::TimeDelta::FromDays(7).InMinutes(), 50); I would say let's use 30 days, just to get a more complete story (for folks who don't relaunch their browser as much). How did you come up with the 50 for the last parameter?
Back to you. Ilya: Question for you inline re: Histograms. https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:182: base::Time now = base::Time::Now(); On 2013/02/22 16:26:25, Alexei Svitkine wrote: > Move this into the if statement. Make it const. Done. https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:183: int64 last_fetch_time_internal = On 2013/02/22 16:26:25, Alexei Svitkine wrote: > Nit: const. Done. https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:186: DCHECK_GE(now.ToInternalValue(), last_fetch_time_internal); Removed the DCHECK. I think the histogram might just log it in the smallest bucket. Ilya, can you comment on what happens if we pass a negative sample into a histogram? https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:187: base::TimeDelta delta = On 2013/02/22 16:26:25, Alexei Svitkine wrote: > Nit: const. Done. https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:191: 1, base::TimeDelta::FromDays(7).InMinutes(), 50); On 2013/02/22 16:26:25, Alexei Svitkine wrote: > I would say let's use 30 days, just to get a more complete story (for folks who > don't relaunch their browser as much). Done. > > How did you come up with the 50 for the last parameter? UMA_HISTOGRAM_CUSTOM_COUNTS_10000 has 50 buckets. 7 days = 10080 minutes and our unit is in minutes. We didn't use UMA_HISTOGRAM_CUSTOM_COUNTS_10000 because expanding out the InMinutes() here makes the code easier to read.
Looking good, pending on Ilya's clarification about the histogram. One comment below, though. https://codereview.chromium.org/12314053/diff/5/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/5/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:307: local_state_->SetInt64(prefs::kVariationsLastFetchTime, Hmm, one little issue with doing this here is that it won't catch the case where there's an early return in StoreSeedData() - e.g. if the server replied with 200 but the payload was bad. But we can't just move this snippet there because it won't catch the 304 case. How about making this a separate function and calling from this function for the 304 case and from the end of StoreSeedData in the 200 other case?
Back to you. What do you think? https://codereview.chromium.org/12314053/diff/5/chrome/browser/metrics/variat... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/5/chrome/browser/metrics/variat... chrome/browser/metrics/variations/variations_service.cc:307: local_state_->SetInt64(prefs::kVariationsLastFetchTime, What do you think of this latest change? Another option is to make it a anonymous namespace function that takes Local State as a param. Not sure which you may prefer (I don't think this is prohibited)
LGTM with a suggestion that can avoid the additional if statement. Please wait for confirmation from Ilya about the histogram question, though. https://codereview.chromium.org/12314053/diff/13004/chrome/browser/metrics/va... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/13004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:315: UMA_HISTOGRAM_MEDIUM_TIMES("Variations.FetchNotModifiedLatency", latency); Might as well put it here.
Okay cool. Just waiting on Ilya, then. https://codereview.chromium.org/12314053/diff/13004/chrome/browser/metrics/va... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/13004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:315: UMA_HISTOGRAM_MEDIUM_TIMES("Variations.FetchNotModifiedLatency", latency); Ah yes, done.
https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... chrome/browser/metrics/variations/variations_service.cc:186: DCHECK_GE(now.ToInternalValue(), last_fetch_time_internal); On 2013/02/22 18:12:03, SteveT wrote: > Removed the DCHECK. > > I think the histogram might just log it in the smallest bucket. Ilya, can you > comment on what happens if we pass a negative sample into a histogram? Yes, negative values end up in the lowest bucket, which is the underflow bucket.
On 2013/02/22 23:26:16, Ilya Sherman wrote: > https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... > File chrome/browser/metrics/variations/variations_service.cc (right): > > https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/va... > chrome/browser/metrics/variations/variations_service.cc:186: > DCHECK_GE(now.ToInternalValue(), last_fetch_time_internal); > On 2013/02/22 18:12:03, SteveT wrote: > > Removed the DCHECK. > > > > I think the histogram might just log it in the smallest bucket. Ilya, can you > > comment on what happens if we pass a negative sample into a histogram? > > Yes, negative values end up in the lowest bucket, which is the underflow bucket. I think were OK with that behaviour? If we see a large spike in the smallest bucket, then we know that a lot of users are experiencing time changes. It's probably not a very likely scenario. Will start the CQ ... feel free to uncheck if you object.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevet@chromium.org/12314053/6023
Retried try job too often on linux_chromeos for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
One thing I realised this isn't handling is the first-run case via master_prefs, per: https://chromiumcodereview.appspot.com/12223062 I'm okay with you doing it in a follow-up CL, but can you add a change that saves the new pref in the first-run-master-prefs case? Or alternatively, just use the value from prefs::kVariationsSeedDate if the other pref isn't set. (Note: we can't just use prefs::kVariationsSeedDate directly instead of the new pref since other than on first-run, that value is in network time units and we must report the delta w.r.t. system time.)
On 2013/02/23 15:42:35, Alexei Svitkine wrote: > One thing I realised this isn't handling is the first-run case via master_prefs, > per: > > https://chromiumcodereview.appspot.com/12223062 > > I'm okay with you doing it in a follow-up CL, but can you add a change that > saves the new pref in the first-run-master-prefs case? Or alternatively, just > use the value from prefs::kVariationsSeedDate if the other pref isn't set. > > (Note: we can't just use prefs::kVariationsSeedDate directly instead of the new > pref since other than on first-run, that value is in network time units and we > must report the delta w.r.t. system time.) Sure. Will follow up after this lands.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevet@chromium.org/12314053/9013
Message was sent while issue was closed.
Change committed as 184452 |