Adding logging of offstore extensions state to user metrics.

Adding logging of offstore extensions state to user metrics.

BUG=395729
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289673

[Alexei Svitkine (slow), 2014-08-11 15:01:58]

Will take a closer look this afternoon - some initial comments.

https://codereview.chromium.org/464463003/diff/20001/chrome/browser/metrics/e...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/20001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:34: bool
IsOffStoreExtension(const extensions::Extension& extension,
Add a comment.

https://codereview.chromium.org/464463003/diff/20001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:41: extension.location()
== extensions::Manifest::EXTERNAL_COMPONENT)
Nit: {}'s

https://codereview.chromium.org/464463003/diff/20001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:151: hasOffStore =
hasOffStore || CheckForOffStore(*extensions.get(), *verifier);
Nit: hacker_style please

https://codereview.chromium.org/464463003/diff/20001/components/metrics/proto...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/464463003/diff/20001/components/metrics/proto...
components/metrics/proto/system_profile.proto:511: optional bool
has_offstore_extensions = 19;
Can you make this an enum so that it's easier to add new states, in case we need
them?

Also, add a comment please.

[jwd, 2014-08-11 19:47:35]

changing to enum

[jwd, 2014-08-11 20:06:27]

https://codereview.chromium.org/464463003/diff/20001/chrome/browser/metrics/e...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/20001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:34: bool
IsOffStoreExtension(const extensions::Extension& extension,
On 2014/08/11 15:01:57, Alexei Svitkine wrote:
> Add a comment.

Done.

https://codereview.chromium.org/464463003/diff/20001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:41: extension.location()
== extensions::Manifest::EXTERNAL_COMPONENT)
On 2014/08/11 15:01:57, Alexei Svitkine wrote:
> Nit: {}'s

Done.

https://codereview.chromium.org/464463003/diff/20001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:151: hasOffStore =
hasOffStore || CheckForOffStore(*extensions.get(), *verifier);
On 2014/08/11 15:01:57, Alexei Svitkine wrote:
> Nit: hacker_style please

Done.

https://codereview.chromium.org/464463003/diff/20001/components/metrics/proto...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/464463003/diff/20001/components/metrics/proto...
components/metrics/proto/system_profile.proto:511: optional bool
has_offstore_extensions = 19;
On 2014/08/11 15:01:58, Alexei Svitkine wrote:
> Can you make this an enum so that it's easier to add new states, in case we
need
> them?
> 
> Also, add a comment please.

Done.

[Alexei Svitkine (slow), 2014-08-12 15:22:40]

Can you also add someone from the extensions team as a reviewer?

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:38: OFF_STORE
Can you use the proto's enum directly instead of this extra enum and a
conversion function?

I guess the reason you don't do this is so you can have the max() logic in the
loops below. If that's the intention, please document that in a comment on this
enum (so that if add new values, we'll preserve such an ordering).

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:59: // webstore, we
attempt to verifie with the |InstallVerifier| by checking if it
Nit: verify

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:60: // has been explicitly
deemed invalid. If the |InstallVerifier| is inactive or
Nit: No need for |'s around InstallVerifier since it's a class name. You can
alternatively change to |verifier| if you want, but I don't have a preference
here.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:88: else
Nit: Remove else and unindent return below.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:97: ++it) {
Nit: for (size_t i = 0; i < extensions.size(); ++i) is 2 lines shorter. Same
below.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:180: std::vector<Profile*>
profiles = profile_manager->GetLoadedProfiles();
Add a comment explaining how we treat multi-profiles.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:190: extensions_state =
std::max(extensions_state,
Nit: This deserves a comment.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:190: extensions_state =
std::max(extensions_state,
Nit: Add a comment about the max() logic.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
File chrome/browser/metrics/extensions_metrics_provider.h (right):

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.h:37: 
Nit: Remove this blank line.

[jwd, 2014-08-12 18:45:29]

+asargent@ for extensions review.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:38: OFF_STORE
On 2014/08/12 15:22:39, Alexei Svitkine wrote:
> Can you use the proto's enum directly instead of this extra enum and a
> conversion function?
> 
> I guess the reason you don't do this is so you can have the max() logic in the
> loops below. If that's the intention, please document that in a comment on
this
> enum (so that if add new values, we'll preserve such an ordering).

Done.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:59: // webstore, we
attempt to verifie with the |InstallVerifier| by checking if it
On 2014/08/12 15:22:39, Alexei Svitkine wrote:
> Nit: verify

Done.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:60: // has been explicitly
deemed invalid. If the |InstallVerifier| is inactive or
On 2014/08/12 15:22:39, Alexei Svitkine wrote:
> Nit: No need for |'s around InstallVerifier since it's a class name. You can
> alternatively change to |verifier| if you want, but I don't have a preference
> here.

Done.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:88: else
On 2014/08/12 15:22:39, Alexei Svitkine wrote:
> Nit: Remove else and unindent return below.

Done.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:97: ++it) {
On 2014/08/12 15:22:39, Alexei Svitkine wrote:
> Nit: for (size_t i = 0; i < extensions.size(); ++i) is 2 lines shorter. Same
> below.

Done below, not done her as discussed.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:180: std::vector<Profile*>
profiles = profile_manager->GetLoadedProfiles();
On 2014/08/12 15:22:40, Alexei Svitkine wrote:
> Add a comment explaining how we treat multi-profiles.

Done.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:190: extensions_state =
std::max(extensions_state,
On 2014/08/12 15:22:39, Alexei Svitkine wrote:
> Nit: This deserves a comment.

Done.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:190: extensions_state =
std::max(extensions_state,
On 2014/08/12 15:22:39, Alexei Svitkine wrote:
> Nit: Add a comment about the max() logic.

Done.

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
File chrome/browser/metrics/extensions_metrics_provider.h (right):

https://codereview.chromium.org/464463003/diff/60001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.h:37: 
On 2014/08/12 15:22:40, Alexei Svitkine wrote:
> Nit: Remove this blank line.

Done.

[Alexei Svitkine (slow), 2014-08-12 19:17:14]

Looks good to me, let's make sure this makes sense to Antony from the extensions
side.

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:34: // Possible states for
extensions. The order of these enum values is importante,
Nit: important

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:97: ExtensionState
CheckForOffStore(const extensions::ExtensionSet& extensions,
Nit: Add a comment.

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:101: it !=
extensions.end();
Nit: Can add && state <= OFF_STORE, so that we can do an early return as soon as
max level reached. Same in the other loop over profiles below.

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:191:
extensions::InstallVerifier* verifier =
Does all this code build on iOS and Android or do we need to sprinkle more  #if
defined(ENABLE_EXTENSIONS) statements around? (Or probably better yet, just not
compile this code on those platforms via gyp).

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:203:
system_profile->set_offstore_extensions_state(
Probably we shouldn't bother setting this on platforms where there are no
extensions (see my comment above).

https://codereview.chromium.org/464463003/diff/80001/components/metrics/proto...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/464463003/diff/80001/components/metrics/proto...
components/metrics/proto/system_profile.proto:514: // extensions not from the
store. If there is a single off store extension,
Nit: "off store" -> "off-store" to make it easier to understand.

[jwd, 2014-08-12 21:16:48]

The CQ bit was checked by jwd@chromium.org

[jwd, 2014-08-12 21:16:50]

The CQ bit was unchecked by jwd@chromium.org

[jwd, 2014-08-12 21:24:17]

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:34: // Possible states for
extensions. The order of these enum values is importante,
On 2014/08/12 19:17:13, Alexei Svitkine wrote:
> Nit: important

Done.

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:97: ExtensionState
CheckForOffStore(const extensions::ExtensionSet& extensions,
On 2014/08/12 19:17:13, Alexei Svitkine wrote:
> Nit: Add a comment.

Done.

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:101: it !=
extensions.end();
On 2014/08/12 19:17:13, Alexei Svitkine wrote:
> Nit: Can add && state <= OFF_STORE, so that we can do an early return as soon
as
> max level reached. Same in the other loop over profiles below.

Done.

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:191:
extensions::InstallVerifier* verifier =
On 2014/08/12 19:17:13, Alexei Svitkine wrote:
> Does all this code build on iOS and Android or do we need to sprinkle more 
#if
> defined(ENABLE_EXTENSIONS) statements around? (Or probably better yet, just
not
> compile this code on those platforms via gyp).

Done.

https://codereview.chromium.org/464463003/diff/80001/chrome/browser/metrics/e...
chrome/browser/metrics/extensions_metrics_provider.cc:203:
system_profile->set_offstore_extensions_state(
On 2014/08/12 19:17:13, Alexei Svitkine wrote:
> Probably we shouldn't bother setting this on platforms where there are no
> extensions (see my comment above).

Done.

https://codereview.chromium.org/464463003/diff/80001/components/metrics/proto...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/464463003/diff/80001/components/metrics/proto...
components/metrics/proto/system_profile.proto:514: // extensions not from the
store. If there is a single off store extension,
On 2014/08/12 19:17:13, Alexei Svitkine wrote:
> Nit: "off store" -> "off-store" to make it easier to understand.

Done.

[jwd, 2014-08-13 14:40:35]

+rdevlin.cronin for extensions review because it looks like Antony might be OOO.

[Devlin, 2014-08-13 18:18:30]

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/extensio...
File chrome/browser/extensions/install_verifier.h (right):

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/extensio...
chrome/browser/extensions/install_verifier.h:63: bool IsVerified(const
std::string& id) const;
Why does this need to be public?

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/extensio...
chrome/browser/extensions/install_verifier.h:87: static bool FromStore(const
Extension& extension);
put static methods near the top of the class.

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:87: // to use the install
verifier to vereify this.
typo "vereify".

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:104: it !=
extensions.end() && state <= OFF_STORE;
nit: why not say state < OFF_STORE?  If we reach OFF_STORE, may as well return
(ain't getting any bigger).

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:192: for (size_t i = 0; i
< profiles.size() && state >= OFF_STORE; ++i) {
Unless I'm missing something, this loop will never run, because state will
always be < OFF_STORE at the start.

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:192: for (size_t i = 0; i
< profiles.size() && state >= OFF_STORE; ++i) {
nit: size_t i = 0u;

https://codereview.chromium.org/464463003/diff/150001/components/metrics/prot...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/464463003/diff/150001/components/metrics/prot...
components/metrics/proto/system_profile.proto:516: enum ExtensionsState {
Make a note to keep this in sync with the other enum?

[jwd, 2014-08-13 19:34:37]

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/extensio...
File chrome/browser/extensions/install_verifier.h (right):

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/extensio...
chrome/browser/extensions/install_verifier.h:63: bool IsVerified(const
std::string& id) const;
On 2014/08/13 18:18:30, Devlin wrote:
> Why does this need to be public?

Done.

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/extensio...
chrome/browser/extensions/install_verifier.h:87: static bool FromStore(const
Extension& extension);
On 2014/08/13 18:18:30, Devlin wrote:
> put static methods near the top of the class.

Done.

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:87: // to use the install
verifier to vereify this.
On 2014/08/13 18:18:30, Devlin wrote:
> typo "vereify".

Done.

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:104: it !=
extensions.end() && state <= OFF_STORE;
On 2014/08/13 18:18:30, Devlin wrote:
> nit: why not say state < OFF_STORE?  If we reach OFF_STORE, may as well return
> (ain't getting any bigger).

Done.

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:192: for (size_t i = 0; i
< profiles.size() && state >= OFF_STORE; ++i) {
On 2014/08/13 18:18:30, Devlin wrote:
> Unless I'm missing something, this loop will never run, because state will
> always be < OFF_STORE at the start.

Done.

https://codereview.chromium.org/464463003/diff/150001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:192: for (size_t i = 0; i
< profiles.size() && state >= OFF_STORE; ++i) {
On 2014/08/13 18:18:30, Devlin wrote:
> nit: size_t i = 0u;

Done.

https://codereview.chromium.org/464463003/diff/150001/components/metrics/prot...
File components/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/464463003/diff/150001/components/metrics/prot...
components/metrics/proto/system_profile.proto:516: enum ExtensionsState {
On 2014/08/13 18:18:30, Devlin wrote:
> Make a note to keep this in sync with the other enum?

Done.

[Alexei Svitkine (slow), 2014-08-13 19:38:10]

LGTM, can you send an internal CL to make the same change to the proto
server-side?

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.h (right):

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.h:36: virtual void
ProvideSystemProfileMetrics(
Nit: Keep the comment (without any blank line right above this):

// metrics::MetricsProvider:

https://codereview.chromium.org/464463003/diff/190001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/464463003/diff/190001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1528:
'browser/metrics/extensions_metrics_provider.h',
Why is this moved all the way here?

[Devlin, 2014-08-13 19:50:07]

sorry, couple more small things I didn't see before.

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/extensio...
File chrome/browser/extensions/install_verifier.h (right):

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/extensio...
chrome/browser/extensions/install_verifier.h:52: static bool FromStore(const
Extension& extension);
nit: Let's actually make this "IsFromStore".

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:77: if
(extension.location() == extensions::Manifest::COMPONENT ||
nit: Prefer extensions::Manifest::IsComponentLocation(extension.location())
(Just on the offchance we ever add another component location)

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:203: state =
std::max(state, CheckForOffStore(*extensions.get(), *verifier));
Do we not need to check |extensions| for NULL-ness here?

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.h (right):

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.h:42: // Retrieves the set of
extensions installed in the current profile.
nit: update comment to say "in the given |profile|"

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.h:43: // TODO(mvrable): If
metrics are ever converted to being per-profile, then
Can we take out the TODO?  Or update it?  (We *are* now returning extensions
installed in a specified profile...)

[jwd, 2014-08-13 20:22:07]

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/extensio...
File chrome/browser/extensions/install_verifier.h (right):

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/extensio...
chrome/browser/extensions/install_verifier.h:52: static bool FromStore(const
Extension& extension);
On 2014/08/13 19:50:07, Devlin wrote:
> nit: Let's actually make this "IsFromStore".

Done.

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:77: if
(extension.location() == extensions::Manifest::COMPONENT ||
On 2014/08/13 19:50:07, Devlin wrote:
> nit: Prefer extensions::Manifest::IsComponentLocation(extension.location())
> (Just on the offchance we ever add another component location)

Oh, nice, hadn't noticed that.

Done.

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:203: state =
std::max(state, CheckForOffStore(*extensions.get(), *verifier));
On 2014/08/13 19:50:07, Devlin wrote:
> Do we not need to check |extensions| for NULL-ness here?

Done.

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.h (right):

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.h:36: virtual void
ProvideSystemProfileMetrics(
On 2014/08/13 19:38:10, Alexei Svitkine wrote:
> Nit: Keep the comment (without any blank line right above this):
> 
> // metrics::MetricsProvider:

Done.

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.h:42: // Retrieves the set of
extensions installed in the current profile.
On 2014/08/13 19:50:07, Devlin wrote:
> nit: update comment to say "in the given |profile|"

Done.

https://codereview.chromium.org/464463003/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.h:43: // TODO(mvrable): If
metrics are ever converted to being per-profile, then
On 2014/08/13 19:50:07, Devlin wrote:
> Can we take out the TODO?  Or update it?  (We *are* now returning extensions
> installed in a specified profile...)

Done.

https://codereview.chromium.org/464463003/diff/190001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/464463003/diff/190001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1528:
'browser/metrics/extensions_metrics_provider.h',
On 2014/08/13 19:38:10, Alexei Svitkine wrote:
> Why is this moved all the way here?

I moved them here so that they only get compiled when extensions are enabled.

Alternate suggestions are welcome :)

[Devlin, 2014-08-13 20:25:46]

lgtm

[Alexei Svitkine (slow), 2014-08-13 20:30:30]

still LGTM with a couple more comments

https://codereview.chromium.org/464463003/diff/190001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/464463003/diff/190001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1528:
'browser/metrics/extensions_metrics_provider.h',
On 2014/08/13 20:22:07, Jesse Doherty wrote:
> On 2014/08/13 19:38:10, Alexei Svitkine wrote:
> > Why is this moved all the way here?
> 
> I moved them here so that they only get compiled when extensions are enabled.
> 
> Alternate suggestions are welcome :)

Ah, got it - I didn't realise it was in a different section. Makes sense.

https://codereview.chromium.org/464463003/diff/210001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/464463003/diff/210001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:284: #if
defined(ENABLE_EXTENSIONS)
Can you ifdef the header inclusion too?

https://codereview.chromium.org/464463003/diff/210001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/210001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:159: #if
defined(ENABLE_EXTENSIONS)
This ifdef is no longer necessary, right?

[Alexei Svitkine (slow), 2014-08-13 20:35:21]

(And also please hold off on submitting this until the server-side proto CL
is landed.)


On Wed, Aug 13, 2014 at 4:30 PM, <asvitkine@chromium.org> wrote:

> still LGTM with a couple more comments
>
>
>
> https://codereview.chromium.org/464463003/diff/190001/
> chrome/chrome_browser.gypi
> File chrome/chrome_browser.gypi (right):
>
> https://codereview.chromium.org/464463003/diff/190001/
> chrome/chrome_browser.gypi#newcode1528
> chrome/chrome_browser.gypi:1528:
> 'browser/metrics/extensions_metrics_provider.h',
> On 2014/08/13 20:22:07, Jesse Doherty wrote:
>
>> On 2014/08/13 19:38:10, Alexei Svitkine wrote:
>> > Why is this moved all the way here?
>>
>
>  I moved them here so that they only get compiled when extensions are
>>
> enabled.
>
>  Alternate suggestions are welcome :)
>>
>
> Ah, got it - I didn't realise it was in a different section. Makes
> sense.
>
> https://codereview.chromium.org/464463003/diff/210001/
> chrome/browser/metrics/chrome_metrics_service_client.cc
> File chrome/browser/metrics/chrome_metrics_service_client.cc (right):
>
> https://codereview.chromium.org/464463003/diff/210001/
> chrome/browser/metrics/chrome_metrics_service_client.cc#newcode284
> chrome/browser/metrics/chrome_metrics_service_client.cc:284: #if
> defined(ENABLE_EXTENSIONS)
> Can you ifdef the header inclusion too?
>
> https://codereview.chromium.org/464463003/diff/210001/
> chrome/browser/metrics/extensions_metrics_provider.cc
> File chrome/browser/metrics/extensions_metrics_provider.cc (right):
>
> https://codereview.chromium.org/464463003/diff/210001/
> chrome/browser/metrics/extensions_metrics_provider.cc#newcode159
> chrome/browser/metrics/extensions_metrics_provider.cc:159: #if
> defined(ENABLE_EXTENSIONS)
> This ifdef is no longer necessary, right?
>
> https://codereview.chromium.org/464463003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jwd, 2014-08-13 20:36:32]

https://codereview.chromium.org/464463003/diff/210001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/464463003/diff/210001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:284: #if
defined(ENABLE_EXTENSIONS)
On 2014/08/13 20:30:30, Alexei Svitkine wrote:
> Can you ifdef the header inclusion too?

Done.

https://codereview.chromium.org/464463003/diff/210001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/210001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:159: #if
defined(ENABLE_EXTENSIONS)
On 2014/08/13 20:30:30, Alexei Svitkine wrote:
> This ifdef is no longer necessary, right?

Done.

[Devlin, 2014-08-13 20:39:48]

https://codereview.chromium.org/464463003/diff/230001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/230001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:196: if (!extensions)
Since we're taking out ifdefs, we actually don't need this check (or the one on
line 217) anymore.  Sorry :/

[Devlin, 2014-08-13 20:40:05]

(still lgtm)

[jwd, 2014-08-14 15:22:54]

https://codereview.chromium.org/464463003/diff/230001/chrome/browser/metrics/...
File chrome/browser/metrics/extensions_metrics_provider.cc (right):

https://codereview.chromium.org/464463003/diff/230001/chrome/browser/metrics/...
chrome/browser/metrics/extensions_metrics_provider.cc:196: if (!extensions)
On 2014/08/13 20:39:47, Devlin wrote:
> Since we're taking out ifdefs, we actually don't need this check (or the one
on
> line 217) anymore.  Sorry :/

Done.

[jwd, 2014-08-14 15:22:59]

The CQ bit was checked by jwd@chromium.org

[commit-bot: I haz the power, 2014-08-14 15:24:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jwd@chromium.org/464463003/250001

[commit-bot: I haz the power, 2014-08-14 19:39:44]

Committed patchset #14 (250001) as 289673