Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13888010:
Improve the code added in r194423 by using the Version class. (Closed)
|
Created:
7 years, 8 months ago by Lei Zhang Modified:
7 years, 8 months ago CC:
chromium-reviews, MAD, Ilya Sherman, jar (doing other things) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionImprove the code added in r194423 by using the Version class.
BUG=224531
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194771
Patch Set 1 #
Total comments: 7
Patch Set 2 : int #Messages
Total messages: 11 (0 generated)
lgtm % comments https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:75: uint16 glibc_major_version = version.components()[0]; here and below, just use int? https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:81: 11 - UMA_LINUX_GLIBC_2_11; wondering if it'll make our life easier if we just letting UMA_LINUX_GLIBC_2_11 == 11. Then we don't need translation.
https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:75: uint16 glibc_major_version = version.components()[0]; On 2013/04/16 22:51:38, xhwang wrote: > here and below, just use int? |version.components()| returns a vector of uint16s. https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:81: 11 - UMA_LINUX_GLIBC_2_11; On 2013/04/16 22:51:38, xhwang wrote: > wondering if it'll make our life easier if we just letting UMA_LINUX_GLIBC_2_11 > == 11. Then we don't need translation. jar@ preferred if the enum did not have any gaps. See previous CL.
LGTM (with comment/question/nit below). https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:80: const uint16 kGlibcMinorVersionTranslationOffset = nit: not a big deal... but why mess with uint16? The natural int is generally nice and simple and fast. https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:81: 11 - UMA_LINUX_GLIBC_2_11; ... and also... we'll eventually support major version 3 (I'm guessing) and it is nice to map each one into its own range. We do waste space with an enumerated histogram when we don't use the slots, so it is just a good practice to not leave gaps. ...in this case.. not the end of the world... but a good practice. On 2013/04/16 23:20:23, Lei Zhang wrote: > On 2013/04/16 22:51:38, xhwang wrote: > > wondering if it'll make our life easier if we just letting > UMA_LINUX_GLIBC_2_11 > > == 11. Then we don't need translation. > > jar@ preferred if the enum did not have any gaps. See previous CL.
https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/13888010/diff/1/chrome/browser/metrics/chrome... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:81: 11 - UMA_LINUX_GLIBC_2_11; On 2013/04/16 23:42:52, jar wrote: > ... and also... we'll eventually support major version 3 (I'm guessing) and it > is nice to map each one into its own range. > > We do waste space with an enumerated histogram when we don't use the slots, so > it is just a good practice to not leave gaps. ...in this case.. not the end of > the world... but a good practice. > > > On 2013/04/16 23:20:23, Lei Zhang wrote: > > On 2013/04/16 22:51:38, xhwang wrote: > > > wondering if it'll make our life easier if we just letting > > UMA_LINUX_GLIBC_2_11 > > > == 11. Then we don't need translation. > > > > jar@ preferred if the enum did not have any gaps. See previous CL. > sg, I just hate the number "11" here :) But not a big deal.
On 2013/04/16 23:42:52, jar wrote: > nit: not a big deal... but why mess with uint16? The natural int is generally > nice and simple and fast. I guess there's no issues converting a uint16 into an int. Int it is.
On 2013/04/17 00:31:58, Lei Zhang wrote: > On 2013/04/16 23:42:52, jar wrote: > > nit: not a big deal... but why mess with uint16? The natural int is generally > > nice and simple and fast. > > I guess there's no issues converting a uint16 into an int. Int it is. +1 lgtm++
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/13888010/10001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/13888010/10001
Message was sent while issue was closed.
Change committed as 194771 |
