Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2798723002:
Fix the UMA stats calculation because of overflow (Closed)
DescriptionFix the UMA stats calculation because of overflow
size_t can be 32 bit, and it can easily overflow when calculating bandwidth.
BUG=708248
Review-Url: https://codereview.chromium.org/2798723002
Cr-Commit-Position: refs/heads/master@{#462221}
Committed: https://chromium.googlesource.com/chromium/src/+/d91c32221adad968b718754a00a6fc4573340e6b
Patch Set 1 #
Total comments: 3
Messages
Total messages: 16 (6 generated)
qinmin@chromium.org changed reviewers: + dtrainor@chromium.org, xingliu@chromium.org
PTAL
PTAL
https://codereview.chromium.org/2798723002/diff/1/content/browser/download/do... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2798723002/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:776: size_t bytes_downloaded_with_parallel_streams, nit%: Not in this patch, but does it make sense to make bytes_downloaded_* and bytes_seen_ to be int64_t.
https://codereview.chromium.org/2798723002/diff/1/content/browser/download/do... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2798723002/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:776: size_t bytes_downloaded_with_parallel_streams, On 2017/04/05 00:29:10, xingliu wrote: > nit%: Not in this patch, but does it make sense to make bytes_downloaded_* and > bytes_seen_ to be int64_t. normally anything non-negative and has file system constraints are size_t. For example. file size and memory size should both use size_t.When the underlying CPU architecture changes from 32bit to 64bit or to 128bit, we don't need to change the code. Bytes_downloaded and bytes_seen_ are related to file size, so I would keep them as size_t unless we really need to worry about the case of downloading a big filewhose size is not supported by the filesystem.
lgtm, Thanks for the fix. https://codereview.chromium.org/2798723002/diff/1/content/browser/download/do... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2798723002/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:776: size_t bytes_downloaded_with_parallel_streams, On 2017/04/05 05:33:55, qinmin wrote: > On 2017/04/05 00:29:10, xingliu wrote: > > nit%: Not in this patch, but does it make sense to make bytes_downloaded_* and > > bytes_seen_ to be int64_t. > > normally anything non-negative and has file system constraints are size_t. > For example. file size and memory size should both use size_t.When the > underlying CPU architecture changes from 32bit to 64bit or to 128bit, we don't > need to change the code. > Bytes_downloaded and bytes_seen_ are related to file size, so I would keep them > as size_t unless we really need to worry about the case of downloading a big > filewhose size is not supported by the filesystem. sgtm, but does cpu architecture limit the size of sdcard on Android? It's a bit weird.
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/05 17:56:38, xingliu wrote: > lgtm, > > Thanks for the fix. > > https://codereview.chromium.org/2798723002/diff/1/content/browser/download/do... > File content/browser/download/download_stats.cc (right): > > https://codereview.chromium.org/2798723002/diff/1/content/browser/download/do... > content/browser/download/download_stats.cc:776: size_t > bytes_downloaded_with_parallel_streams, > On 2017/04/05 05:33:55, qinmin wrote: > > On 2017/04/05 00:29:10, xingliu wrote: > > > nit%: Not in this patch, but does it make sense to make bytes_downloaded_* > and > > > bytes_seen_ to be int64_t. > > > > normally anything non-negative and has file system constraints are size_t. > > For example. file size and memory size should both use size_t.When the > > underlying CPU architecture changes from 32bit to 64bit or to 128bit, we don't > > need to change the code. > > Bytes_downloaded and bytes_seen_ are related to file size, so I would keep > them > > as size_t unless we really need to worry about the case of downloading a big > > filewhose size is not supported by the filesystem. > > sgtm, but does cpu architecture limit the size of sdcard on Android? It's a bit > weird. no, it is just the file size.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1491425486971100, "parent_rev":
"925f9caecc8b0b5bdb25aa8223c98c384f1dccbc", "commit_rev":
"d91c32221adad968b718754a00a6fc4573340e6b"}
Message was sent while issue was closed.
Description was changed from ========== Fix the UMA stats calculation because of overflow size_t can be 32 bit, and it can easily overflow when calculating bandwidth. BUG=708248 ========== to ========== Fix the UMA stats calculation because of overflow size_t can be 32 bit, and it can easily overflow when calculating bandwidth. BUG=708248 Review-Url: https://codereview.chromium.org/2798723002 Cr-Commit-Position: refs/heads/master@{#462221} Committed: https://chromium.googlesource.com/chromium/src/+/d91c32221adad968b718754a00a6... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/d91c32221adad968b718754a00a6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
