Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1333323002:
Add UMA metrics for the frequency of closing dangerous download notifications. (Closed)
DescriptionAdd UMA metrics for the frequency of closing dangerous download notifications.
BUG=530622
Committed: https://crrev.com/8298258e31c8eb1b0886c5782547109d2cb86d8f
Cr-Commit-Position: refs/heads/master@{#350485}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressing nits. #
Total comments: 2
Patch Set 3 : Changes as per review comments. #
Total comments: 10
Patch Set 4 : Changes as per review comments. #Patch Set 5 : Changes as per review comments. #Patch Set 6 : Keeping changes of metrics for recording action only. #
Messages
Total messages: 23 (5 generated)
deepak.m1@samsung.com changed reviewers: + isherman@chromium.org, phajdan.jr@chromium.org
@phajdan for download_item_notification.cc @isherman for actions.xml PTAL
actions.xml lgtm % a nit: https://codereview.chromium.org/1333323002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1333323002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:2930: User close a download notification, which indicates a dangerous download. nit: "close" -> "closed"
@Ilya Thanks for review, nit addressed. @Pawal PTAL Thanks https://codereview.chromium.org/1333323002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1333323002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:2930: User close a download notification, which indicates a dangerous download. On 2015/09/11 20:35:21, Ilya Sherman wrote: > nit: "close" -> "closed" Done.
@Pawel PTAL
deepak.m1@samsung.com changed reviewers: + asanka@chromium.org
@Asanka, PTAL at this small change. Thanks
https://codereview.chromium.org/1333323002/diff/20001/chrome/browser/download... File chrome/browser/download/notification/download_item_notification.cc (right): https://codereview.chromium.org/1333323002/diff/20001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:163: {DownloadCommands::OPEN_WHEN_COMPLETE, DownloadsCommands is a dense enum. Why not just use it as an index into an array instead of doing a linear search? I bet under the covers that's exactly what the previous switch statement compiles into.
@Asanka Thanks for review. Changes done as suggested. PTAL https://codereview.chromium.org/1333323002/diff/20001/chrome/browser/download... File chrome/browser/download/notification/download_item_notification.cc (right): https://codereview.chromium.org/1333323002/diff/20001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:163: {DownloadCommands::OPEN_WHEN_COMPLETE, On 2015/09/16 14:07:35, asanka wrote: > DownloadsCommands is a dense enum. Why not just use it as an index into an array > instead of doing a linear search? I bet under the covers that's exactly what the > previous switch statement compiles into. oops!! Good catch.
https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... File chrome/browser/download/notification/download_item_notification.cc (left): https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:647: switch (command) { The advantage of using a switch is that: 1. Compilers are good about optimizing switches, specially when they are dense. So even though it looks like a control flow structure that looks at each case label, that's not how it behaves. In fact I'd argue that there's not much of a performance gain here. 2. If a new value is added to the DownloadCommands::Command enum, the compiler will fail to build this switch statement since it lacks a label for the new value. You lose this when switching to a map. Get get something like that back, you'll have to add a new enum to the end of the DownloadCommands::Command enum (like MAX), and then add a static_assert() to validate the size of the map (i.e. something like static_assert(arraysize(kDownloadNotificationCommand) == DownloadCommands::MAX, "..."). 3. switches like this are compatible with enum classes. Maps aren't. This is, of course, only a problem if we want to switch to using enum classes for DownloadCommands (which would be a good thing), but that's not currently planned. By the time you accomplish 2, and given 1, you aren't gaining much by switching to a map. https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:652: id = IDS_DOWNLOAD_STATUS_OPEN_WHEN_COMPLETE; How did this slip by? I think it's supposed to say "Open" after the download is done. Which kind of makes the map not work. https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... File chrome/browser/download/notification/download_item_notification.cc (right): https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:157: struct DownloadItemNotificationCommandEntry { This is no longer necessary since you are only using the |id| field. https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:162: DownloadItemNotificationCommandEntry kDownloadNotificationCommand[] = { Pick a name that implies that this is a mapping from DownloadCommands to its corresponding string ID suitable for use as a button label for the action. Something like kDownloadCommandToLabelStringIDMap or something shorter. Also document that the ordering follows the enum. https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:668: int id = kDownloadNotificationCommand[command].id; This lookup is off by one and will access past the end of the array for LEARN_MORE_INTERRUPTED. You need bounds checks before using |command| as an index.
@Asanka Thanks for review. 1) I have added static_assert but used DownloadCommands::LEARN_MORE_INTERRUPTED as max , as |Command| starts with 1 and adding "MAX" required handling at various other swtiches. 2) Documented that "Order of array elements should be same as order of |Command| enum items" 3) Documented that "DownloadCommands::LEARN_MORE_INTERRUPTED should be the last entry in the enum". PTAL Thanks https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... File chrome/browser/download/notification/download_item_notification.cc (left): https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:647: switch (command) { On 2015/09/17 14:29:40, asanka wrote: > The advantage of using a switch is that: > > 1. Compilers are good about optimizing switches, specially when they are dense. > So even though it looks like a control flow structure that looks at each case > label, that's not how it behaves. In fact I'd argue that there's not much of a > performance gain here. > > 2. If a new value is added to the DownloadCommands::Command enum, the compiler > will fail to build this switch statement since it lacks a label for the new > value. You lose this when switching to a map. Get get something like that back, > you'll have to add a new enum to the end of the DownloadCommands::Command enum > (like MAX), and then add a static_assert() to validate the size of the map (i.e. > something like static_assert(arraysize(kDownloadNotificationCommand) == > DownloadCommands::MAX, "..."). > > 3. switches like this are compatible with enum classes. Maps aren't. This is, of > course, only a problem if we want to switch to using enum classes for > DownloadCommands (which would be a good thing), but that's not currently > planned. > > By the time you accomplish 2, and given 1, you aren't gaining much by switching > to a map. Acknowledged. https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:652: id = IDS_DOWNLOAD_STATUS_OPEN_WHEN_COMPLETE; On 2015/09/17 14:29:40, asanka wrote: > How did this slip by? I think it's supposed to say "Open" after the download is > done. Which kind of makes the map not work. I think here the message is "In both the cases when the download is going on, as well as once download get completed, label is same" https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... File chrome/browser/download/notification/download_item_notification.cc (right): https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:157: struct DownloadItemNotificationCommandEntry { On 2015/09/17 14:29:40, asanka wrote: > This is no longer necessary since you are only using the |id| field. Done. https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:162: DownloadItemNotificationCommandEntry kDownloadNotificationCommand[] = { On 2015/09/17 14:29:40, asanka wrote: > Pick a name that implies that this is a mapping from DownloadCommands to its > corresponding string ID suitable for use as a button label for the action. > Something like kDownloadCommandToLabelStringIDMap or something shorter. > > Also document that the ordering follows the enum. Done. https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_item_notification.cc:668: int id = kDownloadNotificationCommand[command].id; On 2015/09/17 14:29:40, asanka wrote: > This lookup is off by one and will access past the end of the array for > LEARN_MORE_INTERRUPTED. You need bounds checks before using |command| as an > index. Done.
On 2015/09/18 at 04:38:02, deepak.m1 wrote: > @Asanka > Thanks for review. > > 1) I have added static_assert but used DownloadCommands::LEARN_MORE_INTERRUPTED as max , as |Command| starts with 1 and adding "MAX" required handling at various other swtiches. Yeah, this is exactly what should happen. i.e. that any change to DownloadCommands::Command enum fails to compile if all the call sites aren't updated to handle a new enum or if a handled enum is removed. Using an actual value in place of the MAX value has the side-effect that the ordering now becomes significant. You can't order the values based on some semantic ordering because LEARN_MORE_INTERRUPTED has to be at the end. > 2) Documented that "Order of array elements should be same as order of |Command| enum items" This isn't enforced via the compiler nor tests. Any reordering or renaming of an enum value will silently succeed. > 3) Documented that "DownloadCommands::LEARN_MORE_INTERRUPTED should be the last entry in the enum". Not good. Unfortunately I don't think this change is an improvement in either readability, maintainability nor efficiency. Hence I'm going to not lgtm this. If you'd like to proceed with just the UMA addition, please update the CL by removing the other changes. > PTAL > Thanks > > https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... > File chrome/browser/download/notification/download_item_notification.cc (left): > > https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... > chrome/browser/download/notification/download_item_notification.cc:647: switch (command) { > On 2015/09/17 14:29:40, asanka wrote: > > The advantage of using a switch is that: > > > > 1. Compilers are good about optimizing switches, specially when they are dense. > > So even though it looks like a control flow structure that looks at each case > > label, that's not how it behaves. In fact I'd argue that there's not much of a > > performance gain here. > > > > 2. If a new value is added to the DownloadCommands::Command enum, the compiler > > will fail to build this switch statement since it lacks a label for the new > > value. You lose this when switching to a map. Get get something like that back, > > you'll have to add a new enum to the end of the DownloadCommands::Command enum > > (like MAX), and then add a static_assert() to validate the size of the map (i.e. > > something like static_assert(arraysize(kDownloadNotificationCommand) == > > DownloadCommands::MAX, "..."). > > > > 3. switches like this are compatible with enum classes. Maps aren't. This is, of > > course, only a problem if we want to switch to using enum classes for > > DownloadCommands (which would be a good thing), but that's not currently > > planned. > > > > By the time you accomplish 2, and given 1, you aren't gaining much by switching > > to a map. > > Acknowledged. > > https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... > chrome/browser/download/notification/download_item_notification.cc:652: id = IDS_DOWNLOAD_STATUS_OPEN_WHEN_COMPLETE; > On 2015/09/17 14:29:40, asanka wrote: > > How did this slip by? I think it's supposed to say "Open" after the download is > > done. Which kind of makes the map not work. > > I think here the message is "In both the cases when the download is going on, as well as once download get completed, label is same" > > https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... > File chrome/browser/download/notification/download_item_notification.cc (right): > > https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... > chrome/browser/download/notification/download_item_notification.cc:157: struct DownloadItemNotificationCommandEntry { > On 2015/09/17 14:29:40, asanka wrote: > > This is no longer necessary since you are only using the |id| field. > > Done. > > https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... > chrome/browser/download/notification/download_item_notification.cc:162: DownloadItemNotificationCommandEntry kDownloadNotificationCommand[] = { > On 2015/09/17 14:29:40, asanka wrote: > > Pick a name that implies that this is a mapping from DownloadCommands to its > > corresponding string ID suitable for use as a button label for the action. > > Something like kDownloadCommandToLabelStringIDMap or something shorter. > > > > Also document that the ordering follows the enum. > > Done. > > https://codereview.chromium.org/1333323002/diff/40001/chrome/browser/download... > chrome/browser/download/notification/download_item_notification.cc:668: int id = kDownloadNotificationCommand[command].id; > On 2015/09/17 14:29:40, asanka wrote: > > This lookup is off by one and will access past the end of the array for > > LEARN_MORE_INTERRUPTED. You need bounds checks before using |command| as an > > index. > > Done.
@Asanka keeping recording metrics information changes only as suggested. PTAL.
asanka@chromium.org changed reviewers: + yoshiki@chromium.org
Over to yoshiki@ The BUG annotation seems no longer applicable. Also "UMA matrices information" -> "UMA metrics" Use the commit title to describe what it's doing. The remainder of the description can be used to describe why, although in this case it's self evident. E.g.: "Add UMA metrics for the frequency of closing dangerous download notifications"
@Asanka Thanks for review, Title updated. @Yoshiki PTAL at this small change. Thanks
On 2015/09/24 02:55:52, Deepak wrote: > @Asanka > Thanks for review, Title updated. > @Yoshiki > PTAL at this small change. > Thanks LGTM
lgtm
The CQ bit was checked by deepak.m1@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1333323002/#ps100001 (title: "Keeping changes of metrics for recording action only.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1333323002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1333323002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8298258e31c8eb1b0886c5782547109d2cb86d8f Cr-Commit-Position: refs/heads/master@{#350485} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
