Adding support for pre-sent messages

Adding support for pre-sent messages

BUG=431988
Committed: https://crrev.com/1549c3f451217ee30fbe00cf0eb318fb0e54e05f
Cr-Commit-Position: refs/heads/master@{#310538}

[Charlie, 2015-01-07 18:13:17]

ckehoe@chromium.org changed reviewers:
+ rkc@chromium.org

[Charlie, 2015-01-07 18:13:17]

Updating Chrome to implement go/copresence-presend.

[rkc, 2015-01-07 21:31:43]

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
File components/copresence/copresence_manager_impl.cc (right):

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:64: base::Unretained(this));
Add a one liner about why unretained is safe here.

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:162: int erase_count =
queued_messages_by_token_.erase(token_id);
Is this the behavior we want? Currently for non-present messages, an app will
get the same message every time the machine hears a token. This will make it so
that the app will get the message just once, after which it won't get it on
subsequent times that the token is heard, which is completely different
behavior.

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
File components/copresence/copresence_manager_impl.h (right):

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.h:71: template<typename
IterableSubscribedMessages>
Since we use a std::vector only in our own map, can we instead use a
RepeatedPtrField there too and remove this templatization?

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.h:95: std::map<std::string,
std::vector<SubscribedMessage>>
Why not use TimedMap?

[Charlie, 2015-01-07 22:06:07]

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
File components/copresence/copresence_manager_impl.cc (right):

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:64: base::Unretained(this));
On 2015/01/07 21:31:43, Rahul Chaturvedi wrote:
> Add a one liner about why unretained is safe here.

Done.

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.cc:162: int erase_count =
queued_messages_by_token_.erase(token_id);
On 2015/01/07 21:31:43, Rahul Chaturvedi wrote:
> Is this the behavior we want? Currently for non-present messages, an app will
> get the same message every time the machine hears a token. This will make it
so
> that the app will get the message just once, after which it won't get it on
> subsequent times that the token is heard, which is completely different
> behavior.

The token will still be reported to the server and the message delivered
normally. Added a clarifying comment.

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
File components/copresence/copresence_manager_impl.h (right):

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.h:71: template<typename
IterableSubscribedMessages>
On 2015/01/07 21:31:43, Rahul Chaturvedi wrote:
> Since we use a std::vector only in our own map, can we instead use a
> RepeatedPtrField there too and remove this templatization?

Yes, but that's kind of hacky. This kind of templating is the C++ version of
Java's Iterable interface.

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.h:95: std::map<std::string,
std::vector<SubscribedMessage>>
On 2015/01/07 21:31:43, Rahul Chaturvedi wrote:
> Why not use TimedMap?

It doesn't have quite the same functionality. map::operator[] returns a mutable
reference, while TimedMap::GetValue returns a const one. And map::erase has no
equivalent.

But it's a good idea. Updated the TODO.

[Charlie, 2015-01-07 22:55:32]

Added a timeout per offline discussion.

[rkc, 2015-01-07 23:06:10]

lgtm % comments

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
File components/copresence/copresence_manager_impl.h (right):

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.h:71: template<typename
IterableSubscribedMessages>
On 2015/01/07 22:06:07, Charlie wrote:
> On 2015/01/07 21:31:43, Rahul Chaturvedi wrote:
> > Since we use a std::vector only in our own map, can we instead use a
> > RepeatedPtrField there too and remove this templatization?
> 
> Yes, but that's kind of hacky. This kind of templating is the C++ version of
> Java's Iterable interface.

Sure but we don't actually need this to be an iterable interface. Templates
introduce compile and run-time costs, which we shouldn't incur unless there is
at least some clear benefit to it.

https://codereview.chromium.org/813553002/diff/60001/components/copresence/ti...
File components/copresence/timed_map.h (right):

https://codereview.chromium.org/813553002/diff/60001/components/copresence/ti...
components/copresence/timed_map.h:57: return &map_[key];
If the element doesn't exist, this will actually add the key to the map. Use
find first to confirm that the key does exist; otherwise return nullptr.

https://codereview.chromium.org/813553002/diff/60001/components/copresence/ti...
components/copresence/timed_map.h:60: size_t Erase(const KeyType& key) {
Add a TODO to add a unit test to test this.

[Charlie, 2015-01-08 16:47:34]

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
File components/copresence/copresence_manager_impl.h (right):

https://codereview.chromium.org/813553002/diff/20001/components/copresence/co...
components/copresence/copresence_manager_impl.h:71: template<typename
IterableSubscribedMessages>
On 2015/01/07 23:06:10, Rahul Chaturvedi wrote:
> On 2015/01/07 22:06:07, Charlie wrote:
> > On 2015/01/07 21:31:43, Rahul Chaturvedi wrote:
> > > Since we use a std::vector only in our own map, can we instead use a
> > > RepeatedPtrField there too and remove this templatization?
> > 
> > Yes, but that's kind of hacky. This kind of templating is the C++ version of
> > Java's Iterable interface.
> 
> Sure but we don't actually need this to be an iterable interface. Templates
> introduce compile and run-time costs, which we shouldn't incur unless there is
> at least some clear benefit to it.

1. I don't think RepeatedPtrField is really intended to be used anywhere outside
a proto.

2. According to Titus Winters, head of the C++ platform for Google: if you can't
write something in the clearest, simplest way, it's a compiler/libraries bug.
Instead of being "clever", write the code clearly and file a bug. Cleverness is
much more costly in the long run.

That said, RepeatedPtrField is actually easier to deal with than I thought.
After branch maybe I'll ask Titus for his opinion here.

Done.

https://codereview.chromium.org/813553002/diff/60001/components/copresence/ti...
File components/copresence/timed_map.h (right):

https://codereview.chromium.org/813553002/diff/60001/components/copresence/ti...
components/copresence/timed_map.h:57: return &map_[key];
On 2015/01/07 23:06:10, Rahul Chaturvedi wrote:
> If the element doesn't exist, this will actually add the key to the map. Use
> find first to confirm that the key does exist; otherwise return nullptr.
> 

That's more consistent with the rest of the API - but I don't think it's a good
API. Added a TODO to discuss later.

Done.

https://codereview.chromium.org/813553002/diff/60001/components/copresence/ti...
components/copresence/timed_map.h:60: size_t Erase(const KeyType& key) {
On 2015/01/07 23:06:10, Rahul Chaturvedi wrote:
> Add a TODO to add a unit test to test this.

Done.

[Charlie, 2015-01-08 16:50:07]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2015-01-08 16:50:56]

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

[commit-bot: I haz the power, 2015-01-08 18:19:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-08 18:20:25]

Patchset 6 (id:??) landed as
https://crrev.com/1549c3f451217ee30fbe00cf0eb318fb0e54e05f
Cr-Commit-Position: refs/heads/master@{#310538}