Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779683004:
Send UMA about requested window state from tabs API (Closed)
DescriptionSend UMA about requested window state from tabs API
As part of the deprecation of docked windows, we need to record some
UMA stats to track the number of times the "docked" window state was
requested, so that we can remove it at the appropriate milestone.
BUG=703733
Review-Url: https://codereview.chromium.org/2779683004
Cr-Commit-Position: refs/heads/master@{#461044}
Committed: https://chromium.googlesource.com/chromium/src/+/b4b59de15639559e9fbdc818d3ca1ca960dbc0c0
Patch Set 1 #
Total comments: 3
Patch Set 2 : Devlin's comment #
Total comments: 6
Patch Set 3 : mpearson's comments #Patch Set 4 : Adding the warning as per mpearson's request. #
Total comments: 4
Patch Set 5 : mpearson's #
Messages
Total messages: 17 (6 generated)
afakhry@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, can you please take a look? https://codereview.chromium.org/2779683004/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2779683004/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:683: ConvertToWindowShowState(params->update_info.state); I'm wondering if I should report UMA here too on update of the state?
lgtm % adding the extra reporting site https://codereview.chromium.org/2779683004/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2779683004/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:683: ConvertToWindowShowState(params->update_info.state); On 2017/03/27 23:55:11, afakhry wrote: > I'm wondering if I should report UMA here too on update of the state? Yep, we should.
afakhry@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson for histograms.xml https://codereview.chromium.org/2779683004/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2779683004/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:683: ConvertToWindowShowState(params->update_info.state); On 2017/03/28 00:43:33, Devlin wrote: > On 2017/03/27 23:55:11, afakhry wrote: > > I'm wondering if I should report UMA here too on update of the state? > > Yep, we should. Done.
https://codereview.chromium.org/2779683004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2779683004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:204: UMA_HISTOGRAM_ENUMERATION("TabsApi.RequestedWindowState", state, It appears this enum comes from out/Debug/gen/chrome/common/extensions/api/windows.h which is auto-generated from chrome/common/extensions/api/windows.json It looks like that latter file allows comments. Can you please put the standard warning label about not changing or deleting any values in the enum? https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2779683004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2779683004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72645: + The requested window creation state from the tabs extensions API. Please state when this is recorded. Any time an "tabs extension" (whatever that is) creates a new window/tab? https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...
https://codereview.chromium.org/2779683004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2779683004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:204: UMA_HISTOGRAM_ENUMERATION("TabsApi.RequestedWindowState", state, On 2017/03/28 18:23:46, Mark P wrote: > It appears this enum comes from > out/Debug/gen/chrome/common/extensions/api/windows.h > which is auto-generated from > chrome/common/extensions/api/windows.json > It looks like that latter file allows comments. > > Can you please put the standard warning label about not changing or deleting any > values in the enum? > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... Hmm, but that's the whole point of this CL though. We're adding this UMA to help determine when we can remove one of the deprecated values "docked" in that enum, so that it is no longer exposed to extensions. Is there another approach? https://codereview.chromium.org/2779683004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2779683004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72645: + The requested window creation state from the tabs extensions API. On 2017/03/28 18:23:46, Mark P wrote: > Please state when this is recorded. Any time an "tabs extension" (whatever that > is) creates a new window/tab? > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... Done.
https://codereview.chromium.org/2779683004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2779683004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:204: UMA_HISTOGRAM_ENUMERATION("TabsApi.RequestedWindowState", state, On 2017/03/28 20:43:16, afakhry wrote: > On 2017/03/28 18:23:46, Mark P wrote: > > It appears this enum comes from > > out/Debug/gen/chrome/common/extensions/api/windows.h > > which is auto-generated from > > chrome/common/extensions/api/windows.json > > It looks like that latter file allows comments. > > > > Can you please put the standard warning label about not changing or deleting > any > > values in the enum? > > > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > > Hmm, but that's the whole point of this CL though. We're adding this UMA to help > determine when we can remove one of the deprecated values "docked" in that enum, > so that it is no longer exposed to extensions. > > Is there another approach? You could put in a warning label that says if something is removed, then this histogram will become unvalid and should be removed from the code and marked as obsolete in histograms.xml.
https://codereview.chromium.org/2779683004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2779683004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:204: UMA_HISTOGRAM_ENUMERATION("TabsApi.RequestedWindowState", state, On 2017/03/28 21:02:44, Mark P wrote: > On 2017/03/28 20:43:16, afakhry wrote: > > On 2017/03/28 18:23:46, Mark P wrote: > > > It appears this enum comes from > > > out/Debug/gen/chrome/common/extensions/api/windows.h > > > which is auto-generated from > > > chrome/common/extensions/api/windows.json > > > It looks like that latter file allows comments. > > > > > > Can you please put the standard warning label about not changing or deleting > > any > > > values in the enum? > > > > > > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > > > > Hmm, but that's the whole point of this CL though. We're adding this UMA to > help > > determine when we can remove one of the deprecated values "docked" in that > enum, > > so that it is no longer exposed to extensions. > > > > Is there another approach? > > You could put in a warning label that says if something is removed, then this > histogram will become unvalid and should be removed from the code and marked as > obsolete in histograms.xml. Done. Please take a look. Thanks!
lgtm https://codereview.chromium.org/2779683004/diff/60001/chrome/common/extension... File chrome/common/extensions/api/windows.json (right): https://codereview.chromium.org/2779683004/diff/60001/chrome/common/extension... chrome/common/extensions/api/windows.json:20: // WARNING: These values are written to logs. New enum values can be\ nit: errant \ https://codereview.chromium.org/2779683004/diff/60001/chrome/common/extension... chrome/common/extensions/api/windows.json:24: // valid, and make sure to mark it as obsolete in histograms.xml. optional nit: move comment next to "enum": [{ line
https://codereview.chromium.org/2779683004/diff/60001/chrome/common/extension... File chrome/common/extensions/api/windows.json (right): https://codereview.chromium.org/2779683004/diff/60001/chrome/common/extension... chrome/common/extensions/api/windows.json:20: // WARNING: These values are written to logs. New enum values can be\ On 2017/03/29 20:05:40, Mark P wrote: > nit: errant \ Done. https://codereview.chromium.org/2779683004/diff/60001/chrome/common/extension... chrome/common/extensions/api/windows.json:24: // valid, and make sure to mark it as obsolete in histograms.xml. On 2017/03/29 20:05:40, Mark P wrote: > optional nit: move comment next to "enum": [{ line Done.
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2779683004/#ps80001 (title: "mpearson's")
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": 80001, "attempt_start_ts": 1490930272953080,
"parent_rev": "c89cc32d2f148978d67d1acdbf855d599bb515d6", "commit_rev":
"b4b59de15639559e9fbdc818d3ca1ca960dbc0c0"}
Message was sent while issue was closed.
Description was changed from ========== Send UMA about requested window state from tabs API As part of the deprecation of docked windows, we need to record some UMA stats to track the number of times the "docked" window state was requested, so that we can remove it at the appropriate milestone. BUG=703733 ========== to ========== Send UMA about requested window state from tabs API As part of the deprecation of docked windows, we need to record some UMA stats to track the number of times the "docked" window state was requested, so that we can remove it at the appropriate milestone. BUG=703733 Review-Url: https://codereview.chromium.org/2779683004 Cr-Commit-Position: refs/heads/master@{#461044} Committed: https://chromium.googlesource.com/chromium/src/+/b4b59de15639559e9fbdc818d3ca... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/b4b59de15639559e9fbdc818d3ca... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
