Add a metrics extensions API....

Add a metrics extensions API.

See http://docs.google.com/View?id=dd4ngnpz_0dpsb8n96 for API proposal.

BUG=0
TEST=See unit tests as part of this CL


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=40130

[Jói, 2010-02-24 03:28:49]

Cool stuff, tiny drive-by:

http://codereview.chromium.org/657037/diff/1/15
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/657037/diff/1/15#newcode208
chrome/common/chrome_switches.cc:208: const char kEnableMetricsExtensionApis[]  
 = "enable-metrics-extension-apis";
apis -> api (for consistency with e.g. kEnableExtensionTimelineApi below)

[(NOT FOR CODE REVIEWS), 2010-02-24 04:20:24]

Hi Joi,

Done as is for consistency with kEnableExperimentalExtensionApis above :-)

Thanks,
Roger

-



On Tue, Feb 23, 2010 at 22:28,  <joi@chromium.org> wrote:
> Cool stuff, tiny drive-by:
>
>
> http://codereview.chromium.org/657037/diff/1/15
> File chrome/common/chrome_switches.cc (right):
>
> http://codereview.chromium.org/657037/diff/1/15#newcode208
> chrome/common/chrome_switches.cc:208: const char
> kEnableMetricsExtensionApis[]    = "enable-metrics-extension-apis";
> apis -> api (for consistency with e.g. kEnableExtensionTimelineApi
> below)
>
> http://codereview.chromium.org/657037
>

[Jói, 2010-02-24 21:58:01]

kEnableExperimentalExtensionApis enables all chrome.experimental.*
APIs if I understand correctly, whereas your flag enables only one
"API" similar to the kEnableExtensionTimelineApi flag.

I don't mind much either way though :)

Cheers,
Jói


On Tue, Feb 23, 2010 at 11:19 PM, Roger Tawa <rogerta@google.com> wrote:
> Hi Joi,
>
> Done as is for consistency with kEnableExperimentalExtensionApis above :-)
>
> Thanks,
> Roger
>
> -
>
>
>
> On Tue, Feb 23, 2010 at 22:28,  <joi@chromium.org> wrote:
>> Cool stuff, tiny drive-by:
>>
>>
>> http://codereview.chromium.org/657037/diff/1/15
>> File chrome/common/chrome_switches.cc (right):
>>
>> http://codereview.chromium.org/657037/diff/1/15#newcode208
>> chrome/common/chrome_switches.cc:208: const char
>> kEnableMetricsExtensionApis[]    = "enable-metrics-extension-apis";
>> apis -> api (for consistency with e.g. kEnableExtensionTimelineApi
>> below)
>>
>> http://codereview.chromium.org/657037
>>
>

[Erik does not do reviews, 2010-02-24 22:10:01]

For the record, I agree with Joi.  It's a minor point though.

Erik


2010/2/24 Jói Sigurðsson <joi@chromium.org>

> kEnableExperimentalExtensionApis enables all chrome.experimental.*
> APIs if I understand correctly, whereas your flag enables only one
> "API" similar to the kEnableExtensionTimelineApi flag.
>
> I don't mind much either way though :)
>
> Cheers,
> Jói
>
>
> On Tue, Feb 23, 2010 at 11:19 PM, Roger Tawa <rogerta@google.com> wrote:
> > Hi Joi,
> >
> > Done as is for consistency with kEnableExperimentalExtensionApis above
> :-)
> >
> > Thanks,
> > Roger
> >
> > -
> >
> >
> >
> > On Tue, Feb 23, 2010 at 22:28,  <joi@chromium.org> wrote:
> >> Cool stuff, tiny drive-by:
> >>
> >>
> >> http://codereview.chromium.org/657037/diff/1/15
> >> File chrome/common/chrome_switches.cc (right):
> >>
> >> http://codereview.chromium.org/657037/diff/1/15#newcode208
> >> chrome/common/chrome_switches.cc:208: const char
> >> kEnableMetricsExtensionApis[]    = "enable-metrics-extension-apis";
> >> apis -> api (for consistency with e.g. kEnableExtensionTimelineApi
> >> below)
> >>
> >> http://codereview.chromium.org/657037
> >>
> >
>

[(NOT FOR CODE REVIEWS), 2010-02-25 13:13:00]

Hi guys,

OK, I'll change that.

Aaron, Evan: after tomorrow, I'm away until March 9th.  If you don't
think you'll have time to look at this today please let me know and
I'll find other reviewers.

Thanks,
Roger

-



2010/2/24 Erik Kay <erikkay@chromium.org>:
> For the record, I agree with Joi.  It's a minor point though.
> Erik
>
> 2010/2/24 Jói Sigurðsson <joi@chromium.org>
>>
>> kEnableExperimentalExtensionApis enables all chrome.experimental.*
>> APIs if I understand correctly, whereas your flag enables only one
>> "API" similar to the kEnableExtensionTimelineApi flag.
>>
>> I don't mind much either way though :)
>>
>> Cheers,
>> Jói
>>
>>
>> On Tue, Feb 23, 2010 at 11:19 PM, Roger Tawa <rogerta@google.com> wrote:
>> > Hi Joi,
>> >
>> > Done as is for consistency with kEnableExperimentalExtensionApis above
>> > :-)
>> >
>> > Thanks,
>> > Roger
>> >
>> > -
>> >
>> >
>> >
>> > On Tue, Feb 23, 2010 at 22:28,  <joi@chromium.org> wrote:
>> >> Cool stuff, tiny drive-by:
>> >>
>> >>
>> >> http://codereview.chromium.org/657037/diff/1/15
>> >> File chrome/common/chrome_switches.cc (right):
>> >>
>> >> http://codereview.chromium.org/657037/diff/1/15#newcode208
>> >> chrome/common/chrome_switches.cc:208: const char
>> >> kEnableMetricsExtensionApis[]    = "enable-metrics-extension-apis";
>> >> apis -> api (for consistency with e.g. kEnableExtensionTimelineApi
>> >> below)
>> >>
>> >> http://codereview.chromium.org/657037
>> >>
>> >
>
>

[Evan Martin, 2010-02-25 13:58:12]

metrics bits seem fine to me

http://codereview.chromium.org/657037/diff/1/9
File chrome/browser/extensions/extension_metrics_module.cc (right):

http://codereview.chromium.org/657037/diff/1/9#newcode27
chrome/browser/extensions/extension_metrics_module.cc:27: //
enable-metrics-extension-apis command line switch is used.  Refer to
make sure this matches the command line flag, whichever you decide :)

[Roger Tawa OOO till Jul 10th, 2010-02-25 15:27:27]

Thanks Erik, Evan, Joi.

Comments addressed, fixed build error found by trybots, changes uploaded.

http://codereview.chromium.org/657037/diff/1/9
File chrome/browser/extensions/extension_metrics_module.cc (right):

http://codereview.chromium.org/657037/diff/1/9#newcode27
chrome/browser/extensions/extension_metrics_module.cc:27: //
enable-metrics-extension-apis command line switch is used.  Refer to
On 2010/02/25 13:58:12, Evan Martin wrote:
> make sure this matches the command line flag, whichever you decide :)

Done.

http://codereview.chromium.org/657037/diff/1/15
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/657037/diff/1/15#newcode208
chrome/common/chrome_switches.cc:208: const char kEnableMetricsExtensionApis[]  
 = "enable-metrics-extension-apis";
On 2010/02/24 03:28:50, Jói wrote:
> apis -> api (for consistency with e.g. kEnableExtensionTimelineApi below)

Done.

[Aaron Boodman, 2010-02-25 21:20:07]

Easy peasy.

http://codereview.chromium.org/657037/diff/3002/3017
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/657037/diff/3002/3017#newcode552
chrome/renderer/resources/extension_process_bindings.js:552:
apiFunctions["metrics.recordPercentage"].handleRequest = function(name,
I think we want to minimize custom bindings.... so I'd prefer to see three
different API functions that just delegate to one shared C++ function, and then
you can eliminate this JS.

[Aaron Boodman, 2010-02-25 21:22:15]

This looks fine to me structurally, and since it is behind a flag, I don't
really care about the API details all that much. Whatever works works.

Do you need me to review the code, or was Joi on that?

[Roger Tawa OOO till Jul 10th, 2010-02-25 21:32:36]

Hi Arron,

Evan reviewed from a metrics points of view, I just wanted you to take
a look from the extensions point of view.  If you think its
structurally fine and not too worried about the api, then I guess
that's it :-)

Thanks,
Roger

-



On Thu, Feb 25, 2010 at 16:22,  <aa@chromium.org> wrote:
> This looks fine to me structurally, and since it is behind a flag, I don't
> really care about the API details all that much. Whatever works works.
>
> Do you need me to review the code, or was Joi on that?
>
> http://codereview.chromium.org/657037
>

[Roger Tawa OOO till Jul 10th, 2010-02-26 02:09:55]

Hi guys,

Aaron thought it was better to implement all the helper functions as C++ instead
of as javascript code, so I reverted the changes to
extension_process_bindings.js.  No behaviour or api change.  Please have another
look if you like.

[Jói, 2010-02-26 12:52:04]

LGTM

[Roger Tawa OOO till Jul 10th, 2010-02-26 17:47:16]

Hi all,

I realized that this change was the source of the failure of browser_tests
ExtensionApiTest.Stubs, because I was not creating stubs for the metrics api
functions.  To fix this, I had to make a change to a new file:
renderer_extension_bindings.js.  Nothing else changed in any of the other files.

I'll commit after making sure that all bots are green.  Thanks.

[Roger Tawa OOO till Jul 10th, 2010-02-26 17:58:27]

For completeness, replying to Arron's comment.

http://codereview.chromium.org/657037/diff/3002/3017
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/657037/diff/3002/3017#newcode552
chrome/renderer/resources/extension_process_bindings.js:552:
apiFunctions["metrics.recordPercentage"].handleRequest = function(name,
On 2010/02/25 21:20:08, Aaron Boodman wrote:
> I think we want to minimize custom bindings.... so I'd prefer to see three
> different API functions that just delegate to one shared C++ function, and
then
> you can eliminate this JS.

Done.