CPM Extension Event Watching Browsertests

CPM Extension Event Watching Browsertests

These are the browsertests for the extension event watching portion of the
PerformanceMonitor.

BUG=130212
TEST=Included browsertests

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

[Aaron Boodman, 2012-06-21 19:01:29]

Mark, do you think you can review this? Or too much missing context?

[Yoyo Zhou, 2012-06-27 19:59:24]

I'm reviewing this one first since the other CLs appear to build upon it.

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:90:
ASSERT_TRUE(events[i]->data()->GetAsDictionary(&value));
data() is already a DictionaryValue, so you don't need to GetAsDictionary:

ValidateExtensionInfo(extension_infos[i], events[i]->data());

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:103: //
PerformanceMonitor comes from extensions, we extend ExtensionBrowserTest.
nit: This is exactly what ExtensionBrowserTest is for; you don't need the
comment.

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:121: //
the background thread. Since we are testing, and can mock synchronicity
nit: s/and/we/

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:142:
PerformanceMonitor* performance_monitor_;
This is leaked. Use a scoped_ptr.

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:197: //
There will be an additional field on the Disable event: Unload Reason.
the Unload event

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:200:
ASSERT_TRUE(events[1]->data()->GetAsDictionary(&value));
ditto

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:230:
std::vector<ExtensionBasicInfo> extension_infos;
Move this down to where the rest of extension_infos is built up.

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:243:
content::NotificationRegistrar registrar;
You probably want something like WindowedNotificationObserver to avoid possible
races.

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:275:
ASSERT_TRUE(events[2]->data()->GetAsDictionary(&value));
ditto

http://codereview.chromium.org/10568014/diff/17001/chrome/test/data/performan...
File
chrome/test/data/performance_monitor/extensions/simple_extension_v1/manifest.json
(right):

http://codereview.chromium.org/10568014/diff/17001/chrome/test/data/performan...
chrome/test/data/performance_monitor/extensions/simple_extension_v1/manifest.json:1:
{
nit: in both manifests,
"manifest_version": 2

[Devlin, 2012-06-27 21:04:06]

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:90:
ASSERT_TRUE(events[i]->data()->GetAsDictionary(&value));
On 2012/06/27 19:59:25, Yoyo Zhou wrote:
> data() is already a DictionaryValue, so you don't need to GetAsDictionary:
> 
> ValidateExtensionInfo(extension_infos[i], events[i]->data());

Whoops, forgot we changed this...
Done.

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:103: //
PerformanceMonitor comes from extensions, we extend ExtensionBrowserTest.
On 2012/06/27 19:59:25, Yoyo Zhou wrote:
> nit: This is exactly what ExtensionBrowserTest is for; you don't need the
> comment.

Done.

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:121: //
the background thread. Since we are testing, and can mock synchronicity
On 2012/06/27 19:59:25, Yoyo Zhou wrote:
> nit: s/and/we/

Done.

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:142:
PerformanceMonitor* performance_monitor_;
On 2012/06/27 19:59:25, Yoyo Zhou wrote:
> This is leaked. Use a scoped_ptr.

I could be wrong, but my understanding is as follows:
 - PerformanceMonitor inherits from base::Singleton
 - Upon GetInstance(), the Singleton will register with the AtExitManager, and
will be destroyed on shutdown (AtExitManager::ProcessCallbacksNow()).
Is this wrong?

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:197: //
There will be an additional field on the Disable event: Unload Reason.
On 2012/06/27 19:59:25, Yoyo Zhou wrote:
> the Unload event

Done.

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:200:
ASSERT_TRUE(events[1]->data()->GetAsDictionary(&value));
On 2012/06/27 19:59:25, Yoyo Zhou wrote:
> ditto

Done.

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:230:
std::vector<ExtensionBasicInfo> extension_infos;
On 2012/06/27 19:59:25, Yoyo Zhou wrote:
> Move this down to where the rest of extension_infos is built up.

Can't :(  InstallExtension returns an Extension*; once we update the extension,
the ptr becomes invalid, so we need to insert the extension prior to updating
the extension (this is one of the reasons I had to make the ExtensionInfo
struct).

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:243:
content::NotificationRegistrar registrar;
On 2012/06/27 19:59:25, Yoyo Zhou wrote:
> You probably want something like WindowedNotificationObserver to avoid
possible
> races.

Ooooh, fancy. Didn't know about that. Thanks :)

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:275:
ASSERT_TRUE(events[2]->data()->GetAsDictionary(&value));
On 2012/06/27 19:59:25, Yoyo Zhou wrote:
> ditto

Done.

[Yoyo Zhou, 2012-06-27 21:55:51]

LGTM

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

http://codereview.chromium.org/10568014/diff/17001/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:142:
PerformanceMonitor* performance_monitor_;
On 2012/06/27 21:04:06, D Cronin wrote:
> On 2012/06/27 19:59:25, Yoyo Zhou wrote:
> > This is leaked. Use a scoped_ptr.
> 
> I could be wrong, but my understanding is as follows:
>  - PerformanceMonitor inherits from base::Singleton
>  - Upon GetInstance(), the Singleton will register with the AtExitManager, and
> will be destroyed on shutdown (AtExitManager::ProcessCallbacksNow()).
> Is this wrong?

Whoops, I wasn't reading carefully enough. It should be fine.

http://codereview.chromium.org/10568014/diff/24008/chrome/browser/performance...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

http://codereview.chromium.org/10568014/diff/24008/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:194: int
unload_reason = 0;
nit: Maybe -1 is better as a known invalid value (doesn't really matter since
you assert in the next line).

http://codereview.chromium.org/10568014/diff/24008/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:232: //
Observe implementation in ExtensionBrowserTest; this will exit the loop
nit: no longer explicitly running a message loop here

[Devlin, 2012-06-27 22:25:35]

http://codereview.chromium.org/10568014/diff/24008/chrome/browser/performance...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

http://codereview.chromium.org/10568014/diff/24008/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:194: int
unload_reason = 0;
On 2012/06/27 21:55:51, Yoyo Zhou wrote:
> nit: Maybe -1 is better as a known invalid value (doesn't really matter since
> you assert in the next line).

Done.

http://codereview.chromium.org/10568014/diff/24008/chrome/browser/performance...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:232: //
Observe implementation in ExtensionBrowserTest; this will exit the loop
On 2012/06/27 21:55:51, Yoyo Zhou wrote:
> nit: no longer explicitly running a message loop here

Done.

[commit-bot: I haz the power, 2012-06-27 22:25:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10568014/2...

[commit-bot: I haz the power, 2012-06-28 00:10:39]

Change committed as 144619