Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(383)

Issue 2758453003: Recording download mime types for normal profile (Closed)

Created:
3 years, 9 months ago by shaktisahu
Modified:
3 years, 9 months ago
CC:
chromium-reviews, asanka, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Recording download mime types for normal profile BUG=699807 Review-Url: https://codereview.chromium.org/2758453003 Cr-Commit-Position: refs/heads/master@{#459344} Committed: https://chromium.googlesource.com/chromium/src/+/d87debc9b133cb364f9b307d9643ebb7c5bd7fb6

Patch Set 1 #

Total comments: 2

Patch Set 2 : started and completed count #

Patch Set 3 : Mime type breakdown #

Total comments: 4

Patch Set 4 : asanka@ comments #

Patch Set 5 : Fixed test #

Patch Set 6 : Rebase #

Total comments: 8

Patch Set 7 : Renamed histogam and added new metrics for new download #

Total comments: 2

Patch Set 8 : comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -24 lines) Patch
M content/browser/download/download_item_impl.cc View 1 2 3 4 5 6 4 chunks +13 lines, -3 lines 0 comments Download
M content/browser/download/download_item_impl_unittest.cc View 1 2 3 4 8 chunks +7 lines, -11 lines 0 comments Download
M content/browser/download/download_request_core.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/download/download_stats.h View 1 2 3 4 5 6 3 chunks +13 lines, -1 line 0 comments Download
M content/browser/download/download_stats.cc View 1 2 3 4 5 6 2 chunks +18 lines, -8 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 3 chunks +19 lines, -0 lines 0 comments Download

Messages

Total messages: 48 (29 generated)
shaktisahu
asanka@ - PTAL. I have a question below. https://codereview.chromium.org/2758453003/diff/1/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2758453003/diff/1/content/browser/download/download_item_impl.cc#newcode1261 content/browser/download/download_item_impl.cc:1261: RecordDownloadMimeType(mime_type_); ...
3 years, 9 months ago (2017-03-16 23:39:50 UTC) #3
asanka
There's a mismatch in counts as noted below, but that discrepancy should be miniscule compared ...
3 years, 9 months ago (2017-03-17 15:19:27 UTC) #4
shaktisahu
+dahlke@ to the conversation for the top level comment from asanka@. I think we just ...
3 years, 9 months ago (2017-03-20 20:16:58 UTC) #7
shaktisahu
On 2017/03/20 20:16:58, shaktisahu wrote: > +dahlke@ to the conversation for the top level comment ...
3 years, 9 months ago (2017-03-20 21:21:03 UTC) #8
shaktisahu
On 2017/03/20 21:21:03, shaktisahu wrote: > On 2017/03/20 20:16:58, shaktisahu wrote: > > +dahlke@ to ...
3 years, 9 months ago (2017-03-21 02:17:59 UTC) #9
David Trainor- moved to gerrit
lgtm % asanka
3 years, 9 months ago (2017-03-21 18:07:12 UTC) #10
asanka
https://codereview.chromium.org/2758453003/diff/40001/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2758453003/diff/40001/content/browser/download/download_item_impl.cc#newcode1163 content/browser/download/download_item_impl.cc:1163: if (GetWebContents() && For a new single resource download, ...
3 years, 9 months ago (2017-03-22 21:03:03 UTC) #11
asanka
And LGTM with the comments addressed and assuming you are okay with the caveats I ...
3 years, 9 months ago (2017-03-22 21:04:24 UTC) #12
shaktisahu
Thanks asanka@. I made the suggested changes. https://codereview.chromium.org/2758453003/diff/40001/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2758453003/diff/40001/content/browser/download/download_item_impl.cc#newcode1163 content/browser/download/download_item_impl.cc:1163: if (GetWebContents() ...
3 years, 9 months ago (2017-03-22 22:21:36 UTC) #15
asanka
https://codereview.chromium.org/2758453003/diff/100001/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2758453003/diff/100001/content/browser/download/download_item_impl.cc#newcode1266 content/browser/download/download_item_impl.cc:1266: RecordDownloadCount(START_COUNT); START_COUNT is probably better if recorded for all ...
3 years, 9 months ago (2017-03-23 14:34:43 UTC) #30
shaktisahu
https://codereview.chromium.org/2758453003/diff/100001/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2758453003/diff/100001/content/browser/download/download_item_impl.cc#newcode1266 content/browser/download/download_item_impl.cc:1266: RecordDownloadCount(START_COUNT); On 2017/03/23 14:34:43, asanka wrote: > START_COUNT is ...
3 years, 9 months ago (2017-03-23 15:33:28 UTC) #31
asanka
https://codereview.chromium.org/2758453003/diff/100001/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2758453003/diff/100001/content/browser/download/download_item_impl.cc#newcode1266 content/browser/download/download_item_impl.cc:1266: RecordDownloadCount(START_COUNT); On 2017/03/23 15:33:28, shaktisahu wrote: > On 2017/03/23 ...
3 years, 9 months ago (2017-03-23 16:23:45 UTC) #32
shaktisahu
https://codereview.chromium.org/2758453003/diff/100001/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2758453003/diff/100001/content/browser/download/download_item_impl.cc#newcode1266 content/browser/download/download_item_impl.cc:1266: RecordDownloadCount(START_COUNT); On 2017/03/23 16:23:45, asanka wrote: > On 2017/03/23 ...
3 years, 9 months ago (2017-03-23 23:05:34 UTC) #37
shaktisahu
isherman@ - PTAL
3 years, 9 months ago (2017-03-23 23:06:53 UTC) #39
asanka
LGTM. Comments are nits. Feel free to ignore :-) https://codereview.chromium.org/2758453003/diff/120001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2758453003/diff/120001/tools/metrics/histograms/histograms.xml#newcode13221 tools/metrics/histograms/histograms.xml:13221: ...
3 years, 9 months ago (2017-03-23 23:54:10 UTC) #40
Ilya Sherman
Metrics LGTM. FWIW, I think we agreed with the privacy team on an email thread ...
3 years, 9 months ago (2017-03-24 00:44:57 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2758453003/140001
3 years, 9 months ago (2017-03-24 01:54:22 UTC) #44
shaktisahu
On 2017/03/24 00:44:57, Ilya Sherman wrote: > Metrics LGTM. FWIW, I think we agreed with ...
3 years, 9 months ago (2017-03-24 01:56:50 UTC) #45
commit-bot: I haz the power
3 years, 9 months ago (2017-03-24 02:49:26 UTC) #48
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/d87debc9b133cb364f9b307d9643...

Powered by Google App Engine
This is Rietveld 408576698