Move "oauth2" manifest key parsing out of Extension class.

Move "oauth2" manifest key parsing out of Extension class.

BUG=159265
TEST=browser_tests --gtest_filter=GetAuthTokenFunctionTest.*
TBR=ben@chromium.org

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

[SanjoyPal, 2013-01-14 19:47:30]

Please review.

[Devlin, 2013-01-14 20:18:34]

FYI, the CL with ManifestHandler::HasNoKey() is going through now (with luck,
this time we won't have conflicts). Once it does, make sure you can merge in
cleanly. Also, in future, when making CLs with pieces from others, it's usually
cleaner to not include those changes, since they are reviewed and submitted
separately (in order to not have them listed in the CL, just change your local
branch's upstream).

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_api.cc:39: }
since you're in the file, please add a "// namespace identity_constants" here
(it's short enough that it's not critical, but typically anything more than five
lines should have it).

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_api.cc:56: const
extensions::OAuth2Info* oauth2_info =
You're in the extensions namespace - you don't need the prefix.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_api.cc:261:
ManifestHandler::Register(extension_manifest_keys::kOAuth2,
Include what you use - c/c/e/manifest_handler.h.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_api.cc:269: g_factory =
LAZY_INSTANCE_INITIALIZER;
Indentation.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/identity/identity_api.h (left):

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_api.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
Update copyright (in all modified files).

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_apitest.cc:31: using
extensions::Extension;
This whole file should probably be in the extensions:: namespace.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_apitest.cc:161: new
extensions::OAuth2ManifestHandler);
Indentation.

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:8: #include
"base/string_number_conversions.h"
Where is this used?

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:13: #include
"ui/base/l10n/l10n_util.h"
Where is this used?

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:35:
DCHECK(info);
This should be a CHECK, not a DCHECK (since any access of the object will result
in a crash, it's better to know exactly where it was, even in a release build).

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:76:
scoped_ptr<OAuth2Info> info(new OAuth2Info);
This can be simplified to
extension->SetManifestData(keys::kOAuth2, new OAuth2Info);

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.h (right):

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.h:24: static const
OAuth2Info* GetOAuth2Info(const Extension* extension);
You should comment that this is never NULL.

It also makes me nervous that we crash if we try to access the OAuth2Info before
we parse the key (this wasn't a problem before, because we had an OAuth2 object,
not a pointer). I don't think it will be a problem currently, but if we keep
this somewhat-risky behavior, you should comment that trying to access it before
the key is parsed will result in a crash.

[Yoyo Zhou, 2013-01-15 01:59:56]

https://chromiumcodereview.appspot.com/11882025/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.h (right):

https://chromiumcodereview.appspot.com/11882025/diff/1/chrome/common/extensio...
chrome/common/extensions/api/identity/oauth2_manifest_handler.h:24: static const
OAuth2Info* GetOAuth2Info(const Extension* extension);
On 2013/01/14 20:18:34, D Cronin wrote:
> You should comment that this is never NULL.
> 
> It also makes me nervous that we crash if we try to access the OAuth2Info
before
> we parse the key (this wasn't a problem before, because we had an OAuth2
object,
> not a pointer). I don't think it will be a problem currently, but if we keep
> this somewhat-risky behavior, you should comment that trying to access it
before
> the key is parsed will result in a crash.

If it's never NULL, it's better to return a const reference than a pointer.

[SanjoyPal, 2013-01-16 18:58:55]

Move "oauth2" manifest key parsing out of Extension class. 

BUG=159265
TEST=browser_tests --gtest_filter=GetAuthTokenFunctionTest.*

[SanjoyPal, 2013-01-16 19:10:51]

Reabsed and fixed the comments. Please review.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_api.cc:39: }
On 2013/01/14 20:18:34, D Cronin wrote:
> since you're in the file, please add a "// namespace identity_constants" here
> (it's short enough that it's not critical, but typically anything more than
five
> lines should have it).

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_api.cc:56: const
extensions::OAuth2Info* oauth2_info =
On 2013/01/14 20:18:34, D Cronin wrote:
> You're in the extensions namespace - you don't need the prefix.

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_api.cc:261:
ManifestHandler::Register(extension_manifest_keys::kOAuth2,
On 2013/01/14 20:18:34, D Cronin wrote:
> Include what you use - c/c/e/manifest_handler.h.

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_api.cc:269: g_factory =
LAZY_INSTANCE_INITIALIZER;
On 2013/01/14 20:18:34, D Cronin wrote:
> Indentation.

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/identity/identity_api.h (left):

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_api.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2013/01/14 20:18:34, D Cronin wrote:
> Update copyright (in all modified files).

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_apitest.cc:161: new
extensions::OAuth2ManifestHandler);
On 2013/01/14 20:18:34, D Cronin wrote:
> Indentation.

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:8: #include
"base/string_number_conversions.h"
On 2013/01/14 20:18:34, D Cronin wrote:
> Where is this used?

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:13: #include
"ui/base/l10n/l10n_util.h"
On 2013/01/14 20:18:34, D Cronin wrote:
> Where is this used?

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:35:
DCHECK(info);
On 2013/01/14 20:18:34, D Cronin wrote:
> This should be a CHECK, not a DCHECK (since any access of the object will
result
> in a crash, it's better to know exactly where it was, even in a release
build).

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:76:
scoped_ptr<OAuth2Info> info(new OAuth2Info);
On 2013/01/14 20:18:34, D Cronin wrote:
> This can be simplified to
> extension->SetManifestData(keys::kOAuth2, new OAuth2Info);

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.h (right):

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.h:24: static const
OAuth2Info* GetOAuth2Info(const Extension* extension);
On 2013/01/15 01:59:56, Yoyo Zhou wrote:
> On 2013/01/14 20:18:34, D Cronin wrote:
> > You should comment that this is never NULL.
> > 
> > It also makes me nervous that we crash if we try to access the OAuth2Info
> before
> > we parse the key (this wasn't a problem before, because we had an OAuth2
> object,
> > not a pointer). I don't think it will be a problem currently, but if we keep
> > this somewhat-risky behavior, you should comment that trying to access it
> before
> > the key is parsed will result in a crash.
> 
> If it's never NULL, it's better to return a const reference than a pointer.

Done.

https://codereview.chromium.org/11882025/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.h:24: static const
OAuth2Info* GetOAuth2Info(const Extension* extension);
On 2013/01/14 20:18:34, D Cronin wrote:
> You should comment that this is never NULL.
> 
> It also makes me nervous that we crash if we try to access the OAuth2Info
before
> we parse the key (this wasn't a problem before, because we had an OAuth2
object,
> not a pointer). I don't think it will be a problem currently, but if we keep
> this somewhat-risky behavior, you should comment that trying to access it
before
> the key is parsed will result in a crash.

Done.

[Devlin, 2013-01-16 19:36:12]

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_apitest.cc:31: using
extensions::Extension;
On 2013/01/14 20:18:34, D Cronin wrote:
> This whole file should probably be in the extensions:: namespace.

I think you missed this one.

https://codereview.chromium.org/11882025/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/11882025/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:42: namespace
GetAuthToken = extensions::api::experimental_identity::GetAuthToken;
Remove all extensions:: prefixes, please.

https://codereview.chromium.org/11882025/diff/11001/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/11001/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:30: const
OAuth2Info& OAuth2Info::GetOAuth2Info(const Extension* extension) {
I interpreted Yoyo's comment as "if it's never going to be NULL under normal
circumstances (i.e., access after key-parse), have it return a const & which is
guaranteed to exist (avoiding ever crashing)." This would mean returning an
empty const & ('empty' here being a default-constructed OAuth2Info) in the cases
where we haven't parsed it.

Yoyo, is this correct?

[SanjoyPal, 2013-01-16 20:11:28]

Updated patch.

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/11882025/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/identity/identity_apitest.cc:31: using
extensions::Extension;
On 2013/01/16 19:36:12, D Cronin wrote:
> On 2013/01/14 20:18:34, D Cronin wrote:
> > This whole file should probably be in the extensions:: namespace.
> 
> I think you missed this one.

Done.

https://codereview.chromium.org/11882025/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/11882025/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.cc:42: namespace
GetAuthToken = extensions::api::experimental_identity::GetAuthToken;
On 2013/01/16 19:36:12, D Cronin wrote:
> Remove all extensions:: prefixes, please.

Done.

https://codereview.chromium.org/11882025/diff/11001/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/11001/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:30: const
OAuth2Info& OAuth2Info::GetOAuth2Info(const Extension* extension) {
Do you mean, we should return a default-constructed OAuth2Info instead of a
CHECK() ?
// static
const OAuth2Info& OAuth2Info::GetOAuth2Info(const Extension* extension) {
  OAuth2Info* info = static_cast<OAuth2Info*>(
      extension->GetManifestData(keys::kOAuth2));
  return info ? *info : *scoped_ptr<OAuth2Info>(new OAuth2Info).get();
}


On 2013/01/16 19:36:12, D Cronin wrote:
> I interpreted Yoyo's comment as "if it's never going to be NULL under normal
> circumstances (i.e., access after key-parse), have it return a const & which
is
> guaranteed to exist (avoiding ever crashing)." This would mean returning an
> empty const & ('empty' here being a default-constructed OAuth2Info) in the
cases
> where we haven't parsed it.
> 
> Yoyo, is this correct?

[Devlin, 2013-01-16 20:26:39]

https://codereview.chromium.org/11882025/diff/11001/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/11001/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:30: const
OAuth2Info& OAuth2Info::GetOAuth2Info(const Extension* extension) {
Yes and no. If we return a const &, it should always be valid, and if the
extension doesn't have one (because the key hasn't been parsed yet), then it
should return a default-constructed OAuth2Info. That part is correct. :)

But the code in your comment would break - it's returning a const & to an object
which is destructed at the end of the method call, the moment the scoped_ptr
goes out of scope.  And creating an object which will not be deleted at the end
of the function won't work, because it'll leak.  Instead, use a LazyInstance or
a Singleton to hold a single default-constructed OAuth2Info, and return the
reference to that when needed.

On 2013/01/16 20:11:28, SanjoyPal wrote:
> Do you mean, we should return a default-constructed OAuth2Info instead of a
> CHECK() ?
> // static
> const OAuth2Info& OAuth2Info::GetOAuth2Info(const Extension* extension) {
>   OAuth2Info* info = static_cast<OAuth2Info*>(
>       extension->GetManifestData(keys::kOAuth2));
>   return info ? *info : *scoped_ptr<OAuth2Info>(new OAuth2Info).get();
> }
> 
> 
> On 2013/01/16 19:36:12, D Cronin wrote:
> > I interpreted Yoyo's comment as "if it's never going to be NULL under normal
> > circumstances (i.e., access after key-parse), have it return a const & which
> is
> > guaranteed to exist (avoiding ever crashing)." This would mean returning an
> > empty const & ('empty' here being a default-constructed OAuth2Info) in the
> cases
> > where we haven't parsed it.
> > 
> > Yoyo, is this correct?
>

https://codereview.chromium.org/11882025/diff/16002/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/11882025/diff/16002/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:31: using
extensions::Extension;
These using statements can be removed now (as can any other extensions::
prefix).

[Yoyo Zhou, 2013-01-16 21:06:12]

https://codereview.chromium.org/11882025/diff/11001/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/11001/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:30: const
OAuth2Info& OAuth2Info::GetOAuth2Info(const Extension* extension) {
On 2013/01/16 20:26:39, D Cronin wrote:
> Yes and no. If we return a const &, it should always be valid, and if the
> extension doesn't have one (because the key hasn't been parsed yet), then it
> should return a default-constructed OAuth2Info. That part is correct. :)
> 
> But the code in your comment would break - it's returning a const & to an
object
> which is destructed at the end of the method call, the moment the scoped_ptr
> goes out of scope.  And creating an object which will not be deleted at the
end
> of the function won't work, because it'll leak.  Instead, use a LazyInstance
or
> a Singleton to hold a single default-constructed OAuth2Info, and return the
> reference to that when needed.

Yes, that's what I meant. You should have an empty OAuth2Info lazy instance that
you return a reference to if the extension doesn't have any OAuth2-associated
data.

[SanjoyPal, 2013-01-16 23:03:13]

Done.

https://codereview.chromium.org/11882025/diff/11001/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/11001/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:30: const
OAuth2Info& OAuth2Info::GetOAuth2Info(const Extension* extension) {
On 2013/01/16 21:06:12, Yoyo Zhou wrote:
> On 2013/01/16 20:26:39, D Cronin wrote:
> > Yes and no. If we return a const &, it should always be valid, and if the
> > extension doesn't have one (because the key hasn't been parsed yet), then it
> > should return a default-constructed OAuth2Info. That part is correct. :)
> > 
> > But the code in your comment would break - it's returning a const & to an
> object
> > which is destructed at the end of the method call, the moment the scoped_ptr
> > goes out of scope.  And creating an object which will not be deleted at the
> end
> > of the function won't work, because it'll leak.  Instead, use a LazyInstance
> or
> > a Singleton to hold a single default-constructed OAuth2Info, and return the
> > reference to that when needed.
> 
> Yes, that's what I meant. You should have an empty OAuth2Info lazy instance
that
> you return a reference to if the extension doesn't have any OAuth2-associated
> data.

Done.

https://codereview.chromium.org/11882025/diff/16002/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/11882025/diff/16002/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:31: using
extensions::Extension;
On 2013/01/16 20:26:40, D Cronin wrote:
> These using statements can be removed now (as can any other extensions::
> prefix).

Done.

[Devlin, 2013-01-16 23:25:40]

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_apitest.cc (left):

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:38: namespace {
Why remove this?

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:155:
ManifestHandler::Register(extension_manifest_keys::kOAuth2,
This registration should probably be done in a SetUp() function, instead of
here.

https://codereview.chromium.org/11882025/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/25001/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:76: bool
OAuth2ManifestHandler::HasNoKey(Extension* extension, string16* error) {
This is no longer needed (if the extension doesn't have a populated OAuth2Info,
it'll return an empty one in GetOAuth2Info(), so there's no point in setting
one).

[SanjoyPal, 2013-01-17 00:11:13]

Updated.

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_apitest.cc (left):

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:38: namespace {
On 2013/01/16 23:25:40, D Cronin wrote:
> Why remove this?
which one?

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:155:
ManifestHandler::Register(extension_manifest_keys::kOAuth2,
On 2013/01/16 23:25:40, D Cronin wrote:
> This registration should probably be done in a SetUp() function, instead of
> here.

Done.

https://codereview.chromium.org/11882025/diff/25001/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/25001/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:76: bool
OAuth2ManifestHandler::HasNoKey(Extension* extension, string16* error) {
On 2013/01/16 23:25:40, D Cronin wrote:
> This is no longer needed (if the extension doesn't have a populated
OAuth2Info,
> it'll return an empty one in GetOAuth2Info(), so there's no point in setting
> one).

Done.

[Devlin, 2013-01-17 10:04:59]

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_apitest.cc (left):

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:38: namespace {
On 2013/01/17 00:11:13, SanjoyPal wrote:
> On 2013/01/16 23:25:40, D Cronin wrote:
> > Why remove this?
> which one?

The anonymous namespace. It won't break anything to take it out; I'm just
curious why it was removed. It seems like it was a fairly standard/logical place
to use one.

https://codereview.chromium.org/11882025/diff/20002/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/11882025/diff/20002/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:144: virtual void
SetUp() {
virtual void SetUp() OVERRIDE {

https://codereview.chromium.org/11882025/diff/20002/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:145:
InProcessBrowserTest::SetUp();
Call the immediate super function (which should, in turn, call its own super's).
I.e., use ExtensionBrowserTest::SetUp().

[SanjoyPal, 2013-01-17 19:13:43]

Fixed as per the comments. PTAL. Thanks.

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_apitest.cc (left):

https://codereview.chromium.org/11882025/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:38: namespace {
On 2013/01/17 10:04:59, D Cronin wrote:
> On 2013/01/17 00:11:13, SanjoyPal wrote:
> > On 2013/01/16 23:25:40, D Cronin wrote:
> > > Why remove this?
> > which one?
> 
> The anonymous namespace. It won't break anything to take it out; I'm just
> curious why it was removed. It seems like it was a fairly standard/logical
place
> to use one.

Done.

https://codereview.chromium.org/11882025/diff/20002/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/11882025/diff/20002/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:144: virtual void
SetUp() {
On 2013/01/17 10:04:59, D Cronin wrote:
> virtual void SetUp() OVERRIDE {

Done.

https://codereview.chromium.org/11882025/diff/20002/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_apitest.cc:145:
InProcessBrowserTest::SetUp();
On 2013/01/17 10:04:59, D Cronin wrote:
> Call the immediate super function (which should, in turn, call its own
super's).
> I.e., use ExtensionBrowserTest::SetUp().

Done.

[Devlin, 2013-01-17 22:53:05]

lgtm with nit.

https://codereview.chromium.org/11882025/diff/14003/chrome/browser/extensions...
File chrome/browser/extensions/permissions_updater.cc (right):

https://codereview.chromium.org/11882025/diff/14003/chrome/browser/extensions...
chrome/browser/extensions/permissions_updater.cc:54:
extensions::OAuth2Info::GetOAuth2Info(extension).client_id,
nit: don't need to have extensions:: prefix in this file.

[SanjoyPal, 2013-01-18 02:08:49]

updated.

https://codereview.chromium.org/11882025/diff/14003/chrome/browser/extensions...
File chrome/browser/extensions/permissions_updater.cc (right):

https://codereview.chromium.org/11882025/diff/14003/chrome/browser/extensions...
chrome/browser/extensions/permissions_updater.cc:54:
extensions::OAuth2Info::GetOAuth2Info(extension).client_id,
On 2013/01/17 22:53:05, D Cronin wrote:
> nit: don't need to have extensions:: prefix in this file.

Done.

[SanjoyPal, 2013-01-18 02:24:53]

+miranda for profiles

[Miranda Callahan, 2013-01-18 13:56:36]

On 2013/01/18 02:24:53, SanjoyPal wrote:
> +miranda for profiles

profiles LGTM -- assuming that patch set 6 is the same as 5 (I see "chunk error"
instead of anything reasonable when I try to view the profile_dependency_manager
file in 6).

[sail, 2013-01-18 16:17:00]

https://codereview.chromium.org/11882025/diff/31001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://codereview.chromium.org/11882025/diff/31001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
don't update the copyright year on files you modify

[commit-bot: I haz the power, 2013-01-18 19:51:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/11882025/38002

[commit-bot: I haz the power, 2013-01-18 19:52:00]

Failed to apply patch for chrome/browser/profiles/profile_dependency_manager.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/profiles/profile_dependency_manager.cc
  Hunk #1 FAILED at 23.
  Hunk #2 FAILED at 259.
  2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/profiles/profile_dependency_manager.cc.rej

Patch:       chrome/browser/profiles/profile_dependency_manager.cc
Index: chrome/browser/profiles/profile_dependency_manager.cc
===================================================================
--- chrome/browser/profiles/profile_dependency_manager.cc	(revision 177049)
+++ chrome/browser/profiles/profile_dependency_manager.cc	(working copy)
@@ -23,6 +23,7 @@
 #include "chrome/browser/extensions/api/file_handlers/file_handlers_api.h"
 #include
"chrome/browser/extensions/api/font_settings/font_settings_api_factory.h"
 #include "chrome/browser/extensions/api/history/history_api_factory.h"
+#include "chrome/browser/extensions/api/identity/identity_api.h"
 #include "chrome/browser/extensions/api/idle/idle_manager_factory.h"
 #include "chrome/browser/extensions/api/managed_mode/managed_mode_api.h"
 #include "chrome/browser/extensions/api/management/management_api.h"
@@ -259,6 +260,7 @@
   extensions::FileHandlersAPI::GetFactoryInstance();
   extensions::FontSettingsAPIFactory::GetInstance();
   extensions::HistoryAPIFactory::GetInstance();
+  extensions::IdentityAPI::GetFactoryInstance();
   extensions::IdleManagerFactory::GetInstance();
 #if defined(OS_CHROMEOS)
   extensions::InputImeAPI::GetFactoryInstance();

[sail, 2013-01-18 20:07:44]

On 2013/01/18 19:51:55, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/ncj674%40motorola.com/11882025/38002

Um.. please wait for LGTM from reviewers before committing.

[Devlin, 2013-01-18 20:48:22]

On 2013/01/18 16:17:00, sail wrote:
>
https://codereview.chromium.org/11882025/diff/31001/chrome/browser/extensions...
> File chrome/browser/extensions/api/identity/identity_api.h (right):
> 
>
https://codereview.chromium.org/11882025/diff/31001/chrome/browser/extensions...
> chrome/browser/extensions/api/identity/identity_api.h:1: // Copyright (c) 2013
> The Chromium Authors. All rights reserved.
> don't update the copyright year on files you modify

This one's my fault (I asked him to update the copyrights) - sorry about that. I
forgot about the new policy.

[Yoyo Zhou, 2013-01-23 17:50:24]

I have a few nits.

https://chromiumcodereview.appspot.com/11882025/diff/38002/chrome/browser/ext...
File chrome/browser/extensions/extension_install_prompt.h (right):

https://chromiumcodereview.appspot.com/11882025/diff/38002/chrome/browser/ext...
chrome/browser/extensions/extension_install_prompt.h:43: class
MockGetAuthTokenFunction;
Alphabetical order

https://chromiumcodereview.appspot.com/11882025/diff/38002/chrome/common/exte...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://chromiumcodereview.appspot.com/11882025/diff/38002/chrome/common/exte...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:20: const char
kClientId[] = "client_id";
If we don't refer to the old constants kOAuth2ClientId and kOAuth2Scopes, can we
delete them?

https://chromiumcodereview.appspot.com/11882025/diff/38002/chrome/common/exte...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.h (right):

https://chromiumcodereview.appspot.com/11882025/diff/38002/chrome/common/exte...
chrome/common/extensions/api/identity/oauth2_manifest_handler.h:25: // Trying to
access the info before the key is parsed will result in a crash.
This is no longer true.

[SanjoyPal, 2013-01-23 20:11:29]

Fixed the review comments. Please have a look.

[Yoyo Zhou, 2013-01-23 20:15:13]

LGTM

[SanjoyPal, 2013-01-23 20:19:37]

On 2013/01/23 20:11:29, SanjoyPal wrote:
> Fixed the review comments. Please have a look.

sail, Can you please review again? Thanks.

[sail, 2013-01-23 20:23:15]

https://codereview.chromium.org/11882025/diff/56001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11882025/diff/56001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
same

https://codereview.chromium.org/11882025/diff/56001/chrome/common/extensions/...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/11882025/diff/56001/chrome/common/extensions/...
chrome/common/extensions/extension.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
is this intentional?

[SanjoyPal, 2013-01-23 20:35:41]

Rebased the patch. Please review one more time. Thanks

https://codereview.chromium.org/11882025/diff/56001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11882025/diff/56001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/01/23 20:23:15, sail wrote:
> same

Done.

https://codereview.chromium.org/11882025/diff/56001/chrome/common/extensions/...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/11882025/diff/56001/chrome/common/extensions/...
chrome/common/extensions/extension.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/01/23 20:23:15, sail wrote:
> is this intentional?

Done. It was not intentional.

[sail, 2013-01-24 16:22:54]

LGTM

https://codereview.chromium.org/11882025/diff/48007/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/48007/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:47: Extension*
extension, string16* error) {
one line per argument

https://codereview.chromium.org/11882025/diff/48007/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.h (right):

https://codereview.chromium.org/11882025/diff/48007/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.h:20:
~OAuth2Info();
virtual

https://codereview.chromium.org/11882025/diff/48007/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.h:36: };
private: DISALLOW_COPY_AND_ASSIGN()

[SanjoyPal, 2013-01-25 00:37:52]

Updated.

https://codereview.chromium.org/11882025/diff/48007/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.cc (right):

https://codereview.chromium.org/11882025/diff/48007/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.cc:47: Extension*
extension, string16* error) {
On 2013/01/24 16:22:54, sail wrote:
> one line per argument

Done.

https://codereview.chromium.org/11882025/diff/48007/chrome/common/extensions/...
File chrome/common/extensions/api/identity/oauth2_manifest_handler.h (right):

https://codereview.chromium.org/11882025/diff/48007/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.h:20:
~OAuth2Info();
On 2013/01/24 16:22:54, sail wrote:
> virtual

Done.

https://codereview.chromium.org/11882025/diff/48007/chrome/common/extensions/...
chrome/common/extensions/api/identity/oauth2_manifest_handler.h:36: };
On 2013/01/24 16:22:54, sail wrote:
> private: DISALLOW_COPY_AND_ASSIGN()

Done.

[commit-bot: I haz the power, 2013-01-25 00:40:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/11882025/50026

[SanjoyPal, 2013-01-25 19:06:48]

Hi yoyo, Fixed the unit_tests. Can you please review the changes one more time.
Thanks

[Yoyo Zhou, 2013-01-25 19:17:43]

LGTM

[commit-bot: I haz the power, 2013-01-25 19:25:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/11882025/60004

[commit-bot: I haz the power, 2013-01-25 19:25:22]

Failed to apply patch for
chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  A         chrome/common/extensions/api/identity
  Created missing directory chrome/common/extensions/api/identity.
  patching file
chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc
  Hunk #1 FAILED at 5.
  Hunk #2 FAILED at 32.
  Hunk #3 FAILED at 48.
  Hunk #4 FAILED at 69.
  4 out of 4 hunks FAILED -- saving rejects to file
chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc.rej

Patch:   N  
chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc
Index:
chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc
===================================================================
---
chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc	(revision
0)
+++
chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc	(working
copy)
@@ -5,11 +5,22 @@
 #include "chrome/common/extensions/manifest_tests/extension_manifest_test.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
+#include "chrome/common/extensions/api/identity/oauth2_manifest_handler.h"
 #include "chrome/common/extensions/extension_manifest_constants.h"
 
 namespace keys = extension_manifest_keys;
 
-TEST_F(ExtensionManifestTest, OAuth2SectionParsing) {
+namespace extensions {
+
+class OAuth2ManifestTest : public ExtensionManifestTest {
+ protected:
+  virtual void SetUp() OVERRIDE {
+    ManifestHandler::Register(extension_manifest_keys::kOAuth2,
+                              new OAuth2ManifestHandler);
+  }
+};
+
+TEST_F(OAuth2ManifestTest, OAuth2SectionParsing) {
   DictionaryValue base_manifest;
 
   base_manifest.SetString(keys::kName, "test");
@@ -32,10 +43,10 @@
     scoped_refptr<extensions::Extension> extension =
         LoadAndExpectSuccess(manifest);
     EXPECT_TRUE(extension->install_warnings().empty());
-    EXPECT_EQ("client1", extension->oauth2_info().client_id);
-    EXPECT_EQ(2U, extension->oauth2_info().scopes.size());
-    EXPECT_EQ("scope1", extension->oauth2_info().scopes[0]);
-    EXPECT_EQ("scope2", extension->oauth2_info().scopes[1]);
+    EXPECT_EQ("client1", OAuth2Info::GetOAuth2Info(extension).client_id);
+    EXPECT_EQ(2U, OAuth2Info::GetOAuth2Info(extension).scopes.size());
+    EXPECT_EQ("scope1", OAuth2Info::GetOAuth2Info(extension).scopes[0]);
+    EXPECT_EQ("scope2", OAuth2Info::GetOAuth2Info(extension).scopes[1]);
   }
 
   // OAuth2 section should be parsed for a packaged app.
@@ -48,10 +59,10 @@
     scoped_refptr<extensions::Extension> extension =
         LoadAndExpectSuccess(manifest);
     EXPECT_TRUE(extension->install_warnings().empty());
-    EXPECT_EQ("client1", extension->oauth2_info().client_id);
-    EXPECT_EQ(2U, extension->oauth2_info().scopes.size());
-    EXPECT_EQ("scope1", extension->oauth2_info().scopes[0]);
-    EXPECT_EQ("scope2", extension->oauth2_info().scopes[1]);
+    EXPECT_EQ("client1", OAuth2Info::GetOAuth2Info(extension).client_id);
+    EXPECT_EQ(2U, OAuth2Info::GetOAuth2Info(extension).scopes.size());
+    EXPECT_EQ("scope1", OAuth2Info::GetOAuth2Info(extension).scopes[0]);
+    EXPECT_EQ("scope2", OAuth2Info::GetOAuth2Info(extension).scopes[1]);
   }
 
   // OAuth2 section should NOT be parsed for a hosted app.
@@ -69,7 +80,9 @@
     EXPECT_EQ("'oauth2' is only allowed for extensions, legacy packaged apps "
                   "and packaged apps, and this is a hosted app.",
               warning.message);
-    EXPECT_EQ("", extension->oauth2_info().client_id);
-    EXPECT_TRUE(extension->oauth2_info().scopes.empty());
+    EXPECT_EQ("", OAuth2Info::GetOAuth2Info(extension).client_id);
+    EXPECT_TRUE(OAuth2Info::GetOAuth2Info(extension).scopes.empty());
   }
 }
+
+}  // namespace extensions

[commit-bot: I haz the power, 2013-01-26 00:07:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/11882025/74001

[commit-bot: I haz the power, 2013-01-26 00:59:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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.

[commit-bot: I haz the power, 2013-01-27 06:51:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/11882025/74001

[commit-bot: I haz the power, 2013-01-27 07:09:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
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.

[commit-bot: I haz the power, 2013-01-29 23:09:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/11882025/74001

[commit-bot: I haz the power, 2013-01-30 00:13:30]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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.

[commit-bot: I haz the power, 2013-01-31 21:01:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/11882025/85001

[commit-bot: I haz the power, 2013-01-31 21:01:33]

Presubmit check for 11882025-85001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
     
chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc:8

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.8s to calculate.

[commit-bot: I haz the power, 2013-01-31 21:49:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/11882025/85001

[commit-bot: I haz the power, 2013-01-31 22:25:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncj674@motorola.com/11882025/94003

[commit-bot: I haz the power, 2013-02-01 04:43:01]

Change committed as 180069