Introduce browser_sync::ExtensionsActivityMonitor to collect extensions API u...

Introduce browser_sync::ExtensionsActivityMonitor to collect extensions API usage
for correlation to sync commit requests.  Add ChromiumExtensionsActivity to sync.proto
to allow passing this data to sync servers.

BUG=25323
TEST=Added ExtensionsActivityMonitorTest. +Performing mutations on the bookmarks model via an extension should result in
ChromiumExtensionsActivity for each such extension showing up in CommitMessages.


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

[tim (not reviewing), 2009-10-22 20:47:29]

Erik, can you please check out /extensions/ and the new NotificationType?

Nick, please review everything.

I'm in the process of adding a simple unittest for extensions_activity_monitor,
at the same time as running some live tests with Idan.

[tim (not reviewing), 2009-10-22 20:52:47]

Oops, actually uploaded extensions_activity_monitor.h/cc now.

On 2009/10/22 20:47:29, timsteele wrote:
> Erik, can you please check out /extensions/ and the new NotificationType?
> 
> Nick, please review everything.
> 
> I'm in the process of adding a simple unittest for
extensions_activity_monitor,
> at the same time as running some live tests with Idan.

[ncarter (slow), 2009-10-22 23:21:15]

Overall, nice.

http://codereview.chromium.org/325001/diff/27/1031
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/325001/diff/27/1031#newcode80
Line 80: ui_loop->DeleteSoon(FROM_HERE, extensions_monitor_);
Is there any chance that |ui_loop| could be null here (eg. in a test)?

http://codereview.chromium.org/325001/diff/27/1031#newcode230
Line 230: }
Can we move this to ProcessCommitResponseCommand?  It's unusual for this kind of
work to happen in this function (maybe you're trying to avoid plumbing the
monitor through the syncprocessstate?)

http://codereview.chromium.org/325001/diff/27/1036
File chrome/browser/sync/util/extensions_activity_monitor.cc (right):

http://codereview.chromium.org/325001/diff/27/1036#newcode18
Line 18: // This precludes ExtensionsActivityMonitor from having to be ref
counted.
'precludes' is not the right word here; it connotes a blocking inhibition rather
than an enabling freedom.  Maybe 'liberates'?

http://codereview.chromium.org/325001/diff/27/1036#newcode97
Line 97: // time, perhaps we can find a better design.
I think what you're doing is fine.  I'd remove this TODO and just go with what
you have.

http://codereview.chromium.org/325001/diff/27/1037
File chrome/browser/sync/util/extensions_activity_monitor.h (right):

http://codereview.chromium.org/325001/diff/27/1037#newcode40
Line 40: Record() : bookmark_write_count(0U) {}
ctor should be first, right?

http://codereview.chromium.org/325001/diff/27/1037#newcode47
Line 47: typedef base::hash_map<std::string, Record> Records;
Because you iterate over the items in this data structure in several places,
hash_map may not be a good choice.  I'm also not sure of the memory efficiency
of copying hash maps all over the place.

Consider a exposing something like a double-buffered interface to the monitor's
consumer, where you have a UpdateSample() activity and then offer a way to
iterate over the most recently taken sample (and it would be allowed to return a
reference to the record snapshot, unlike how it works now).  Such a change will
limit you to one consumer, but I think that's okay.

http://codereview.chromium.org/325001/diff/27/1037#newcode53
Line 53: const Records& records() const;
It's only thread-safe if you return a copy, I think.

[tim (not reviewing), 2009-10-26 22:56:29]

Addressed your comments, and have a test in progress (having some build issues
atm)

http://codereview.chromium.org/325001/diff/27/1031
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/325001/diff/27/1031#newcode80
Line 80: ui_loop->DeleteSoon(FROM_HERE, extensions_monitor_);
On 2009/10/22 23:21:15, nick wrote:
> Is there any chance that |ui_loop| could be null here (eg. in a test)?

Done.

http://codereview.chromium.org/325001/diff/27/1031#newcode230
Line 230: }
On 2009/10/22 23:21:15, nick wrote:
> Can we move this to ProcessCommitResponseCommand?  It's unusual for this kind
of
> work to happen in this function (maybe you're trying to avoid plumbing the
> monitor through the syncprocessstate?)

Done.

http://codereview.chromium.org/325001/diff/27/1036
File chrome/browser/sync/util/extensions_activity_monitor.cc (right):

http://codereview.chromium.org/325001/diff/27/1036#newcode18
Line 18: // This precludes ExtensionsActivityMonitor from having to be ref
counted.
On 2009/10/22 23:21:15, nick wrote:
> 'precludes' is not the right word here; it connotes a blocking inhibition
rather
> than an enabling freedom.  Maybe 'liberates'?

Done.

http://codereview.chromium.org/325001/diff/27/1036#newcode97
Line 97: // time, perhaps we can find a better design.
On 2009/10/22 23:21:15, nick wrote:
> I think what you're doing is fine.  I'd remove this TODO and just go with what
> you have.

Done.

http://codereview.chromium.org/325001/diff/27/1037
File chrome/browser/sync/util/extensions_activity_monitor.h (right):

http://codereview.chromium.org/325001/diff/27/1037#newcode40
Line 40: Record() : bookmark_write_count(0U) {}
On 2009/10/22 23:21:15, nick wrote:
> ctor should be first, right?

Done.

http://codereview.chromium.org/325001/diff/27/1037#newcode47
Line 47: typedef base::hash_map<std::string, Record> Records;
On 2009/10/22 23:21:15, nick wrote:
> Because you iterate over the items in this data structure in several places,
> hash_map may not be a good choice.  I'm also not sure of the memory efficiency
> of copying hash maps all over the place.
> 
> Consider a exposing something like a double-buffered interface to the
monitor's
> consumer, where you have a UpdateSample() activity and then offer a way to
> iterate over the most recently taken sample (and it would be allowed to return
a
> reference to the record snapshot, unlike how it works now).  Such a change
will
> limit you to one consumer, but I think that's okay.

I had not expected to need all the iterating when I chose hash map, but I think
I agree with you now.  I switched to using a map.  I also changed the API a bit
to address the thread-safety and a quicker copy on GetRecords.

http://codereview.chromium.org/325001/diff/27/1037#newcode53
Line 53: const Records& records() const;
On 2009/10/22 23:21:15, nick wrote:
> It's only thread-safe if you return a copy, I think.

Uses swap now.

[tim (not reviewing), 2009-10-27 00:46:16]

All ready for a look now.

[ncarter (slow), 2009-10-27 01:25:58]

LGTM with the following

http://codereview.chromium.org/325001/diff/10022/9046
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/325001/diff/10022/9046#newcode66
Line 66: void ProcessCommitResponseCommand::DoModelChangingExecuteImpl(
Please give a less generic name.

http://codereview.chromium.org/325001/diff/10022/9039
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/325001/diff/10022/9039#newcode117
Line 117: // analysis purposes, so Law of Large Numbers FTW.
There's no actionable TODO here, since you wind up concluding that the current
behavior is good enough.

http://codereview.chromium.org/325001/diff/10022/9048
File chrome/browser/sync/util/extensions_activity_monitor_unittest.cc (right):

http://codereview.chromium.org/325001/diff/10022/9048#newcode163
Line 163: DCHECK_EQ(2, results.size());
This is a unit test; use EXPECT_ or ASSERT_, not DCHECK.

http://codereview.chromium.org/325001/diff/10022/9048#newcode167
Line 167: DCHECK_EQ(writes_by_extension2, results[id2].bookmark_write_count);
Could you add a test clause that verifies the clear behavior when there's more
than one GetAndClear() without a Put()?

[tim (not reviewing), 2009-10-27 01:38:15]

Thanks, trying to get the try server to co-operate now...

http://codereview.chromium.org/325001/diff/10022/9046
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/325001/diff/10022/9046#newcode66
Line 66: void ProcessCommitResponseCommand::DoModelChangingExecuteImpl(
On 2009/10/27 01:25:58, nick wrote:
> Please give a less generic name.

Done.

http://codereview.chromium.org/325001/diff/10022/9039
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/325001/diff/10022/9039#newcode117
Line 117: // analysis purposes, so Law of Large Numbers FTW.
On 2009/10/27 01:25:58, nick wrote:
> There's no actionable TODO here, since you wind up concluding that the current
> behavior is good enough.

Turned it into a comment.

http://codereview.chromium.org/325001/diff/10022/9048
File chrome/browser/sync/util/extensions_activity_monitor_unittest.cc (right):

http://codereview.chromium.org/325001/diff/10022/9048#newcode163
Line 163: DCHECK_EQ(2, results.size());
On 2009/10/27 01:25:58, nick wrote:
> This is a unit test; use EXPECT_ or ASSERT_, not DCHECK.

oops! done.

http://codereview.chromium.org/325001/diff/10022/9048#newcode167
Line 167: DCHECK_EQ(writes_by_extension2, results[id2].bookmark_write_count);
On 2009/10/27 01:25:58, nick wrote:
> Could you add a test clause that verifies the clear behavior when there's more
> than one GetAndClear() without a Put()?

Done.

[Erik does not do reviews, 2009-10-30 18:14:59]

Missed this while out on vacation.  The extension stuff LGTM.

Have you guys started getting data from this yet?

Erik


On Thu, Oct 22, 2009 at 1:47 PM,  <tim@chromium.org> wrote:
> Reviewers: nick, Erik Kay,
>
> Message:
> Erik, can you please check out /extensions/ and the new NotificationType?
>
> Nick, please review everything.
>
> I'm in the process of adding a simple unittest for
> extensions_activity_monitor,
> at the same time as running some live tests with Idan.
>
> Description:
> Introduce browser_sync::ExtensionsActivityMonitor to collect extensions A=
PI
> usage
> for correlation to sync commit requests. =A0Add ChromiumExtensionsActivit=
y to
> sync.proto
> to allow passing this data to sync servers.
>
> BUG=3D25323
> TEST=3DPerforming mutations on the bookmarks model via an extension shoul=
d
> result
> in
> ChromiumExtensionsActivity for each such extension showing up in
> CommitMessages.
>
>
> Please review this at http://codereview.chromium.org/325001
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 chrome/browser/extensions/extension_bookmarks_module.h
> =A0M =A0 =A0 chrome/browser/extensions/extension_bookmarks_module.cc
> =A0M =A0 =A0 chrome/browser/extensions/extension_function.h
> =A0M =A0 =A0 chrome/browser/sync/engine/build_commit_command.h
> =A0M =A0 =A0 chrome/browser/sync/engine/build_commit_command.cc
> =A0M =A0 =A0 chrome/browser/sync/engine/syncer.h
> =A0M =A0 =A0 chrome/browser/sync/engine/syncer.cc
> =A0M =A0 =A0 chrome/browser/sync/engine/syncer_session.h
> =A0M =A0 =A0 chrome/browser/sync/protocol/sync.proto
> =A0M =A0 =A0 chrome/chrome.gyp
> =A0M =A0 =A0 chrome/common/notification_type.h
>
>
>

[ncarter (slow), 2009-10-30 18:24:37]

Not yet.

 - nick

On Fri, Oct 30, 2009 at 11:14 AM, Erik Kay <erikkay@chromium.org> wrote:

> Missed this while out on vacation.  The extension stuff LGTM.
>
> Have you guys started getting data from this yet?
>
> Erik
>
>
> On Thu, Oct 22, 2009 at 1:47 PM,  <tim@chromium.org> wrote:
> > Reviewers: nick, Erik Kay,
> >
> > Message:
> > Erik, can you please check out /extensions/ and the new NotificationType?
> >
> > Nick, please review everything.
> >
> > I'm in the process of adding a simple unittest for
> > extensions_activity_monitor,
> > at the same time as running some live tests with Idan.
> >
> > Description:
> > Introduce browser_sync::ExtensionsActivityMonitor to collect extensions
> API
> > usage
> > for correlation to sync commit requests.  Add ChromiumExtensionsActivity
> to
> > sync.proto
> > to allow passing this data to sync servers.
> >
> > BUG=25323
> > TEST=Performing mutations on the bookmarks model via an extension should
> > result
> > in
> > ChromiumExtensionsActivity for each such extension showing up in
> > CommitMessages.
> >
> >
> > Please review this at http://codereview.chromium.org/325001
> >
> > SVN Base: svn://chrome-svn/chrome/trunk/src/
> >
> > Affected files:
> >  M     chrome/browser/extensions/extension_bookmarks_module.h
> >  M     chrome/browser/extensions/extension_bookmarks_module.cc
> >  M     chrome/browser/extensions/extension_function.h
> >  M     chrome/browser/sync/engine/build_commit_command.h
> >  M     chrome/browser/sync/engine/build_commit_command.cc
> >  M     chrome/browser/sync/engine/syncer.h
> >  M     chrome/browser/sync/engine/syncer.cc
> >  M     chrome/browser/sync/engine/syncer_session.h
> >  M     chrome/browser/sync/protocol/sync.proto
> >  M     chrome/chrome.gyp
> >  M     chrome/common/notification_type.h
> >
> >
> >
>