Add the Copresence API.

Add the Copresence API.

This CL adds the main Copresence API. This CL is tracking,
https://codereview.chromium.org/438513002.

This CL also requires
https://codereview.chromium.org/419073002 and
https://codereview.chromium.org/433283002 to land to be fully functional.

API tests for this API are being worked on by ckehoe@ and we'll be sending a
follow on CL tracking this one very soon.

Reviews requested,
xiyuan@ - General review.
kalman@ - API review.
isherman@ - histograms.xml OWNERs review.

R=derat@chromium.org, isherman@chromium.org, kalman@chromium.org, xiyuan@chromium.org
BUG=365493
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288230

[Pam (message me for reviews), 2014-08-05 06:27:38]

c/b/prefs/* lgtm

[rkc, 2014-08-05 07:01:50]

+derat@,+xiyuan@ for FYI (or any comments they may have).

[not at google - send to devlin, 2014-08-05 16:58:40]

I'm happy to review the general infrastructure of this, but given you've added
ckehoe@ as an OWNER perhaps he is a better reviewer for the specifics.

[not at google - send to devlin, 2014-08-05 17:09:55]

Had a quick skim of the code outside of chrome/browser/extensions/api/ which led
me to the question about the pref.

Other than that seems fine, but I'll have a high-level look at
chrome/browser/extensions/api/ as well.

As I said I don't understand the complexity of the copresence implementation so
I'm not the best person to review that.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:116: const
std::string CopresenceService::GetDeviceId() const {
I don't see Get/SaveDeviceId used anywhere, which means I don't understand all
of the preference infrastructure. What is it for?

[rkc, 2014-08-05 18:06:25]

I've asked xiyuan@ to look at the general review since he is familiar with all
of the copresence code.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:116: const
std::string CopresenceService::GetDeviceId() const {
On 2014/08/05 17:09:55, kalman wrote:
> I don't see Get/SaveDeviceId used anywhere, which means I don't understand all
> of the preference infrastructure. What is it for?

Get/SaveDeviceId are called from
https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp...
Since we don't want any dependencies from the Copresence component on the
browser, we've extracted all browser related functionality to the
CopresenceDelegate which we implement and pass down into the CopresenceClient.

[not at google - send to devlin, 2014-08-05 18:17:21]

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:116: const
std::string CopresenceService::GetDeviceId() const {
On 2014/08/05 18:06:24, Rahul Chaturvedi wrote:
> On 2014/08/05 17:09:55, kalman wrote:
> > I don't see Get/SaveDeviceId used anywhere, which means I don't understand
all
> > of the preference infrastructure. What is it for?
> 
> Get/SaveDeviceId are called from
>
https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp...
> Since we don't want any dependencies from the Copresence component on the
> browser, we've extracted all browser related functionality to the
> CopresenceDelegate which we implement and pass down into the CopresenceClient.

I see.

That is never exposed to the extension, right?

Even if not, this has privacy implications. I'm not able to consult here, so
check with battre@.

[Ilya Sherman, 2014-08-05 18:46:51]

histograms lgtm

[xiyuan, 2014-08-05 21:02:35]

Mostly good.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:13: #include
"chrome/browser/profiles/profile.h"
nit: seems not used

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:75: std::string
app_id = GetCopresenceService(browser_context_)
What is difference between this |app_id| from subscription registry and the one
passed in? If this is so because |app_id| is not passed in, let's add a TODO to
fix it or something.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:85:
std::vector<linked_ptr<api::copresence::Message> > api_messages(
nit: #include "base/memory/linked_ptr.h"

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.h (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:38:
copresence::WhispernetClient* whispernet_client();
nit: move accessors to the last.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:40: virtual void
Shutdown() OVERRIDE;
nit: // BrowserContextKeyedAPI implementation.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_translations.cc
(right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_translations.cc:29: if
(strategy->only_broadcast && strategy->only_broadcast)
missing '*' for the second "strategy->only_broadcast"

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_translations.cc:103:
std::map<std::string, std::string>::iterator previous_subscription =
nit: SubscriptionToAppMap::iterator since we have the typedef

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_translations.cc:158:
std::map<std::string, std::string>::iterator subscription =
nit: SubscriptionToAppMap::iterator

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/copresence.idl (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:143: // Fired when we new messages
arrive.
nit: get rid of "we"

[not at google - send to devlin, 2014-08-05 21:05:39]

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:151: return
RespondNow(BadMessage());
nit: multi-line condition should have {} around the body.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.h (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:43:
SubscriptionToAppMap& SubscriptionToAppsRegistry() {
should be subscription_to_apps_registry(), or really apps_by_subscription_id().

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:88: void
SendResult(copresence::CopresenceStatus status);
this can be private

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:99: void
SendResult(copresence::CopresenceStatus status);
this is never implemented

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/copresence.idl (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:29: [inline_doc] dictionary Message
{
it will automatically inline since it's only used in 1 place

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:90: OperationType type;
why do you need the OperationType? it can be determined from which property was
set (publish|subscribe|unpublish|unsubscribe).

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:104: [inline_doc] enum
BatchExecuteStatus {
it will automatically inline since it's only used in 1 place

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:116: [inline_doc] enum Status {
it will automatically inline since it's only used in 1 place

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:129: // the api key, apps on any
platform that are using this API key can
what's to stop some MyRandomApp from setting the same API key and listening to
messages?

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:138: static void
batchExecute(Operation[] operations,
IMO this should just be "execute" because the fact it batches is rather obvious,
and there's no other option other than batch. But IIRC you called it
"batchExecute" for consistency with other platforms?

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:148: static void
onStatusReceived(Status status);
onStatusUpdated?

[rkc, 2014-08-05 22:25:23]

Removing battre@ and pam@ since I removed the prefs change.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:13: #include
"chrome/browser/profiles/profile.h"
On 2014/08/05 21:02:35, xiyuan wrote:
> nit: seems not used

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:75: std::string
app_id = GetCopresenceService(browser_context_)
On 2014/08/05 21:02:35, xiyuan wrote:
> What is difference between this |app_id| from subscription registry and the
one
> passed in? If this is so because |app_id| is not passed in, let's add a TODO
to
> fix it or something.

Eventually the server API will return the app_id to us for a subscription, then
we won't need to keep this subid->appid map. Added a TODO.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:85:
std::vector<linked_ptr<api::copresence::Message> > api_messages(
On 2014/08/05 21:02:35, xiyuan wrote:
> nit: #include "base/memory/linked_ptr.h"

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:116: const
std::string CopresenceService::GetDeviceId() const {
On 2014/08/05 18:17:21, kalman wrote:
> On 2014/08/05 18:06:24, Rahul Chaturvedi wrote:
> > On 2014/08/05 17:09:55, kalman wrote:
> > > I don't see Get/SaveDeviceId used anywhere, which means I don't understand
> all
> > > of the preference infrastructure. What is it for?
> > 
> > Get/SaveDeviceId are called from
> >
>
https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp...
> > Since we don't want any dependencies from the Copresence component on the
> > browser, we've extracted all browser related functionality to the
> > CopresenceDelegate which we implement and pass down into the
CopresenceClient.
> 
> I see.
> 
> That is never exposed to the extension, right?
> 
> Even if not, this has privacy implications. I'm not able to consult here, so
> check with battre@.

Spoke with jyasskin@
Removing all of this code.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:151: return
RespondNow(BadMessage());
On 2014/08/05 21:05:38, kalman wrote:
> nit: multi-line condition should have {} around the body.

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.h (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:38:
copresence::WhispernetClient* whispernet_client();
On 2014/08/05 21:02:35, xiyuan wrote:
> nit: move accessors to the last.

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:40: virtual void
Shutdown() OVERRIDE;
On 2014/08/05 21:02:35, xiyuan wrote:
> nit: // BrowserContextKeyedAPI implementation.

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:43:
SubscriptionToAppMap& SubscriptionToAppsRegistry() {
On 2014/08/05 21:05:38, kalman wrote:
> should be subscription_to_apps_registry(), or really
apps_by_subscription_id().

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:88: void
SendResult(copresence::CopresenceStatus status);
On 2014/08/05 21:05:38, kalman wrote:
> this can be private

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.h:99: void
SendResult(copresence::CopresenceStatus status);
On 2014/08/05 21:05:38, kalman wrote:
> this is never implemented

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_translations.cc
(right):

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_translations.cc:29: if
(strategy->only_broadcast && strategy->only_broadcast)
On 2014/08/05 21:02:35, xiyuan wrote:
> missing '*' for the second "strategy->only_broadcast"

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_translations.cc:103:
std::map<std::string, std::string>::iterator previous_subscription =
On 2014/08/05 21:02:35, xiyuan wrote:
> nit: SubscriptionToAppMap::iterator since we have the typedef

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_translations.cc:158:
std::map<std::string, std::string>::iterator subscription =
On 2014/08/05 21:02:35, xiyuan wrote:
> nit: SubscriptionToAppMap::iterator

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/copresence.idl (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:29: [inline_doc] dictionary Message
{
On 2014/08/05 21:05:38, kalman wrote:
> it will automatically inline since it's only used in 1 place

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:90: OperationType type;
On 2014/08/05 21:05:39, kalman wrote:
> why do you need the OperationType? it can be determined from which property
was
> set (publish|subscribe|unpublish|unsubscribe).

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:104: [inline_doc] enum
BatchExecuteStatus {
On 2014/08/05 21:05:39, kalman wrote:
> it will automatically inline since it's only used in 1 place

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:116: [inline_doc] enum Status {
On 2014/08/05 21:05:38, kalman wrote:
> it will automatically inline since it's only used in 1 place

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:129: // the api key, apps on any
platform that are using this API key can
On 2014/08/05 21:05:39, kalman wrote:
> what's to stop some MyRandomApp from setting the same API key and listening to
> messages?

This isn't a security feature, it is a scoping feature. In an unauthenticated
API, there is literally no way we can prevent one app from getting messages from
any other app. For example, if we scoped by app ID, one could simply install a
local app, set the key: manifest field and masquerade its app id.

The API key is used as a simple scoping mechanism since it allows multiple apps
across platforms to be associated with one single 'key' value.

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:138: static void
batchExecute(Operation[] operations,
On 2014/08/05 21:05:39, kalman wrote:
> IMO this should just be "execute" because the fact it batches is rather
obvious,
> and there's no other option other than batch. But IIRC you called it
> "batchExecute" for consistency with other platforms?

Yes, but execute is fine too.
Changed this and all other references to [B|b]atchExecute
Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:143: // Fired when we new messages
arrive.
On 2014/08/05 21:02:35, xiyuan wrote:
> nit: get rid of "we"

Done.

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:148: static void
onStatusReceived(Status status);
On 2014/08/05 21:05:39, kalman wrote:
> onStatusUpdated?

Done.

[xiyuan, 2014-08-05 22:30:04]

lgtm

[not at google - send to devlin, 2014-08-05 22:38:14]

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/copresence.idl (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:129: // the api key, apps on any
platform that are using this API key can
On 2014/08/05 22:25:23, Rahul Chaturvedi wrote:
> On 2014/08/05 21:05:39, kalman wrote:
> > what's to stop some MyRandomApp from setting the same API key and listening
to
> > messages?
> 
> This isn't a security feature, it is a scoping feature. In an unauthenticated
> API, there is literally no way we can prevent one app from getting messages
from
> any other app. For example, if we scoped by app ID, one could simply install a
> local app, set the key: manifest field and masquerade its app id.
> 
> The API key is used as a simple scoping mechanism since it allows multiple
apps
> across platforms to be associated with one single 'key' value.

I see. out of curiosity why isn't this a message filter?

[rkc, 2014-08-05 23:06:29]

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/copresence.idl (right):

https://codereview.chromium.org/444513005/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/copresence.idl:129: // the api key, apps on any
platform that are using this API key can
On 2014/08/05 22:38:14, kalman wrote:
> On 2014/08/05 22:25:23, Rahul Chaturvedi wrote:
> > On 2014/08/05 21:05:39, kalman wrote:
> > > what's to stop some MyRandomApp from setting the same API key and
listening
> to
> > > messages?
> > 
> > This isn't a security feature, it is a scoping feature. In an
unauthenticated
> > API, there is literally no way we can prevent one app from getting messages
> from
> > any other app. For example, if we scoped by app ID, one could simply install
a
> > local app, set the key: manifest field and masquerade its app id.
> > 
> > The API key is used as a simple scoping mechanism since it allows multiple
> apps
> > across platforms to be associated with one single 'key' value.
> 
> I see. out of curiosity why isn't this a message filter?

We don't want an app to keep setting arbitrary keys based on which app it wants
messages from. Putting this in the message filter indicates that we should be
using this key to filter on, while we really want an app to use the API key only
if it wants to associate the current app with other apps on other platforms.

It is a way to say "this app is the same as all the other apps that are using an
API key from the same project". It is not meant to be "give me messages from app
x, y and z". For the latter, we want to eventually provide a clear way to do so
(if enough people actually ask for it; as of now, no one has).

[rkc, 2014-08-06 20:26:58]

Tests for the API are in this CL,
https://codereview.chromium.org/441103002/

[Daniel Erat, 2014-08-06 22:59:38]

high-level question: would this api ever make sense for non-chrome binaries like
app_shell? if so, it may make sense to see what it'd take to put it in
extensions/ rather than chrome/.

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/copresen...
File chrome/browser/copresence/chrome_whispernet_client_browsertest.cc (right):

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/copresen...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:105:
ASSERT_TRUE(extensions::GetWhispernetClient(context_));
nit: cache this so you don't need to get it multiple times in this method, e.g.

  // i dunno what the actual type is
  WhispernetClient* client = extensions::GetWhispernetClient(...);
  ASSERT_TRUE(client);
  ...
  client->RegisterDetectBroadcastCallback(...);
  client->DetectBroadcast(...);

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_translations.cc
(right):

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:21: :
*user_ttl <= 0 || *user_ttl > kMaxTimeToLiveMs ? -1 : *user_ttl;
use parentheses for nested ternary operators

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:25:
extensions::api::copresence::Strategy* strategy) {
const reference?

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:27: bool
only_scan = false;
this isn't limited to this change, but the word "only" doesn't make sense if
both of these can be set. should it be something like "should_broadcast" and
"should_scan" instead?

and how does this relate to the comments in the idl?

+  dictionary Strategy {
+    // Attempt to use low power mode. Defaults to false.
+    boolean? lowPower;
+    // Attempt to only broadcast. Using this with onlyScan can result in both
+    // being ignored. Defaults to false.
+    boolean? onlyBroadcast;
+    // Attempt to only scan. Using this with onlyBroadcast can result in both
+    // being ignored. Defaults to false.
+    boolean? onlyScan;
+    // Attempt to use audible audio. Defaults to false.
+    boolean? audible;
+  };

and if you really want only one mode to be set, why not use an enum?

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:29: if
(strategy->only_broadcast && *strategy->only_broadcast)
how about just combining these with the earlier statements?

  bool broadcast = strategy.broadcast && *strategy.broadcast;

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:61: if (ttl
< 0)
is there any way to report what the problem is to the caller?

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:200: }
nit: since you're already checking earlier that only one is set, you could
condense this a bit by using if-else

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_translations.h (right):

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.h:36: bool
AddPublishToRequest(const std::string& app_id,
if you don't call any of these Add functions outside of
PrepareReportRequestProto(), put them in an anonymous namespace in the .cc file

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/copresence.idl (right):

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:5: // Use the
<code>chrome.copresence</code> API to communicate with other devices
nit: "... to communicate with other nearby devices using Google's copresence
service."

(capitalize "Copresence" if that's how it's written publicly)

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:25: // The type of message that
we're publishing. Cannot be empty.
nit: s/that we're publishing/being published/

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:37: // Only send this message to
people within hearing range.
nit: s/people/devices/

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:48: // This is capped at 24 hours.
If not provided, we default to 5 minutes.
nit: s/we default to 5 minutes/a default of 5 minutes is used/

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:63: // This is capped at 24 hours.
If not provided, we default to 5 minutes.
nit: s/we default to 5 minutes/a default of 5 minutes is used/

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:71: // The id of a message to
unpublish. Required if the operation type
nit: s/id/ID/ here and elsewhere in this file

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:118: // Sets the API key to use with
our app. This parameter only needs to be
nit: s/our app/the app/

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:119: // set if we need to
communicate with apps on other platforms. Once we set
nit: "set to communicate with apps on other platforms"

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:120: // the api key, apps on any
platform that are using this API key can
nit: s/api/API/, also change to "Once the API key is set, ..."

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:137: // Fired when we have a new
status update for copresence.
nit: Fired when a new Copresence status update is available.

[rkc, 2014-08-07 02:18:34]

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/copresen...
File chrome/browser/copresence/chrome_whispernet_client_browsertest.cc (right):

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/copresen...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:105:
ASSERT_TRUE(extensions::GetWhispernetClient(context_));
On 2014/08/06 22:59:36, Daniel Erat wrote:
> nit: cache this so you don't need to get it multiple times in this method,
e.g.
> 
>   // i dunno what the actual type is
>   WhispernetClient* client = extensions::GetWhispernetClient(...);
>   ASSERT_TRUE(client);
>   ...
>   client->RegisterDetectBroadcastCallback(...);
>   client->DetectBroadcast(...);

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_translations.cc
(right):

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:21: :
*user_ttl <= 0 || *user_ttl > kMaxTimeToLiveMs ? -1 : *user_ttl;
On 2014/08/06 22:59:37, Daniel Erat wrote:
> use parentheses for nested ternary operators

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:25:
extensions::api::copresence::Strategy* strategy) {
On 2014/08/06 22:59:37, Daniel Erat wrote:
> const reference?

copresence::BroadcastScanConfiguration is an enum, so const ref doesn't do much
for us.

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:27: bool
only_scan = false;
On 2014/08/06 22:59:37, Daniel Erat wrote:
> this isn't limited to this change, but the word "only" doesn't make sense if
> both of these can be set. should it be something like "should_broadcast" and
> "should_scan" instead?
> 
> and how does this relate to the comments in the idl?
> 
> +  dictionary Strategy {
> +    // Attempt to use low power mode. Defaults to false.
> +    boolean? lowPower;
> +    // Attempt to only broadcast. Using this with onlyScan can result in both
> +    // being ignored. Defaults to false.
> +    boolean? onlyBroadcast;
> +    // Attempt to only scan. Using this with onlyBroadcast can result in both
> +    // being ignored. Defaults to false.
> +    boolean? onlyScan;
> +    // Attempt to use audible audio. Defaults to false.
> +    boolean? audible;
> +  };
> 
> and if you really want only one mode to be set, why not use an enum?

There is no good way to restrict an app to setting only one of the two
(onlyScan/onlyBroadcast) strategies. If this was an enum, the app could simply
pass in two strategy objects, one setting onlyScan and one setting
onlyBroadcast. Furthermore, if one app sets onlyScan, and another sets
onlyBroadcast, we'll end up doing both anyway since the server will give us a
broadcast directive for one app, and a scan directive for the other.

So with these restrictions, it made sense to optimize for the simplicity of the
strategy object. This used to be just a list of enums. The app would pass us an
array of all the enums it wanted to use (that also had the same problem though,
an app could pass in an onlyScan and onlyBroadcast enum). We changed this to be
an object so we can later add maybe more complex strategies which would be hard
to express as an enum.

In scenarios that we don't have conflicting strategies, "only" does truly mean
"only", hence the naming. We do make it clear in documentation that these are
just suggestions and that in various situations (this is one case, more might
show up later), we may or may not respect the suggestion given to us.

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:29: if
(strategy->only_broadcast && *strategy->only_broadcast)
On 2014/08/06 22:59:37, Daniel Erat wrote:
> how about just combining these with the earlier statements?
> 
>   bool broadcast = strategy.broadcast && *strategy.broadcast;

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:61: if (ttl
< 0)
On 2014/08/06 22:59:37, Daniel Erat wrote:
> is there any way to report what the problem is to the caller?

Yes, we have a bug open tracking better error reporting to the caller
(crbug.com/390389). We'll be doing another parse through all of this code to add
in better error reporting when we work on that issue.

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:200: }
On 2014/08/06 22:59:37, Daniel Erat wrote:
> nit: since you're already checking earlier that only one is set, you could
> condense this a bit by using if-else

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_translations.h (right):

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.h:36: bool
AddPublishToRequest(const std::string& app_id,
On 2014/08/06 22:59:37, Daniel Erat wrote:
> if you don't call any of these Add functions outside of
> PrepareReportRequestProto(), put them in an anonymous namespace in the .cc
file

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/copresence.idl (right):

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:5: // Use the
<code>chrome.copresence</code> API to communicate with other devices
On 2014/08/06 22:59:37, Daniel Erat wrote:
> nit: "... to communicate with other nearby devices using Google's copresence
> service."
> 
> (capitalize "Copresence" if that's how it's written publicly)

We don't use Copresence for branding (at least yet) so I tend to use it lower
case most places.
Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:25: // The type of message that
we're publishing. Cannot be empty.
On 2014/08/06 22:59:38, Daniel Erat wrote:
> nit: s/that we're publishing/being published/

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:37: // Only send this message to
people within hearing range.
On 2014/08/06 22:59:38, Daniel Erat wrote:
> nit: s/people/devices/

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:48: // This is capped at 24 hours.
If not provided, we default to 5 minutes.
On 2014/08/06 22:59:37, Daniel Erat wrote:
> nit: s/we default to 5 minutes/a default of 5 minutes is used/

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:63: // This is capped at 24 hours.
If not provided, we default to 5 minutes.
On 2014/08/06 22:59:38, Daniel Erat wrote:
> nit: s/we default to 5 minutes/a default of 5 minutes is used/

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:71: // The id of a message to
unpublish. Required if the operation type
On 2014/08/06 22:59:37, Daniel Erat wrote:
> nit: s/id/ID/ here and elsewhere in this file

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:118: // Sets the API key to use with
our app. This parameter only needs to be
On 2014/08/06 22:59:37, Daniel Erat wrote:
> nit: s/our app/the app/

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:119: // set if we need to
communicate with apps on other platforms. Once we set
On 2014/08/06 22:59:38, Daniel Erat wrote:
> nit: "set to communicate with apps on other platforms"

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:120: // the api key, apps on any
platform that are using this API key can
On 2014/08/06 22:59:37, Daniel Erat wrote:
> nit: s/api/API/, also change to "Once the API key is set, ..."

Done.

https://codereview.chromium.org/444513005/diff/100001/chrome/common/extension...
chrome/common/extensions/api/copresence.idl:137: // Fired when we have a new
status update for copresence.
On 2014/08/06 22:59:37, Daniel Erat wrote:
> nit: Fired when a new Copresence status update is available.

Done.

[rkc, 2014-08-07 02:42:36]

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_translations.cc
(right):

https://codereview.chromium.org/444513005/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_translations.cc:25:
extensions::api::copresence::Strategy* strategy) {
On 2014/08/06 22:59:37, Daniel Erat wrote:
> const reference?

Oh, for some weird reason I thought you were saying this should 'return' a const
reference.
Fixed the parameter to be const ref.
Done.

[rkc, 2014-08-07 02:51:16]

On 2014/08/06 22:59:38, Daniel Erat wrote:
> high-level question: would this api ever make sense for non-chrome binaries
like
> app_shell? if so, it may make sense to see what it'd take to put it in
> extensions/ rather than chrome/.

We will most definitely need to use this from app_shell. Keeping this code in
c/b is only for M-38 since we have a dependency on the ChromeWhispernetClient
which itself is dependent on the WhispernetProxyExtension, which being a
component extension is dependent on the browser.
For M-39 we'll be looking at better ways to talk to Whispernet (this will most
probably use Mojo) which should remove our dependency on chrome/browser.  As
soon as that dependency is gone, we'll move all of this code to
//extensions/copresence instead. I've taken quite a bit of care to not have any
other dependency on the browser except for the ChromeWhispernetClient. We had an
additional dependency on local_state, which would have tied us to the browser (I
spoke to the app shell folks about exposing a local_state like ability to
//extensions) but since we aren't doing that anymore, the only thing tying us
down here is ChromeWhispernetClient.

Looking through the code, I realized that we also have a dependency on
ChromeVersion; I'll work with the app_shell folks to figure out how to get rid
of that (we need some way of getting the underlying platform version).

[Daniel Erat, 2014-08-07 18:12:30]

thanks for the explanation; lgtm

[rkc, 2014-08-07 19:30:13]

Added [nodoc] to setApiKeys. Spoke to kalman@ offline about this. setApiKeys is
not the permanent way to solve the multi-platform interop but more of a stop-gap
for our current customers in M38. In M39 we'll have a better solution for this.

[not at google - send to devlin, 2014-08-07 20:41:19]

nitty things left.

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:71: std::string
app_id = GetCopresenceService(browser_context_)
I don't understand this call. Why is a CopresenceService calling
GetCopresenceService? isn't it |this|?

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:97:
event->restrict_to_browser_context = browser_context_;
you can construct this directly:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:129:
GetCopresenceService(browser_context())->apps_by_subscription_id();
you seem to be checking for a NULL copresence service in some places but not
others.

when would there not be a copresence service?

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_util.h (right):

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_util.h:20: 
these utils have a pretty small value, and it seems like they're basically used
in copresence_api.cc only. just put them in there, if really necessary?

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
File chrome/common/extensions/docs/templates/public/apps/copresence.html
(right):

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
chrome/common/extensions/docs/templates/public/apps/copresence.html:1:
{{+partials.standard_apps_api api:apis.apps.copresence/}}
also add a page for extensions

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_api_permissions.cc:112:
{APIPermission::kCopresence, "copresence"},
i can't remember the conclusion about an install warning, but I guess one wasn't
required?

[rkc, 2014-08-07 21:43:03]

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:71: std::string
app_id = GetCopresenceService(browser_context_)
On 2014/08/07 20:41:18, kalman wrote:
> I don't understand this call. Why is a CopresenceService calling
> GetCopresenceService? isn't it |this|?

Whoops, that's kinda silly :) Fixed.
Done.

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:97:
event->restrict_to_browser_context = browser_context_;
On 2014/08/07 20:41:18, kalman wrote:
> you can construct this directly:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

Done.

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:129:
GetCopresenceService(browser_context())->apps_by_subscription_id();
On 2014/08/07 20:41:18, kalman wrote:
> you seem to be checking for a NULL copresence service in some places but not
> others.
> 
> when would there not be a copresence service?

Copresence service should always be available (or get constructed if not
available). Removed all checks for the service, but added a check to the
beginning of this method for checking if the client is NULL.

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_util.h (right):

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_util.h:20: 
On 2014/08/07 20:41:18, kalman wrote:
> these utils have a pretty small value, and it seems like they're basically
used
> in copresence_api.cc only. just put them in there, if really necessary?

They're used from copresence_private_api.cc, otherwise I'd have put them only in
copresence_api.cc

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
File chrome/common/extensions/docs/templates/public/apps/copresence.html
(right):

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
chrome/common/extensions/docs/templates/public/apps/copresence.html:1:
{{+partials.standard_apps_api api:apis.apps.copresence/}}
On 2014/08/07 20:41:19, kalman wrote:
> also add a page for extensions

Done.

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_api_permissions.cc:112:
{APIPermission::kCopresence, "copresence"},
On 2014/08/07 20:41:19, kalman wrote:
> i can't remember the conclusion about an install warning, but I guess one
wasn't
> required?

Not for dev channel. Whitelisted apps will have their own privacy review and may
be required to show some sort of permissions dialog (considering we are
unauthenticated now, the privacy requires are somewhat relaxed).

The final install warning for this has not been decided yet, but before we open
the API up, it definitely will be.

[not at google - send to devlin, 2014-08-07 21:47:44]

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_util.h (right):

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_util.h:20: 
On 2014/08/07 21:43:02, Rahul Chaturvedi wrote:
> On 2014/08/07 20:41:18, kalman wrote:
> > these utils have a pretty small value, and it seems like they're basically
> used
> > in copresence_api.cc only. just put them in there, if really necessary?
> 
> They're used from copresence_private_api.cc, otherwise I'd have put them only
in
> copresence_api.cc

they're such low value though. A null check? It's hardly worth creating a whole
util system for. it's another file I went and had a look at, and anybody else
reading this API will need to check it out as well. just added complexity for no
gain.

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_api_permissions.cc:112:
{APIPermission::kCopresence, "copresence"},
On 2014/08/07 21:43:03, Rahul Chaturvedi wrote:
> On 2014/08/07 20:41:19, kalman wrote:
> > i can't remember the conclusion about an install warning, but I guess one
> wasn't
> > required?
> 
> Not for dev channel. Whitelisted apps will have their own privacy review and
may
> be required to show some sort of permissions dialog (considering we are
> unauthenticated now, the privacy requires are somewhat relaxed).
> 
> The final install warning for this has not been decided yet, but before we
open
> the API up, it definitely will be.

I don't think that "it's dev channel" is a good reason to avoid an install
warning. And I'm not sure that the privacy implications are much different, an
extension can still figure out who you are through other APIs. Could you at
least make it "communicate with nearby devices" or something? anything?

[rkc, 2014-08-07 22:03:45]

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_util.h (right):

https://codereview.chromium.org/444513005/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_util.h:20: 
On 2014/08/07 21:47:43, kalman wrote:
> On 2014/08/07 21:43:02, Rahul Chaturvedi wrote:
> > On 2014/08/07 20:41:18, kalman wrote:
> > > these utils have a pretty small value, and it seems like they're basically
> > used
> > > in copresence_api.cc only. just put them in there, if really necessary?
> > 
> > They're used from copresence_private_api.cc, otherwise I'd have put them
only
> in
> > copresence_api.cc
> 
> they're such low value though. A null check? It's hardly worth creating a
whole
> util system for. it's another file I went and had a look at, and anybody else
> reading this API will need to check it out as well. just added complexity for
no
> gain.

Agreed, it isn't really a big benefit. Removed.
Done.

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/444513005/diff/180001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_api_permissions.cc:112:
{APIPermission::kCopresence, "copresence"},
On 2014/08/07 21:47:43, kalman wrote:
> On 2014/08/07 21:43:03, Rahul Chaturvedi wrote:
> > On 2014/08/07 20:41:19, kalman wrote:
> > > i can't remember the conclusion about an install warning, but I guess one
> > wasn't
> > > required?
> > 
> > Not for dev channel. Whitelisted apps will have their own privacy review and
> may
> > be required to show some sort of permissions dialog (considering we are
> > unauthenticated now, the privacy requires are somewhat relaxed).
> > 
> > The final install warning for this has not been decided yet, but before we
> open
> > the API up, it definitely will be.
> 
> I don't think that "it's dev channel" is a good reason to avoid an install
> warning. And I'm not sure that the privacy implications are much different, an
> extension can still figure out who you are through other APIs. Could you at
> least make it "communicate with nearby devices" or something? anything?

Done.

[not at google - send to devlin, 2014-08-07 22:23:38]

https://codereview.chromium.org/444513005/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:37:
content::BrowserContext* browser_context) {
make both of these instance methods, then you don't need to pass in the browser
context.

https://codereview.chromium.org/444513005/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:53:
copresence::CopresenceClient* CopresenceService::client() {
what is the difference between this method and GetCopresenceClient? seems like
they could be merged without much drama.

i'm finding it very hard to follow the flow of getting CopresenceClients and
CopresenceServices from this and that.

https://codereview.chromium.org/444513005/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:56: return
client_.get();
this returns the current client if it's shutting down. is that what you want?
the comment below makes it seems like you always want to return null if it's
shutting down.

[rkc, 2014-08-07 22:32:37]

https://codereview.chromium.org/444513005/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:37:
content::BrowserContext* browser_context) {
On 2014/08/07 22:23:38, kalman wrote:
> make both of these instance methods, then you don't need to pass in the
browser
> context.

Actually, removing them completely doesn't really hurt the code. Removed.
Done.

https://codereview.chromium.org/444513005/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:53:
copresence::CopresenceClient* CopresenceService::client() {
On 2014/08/07 22:23:38, kalman wrote:
> what is the difference between this method and GetCopresenceClient? seems like
> they could be merged without much drama.
> 
> i'm finding it very hard to follow the flow of getting CopresenceClients and
> CopresenceServices from this and that.

Removed all the GetCopresenceXXX() functions.
Done.

https://codereview.chromium.org/444513005/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:56: return
client_.get();
On 2014/08/07 22:23:38, kalman wrote:
> this returns the current client if it's shutting down. is that what you want?
> the comment below makes it seems like you always want to return null if it's
> shutting down.

Yes, and the current client is set to NULL if we're shutting down, so in effect,
if we're shutting down, we'll return NULL (as intended).

[not at google - send to devlin, 2014-08-07 22:41:27]

https://codereview.chromium.org/444513005/diff/220002/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/220002/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:136:
SubscriptionToAppMap& apps_by_subscription_id =
inline this?

[not at google - send to devlin, 2014-08-07 22:41:34]

(lgtm)

[rkc, 2014-08-07 22:44:33]

https://codereview.chromium.org/444513005/diff/220002/chrome/browser/extensio...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/444513005/diff/220002/chrome/browser/extensio...
chrome/browser/extensions/api/copresence/copresence_api.cc:136:
SubscriptionToAppMap& apps_by_subscription_id =
On 2014/08/07 22:41:27, kalman wrote:
> inline this?

Done.

[rkc, 2014-08-07 22:45:33]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-07 23:01:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rkc@chromium.org/444513005/250001

[rkc, 2014-08-07 23:34:34]

The CQ bit was unchecked by rkc@chromium.org

[rkc, 2014-08-07 23:42:05]

The CQ bit was checked by rkc@chromium.org

[rkc, 2014-08-08 01:49:19]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-08 01:52:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rkc@chromium.org/444513005/290001

[rkc, 2014-08-08 06:36:11]

Committed patchset #16 manually as 288230 (presubmit successful).

[brettw, 2014-08-20 17:03:23]

https://codereview.chromium.org/444513005/diff/290001/chrome/chrome_browser_e...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/444513005/diff/290001/chrome/chrome_browser_e...
chrome/chrome_browser_extensions.gypi:988:
'../components/components.gyp:copresence',
This updated the GYP build but didn't update the GN build. The build works if
the copresence proto happens to be built before this target (which I guess is
the case on the bots, at least for the incremental builds they do) but is not
the case necessarily and this fails on some systems.

If you add dependencies, please be sure to look for the corresponding thing in
the GN build. I'll fix this in a patch I'm working on.

[Charlie, 2014-08-20 17:22:53]

https://codereview.chromium.org/444513005/diff/290001/chrome/chrome_browser_e...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/444513005/diff/290001/chrome/chrome_browser_e...
chrome/chrome_browser_extensions.gypi:988:
'../components/components.gyp:copresence',
On 2014/08/20 17:03:23, brettw wrote:
> This updated the GYP build but didn't update the GN build. The build works if
> the copresence proto happens to be built before this target (which I guess is
> the case on the bots, at least for the incremental builds they do) but is not
> the case necessarily and this fails on some systems.
> 
> If you add dependencies, please be sure to look for the corresponding thing in
> the GN build. I'll fix this in a patch I'm working on.

Sorry about that. Looks like the GN build does fail on my machine. I'll try to
check it more regularly, although we really should get the incremental builds to
fail in cases like these.