Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
763933002:
Record container for hosted app launches. (Closed)
|
Created:
6 years ago by benwells Modified:
6 years ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, asvitkine+watch_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRecord container for hosted app launches.
This will let us see how much the various launch containers are used,
without diluting those launches for platform apps which always use
the window container.
BUG=437363
Committed: https://crrev.com/6657c7a9f11b17e3a378b0bb2112ecc314ec9cad
Cr-Commit-Position: refs/heads/master@{#306735}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Feedback #
Total comments: 4
Patch Set 3 : Feedback #
Total comments: 2
Patch Set 4 : Feedback #
Messages
Total messages: 24 (8 generated)
benwells@chromium.org changed reviewers: + tapted@chromium.org
https://codereview.chromium.org/763933002/diff/1/chrome/browser/ui/extensions... File chrome/browser/ui/extensions/application_launch.cc (right): https://codereview.chromium.org/763933002/diff/1/chrome/browser/ui/extensions... chrome/browser/ui/extensions/application_launch.cc:147: "Extensions.AppLaunchContainer", params.container, 100); Should we just delete this? params.container doesn't change, and isn't passed to anything between here and the new one, so has no way of influencing anything. Or: just move it? Consistency of the UMA data could be a reason why not, but keeping the old metadata in histograms.xml feels like a debt. We could just comment in the field description that before Mwhatever this included packaged apps. https://codereview.chromium.org/763933002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/763933002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:525: +<histogram name="Apps.HostedAppLaunchContainer"> I think you need ` enum="AppLaunchContainer"` on this line as well (if we still want a newly named histogram)
On 2014/11/28 00:35:16, tapted wrote: > https://codereview.chromium.org/763933002/diff/1/chrome/browser/ui/extensions... > File chrome/browser/ui/extensions/application_launch.cc (right): > > https://codereview.chromium.org/763933002/diff/1/chrome/browser/ui/extensions... > chrome/browser/ui/extensions/application_launch.cc:147: > "Extensions.AppLaunchContainer", params.container, 100); > Should we just delete this? params.container doesn't change, and isn't passed to > anything between here and the new one, so has no way of influencing anything. > > Or: just move it? Consistency of the UMA data could be a reason why not, but > keeping the old metadata in histograms.xml feels like a debt. We could just > comment in the field description that before Mwhatever this included packaged > apps. I don't want to move it for consistency. For now I've kept the histogram but am not recording it any more. > > https://codereview.chromium.org/763933002/diff/1/tools/metrics/histograms/his... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/763933002/diff/1/tools/metrics/histograms/his... > tools/metrics/histograms/histograms.xml:525: +<histogram > name="Apps.HostedAppLaunchContainer"> > I think you need ` enum="AppLaunchContainer"` on this line as well (if we still > want a newly named histogram) Oops, done.
lgtm
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/763933002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
benwells@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for histograms.xml
benwells@chromium.org changed reviewers: - asvitkine@chromium.org
benwells@chromium.org changed reviewers: + isherman@chromium.org
-asvitkine who is ooo, +isherman for histograms.xml.
https://codereview.chromium.org/763933002/diff/20001/chrome/browser/ui/extens... File chrome/browser/ui/extensions/application_launch.cc (left): https://codereview.chromium.org/763933002/diff/20001/chrome/browser/ui/extens... chrome/browser/ui/extensions/application_launch.cc:147: "Extensions.AppLaunchContainer", params.container, 100); Please mark this histogram as <obsolete> https://codereview.chromium.org/763933002/diff/20001/chrome/browser/ui/extens... File chrome/browser/ui/extensions/application_launch.cc (right): https://codereview.chromium.org/763933002/diff/20001/chrome/browser/ui/extens... chrome/browser/ui/extensions/application_launch.cc:161: "Extensions.HostedAppLaunchContainer", params.container, 100); Why are you setting the max for this histogram at 100, rather than 4, which appears to be the number of buckets actually used by the histogram?
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/763933002/diff/20001/chrome/browser/ui/extens... File chrome/browser/ui/extensions/application_launch.cc (left): https://codereview.chromium.org/763933002/diff/20001/chrome/browser/ui/extens... chrome/browser/ui/extensions/application_launch.cc:147: "Extensions.AppLaunchContainer", params.container, 100); On 2014/12/02 02:52:01, Ilya Sherman wrote: > Please mark this histogram as <obsolete> Done. https://codereview.chromium.org/763933002/diff/20001/chrome/browser/ui/extens... File chrome/browser/ui/extensions/application_launch.cc (right): https://codereview.chromium.org/763933002/diff/20001/chrome/browser/ui/extens... chrome/browser/ui/extensions/application_launch.cc:161: "Extensions.HostedAppLaunchContainer", params.container, 100); On 2014/12/02 02:52:02, Ilya Sherman wrote: > Why are you setting the max for this histogram at 100, rather than 4, which > appears to be the number of buckets actually used by the histogram? No good reason. Updated to 4.
LGTM % a comment: https://codereview.chromium.org/763933002/diff/60001/chrome/browser/ui/extens... File chrome/browser/ui/extensions/application_launch.cc (right): https://codereview.chromium.org/763933002/diff/60001/chrome/browser/ui/extens... chrome/browser/ui/extensions/application_launch.cc:162: "Extensions.HostedAppLaunchContainer", params.container, 4); Rather than hardcoding the constant "4" here, please add an item like "LAUNCH_CONTAINER_COUNT" to the LaunchContainer enum, and reference that here. That way, if you ever add any items to the enum, the histogram code will remain correct.
https://codereview.chromium.org/763933002/diff/60001/chrome/browser/ui/extens... File chrome/browser/ui/extensions/application_launch.cc (right): https://codereview.chromium.org/763933002/diff/60001/chrome/browser/ui/extens... chrome/browser/ui/extensions/application_launch.cc:162: "Extensions.HostedAppLaunchContainer", params.container, 4); On 2014/12/02 23:51:52, Ilya Sherman wrote: > Rather than hardcoding the constant "4" here, please add an item like > "LAUNCH_CONTAINER_COUNT" to the LaunchContainer enum, and reference that here. > That way, if you ever add any items to the enum, the histogram code will remain > correct. Done.
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/763933002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6657c7a9f11b17e3a378b0bb2112ecc314ec9cad Cr-Commit-Position: refs/heads/master@{#306735} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
