Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1441683010:
Add UMA histogram for save link file type (Closed)
DescriptionAdd UMA histogram for save link file type
This CL adds a simple logic to record the file type when user long
presses the save link menu.
It guesses the mime type by checking the url extensions.
This may not work for all cases, but it should provide a good case study for
the provided crbug.
BUG=550711
Committed: https://crrev.com/63ad634082331cd41d47a1fee3e183cdc2b146ae
Cr-Commit-Position: refs/heads/master@{#360536}
Patch Set 1 #
Total comments: 4
Patch Set 2 : nit #Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : rebase #
Messages
Total messages: 29 (14 generated)
qinmin@chromium.org changed reviewers: + isherman@chromium.org, tedchoc@chromium.org
PTAL
LGTM % nits: https://codereview.chromium.org/1441683010/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/1441683010/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:87: // Note: these values must match the ContextMenuSaveLinkType enum in histograms.xml. nit: Please also document that this enum-like-thing should be treated as append-only, since it backs an UMA histogram. https://codereview.chromium.org/1441683010/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1441683010/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:55863: + <int value="5" label="Unkown"/> I'd recommend moving this to the front of the list. That way, if you ever add more items to the list, this one won't end up somewhere in the middle.
lgtm
https://codereview.chromium.org/1441683010/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/1441683010/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:87: // Note: these values must match the ContextMenuSaveLinkType enum in histograms.xml. On 2015/11/14 00:22:48, Ilya Sherman wrote: > nit: Please also document that this enum-like-thing should be treated as > append-only, since it backs an UMA histogram. Done. https://codereview.chromium.org/1441683010/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1441683010/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:55863: + <int value="5" label="Unkown"/> On 2015/11/14 00:22:48, Ilya Sherman wrote: > I'd recommend moving this to the front of the list. That way, if you ever add > more items to the list, this one won't end up somewhere in the middle. Done.
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1441683010/#ps20001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441683010/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441683010/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1441683010/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441683010/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441683010/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1441683010/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441683010/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441683010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on 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
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1441683010/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441683010/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441683010/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441683010/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441683010/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/63ad634082331cd41d47a1fee3e183cdc2b146ae Cr-Commit-Position: refs/heads/master@{#360536} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
