Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
888693003:
Process WM_ENDSESSION in the traybar icon window. (Closed)
|
Created:
5 years, 10 months ago by Sigurður Ásgeirsson Modified:
5 years, 10 months ago CC:
chromium-reviews, tfarina, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionProcess WM_ENDSESSION in the traybar icon window.
BUG=441960
Committed: https://crrev.com/e6904ac0e7eddc08252f22cc26aae6919e90685f
Cr-Commit-Position: refs/heads/master@{#313934}
Patch Set 1 #Patch Set 2 : Now new and improved - compiles, even. #
Total comments: 8
Patch Set 3 : Address Will's nit. #Patch Set 4 : Address Alexei's comment. #
Messages
Total messages: 18 (5 generated)
siggi@chromium.org changed reviewers: + sky@chromium.org, wfh@chromium.org
siggi@chromium.org changed reviewers: + asvitkine@chromium.org
Hey Will, this needs some testing still, or we can just ship it on canary and see what we get in metrics. Sky, Alexei; owners approval please? Siggi
lgtm % nits https://codereview.chromium.org/888693003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/status_icons/status_tray_win.cc (right): https://codereview.chromium.org/888693003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/status_icons/status_tray_win.cc:230: // If chrome is in background-only mode, this is the only notification nit: Chrome https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:36209: +</histogram> this new histogram doesn't appear to be used
https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:36206: +<histogram name="Stability.ExitFunnel.TraybarEndSession" units="milliseconds"> Please use a histogram_suffixes construct for all of these which will make this much more concise.
LGTM
Alexei - good to go as is, or do you insist on a change? https://codereview.chromium.org/888693003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/status_icons/status_tray_win.cc (right): https://codereview.chromium.org/888693003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/status_icons/status_tray_win.cc:230: // If chrome is in background-only mode, this is the only notification On 2015/01/29 21:56:02, Will Harris wrote: > nit: Chrome Done. https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:36206: +<histogram name="Stability.ExitFunnel.TraybarEndSession" units="milliseconds"> On 2015/01/29 22:05:34, Alexei Svitkine wrote: > Please use a histogram_suffixes construct for all of these which will make this > much more concise. This will change the names and hashes of the existing histograms, as the suffixes seem to suffix with an underscore instead of a dot. If you insist, I can do this, but seeing as this is temporary instrumentation and we'll need matching changes to the processing pipeline, I don't see the benefit? https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:36209: +</histogram> On 2015/01/29 21:56:02, Will Harris wrote: > this new histogram doesn't appear to be used It's recorded in a fairly oblique manner by the exit funnel.
https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:36206: +<histogram name="Stability.ExitFunnel.TraybarEndSession" units="milliseconds"> On 2015/01/30 13:52:12, Sigurður Ásgeirsson wrote: > On 2015/01/29 22:05:34, Alexei Svitkine wrote: > > Please use a histogram_suffixes construct for all of these which will make > this > > much more concise. > > This will change the names and hashes of the existing histograms, as the > suffixes seem to suffix with an underscore instead of a dot. If you insist, I > can do this, but seeing as this is temporary instrumentation and we'll need > matching changes to the processing pipeline, I don't see the benefit? You can use separator="." to use the same names. The benefit is things are never removed from histograms.xml, only marked as <obsolete>. The suffixes approach will result in less clutter sticking around.
One moar look? https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/888693003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:36206: +<histogram name="Stability.ExitFunnel.TraybarEndSession" units="milliseconds"> On 2015/01/30 14:14:55, Alexei Svitkine wrote: > On 2015/01/30 13:52:12, Sigurður Ásgeirsson wrote: > > On 2015/01/29 22:05:34, Alexei Svitkine wrote: > > > Please use a histogram_suffixes construct for all of these which will make > > this > > > much more concise. > > > > This will change the names and hashes of the existing histograms, as the > > suffixes seem to suffix with an underscore instead of a dot. If you insist, I > > can do this, but seeing as this is temporary instrumentation and we'll need > > matching changes to the processing pipeline, I don't see the benefit? > > You can use separator="." to use the same names. > > The benefit is things are never removed from histograms.xml, only marked as > <obsolete>. The suffixes approach will result in less clutter sticking around. Ah, I didn't see that - maybe reason to amend the documentation at the top of the file :). So amended, and this is much more concise.
LGTM, thanks!
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/888693003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by siggi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/888693003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e6904ac0e7eddc08252f22cc26aae6919e90685f Cr-Commit-Position: refs/heads/master@{#313934} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
