use push messaging in cache invalidation xmpp channel

- replace custom <iq>-stanza protocol with push notifications (using PushNotifications* classes) in CacheInvalidationPacketHandler, and update unit test accordingly

- extend notifier::Notification and PushNotificationsSendUpdateTask to allow specification of recipients (and recipient-specific data)

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

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

[akalin, 2012-01-12 23:06:44]

Please add a summary of the changes you made to the CL description (add
recipient support, etc.)

Are you still having problems with unit tests and empty JIDs?  If so, you had a
(possibly hacky) solution, right?  Can you upload that?  It'll make it easier
for me to hunt down the right solution.

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc (right):

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:88: // Owned
by base_task_.
this comment should be right above push_subscription_task

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:116: <<
decoded_message;
just to make sure, decoded_message is base64-encoded, right?

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.h (left):

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
chrome/browser/sync/notifier/cache_invalidation_packet_handler.h:46: 
The Chrome style is:

// FooInterface implementation.
virtual void DoFoo();
virtual int DoOtherFoo();

// BarInterface implementation.
virtual blah DoBar();
virtual stuff DoOtherBar() const;

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
File chrome/browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc
(right):

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
chrome/browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc:38:
void InitNotification(
seems cleaner to just have InitNotification return a Notification object
directly (and rename it to MakeNotification)

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ch...
File chrome/browser/sync/notifier/chrome_system_resources.cc (right):

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ch...
chrome/browser/sync/notifier/chrome_system_resources.cc:238: is_started_ = true;
this is already in your other CL, right?

http://codereview.chromium.org/9190029/diff/1/jingle/notifier/listener/push_n...
File jingle/notifier/listener/push_notifications_send_update_task.cc (right):

http://codereview.chromium.org/9190029/diff/1/jingle/notifier/listener/push_n...
jingle/notifier/listener/push_notifications_send_update_task.cc:80:
push->AddElement(recipient_element);
i think this should be moved to right after the recipient_element is created (so
it's obvious what the ownership is)

[ghc, 2012-01-13 01:23:07]

Yes, still having problems with the tests.  Easiest hack is to disable the
DCHECK on line 82 of push_notifications_subscribe_task.cc.  That's probably not
worth uploading ...

The alternative looks complicated, since there seems to be a real XmppClient
buried a few levels down inside the FakeBaseTask that's used in a lot of unit
tests, and that's apparently where the jid comes from.  I assume I could make it
work through sufficient mocking/subclassing/addition of test methods.  I'd
rather not invest too much in that if there's an easier way, but I can if
necessary.

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc (right):

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:88: // Owned
by base_task_.
On 2012/01/12 23:06:44, akalin wrote:
> this comment should be right above push_subscription_task

Done.

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:116: <<
decoded_message;
On 2012/01/12 23:06:44, akalin wrote:
> just to make sure, decoded_message is base64-encoded, right?

No -- the data on the wire is base64-encoded, but the
PushNotificationsListenTask decodes it before setting the data field in the
Notification.

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.h (left):

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
chrome/browser/sync/notifier/cache_invalidation_packet_handler.h:46: 
On 2012/01/12 23:06:44, akalin wrote:
> The Chrome style is:
> 
> // FooInterface implementation.
> virtual void DoFoo();
> virtual int DoOtherFoo();
> 
> // BarInterface implementation.
> virtual blah DoBar();
> virtual stuff DoOtherBar() const;

Done.

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
File chrome/browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc
(right):

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ca...
chrome/browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc:38:
void InitNotification(
On 2012/01/12 23:06:44, akalin wrote:
> seems cleaner to just have InitNotification return a Notification object
> directly (and rename it to MakeNotification)

Done.

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ch...
File chrome/browser/sync/notifier/chrome_system_resources.cc (right):

http://codereview.chromium.org/9190029/diff/1/chrome/browser/sync/notifier/ch...
chrome/browser/sync/notifier/chrome_system_resources.cc:238: is_started_ = true;
On 2012/01/12 23:06:44, akalin wrote:
> this is already in your other CL, right?

Yes.  I could undo it, but this way it gets in sooner ...

http://codereview.chromium.org/9190029/diff/1/jingle/notifier/listener/push_n...
File jingle/notifier/listener/push_notifications_send_update_task.cc (right):

http://codereview.chromium.org/9190029/diff/1/jingle/notifier/listener/push_n...
jingle/notifier/listener/push_notifications_send_update_task.cc:80:
push->AddElement(recipient_element);
On 2012/01/12 23:06:44, akalin wrote:
> i think this should be moved to right after the recipient_element is created
(so
> it's obvious what the ownership is)

Done.

[akalin, 2012-01-24 01:59:46]

hey can you rebase and upload a new patch?  when you do that, i can download and
then find the simplest way to fix the tests

[ghc, 2012-01-24 18:13:19]

On 2012/01/24 01:59:46, akalin wrote:
> hey can you rebase and upload a new patch?  when you do that, i can download
and
> then find the simplest way to fix the tests

done.  two possibilities come to mind:

- put in a hack to allow skipping the problematic code in the unit tests

- make a MockWeakXmppClient with a mock Jid() method, and have FakeBaseTask use
this instead of the normal WeakXmppClient

[akalin, 2012-01-24 23:11:01]

patch still doesn't apply.  I get:

$ git cl patch 9190029
Loaded authentication cookies from
/usr/local/google/home/akalin/.codereview_upload_cookies
error: patch failed:
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:27
error: chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc: patch
does not apply
Failed to apply the patch

I'm on r118929.

On 2012/01/24 18:13:19, ghc wrote:
> On 2012/01/24 01:59:46, akalin wrote:
> > hey can you rebase and upload a new patch?  when you do that, i can download
> and
> > then find the simplest way to fix the tests
> 
> done.  two possibilities come to mind:
> 
> - put in a hack to allow skipping the problematic code in the unit tests
> 
> - make a MockWeakXmppClient with a mock Jid() method, and have FakeBaseTask
use
> this instead of the normal WeakXmppClient

[akalin, 2012-01-24 23:16:36]

It seems to not like the large delete in the anonymous namespace.  I'll just
manually patch that in for now.  I hope the CQ can handle it anyway.

On 2012/01/24 23:11:01, akalin wrote:
> patch still doesn't apply.  I get:
> 
> $ git cl patch 9190029
> Loaded authentication cookies from
> /usr/local/google/home/akalin/.codereview_upload_cookies
> error: patch failed:
> chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:27
> error: chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:
patch
> does not apply
> Failed to apply the patch
> 
> I'm on r118929.
> 
> On 2012/01/24 18:13:19, ghc wrote:
> > On 2012/01/24 01:59:46, akalin wrote:
> > > hey can you rebase and upload a new patch?  when you do that, i can
download
> > and
> > > then find the simplest way to fix the tests
> > 
> > done.  two possibilities come to mind:
> > 
> > - put in a hack to allow skipping the problematic code in the unit tests
> > 
> > - make a MockWeakXmppClient with a mock Jid() method, and have FakeBaseTask
> use
> > this instead of the normal WeakXmppClient

[ghc, 2012-01-25 02:25:00]

On 2012/01/24 23:16:36, akalin wrote:
> It seems to not like the large delete in the anonymous namespace.  I'll just
> manually patch that in for now.  I hope the CQ can handle it anyway.
> 
> On 2012/01/24 23:11:01, akalin wrote:
> > patch still doesn't apply.  I get:
> > 
> > $ git cl patch 9190029
> > Loaded authentication cookies from
> > /usr/local/google/home/akalin/.codereview_upload_cookies
> > error: patch failed:
> > chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:27
> > error: chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:
> patch
> > does not apply
> > Failed to apply the patch
> > 
> > I'm on r118929.
> > 
> > On 2012/01/24 18:13:19, ghc wrote:
> > > On 2012/01/24 01:59:46, akalin wrote:
> > > > hey can you rebase and upload a new patch?  when you do that, i can
> download
> > > and
> > > > then find the simplest way to fix the tests
> > > 
> > > done.  two possibilities come to mind:
> > > 
> > > - put in a hack to allow skipping the problematic code in the unit tests
> > > 
> > > - make a MockWeakXmppClient with a mock Jid() method, and have
FakeBaseTask
> > use
> > > this instead of the normal WeakXmppClient

Uploaded again.  Everything should work now.

[akalin, 2012-01-25 02:45:13]

LGTM, but please fix the nits below before I put it through the CQ.

Also, can you build Chrome with this change and make sure sync etc. still works?
 You can peek at the notifications tab in about:sync to know when notifications
come in.  Also, you can build the sync_listen_notifications tool to get a
command-line tool that'll print when notifications arrive (although you'll have
to set breakpoints in Chrome to get the right token to use -- you can get
instructions by running sync_listen_notifications w/o any params.)

http://codereview.chromium.org/9190029/diff/19002/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc (right):

http://codereview.chromium.org/9190029/diff/19002/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:20: #include
"jingle/notifier/listener/push_notifications_listen_task.h"
don't need this include since you do so in header

http://codereview.chromium.org/9190029/diff/19002/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:22: #include
"jingle/notifier/listener/push_notifications_subscribe_task.h"
this too

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/base/fake_b...
File jingle/notifier/base/fake_base_task.cc (right):

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/base/fake_b...
jingle/notifier/base/fake_base_task.cc:31: class FakeWeakXmppClient : public
notifier::WeakXmppClient {
put in anon namespace

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/base/fake_b...
jingle/notifier/base/fake_base_task.cc:31: class FakeWeakXmppClient : public
notifier::WeakXmppClient {
add comment describing class (i.e. extending it to provide a non-empty jid for
tests)

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/listener/pu...
File jingle/notifier/listener/push_notifications_send_update_task.cc (right):

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/listener/pu...
jingle/notifier/listener/push_notifications_send_update_task.cc:66: for (uint i
= 0; i < recipients.size(); ++i) {
use size_t (don't think uint is the standard typedef) or iterators

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/listener/pu...
File jingle/notifier/listener/push_notifications_subscribe_task.cc (right):

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/listener/pu...
jingle/notifier/listener/push_notifications_subscribe_task.cc:82:
DCHECK(jid.IsFull()) << "jid is: " << jid.Str();
do you still need this?

[ghc, 2012-01-25 03:18:28]

Tested manually on Linux (read logs to check that messages were flowing and that
the client and protocol were behaving as expected).

http://codereview.chromium.org/9190029/diff/19002/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc (right):

http://codereview.chromium.org/9190029/diff/19002/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:20: #include
"jingle/notifier/listener/push_notifications_listen_task.h"
On 2012/01/25 02:45:13, akalin wrote:
> don't need this include since you do so in header

Done.

http://codereview.chromium.org/9190029/diff/19002/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc:22: #include
"jingle/notifier/listener/push_notifications_subscribe_task.h"
On 2012/01/25 02:45:13, akalin wrote:
> this too

Done.

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/base/fake_b...
File jingle/notifier/base/fake_base_task.cc (right):

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/base/fake_b...
jingle/notifier/base/fake_base_task.cc:31: class FakeWeakXmppClient : public
notifier::WeakXmppClient {
On 2012/01/25 02:45:13, akalin wrote:
> put in anon namespace

Done.

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/base/fake_b...
jingle/notifier/base/fake_base_task.cc:31: class FakeWeakXmppClient : public
notifier::WeakXmppClient {
On 2012/01/25 02:45:13, akalin wrote:
> add comment describing class (i.e. extending it to provide a non-empty jid for
> tests)

Done.

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/listener/pu...
File jingle/notifier/listener/push_notifications_send_update_task.cc (right):

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/listener/pu...
jingle/notifier/listener/push_notifications_send_update_task.cc:66: for (uint i
= 0; i < recipients.size(); ++i) {
On 2012/01/25 02:45:13, akalin wrote:
> use size_t (don't think uint is the standard typedef) or iterators

Done.

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/listener/pu...
File jingle/notifier/listener/push_notifications_subscribe_task.cc (right):

http://codereview.chromium.org/9190029/diff/19002/jingle/notifier/listener/pu...
jingle/notifier/listener/push_notifications_subscribe_task.cc:82:
DCHECK(jid.IsFull()) << "jid is: " << jid.Str();
On 2012/01/25 02:45:13, akalin wrote:
> do you still need this?

reverted

[commit-bot: I haz the power, 2012-01-25 03:29:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ghc@google.com/9190029/16018

[commit-bot: I haz the power, 2012-01-25 04:02:16]

Try job failure for 9190029-16018 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[akalin, 2012-01-25 04:34:35]

On 2012/01/25 04:02:16, I haz the power (commit-bot) wrote:
> Try job failure for 9190029-16018 (retry) on linux_clang for step "compile"
> (clobber build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

looks like you need to define constructors/destructors for the new struct in
notification_defines and put them in a new .cc file. (For Recipient and
Notification)

[ghc, 2012-01-25 19:14:18]

On 2012/01/25 04:34:35, akalin wrote:
> On 2012/01/25 04:02:16, I haz the power (commit-bot) wrote:
> > Try job failure for 9190029-16018 (retry) on linux_clang for step "compile"
> > (clobber build).
> > It's a second try, previously, step "compile" failed.
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
> 
> looks like you need to define constructors/destructors for the new struct in
> notification_defines and put them in a new .cc file. (For Recipient and
> Notification)

I only see an error for Notification (which now contains a vector of
Recipients--I assume that's why it's considered "complex").  Added ctor and dtor
to notifications_defines.cc, which already existed with implementation of
Notification::ToString().  (I can also add ctors/dtors for the other types in
the .h if you want, but it seems they shouldn't be needed.)  Since these are
just structs, I assume it's ok to have a non-virtual dtor ...

[commit-bot: I haz the power, 2012-01-25 21:43:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ghc@google.com/9190029/21008

[akalin, 2012-01-25 21:43:22]

On 2012/01/25 19:14:18, ghc wrote:
> On 2012/01/25 04:34:35, akalin wrote:
> > On 2012/01/25 04:02:16, I haz the power (commit-bot) wrote:
> > > Try job failure for 9190029-16018 (retry) on linux_clang for step
"compile"
> > > (clobber build).
> > > It's a second try, previously, step "compile" failed.
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
> > 
> > looks like you need to define constructors/destructors for the new struct in
> > notification_defines and put them in a new .cc file. (For Recipient and
> > Notification)
> 
> I only see an error for Notification (which now contains a vector of
> Recipients--I assume that's why it's considered "complex").  Added ctor and
dtor
> to notifications_defines.cc, which already existed with implementation of
> Notification::ToString().  (I can also add ctors/dtors for the other types in
> the .h if you want, but it seems they shouldn't be needed.)  Since these are
> just structs, I assume it's ok to have a non-virtual dtor ...

ah, yes, should be okay.  trying again...

[commit-bot: I haz the power, 2012-01-25 23:50:50]

Try job failure for 9190029-21008 (retry) on mac_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-01-25 23:53:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ghc@google.com/9190029/21008

[commit-bot: I haz the power, 2012-01-26 01:53:19]

Change committed as 119171

[commit-bot: I haz the power, 2012-01-26 01:54:33]

Change committed as 119171

[akalin, 2012-01-26 03:51:14]

On 2012/01/26 01:54:33, I haz the power (commit-bot) wrote:
> Change committed as 119171

this got reverted because it somehow introduced a static initializer.  Hmm...

[akalin, 2012-01-26 03:54:25]

http://codereview.chromium.org/9190029/diff/21008/jingle/notifier/base/fake_b...
File jingle/notifier/base/fake_base_task.cc (right):

http://codereview.chromium.org/9190029/diff/21008/jingle/notifier/base/fake_b...
jingle/notifier/base/fake_base_task.cc:32: 
this should be in an anon namespace, not the global namespace, i.e.:

namespace notifier {
...
namespace {

class FakeWea... {
};

}

}  // namespace

[akalin, 2012-01-26 03:54:54]

also you may have inadvertently changed some svn props?

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

On 2012/01/26 03:54:25, akalin wrote:
>
http://codereview.chromium.org/9190029/diff/21008/jingle/notifier/base/fake_b...
> File jingle/notifier/base/fake_base_task.cc (right):
> 
>
http://codereview.chromium.org/9190029/diff/21008/jingle/notifier/base/fake_b...
> jingle/notifier/base/fake_base_task.cc:32: 
> this should be in an anon namespace, not the global namespace, i.e.:
> 
> namespace notifier {
> ...
> namespace {
> 
> class FakeWea... {
> };
> 
> }
> 
> }  // namespace

[akalin, 2012-01-26 03:55:30]

looks like it's the inclusion of the new client_gateway.pb.h file.  We can
recommit tomorrow and bump the expectations file.

On 2012/01/26 03:54:54, akalin wrote:
> also you may have inadvertently changed some svn props?
> 
> http://src.chromium.org/viewvc/chrome?view=rev&revision=119171
> 
> On 2012/01/26 03:54:25, akalin wrote:
> >
>
http://codereview.chromium.org/9190029/diff/21008/jingle/notifier/base/fake_b...
> > File jingle/notifier/base/fake_base_task.cc (right):
> > 
> >
>
http://codereview.chromium.org/9190029/diff/21008/jingle/notifier/base/fake_b...
> > jingle/notifier/base/fake_base_task.cc:32: 
> > this should be in an anon namespace, not the global namespace, i.e.:
> > 
> > namespace notifier {
> > ...
> > namespace {
> > 
> > class FakeWea... {
> > };
> > 
> > }
> > 
> > }  // namespace

[akalin, 2012-01-26 04:10:37]

procedure for updating expectations:
http://src.chromium.org/viewvc/chrome?view=rev&revision=119171 .  I can do this
after we reland.

Can you fix the couple of nits I found above and then reupload to this CL? 
('git cl issue 9190029' before re-uploading, not sure if there's a way to do it
with gcl)

On 2012/01/26 03:55:30, akalin wrote:
> looks like it's the inclusion of the new client_gateway.pb.h file.  We can
> recommit tomorrow and bump the expectations file.
> 
> On 2012/01/26 03:54:54, akalin wrote:
> > also you may have inadvertently changed some svn props?
> > 
> > http://src.chromium.org/viewvc/chrome?view=rev&revision=119171
> > 
> > On 2012/01/26 03:54:25, akalin wrote:
> > >
> >
>
http://codereview.chromium.org/9190029/diff/21008/jingle/notifier/base/fake_b...
> > > File jingle/notifier/base/fake_base_task.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9190029/diff/21008/jingle/notifier/base/fake_b...
> > > jingle/notifier/base/fake_base_task.cc:32: 
> > > this should be in an anon namespace, not the global namespace, i.e.:
> > > 
> > > namespace notifier {
> > > ...
> > > namespace {
> > > 
> > > class FakeWea... {
> > > };
> > > 
> > > }
> > > 
> > > }  // namespace

[ghc, 2012-01-26 20:16:34]

On 2012/01/26 04:10:37, akalin wrote:
> procedure for updating expectations:
> http://src.chromium.org/viewvc/chrome?view=rev&revision=119171 .  I can do
this
> after we reland.
> 
> Can you fix the couple of nits I found above and then reupload to this CL? 
> ('git cl issue 9190029' before re-uploading, not sure if there's a way to do
it
> with gcl)
> 
> On 2012/01/26 03:55:30, akalin wrote:
> > looks like it's the inclusion of the new client_gateway.pb.h file.  We can
> > recommit tomorrow and bump the expectations file.
> > 
> > On 2012/01/26 03:54:54, akalin wrote:
> > > also you may have inadvertently changed some svn props?
> > > 
> > > http://src.chromium.org/viewvc/chrome?view=rev&revision=119171
> > > 
> > > On 2012/01/26 03:54:25, akalin wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/9190029/diff/21008/jingle/notifier/base/fake_b...
> > > > File jingle/notifier/base/fake_base_task.cc (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/9190029/diff/21008/jingle/notifier/base/fake_b...
> > > > jingle/notifier/base/fake_base_task.cc:32: 
> > > > this should be in an anon namespace, not the global namespace, i.e.:
> > > > 
> > > > namespace notifier {
> > > > ...
> > > > namespace {
> > > > 
> > > > class FakeWea... {
> > > > };
> > > > 
> > > > }
> > > > 
> > > > }  // namespace

Fixed the namespace issue in patch set 11.  (Sorry for the confusion about
that.)

Running "svn proplist" on any of the changed files returns nothing, so I don't
know which properties could have changed, or how.  (I'm not even committing out
of my client -- does "gcl upload" send property changes?)

So the use of the new proto caused a measurable performance regression?  That's
surprising -- in general, this change should make things more efficient, since
it eliminates all the acking from the old <iq> protocol.  Do I need to do
anything about this?

[ghc, 2012-01-26 20:16:43]

https://chromiumcodereview.appspot.com/9190029/diff/21008/jingle/notifier/bas...
File jingle/notifier/base/fake_base_task.cc (right):

https://chromiumcodereview.appspot.com/9190029/diff/21008/jingle/notifier/bas...
jingle/notifier/base/fake_base_task.cc:32: 
On 2012/01/26 03:54:25, akalin wrote:
> this should be in an anon namespace, not the global namespace, i.e.:
> 
> namespace notifier {
> ...
> namespace {
> 
> class FakeWea... {
> };
> 
> }
> 
> }  // namespace

Done.

[akalin, 2012-01-26 20:24:57]

On 2012/01/26 20:16:34, ghc wrote:
> So the use of the new proto caused a measurable performance regression? 
That's
> surprising -- in general, this change should make things more efficient, since
> it eliminates all the acking from the old <iq> protocol.  Do I need to do
> anything about this?

Not exactly.  It introduces a static initializer, which is measured using the
perf bots.  Well, the static initializer was always there, but it was always
optimized out since it was not used, until this CL.

[commit-bot: I haz the power, 2012-01-26 20:25:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ghc@google.com/9190029/17022

[akalin, 2012-01-26 20:25:36]

On 2012/01/26 20:24:57, akalin wrote:
> On 2012/01/26 20:16:34, ghc wrote:
> > So the use of the new proto caused a measurable performance regression? 
> That's
> > surprising -- in general, this change should make things more efficient,
since
> > it eliminates all the acking from the old <iq> protocol.  Do I need to do
> > anything about this?
> 
> Not exactly.  It introduces a static initializer, which is measured using the
> perf bots.  Well, the static initializer was always there, but it was always
> optimized out since it was not used, until this CL.

Once this lands, I'll update the SI expectations.

[commit-bot: I haz the power, 2012-01-27 00:40:25]

Change committed as 119316

[akalin, 2012-01-27 01:20:43]

On 2012/01/27 00:40:25, I haz the power (commit-bot) wrote:
> Change committed as 119316

realized I pasted wrong link for perf expectation instructions.  here it is:
http://dev.chromium.org/developers/tree-sheriffs/perf-sheriffs