Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903883003:
Log printer additions and removals by protocol. (Closed)
|
Created:
3 years, 7 months ago by skau Modified:
3 years, 6 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, asvitkine+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLog printer additions and removals by protocol.
In order to track printer churn, we record an event when a printer is
succesfully added and when a printer is removed in the settings dialogs.
This data is tracked by protocol.
BUG=726103
Review-Url: https://codereview.chromium.org/2903883003
Cr-Commit-Position: refs/heads/master@{#476146}
Committed: https://chromium.googlesource.com/chromium/src/+/6d46ea6e7636ecdb53b00795f4893cbc32694f30
Patch Set 1 #Patch Set 2 : record the add #Patch Set 3 : rebase #
Total comments: 2
Patch Set 4 : rebased #Patch Set 5 : minor corrections #Patch Set 6 : log removal unconditionally #Patch Set 7 : rebase #Patch Set 8 : rebase again #
Total comments: 3
Patch Set 9 : address comments #Patch Set 10 : rebase #Patch Set 11 : rebase #
Messages
Total messages: 57 (35 generated)
skau@chromium.org changed reviewers: + mpearson@chromium.org, xdai@chromium.org
xdai@: Original author mpearson@: metrics
On 2017/05/25 05:16:03, skau wrote: > xdai@: Original author > mpearson@: metrics lgtm
skau@chromium.org changed reviewers: + hcarmona@chromium.org
hcarmona@: OWNER of settings
If you have a concrete answer to the note below, feel free to submit this with a TBR=mpearson. If you're adding an enums.xml entry, I'd like to re-review. --mark https://codereview.chromium.org/2903883003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2903883003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56543: +<histogram name="Printing.CUPS.PrinterAdded" enum="PrinterProtocol"> I don't see this already existing in enums.xml. Did you forget to add enums.xml to this changelist?
Description was changed from ========== Log printer additions and removals by protocol. In order to track printer churn, we record an event when a printer is succesfully added and when a printer is removed in the settings dialogs. This data is tracked by protocol. BUG=726103 ========== to ========== Log printer additions and removals by protocol. In order to track printer churn, we record an event when a printer is succesfully added and when a printer is removed in the settings dialogs. This data is tracked by protocol. BUG=726103 TBR=mpearson ==========
Thanks. I just rebased it on top of the CL with the enum. https://codereview.chromium.org/2903883003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2903883003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56543: +<histogram name="Printing.CUPS.PrinterAdded" enum="PrinterProtocol"> On 2017/05/25 20:48:17, Mark P wrote: > I don't see this already existing in enums.xml. Did you forget to add enums.xml > to this changelist? It's in the dependent patch set that I just landed.
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org Link to the patchset: https://codereview.chromium.org/2903883003/#ps60001 (title: "rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm for the record For future reference, if you touch printer_configuration.h again, can you please correct the warning label by PrinterProtocol // An enumeration of printer protocols. Do not change these values as they // are used in enums.xml. https://cs.chromium.org/chromium/src/chromeos/printing/printer_configuration.... to the standard one? https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... The current label implies the values can be changed if enums.xml is changed simultaneously. That's not true. On 2017/05/26 03:10:11, skau wrote: > Thanks. I just rebased it on top of the CL with the enum. Okay. --mark > https://codereview.chromium.org/2903883003/diff/40001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2903883003/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:56543: +<histogram > name="Printing.CUPS.PrinterAdded" enum="PrinterProtocol"> > On 2017/05/25 20:48:17, Mark P wrote: > > I don't see this already existing in enums.xml. Did you forget to add > enums.xml > > to this changelist? > > It's in the dependent patch set that I just landed.
Description was changed from ========== Log printer additions and removals by protocol. In order to track printer churn, we record an event when a printer is succesfully added and when a printer is removed in the settings dialogs. This data is tracked by protocol. BUG=726103 TBR=mpearson ========== to ========== Log printer additions and removals by protocol. In order to track printer churn, we record an event when a printer is succesfully added and when a printer is removed in the settings dialogs. This data is tracked by protocol. BUG=726103 ==========
Thanks. I just fixed the comment now.
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2903883003/#ps80001 (title: "minor corrections")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by skau@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by skau@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by skau@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hcarmona@: PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
skau@chromium.org changed reviewers: + michaelpg@chromium.org - hcarmona@chromium.org
michaelpg@: Can you take a look as OWNER?
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
lgtm https://codereview.chromium.org/2903883003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2903883003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:187: LOG(WARNING) << "Tried to remove non-existant printer"; existent https://codereview.chromium.org/2903883003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:187: LOG(WARNING) << "Tried to remove non-existant printer"; do we really need this LOG()?
Thanks for the reviews. https://codereview.chromium.org/2903883003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2903883003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:187: LOG(WARNING) << "Tried to remove non-existant printer"; On 2017/05/31 18:06:17, Dan Beam wrote: > do we really need this LOG()? Now that you mention it, it's not that useful. Removed.
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, mpearson@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2903883003/#ps160001 (title: "address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, mpearson@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2903883003/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by skau@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, mpearson@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2903883003/#ps200001 (title: "rebase")
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": 200001, "attempt_start_ts": 1496272954610590,
"parent_rev": "8cb1774575276a4e506ffed6270b5f14415c6f6a", "commit_rev":
"6d46ea6e7636ecdb53b00795f4893cbc32694f30"}
Message was sent while issue was closed.
Description was changed from ========== Log printer additions and removals by protocol. In order to track printer churn, we record an event when a printer is succesfully added and when a printer is removed in the settings dialogs. This data is tracked by protocol. BUG=726103 ========== to ========== Log printer additions and removals by protocol. In order to track printer churn, we record an event when a printer is succesfully added and when a printer is removed in the settings dialogs. This data is tracked by protocol. BUG=726103 Review-Url: https://codereview.chromium.org/2903883003 Cr-Commit-Position: refs/heads/master@{#476146} Committed: https://chromium.googlesource.com/chromium/src/+/6d46ea6e7636ecdb53b00795f489... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/6d46ea6e7636ecdb53b00795f489... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
