Identity API: Add chrome.identity.onSignInChanged routing and IDL

Identity API: Add chrome.identity.onSignInChanged routing and IDL

This is the first part of adding the chrome.identity.onSignInChanged
event. This change creates the event in dev channel, and an event
router that can dispatch sign-in events to listening extensions.

A new permission, "identity.email" determines whether or not the app
may be given the email address of the account associated with the
user's profile. Apps without the permission still receive events, but
without the email address.

The code to actually generate the events will come in a future CL.

BUG=305830
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229345

[Michael Courage, 2013-10-15 22:16:38]

doh, a last minute "simplification" of the dispatcher introduced a bug. patch 3
fixes it.

[not at google - send to devlin, 2013-10-16 16:49:36]

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/identity_event_router.cc (right):

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router.cc:28:
ExtensionSystem::Get(profile_)->extension_service();
ExtensionSystem::Get(profile_) being used twice. Save reference to it?

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router.cc:36:
account_info_email.email = scoped_ptr<std::string>(new std::string(email));
could you just have a single AccountInfo object and conditionally set the
"email" field based on the HasAPIPermission method? Then you couldn't need the
check on line 48 either.

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router.cc:41: 
nit if you want: "it != listeners.end(); ++it" (no line break) then no blank
line here.

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router.cc:42: const
std::string extension_id = (*it)->extension_id;
const std::string& ?

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router.cc:57: new
Event(api::identity::OnSignInChanged::kEventName, args.Pass()));
I think there's an event constructor that will let you assign
"restrict_to_profile" immediately:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

https://codereview.chromium.org/27283002/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/27283002/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:354: },
hm. I thought this was just going to be a separate permission (like outside the
identity API)?

I thought of some other places where such a permission would be nice, for
example, the feedback API might only include the email address of the user if
the permission is requested. That's a bad example I think since it's not the UI
the feedback API would end up having (it'd probably just always show the email
and have the user switch it off) but... there may be other places too?

Not sure about this point, but, wdyt?

https://codereview.chromium.org/27283002/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/identity.idl (right):

https://codereview.chromium.org/27283002/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/identity.idl:44: DOMString? email;
documentation needed for these (and include "email only present when..." in it).

https://codereview.chromium.org/27283002/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/identity_event_router_unittest.cc
(right):

https://codereview.chromium.org/27283002/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router_unittest.cc:77:
std::map<std::string, EventInfo> extension_id_to_event_;
with a map you lose ordering + repetition, right?

FWIW - it's a more severe change - but the idiom I like is what
extension_notification_observer.h uses (and a number of other test classes).
Perhaps you could set up something similar. It would clean up call sites too.

https://codereview.chromium.org/27283002/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router_unittest.cc:192:
scoped_refptr<const Extension> ext = CreateExtension(false);
for what it's worth: the scoped_refptr<> here (and CreateExtension) is
unnecessary because the ExtensionSet holds onto a reference. but it's not
hurting anybody.

[Michael Courage, 2013-10-16 20:24:52]

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/identity_event_router.cc (right):

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router.cc:28:
ExtensionSystem::Get(profile_)->extension_service();
On 2013/10/16 16:49:37, kalman wrote:
> ExtensionSystem::Get(profile_) being used twice. Save reference to it?

Done.

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router.cc:36:
account_info_email.email = scoped_ptr<std::string>(new std::string(email));
On 2013/10/16 16:49:37, kalman wrote:
> could you just have a single AccountInfo object and conditionally set the
> "email" field based on the HasAPIPermission method? Then you couldn't need the
> check on line 48 either.

Done.

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router.cc:41: 
On 2013/10/16 16:49:37, kalman wrote:
> nit if you want: "it != listeners.end(); ++it" (no line break) then no blank
> line here.

Done.

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router.cc:42: const
std::string extension_id = (*it)->extension_id;
On 2013/10/16 16:49:37, kalman wrote:
> const std::string& ?

Done.

https://codereview.chromium.org/27283002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router.cc:57: new
Event(api::identity::OnSignInChanged::kEventName, args.Pass()));
On 2013/10/16 16:49:37, kalman wrote:
> I think there's an event constructor that will let you assign
> "restrict_to_profile" immediately:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Done.

https://codereview.chromium.org/27283002/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/27283002/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:354: },
On 2013/10/16 16:49:37, kalman wrote:
> hm. I thought this was just going to be a separate permission (like outside
the
> identity API)?

That is what I originally proposed. 
 
> I thought of some other places where such a permission would be nice, for
> example, the feedback API might only include the email address of the user if
> the permission is requested. That's a bad example I think since it's not the
UI
> the feedback API would end up having (it'd probably just always show the email
> and have the user switch it off) but... there may be other places too?
> 
> Not sure about this point, but, wdyt?

I stuck it under identity because chrome.identity is becoming the home of
information about accounts on your profile, so I thought this seemed natural,
even though other APIs also make use of your account.

In the API space, developers very often think they should be doing something
with the Identity API whenever identity is involved, like when they are using
GCM for Chrome. Argument in favor of "identity.email" -> This seems to be where
developers look when they want to deal with identity. Argument against -> Maybe
I'm enhancing their confusion about the boundaries of the Identity API.

Anyway, if you're not super-convinced that "identity.email" is right after
reading this comment, I'll go ahead and change it to the originally proposed
"userEmail" since that's the string everyone else already saw & lgtm'd.

https://codereview.chromium.org/27283002/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/identity.idl (right):

https://codereview.chromium.org/27283002/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/identity.idl:44: DOMString? email;
On 2013/10/16 16:49:37, kalman wrote:
> documentation needed for these (and include "email only present when..." in
it).

Done.

https://codereview.chromium.org/27283002/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/identity_event_router_unittest.cc
(right):

https://codereview.chromium.org/27283002/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router_unittest.cc:77:
std::map<std::string, EventInfo> extension_id_to_event_;
On 2013/10/16 16:49:37, kalman wrote:
> with a map you lose ordering + repetition, right?

Line 60 forbids repetition of any kind. FakeEventRouter only handles one event.
Since the router only handles one event at a time, this seems adequate.

> FWIW - it's a more severe change - but the idiom I like is what
> extension_notification_observer.h uses (and a number of other test classes).
> Perhaps you could set up something similar. It would clean up call sites too.

That's a cool setup. I like it, but yeah, it's a large change. My next CL in
here is going to start firing the events, and at that point I will care about
sequences of events, and I'll copy the ExtensionNotificationObserver pattern.

https://codereview.chromium.org/27283002/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_event_router_unittest.cc:192:
scoped_refptr<const Extension> ext = CreateExtension(false);
On 2013/10/16 16:49:37, kalman wrote:
> for what it's worth: the scoped_refptr<> here (and CreateExtension) is
> unnecessary because the ExtensionSet holds onto a reference. but it's not
> hurting anybody.

Done.

[not at google - send to devlin, 2013-10-16 20:36:00]

lgtm

https://codereview.chromium.org/27283002/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_event_router.cc (right):

https://codereview.chromium.org/27283002/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_event_router.cc:55:
account_info.email.reset(info_email.get());
seems just as valid to use &email here and not have the info_email at all.

https://codereview.chromium.org/27283002/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_event_router_unittest.cc
(right):

https://codereview.chromium.org/27283002/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_event_router_unittest.cc:217:
typedef std::vector<const Extension* > ExtensionVector;
*> not * >

[not at google - send to devlin, 2013-10-16 20:36:22]

Oh, re "identity.email" - yes that string seems fine to me.

[Michael Courage, 2013-10-16 21:18:53]

https://codereview.chromium.org/27283002/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_event_router.cc (right):

https://codereview.chromium.org/27283002/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_event_router.cc:55:
account_info.email.reset(info_email.get());
On 2013/10/16 20:36:01, kalman wrote:
> seems just as valid to use &email here and not have the info_email at all.

Done.

https://codereview.chromium.org/27283002/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_event_router_unittest.cc
(right):

https://codereview.chromium.org/27283002/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_event_router_unittest.cc:217:
typedef std::vector<const Extension* > ExtensionVector;
On 2013/10/16 20:36:01, kalman wrote:
> *> not * >

Done.

[commit-bot: I haz the power, 2013-10-17 01:53:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/courage@chromium.org/27283002/36001

[commit-bot: I haz the power, 2013-10-17 01:53:43]

Failed to apply patch for chrome/common/extensions/permissions/api_permission.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: chrome/common/extensions/permissions/api_permission.h
  |diff --git a/chrome/common/extensions/permissions/api_permission.h
b/chrome/common/extensions/permissions/api_permission.h
  |index
6d7ddfd4c168d66fd49fe09b955569622f70826c..07dab6705c592e9da32d0dfc86cfc1a35fdfee15
100644
  |--- a/chrome/common/extensions/permissions/api_permission.h
  |+++ b/chrome/common/extensions/permissions/api_permission.h
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       chrome/common/extensions/permissions/api_permission.h
Index: chrome/common/extensions/permissions/api_permission.h
diff --git a/chrome/common/extensions/permissions/api_permission.h
b/chrome/common/extensions/permissions/api_permission.h
index
6d7ddfd4c168d66fd49fe09b955569622f70826c..07dab6705c592e9da32d0dfc86cfc1a35fdfee15
100644
--- a/chrome/common/extensions/permissions/api_permission.h
+++ b/chrome/common/extensions/permissions/api_permission.h
@@ -90,6 +90,7 @@ class APIPermission {
     kGeolocation,
     kHistory,
     kIdentity,
+    kIdentityEmail,
     kIdentityPrivate,
     kIdltest,
     kIdle,

[commit-bot: I haz the power, 2013-10-17 18:35:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/courage@chromium.org/27283002/42001

[commit-bot: I haz the power, 2013-10-17 22:07:25]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Michael Courage, 2013-10-18 02:13:59]

build fix

[commit-bot: I haz the power, 2013-10-18 02:23:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/courage@chromium.org/27283002/102001

[commit-bot: I haz the power, 2013-10-18 10:17:05]

Change committed as 229345