Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1403803003:
Add metrics for window.external.AddSearchProvider and (Closed)
|
Created:
5 years, 2 months ago by Evan Stade Modified:
5 years, 2 months ago CC:
asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, dcheng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metrics for window.external.AddSearchProvider and
IsSeachProviderInstalled
BUG=542487
Patch Set 1 #
Total comments: 6
Messages
Total messages: 18 (3 generated)
estade@chromium.org changed reviewers: + jochen@chromium.org, mpearson@chromium.org, pkasting@chromium.org
I don't think we can use the typical feature counter because this feature is not core blink. Please review whatever files you care about. Owner-wise, jochen: chrome/browser/ui/browser.cc chrome/renderer/external_extension.cc pkasting: chrome/browser/ui/browser.cc mpearson: tools/metrics/actions/actions.xml
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403803003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403803003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
LGTM https://codereview.chromium.org/1403803003/diff/1/chrome/renderer/external_ex... File chrome/renderer/external_extension.cc (right): https://codereview.chromium.org/1403803003/diff/1/chrome/renderer/external_ex... chrome/renderer/external_extension.cc:167: base::UserMetricsAction("window.external.IsSearchProviderInstalled_success")); 80 columns
https://codereview.chromium.org/1403803003/diff/1/chrome/renderer/external_ex... File chrome/renderer/external_extension.cc (right): https://codereview.chromium.org/1403803003/diff/1/chrome/renderer/external_ex... chrome/renderer/external_extension.cc:167: base::UserMetricsAction("window.external.IsSearchProviderInstalled_success")); On 2015/10/13 18:36:20, Peter Kasting wrote: > 80 columns I don't think I can because of this comment[1], unless I reduce the indentation. [1] // WARNING: When using base::UserMetricsAction, base::UserMetricsAction // and a string literal parameter must be on the same line, e.g. // RenderThread::Get()->RecordAction( // base::UserMetricsAction("my extremely long action name")); // because otherwise our processing scripts won't pick up on new actions.
https://codereview.chromium.org/1403803003/diff/1/chrome/renderer/external_ex... File chrome/renderer/external_extension.cc (right): https://codereview.chromium.org/1403803003/diff/1/chrome/renderer/external_ex... chrome/renderer/external_extension.cc:167: base::UserMetricsAction("window.external.IsSearchProviderInstalled_success")); On 2015/10/13 18:40:57, Evan Stade wrote: > On 2015/10/13 18:36:20, Peter Kasting wrote: > > 80 columns > > I don't think I can because of this comment[1], unless I reduce the indentation. > > [1] > // WARNING: When using base::UserMetricsAction, base::UserMetricsAction > // and a string literal parameter must be on the same line, e.g. > // RenderThread::Get()->RecordAction( > // base::UserMetricsAction("my extremely long action name")); > // because otherwise our processing scripts won't pick up on new actions. Based on extract_actions.py line 53, I think this comment is incorrect. You should check to be sure, but I think the right thing to do is to remove or modify that comment and rewrap your code.
was it possible to add a Web API to https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... and use that here? This would give us counts normalized to page loads, the counts would show up on chromestatus.com, and it would allow for automatically creating deprecation messages in the devtools console.
On 2015/10/13 19:10:37, jochen wrote: > was it possible to add a Web API to > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > and use that here? It seems like it would be a lot of work to move UseCounter::Feature to a public directory. > > This would give us counts normalized to page loads, the counts would show up on > http://chromestatus.com, and it would allow for automatically creating deprecation > messages in the devtools console. How would you distinguish between "API was called" and "API was called but nothing happened because it was used incorrectly"? Why is normalizing over page loads preferable to normalizing over users?
On 2015/10/13 at 19:16:58, estade wrote: > On 2015/10/13 19:10:37, jochen wrote: > > was it possible to add a Web API to > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > and use that here? > > It seems like it would be a lot of work to move UseCounter::Feature to a public directory. you only need to expose the enums you need, and translate them to UseCounter::Feature internally. we do this e.g. for v8 features as well here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > This would give us counts normalized to page loads, the counts would show up on > > http://chromestatus.com, and it would allow for automatically creating deprecation > > messages in the devtools console. > > How would you distinguish between "API was called" and "API was called but nothing happened because it was used incorrectly"? Why is normalizing over page loads preferable to normalizing over users? the cut-off for removing a web exposed feature is defined as normalized over page loads. for distinguishing those two cases, I'd introduce two features in the UseCounter. btw, I'm OOO for the next few days, so you might want to add somebody else as reviewer for this
On 2015/10/13 19:35:20, jochen wrote: > On 2015/10/13 at 19:16:58, estade wrote: > > On 2015/10/13 19:10:37, jochen wrote: > > > was it possible to add a Web API to > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > and use that here? > > > > It seems like it would be a lot of work to move UseCounter::Feature to a > public directory. > > you only need to expose the enums you need, and translate them to > UseCounter::Feature internally. we do this e.g. for v8 features as well here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > > This would give us counts normalized to page loads, the counts would show up > on > > > http://chromestatus.com, and it would allow for automatically creating > deprecation > > > messages in the devtools console. > > > > How would you distinguish between "API was called" and "API was called but > nothing happened because it was used incorrectly"? Why is normalizing over page > loads preferable to normalizing over users? > > the cut-off for removing a web exposed feature is defined as normalized over > page loads. > > for distinguishing those two cases, I'd introduce two features in the > UseCounter. ok. So this requires an addition to the Web API and also additional browser->renderer IPC. > > btw, I'm OOO for the next few days, so you might want to add somebody else as > reviewer for this
On 2015/10/13 at 20:15:45, estade wrote: > On 2015/10/13 19:35:20, jochen wrote: > > On 2015/10/13 at 19:16:58, estade wrote: > > > On 2015/10/13 19:10:37, jochen wrote: > > > > was it possible to add a Web API to > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > and use that here? > > > > > > It seems like it would be a lot of work to move UseCounter::Feature to a > > public directory. > > > > you only need to expose the enums you need, and translate them to > > UseCounter::Feature internally. we do this e.g. for v8 features as well here: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > > > > > > This would give us counts normalized to page loads, the counts would show up > > on > > > > http://chromestatus.com, and it would allow for automatically creating > > deprecation > > > > messages in the devtools console. > > > > > > How would you distinguish between "API was called" and "API was called but > > nothing happened because it was used incorrectly"? Why is normalizing over page > > loads preferable to normalizing over users? > > > > the cut-off for removing a web exposed feature is defined as normalized over > > page loads. > > > > for distinguishing those two cases, I'd introduce two features in the > > UseCounter. > > ok. So this requires an addition to the Web API and also additional browser->renderer IPC. > well, alternatively keep measuring the incorrect usage using the metrics action. adding a web api is cheap these days, you can do it all in one cl > > > > btw, I'm OOO for the next few days, so you might want to add somebody else as > > reviewer for this
A broader question: why do you need this as user actions rather than histograms? Are you doing something with the additional timing information you'd get? --mark https://codereview.chromium.org/1403803003/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1403803003/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:13043: <action name="window.external.AddSearchProvider_success"> _success doesn't seem like the right name for this perhaps _requestedconfirmation
> A broader question: why do you need this as user actions rather than histograms? > Are you doing something with the additional timing information you'd get? No reason. I guess I can use the "user counts" checkbox on a histogram. Given Jochen's insistence on using blink metrics, I was thinking of changing this into a histogram to get the success:attempts ratio and then in blink just measure attempts. https://codereview.chromium.org/1403803003/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1403803003/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:13043: <action name="window.external.AddSearchProvider_success"> On 2015/10/13 21:16:29, Mark P wrote: > _success doesn't seem like the right name for this > perhaps _requestedconfirmation We always either do nothing or request confirmation. There's no silent success.
https://codereview.chromium.org/1403803003/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1403803003/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:13043: <action name="window.external.AddSearchProvider_success"> On 2015/10/13 21:36:34, Evan Stade wrote: > On 2015/10/13 21:16:29, Mark P wrote: > > _success doesn't seem like the right name for this > > perhaps _requestedconfirmation > > We always either do nothing or request confirmation. There's no silent success. I know this point is basically moot now, but I read window.external.AddSearchProvider_success as the AddSearchProvider code was called and the user approved the request (if necessary).
closing this in favor of https://codereview.chromium.org/1410823003/ and https://codereview.chromium.org/1407163002 +cc dcheng for reference |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
