Chromecast: adds stability metrics provider to track crashes.

Chromecast: adds stability metrics provider to track crashes.

R=lcwu@chromium.org,asvitkine@chromium.org
BUG=400925
Committed: https://crrev.com/8580f192f3e94f06a1f1e577b8b734527427d942
Cr-Commit-Position: refs/heads/master@{#297912}

[lcwu1, 2014-09-26 22:58:03]

lgtm

[gunsch, 2014-09-26 23:29:38]

gunsch@chromium.org changed reviewers:
+ jochen@chromium.org

[gunsch, 2014-09-26 23:31:28]

On 2014/09/26 22:58:03, lcwu1 wrote:
> lgtm

+jochen for content/public/browser (new DEPS entry).

CastStabilityMetricsProvider is both a content::BrowserChildProcessObserver and
a content::NotificationObserver for the Chromecast content embedder. Added a
DEPS entry to content/public/browser from chromecast/metrics.

[Alexei Svitkine (slow), 2014-09-29 12:41:48]

I would prefer if we find some way to share code, rather duplicating a subset of
the Chrome class.

Perhaps we can add a helper utility class that both providers could use that
encompasses this logic?

[jochen (gone - plz use gerrit), 2014-09-30 08:26:41]

hum, so chromecast/common/chromecast_config.cc includes chromecast/metrics

now you're moving something that depends on browser/ into metrics

that won't work. not lgtm

https://codereview.chromium.org/602393004/diff/20001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/602393004/diff/20001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:55: '../components/components.gyp:metrics_profiler',
if you use headers from content/browser, you should also depend on it

[gunsch, 2014-09-30 16:34:12]

jochen: good catch, the common->metrics dependency shouldn't be there. Broke
that.

Alexei: I agree; I'm looking at ChromeStabilityMetricsProvider more and the
common functionality might be tricky to extricate. I could build a common
StabilityMetricsProvider (in, say, components/metrics/content) that implementers
either subclass or implement a StabilityMetricsProviderClient interface to
actually log. In the client case, there would still be some duplicated code, but
it would be much less, at the cost of more complexity. Thoughts?

https://codereview.chromium.org/602393004/diff/20001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/602393004/diff/20001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:55: '../components/components.gyp:metrics_profiler',
On 2014/09/30 08:26:40, jochen (slow for reviews) wrote:
> if you use headers from content/browser, you should also depend on it

Done.

[jochen (gone - plz use gerrit), 2014-10-01 07:33:30]

lgtm with nit

consider adding a rule "-chromecast" and "+chromecast/common" to
chromecast/DEPS. This will force you to have each subdirectory to explicitly
specify which other directories it can include (besides chromecast/common)

https://codereview.chromium.org/602393004/diff/30001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_prefs.cc (right):

https://codereview.chromium.org/602393004/diff/30001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_prefs.cc:15:
metrics::CastStabilityMetricsProvider::RegisterPrefs(registry);
you're already in namespace metrics, no?

[gunsch, 2014-10-01 23:42:46]

jochen@: thanks for the DEPS suggestion, added that. That should help enforce
our code dependencies a little better.

Alexei: after some more thought about the stability metrics provider code and
talking with Le-Chun, I'm not actually so sure that trying to make any of that
code common is a good idea. We could de-dupe some of it, but I think most
solutions will end up with the end behavior for each embedder awkwardly split. I
like the metrics providers we moved/refactored for better sharing, but I don't
think this will work out nearly so cleanly. I'd like to go ahead and merge this
as-is for now unless you have other thoughts.

https://codereview.chromium.org/602393004/diff/30001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_prefs.cc (right):

https://codereview.chromium.org/602393004/diff/30001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_prefs.cc:15:
metrics::CastStabilityMetricsProvider::RegisterPrefs(registry);
On 2014/10/01 07:33:30, jochen wrote:
> you're already in namespace metrics, no?

Done.

[gunsch, 2014-10-02 19:03:55]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-10-02 19:04:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/602393004/50001

[commit-bot: I haz the power, 2014-10-02 19:20:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 19:20:04]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Alexei Svitkine (slow), 2014-10-02 19:56:32]

Sorry for being unresponsive, I had walls of meetings the last 3 days due to MTV
team members visiting our office.

From my experience, this sort of thing ends up biting us (Chromium) in the
future - where implementations diverge and you end up with several slightly
different classes that share common roots and its hard to manage. I can provide
examples if you'd like.

So I still strongly prefer that we share code. I think the best way to do this
is to have a BrowserChildProcessStabilityTracker class that encapsulates most of
this logic and then have both the Chromecast and Chrome stability provider have
it as a member.

If you prefer, I guess you can still land this as-is now and the refactoring can
be done later. If you don't do it, I can take a shot at it.

[gunsch, 2014-10-02 20:25:31]

On 2014/10/02 19:56:32, Alexei Svitkine wrote:
> Sorry for being unresponsive, I had walls of meetings the last 3 days due to
MTV
> team members visiting our office.
> 
> From my experience, this sort of thing ends up biting us (Chromium) in the
> future - where implementations diverge and you end up with several slightly
> different classes that share common roots and its hard to manage. I can
provide
> examples if you'd like.
> 
> So I still strongly prefer that we share code. I think the best way to do this
> is to have a BrowserChildProcessStabilityTracker class that encapsulates most
of
> this logic and then have both the Chromecast and Chrome stability provider
have
> it as a member.
> 
> If you prefer, I guess you can still land this as-is now and the refactoring
can
> be done later. If you don't do it, I can take a shot at it.

ChromeStabilityMetricsProvider also handles non-stability-related
content::NotificationObserver events (specifically, NOTIFICATION_LOAD_START). So
it seems to me that any such refactoring would result in having to choose one of
the following:

a) Chrome ends up with two NotificationObservers powering MetricsProvider
instances, one for stability events and one for page load

b) The BrowserChildProcessStabilityTracker class you suggest leaks its internals
to allow Chrome to re-use its NotificationObserver for NOTIFICATION_LOAD_START
events.

c) The BrowserChildProcessStabilityTracker class handles more than just
"child-process stability" and also listens for NOTIFICATION_LOAD_START events

It's not clear to me which of those is preferable.

I'd like to go ahead and check this in for now, but happy to continue a
conversation about what that refactoring looks like.

[gunsch, 2014-10-02 20:27:27]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-10-02 20:28:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/602393004/70001

[Alexei Svitkine (slow), 2014-10-02 20:28:58]

I think a) is fine, but as I said, I'm OK with you checking this now and
doing the refactoring later.

On Thu, Oct 2, 2014 at 4:25 PM, <gunsch@chromium.org> wrote:

> On 2014/10/02 19:56:32, Alexei Svitkine wrote:
>
>> Sorry for being unresponsive, I had walls of meetings the last 3 days due
>> to
>>
> MTV
>
>> team members visiting our office.
>>
>
>   From my experience, this sort of thing ends up biting us (Chromium) in
>> the
>> future - where implementations diverge and you end up with several
>> slightly
>> different classes that share common roots and its hard to manage. I can
>>
> provide
>
>> examples if you'd like.
>>
>
>  So I still strongly prefer that we share code. I think the best way to do
>> this
>> is to have a BrowserChildProcessStabilityTracker class that encapsulates
>> most
>>
> of
>
>> this logic and then have both the Chromecast and Chrome stability provider
>>
> have
>
>> it as a member.
>>
>
>  If you prefer, I guess you can still land this as-is now and the
>> refactoring
>>
> can
>
>> be done later. If you don't do it, I can take a shot at it.
>>
>
> ChromeStabilityMetricsProvider also handles non-stability-related
> content::NotificationObserver events (specifically,
> NOTIFICATION_LOAD_START). So
> it seems to me that any such refactoring would result in having to choose
> one of
> the following:
>
> a) Chrome ends up with two NotificationObservers powering MetricsProvider
> instances, one for stability events and one for page load
>
> b) The BrowserChildProcessStabilityTracker class you suggest leaks its
> internals
> to allow Chrome to re-use its NotificationObserver for
> NOTIFICATION_LOAD_START
> events.
>
> c) The BrowserChildProcessStabilityTracker class handles more than just
> "child-process stability" and also listens for NOTIFICATION_LOAD_START
> events
>
> It's not clear to me which of those is preferable.
>
> I'd like to go ahead and check this in for now, but happy to continue a
> conversation about what that refactoring looks like.
>
> https://codereview.chromium.org/602393004/
>

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

[gunsch, 2014-10-02 20:31:25]

Sounds good, thanks for taking a look.

[commit-bot: I haz the power, 2014-10-02 21:46:30]

Committed patchset #5 (id:70001) as e206c148f38c9e94e03b0f70cb0a1ac67b598207

[commit-bot: I haz the power, 2014-10-02 21:47:00]

Patchset 5 (id:??) landed as
https://crrev.com/8580f192f3e94f06a1f1e577b8b734527427d942
Cr-Commit-Position: refs/heads/master@{#297912}