Chromecast fix: MetricsStateManager segfaults if callbacks are empty.

Chromecast fix: MetricsStateManager segfaults if callbacks are empty.

R=lcwu@chromium.org,asvitkine@chromium.org
BUG=None

Committed: https://crrev.com/79597a807f9225fa87a7b1810b11444cf2622fde
Cr-Commit-Position: refs/heads/master@{#292691}

[Alexei Svitkine (slow), 2014-08-29 17:16:10]

Could you add a unit test that creates the client that would catch this? (i.e.
would crash without this fix)

[gunsch, 2014-08-29 17:44:43]

On 2014/08/29 17:16:10, Alexei Svitkine wrote:
> Could you add a unit test that creates the client that would catch this? (i.e.
> would crash without this fix)

Sure.

We have an end-to-end browser test locally (inspired by the ContentShell-based
BrowserTests) that would have caught this, but that's outside the scope of this
CL to port. Hopefully can bring that over soon to verify shell behavior as we
continue porting.

[Alexei Svitkine (slow), 2014-08-29 17:58:30]

Thanks, LGTM!

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_unittest.cc (right):

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_unittest.cc:8: #include
"chromecast/metrics/cast_metrics_service_client.h"
Nit: Chromium convention is now for this to be the first include followed by an
empty line.

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_unittest.cc:24: 
Nit: Not sure you need to override this.

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_unittest.cc:25: scoped_ptr<base::MessageLoop>
message_loop_;
Nit: Put a private: section above these and add DISALLOW_COPY_AND_ASSIGN().

[gunsch, 2014-08-29 18:26:05]

Thanks for the comments!

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_unittest.cc (right):

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_unittest.cc:8: #include
"chromecast/metrics/cast_metrics_service_client.h"
On 2014/08/29 17:58:29, Alexei Svitkine wrote:
> Nit: Chromium convention is now for this to be the first include followed by
an
> empty line.

Done.

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_unittest.cc:24: 
On 2014/08/29 17:58:29, Alexei Svitkine wrote:
> Nit: Not sure you need to override this.

Done.

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_unittest.cc:25: scoped_ptr<base::MessageLoop>
message_loop_;
On 2014/08/29 17:58:29, Alexei Svitkine wrote:
> Nit: Put a private: section above these and add DISALLOW_COPY_AND_ASSIGN().

Done.

[gunsch, 2014-08-29 18:26:47]

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_unittest.cc (right):

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_unittest.cc:8: #include
"chromecast/metrics/cast_metrics_service_client.h"
On 2014/08/29 18:26:05, gunsch wrote:
> On 2014/08/29 17:58:29, Alexei Svitkine wrote:
> > Nit: Chromium convention is now for this to be the first include followed by
> an
> > empty line.
> 
> Done.

Actually, had to put this back. It seems for unit tests, the presubmit check
still enforces these are all alphabetical.

[gunsch, 2014-08-29 18:27:20]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-08-29 18:28:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gunsch@chromium.org/522853002/60001

[lcwu1, 2014-08-29 18:39:24]

https://codereview.chromium.org/522853002/diff/60001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_unittest.cc (right):

https://codereview.chromium.org/522853002/diff/60001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_unittest.cc:17: CastMetricsTest() {}
We should add a virtual destructor here.

[Alexei Svitkine (slow), 2014-08-29 18:43:15]

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_unittest.cc (right):

https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_unittest.cc:8: #include
"chromecast/metrics/cast_metrics_service_client.h"
On 2014/08/29 18:26:47, gunsch wrote:
> On 2014/08/29 18:26:05, gunsch wrote:
> > On 2014/08/29 17:58:29, Alexei Svitkine wrote:
> > > Nit: Chromium convention is now for this to be the first include followed
by
> > an
> > > empty line.
> > 
> > Done.
> 
> Actually, had to put this back. It seems for unit tests, the presubmit check
> still enforces these are all alphabetical.

Odd. Which presubmit?

It certainly doesn't for other parts of the codebase:

https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

Per the C++ style:

In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test the
stuff in dir2/foo2.h, order your includes as follows:

dir2/foo2.h (preferred location — see details below).
C system files.
C++ system files.
Other libraries' .h files.
Your project's .h files.

So if a presubmit is flagging those, we should file a bug against it. (Don't
block landing this change on it, just want to make sure the issue isn't
ignored.)

[gunsch, 2014-08-29 18:45:23]

On 2014/08/29 18:43:15, Alexei Svitkine wrote:
>
https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
> File chromecast/metrics/cast_metrics_unittest.cc (right):
> 
>
https://codereview.chromium.org/522853002/diff/40001/chromecast/metrics/cast_...
> chromecast/metrics/cast_metrics_unittest.cc:8: #include
> "chromecast/metrics/cast_metrics_service_client.h"
> On 2014/08/29 18:26:47, gunsch wrote:
> > On 2014/08/29 18:26:05, gunsch wrote:
> > > On 2014/08/29 17:58:29, Alexei Svitkine wrote:
> > > > Nit: Chromium convention is now for this to be the first include
followed
> by
> > > an
> > > > empty line.
> > > 
> > > Done.
> > 
> > Actually, had to put this back. It seems for unit tests, the presubmit check
> > still enforces these are all alphabetical.
> 
> Odd. Which presubmit?
> 
> It certainly doesn't for other parts of the codebase:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...
> 
> Per the C++ style:
> 
> In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test
the
> stuff in dir2/foo2.h, order your includes as follows:
> 
> dir2/foo2.h (preferred location — see details below).
> C system files.
> C++ system files.
> Other libraries' .h files.
> Your project's .h files.
> 
> So if a presubmit is flagging those, we should file a bug against it. (Don't
> block landing this change on it, just want to make sure the issue isn't
> ignored.)

It seems to be the filename specifically.

Since I'm testing |cast_metrics_service_client.h|, I can put it first if the
filename of the includer is |cast_metrics_service_client*.cc|.
|cast_metrics_unittest.cc| does not qualify.

[gunsch, 2014-08-29 18:45:35]

The CQ bit was unchecked by gunsch@chromium.org

[Alexei Svitkine (slow), 2014-08-29 18:49:46]

Any idea where that presubmit code is? (And why it differs from other paths
in the codebase.) Maybe do a codesearch on the warning string?


On Fri, Aug 29, 2014 at 11:45 AM, <gunsch@chromium.org> wrote:

> On 2014/08/29 18:43:15, Alexei Svitkine wrote:
>
> https://codereview.chromium.org/522853002/diff/40001/
> chromecast/metrics/cast_metrics_unittest.cc
>
>> File chromecast/metrics/cast_metrics_unittest.cc (right):
>>
>
>
> https://codereview.chromium.org/522853002/diff/40001/
> chromecast/metrics/cast_metrics_unittest.cc#newcode8
>
>> chromecast/metrics/cast_metrics_unittest.cc:8: #include
>> "chromecast/metrics/cast_metrics_service_client.h"
>> On 2014/08/29 18:26:47, gunsch wrote:
>> > On 2014/08/29 18:26:05, gunsch wrote:
>> > > On 2014/08/29 17:58:29, Alexei Svitkine wrote:
>> > > > Nit: Chromium convention is now for this to be the first include
>>
> followed
>
>> by
>> > > an
>> > > > empty line.
>> > >
>> > > Done.
>> >
>> > Actually, had to put this back. It seems for unit tests, the presubmit
>> check
>> > still enforces these are all alphabetical.
>>
>
>  Odd. Which presubmit?
>>
>
>  It certainly doesn't for other parts of the codebase:
>>
>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/components/metrics/metrics_service_unittest.cc&q=
> metrics_service_unittest.cc&sq=package:chromium&type=cs
>
>  Per the C++ style:
>>
>
>  In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or
>> test
>>
> the
>
>> stuff in dir2/foo2.h, order your includes as follows:
>>
>
>  dir2/foo2.h (preferred location -- see details below).
>> C system files.
>> C++ system files.
>> Other libraries' .h files.
>> Your project's .h files.
>>
>
>  So if a presubmit is flagging those, we should file a bug against it.
>> (Don't
>> block landing this change on it, just want to make sure the issue isn't
>> ignored.)
>>
>
> It seems to be the filename specifically.
>
> Since I'm testing |cast_metrics_service_client.h|, I can put it first if
> the
> filename of the includer is |cast_metrics_service_client*.cc|.
> |cast_metrics_unittest.cc| does not qualify.
>
> https://codereview.chromium.org/522853002/
>

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

[gunsch, 2014-08-29 18:52:01]

Presubmit is in

https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&q=pre...

The example you linked to is "metrics_service_unittest.cc", testing
"metrics_service.h". I renamed my unit test file to match the header, and the
presubmit test passed.

https://codereview.chromium.org/522853002/diff/60001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_unittest.cc (right):

https://codereview.chromium.org/522853002/diff/60001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_unittest.cc:17: CastMetricsTest() {}
On 2014/08/29 18:39:24, lcwu1 wrote:
> We should add a virtual destructor here.

Done.

[gunsch, 2014-08-29 18:52:05]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-08-29 18:53:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gunsch@chromium.org/522853002/80001

[lcwu1, 2014-08-29 18:54:23]

lgtm

[Alexei Svitkine (slow), 2014-08-29 18:57:49]

Ah got it, because the previous name wasn't matching the file name.

Rename LGTM, thanks!

[commit-bot: I haz the power, 2014-08-29 22:29:35]

Committed patchset #5 (id:80001) as 785cfd5656cfa229803d26c1bb85e7ec84156970

[commit-bot: I haz the power, 2014-09-10 03:10:49]

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