Implementation of the DeviceInfo get API.

Implementation of DeviceInfo API (get). The API is not yet exposed the dev channel(the permission and manifest entries are missing)

Nicolas - would you mind reviewing the device_info.cc and pss_mock changes. In DeviceInfo the changes are to make the tovalue method consistent with the API docs. However there still remains some work particularly for ios devices which are marked as todo and if you are OK with it we will do it as a seperate review.

Matt - Please review the rest.
BUG=170375
R=mpcomplete@chromium.org, zea@chromium.org

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

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

[Nicolas Zea, 2013-08-10 00:49:20]

sync lgtm

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.cc (right):

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:72: // TODO(lipalani): Add support for
ios phones and tablets.
nit: mention the bug tracking this work as well.

[Matt Perry, 2013-08-12 22:21:01]

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/api/signedin_devices/signedin_devices_api.cc
(right):

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/signedin_devices/signedin_devices_api.cc:96: if
(is_local == true) {
don't compare to true/false. just if (is_local)

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/signedin_devices/signedin_devices_api_unittest.cc
(right):

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/signedin_devices/signedin_devices_api_unittest.cc:182:
(ProfileSyncServiceMock*)
don't use C-style casts

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.cc (right):

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:191: value->SetString("OS",
GetOS(device_type_));
lowercase "os" (notice "id" is lowercase)

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
File chrome/common/extensions/api/signedin_devices.idl (right):

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Add an entry to chrome/extensions/docs/templates/public/extensions for this to
show up in the public docs.

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:8: namespace signedinDevices {
Is "signed in" one word or two? I think two. So this should be singedInDevices
(and likewise everywhere - the permission, the namespace, the code directory).
Ditto for hacker style: signed_in_devices.

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:31: // Unique Id for this
device. Note: The id is meaningul only
meaningful*

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:54: // |callback| The callback
to be invoked with the array of DeviceInfo
use consistent punctuation. single : after the |variableName|

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:60: // Fired If the DeviceInfo
object of any of the signed in devices
lowercase if*

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:62: // |callback|-The callback
to be invoked with the array of DeviceInfo
same here

[lipalani1, 2013-08-13 22:42:37]

Matt - PTAL

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/api/signedin_devices/signedin_devices_api.cc
(right):

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/signedin_devices/signedin_devices_api.cc:96: if
(is_local == true) {
On 2013/08/12 22:21:01, Matt Perry wrote:
> don't compare to true/false. just if (is_local)

Done.

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/signedin_devices/signedin_devices_api_unittest.cc
(right):

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/signedin_devices/signedin_devices_api_unittest.cc:182:
(ProfileSyncServiceMock*)
On 2013/08/12 22:21:01, Matt Perry wrote:
> don't use C-style casts

Done.

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.cc (right):

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:72: // TODO(lipalani): Add support for
ios phones and tablets.
On 2013/08/10 00:49:20, Nicolas Zea wrote:
> nit: mention the bug tracking this work as well.

Done.

https://codereview.chromium.org/22706006/diff/5001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:191: value->SetString("OS",
GetOS(device_type_));
On 2013/08/12 22:21:01, Matt Perry wrote:
> lowercase "os" (notice "id" is lowercase)

Done.

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
File chrome/common/extensions/api/signedin_devices.idl (right):

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Done. Not sure if I have done it right! But it follows the same pattern as
alarms. When I try to upload I get this error:
".Rendering extensions/signedin_devices.html...
<Internal>(5) : Unexpected EoF reached after
^
'canning error. Illegal character ' "
On 2013/08/12 22:21:01, Matt Perry wrote:
> Add an entry to chrome/extensions/docs/templates/public/extensions for this to
> show up in the public docs.

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:8: namespace signedinDevices {
I decided on one word after looking at the word Signin in chromium code base
which has a wide usage. Let me know what you think.
(I have a couple of helper functions from previous check ins that used "signed
in"(2 words), but I plan to fix that in a future commit. Rest of the code is
consistently using the single word "Signedin".)
On 2013/08/12 22:21:01, Matt Perry wrote:
> Is "signed in" one word or two? I think two. So this should be singedInDevices
> (and likewise everywhere - the permission, the namespace, the code directory).
> Ditto for hacker style: signed_in_devices.

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:31: // Unique Id for this
device. Note: The id is meaningul only
On 2013/08/12 22:21:01, Matt Perry wrote:
> meaningful*

Done.

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:54: // |callback| The callback
to be invoked with the array of DeviceInfo
On 2013/08/12 22:21:01, Matt Perry wrote:
> use consistent punctuation. single : after the |variableName|

Done.

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:60: // Fired If the DeviceInfo
object of any of the signed in devices
On 2013/08/12 22:21:01, Matt Perry wrote:
> lowercase if*

Done.

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:62: // |callback|-The callback
to be invoked with the array of DeviceInfo
On 2013/08/12 22:21:01, Matt Perry wrote:
> same here

Done.

[Matt Perry, 2013-08-13 23:01:38]

https://codereview.chromium.org/22706006/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/api/signedin_devices/signedin_devices_api.cc
(right):

https://codereview.chromium.org/22706006/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/api/signedin_devices/signedin_devices_api.cc:51:
ScopedVector<DeviceInfo> GetAllSignedInDevices(
this file uses 2 words for "signed in"

https://codereview.chromium.org/22706006/diff/29001/chrome/browser/extensions...
File
chrome/browser/extensions/api/signedin_devices/signedin_devices_api_unittest.cc
(right):

https://codereview.chromium.org/22706006/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/api/signedin_devices/signedin_devices_api_unittest.cc:182:
reinterpret_cast<ProfileSyncServiceMock*>(
use static_cast for casting between members of a class hierarchy

[lipalani1, 2013-08-13 23:36:16]

PTAL.

https://codereview.chromium.org/22706006/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/api/signedin_devices/signedin_devices_api.cc
(right):

https://codereview.chromium.org/22706006/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/api/signedin_devices/signedin_devices_api.cc:51:
ScopedVector<DeviceInfo> GetAllSignedInDevices(
On 2013/08/13 23:01:38, Matt Perry wrote:
> this file uses 2 words for "signed in"

Done.

https://codereview.chromium.org/22706006/diff/29001/chrome/browser/extensions...
File
chrome/browser/extensions/api/signedin_devices/signedin_devices_api_unittest.cc
(right):

https://codereview.chromium.org/22706006/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/api/signedin_devices/signedin_devices_api_unittest.cc:182:
reinterpret_cast<ProfileSyncServiceMock*>(
On 2013/08/13 23:01:38, Matt Perry wrote:
> use static_cast for casting between members of a class hierarchy

Done.

[Matt Perry, 2013-08-14 00:03:38]

lgtm

[lipalani, 2013-08-14 19:38:58]

Matt - Ever since I added
chrome/extensions/docs/**templates/public/extensions/signedin_devices.html,
 When I try to upload(using git-cl upload) I get this error:
".Rendering extensions/signedin_devices.html...
<Internal>(5) : Unexpected EoF reached after
^
'canning error. Illegal character ' "

I tried following the same pattern as in alarms. Do you know what is
happening? (I have not made any local changes that are not in this
review)


Thanks!



On Tue, Aug 13, 2013 at 5:03 PM, <mpcomplete@chromium.org> wrote:

> lgtm
>
>
>
>
https://codereview.chromium.**org/22706006/<https://codereview.chromium.org/2...
>

[Matt Perry, 2013-08-14 19:48:00]

Not sure what's happening. How about you check in the .html file in a followup
CL?

On 2013/08/14 19:38:58, lipalani wrote:
> Matt - Ever since I added
> chrome/extensions/docs/**templates/public/extensions/signedin_devices.html,
>  When I try to upload(using git-cl upload) I get this error:
> ".Rendering extensions/signedin_devices.html...
> <Internal>(5) : Unexpected EoF reached after
> ^
> 'canning error. Illegal character ' "
> 
> I tried following the same pattern as in alarms. Do you know what is
> happening? (I have not made any local changes that are not in this
> review)
> 
> 
> Thanks!
> 
> 
> 
> On Tue, Aug 13, 2013 at 5:03 PM, <mailto:mpcomplete@chromium.org> wrote:
> 
> > lgtm
> >
> >
> >
> >
>
https://codereview.chromium.**org/22706006/%3Chttps://codereview.chromium.org...>
> >

[lipalani, 2013-08-14 20:36:04]

sure i can do that.


On Wed, Aug 14, 2013 at 12:48 PM, <mpcomplete@chromium.org> wrote:

> Not sure what's happening. How about you check in the .html file in a
> followup
> CL?
>
>
> On 2013/08/14 19:38:58, lipalani wrote:
>
>> Matt - Ever since I added
>> chrome/extensions/docs/****templates/public/extensions/**
>> signedin_devices.html,
>>
>>   When I try to upload(using git-cl upload) I get this error:
>> ".Rendering extensions/signedin_devices.**html...
>> <Internal>(5) : Unexpected EoF reached after
>> ^
>> 'canning error. Illegal character ' "
>>
>
>  I tried following the same pattern as in alarms. Do you know what is
>> happening? (I have not made any local changes that are not in this
>> review)
>>
>
>
>  Thanks!
>>
>
>
>
>  On Tue, Aug 13, 2013 at 5:03 PM,
<mailto:mpcomplete@chromium.**org<mpcomplete@chromium.org>>
>> wrote:
>>
>
>  > lgtm
>> >
>> >
>> >
>> >
>>
>
> https://codereview.chromium.****org/22706006/%3Chttps://codere**
> view.chromium.org/22706006/ <http://codereview.chromium.org/22706006/>>
>
>> >
>>
>
>
>
>
https://codereview.chromium.**org/22706006/<https://codereview.chromium.org/2...
>

[dharcourt, 2013-08-15 22:12:04]

If there's any chance to do that, I would highly recommend changing
signedinDevices to signedInDevices as developers using our APIs should be able
to depend on regular camel casing rules and shouldn't need to know the
implementation conventions of our code base.

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
File chrome/common/extensions/api/signedin_devices.idl (right):

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:8: namespace signedinDevices {
On 2013/08/13 22:42:37, lipalani1 wrote:
> I decided on one word after looking at the word Signin in chromium code base
> which has a wide usage. Let me know what you think.
> (I have a couple of helper functions from previous check ins that used "signed
> in"(2 words), but I plan to fix that in a future commit. Rest of the code is
> consistently using the single word "Signedin".)
> On 2013/08/12 22:21:01, Matt Perry wrote:
> > Is "signed in" one word or two? I think two. So this should be
singedInDevices
> > (and likewise everywhere - the permission, the namespace, the code
directory).
> > Ditto for hacker style: signed_in_devices.
> 

Came across this in passing: Since API callers won't be familiar with the
Chromium code base, it doesn't seem that code base usage should be a determining
factor in API choice... do we have any public APIs with "signedin" as a single
word?

If not, it seems we should follow english language conventions, and I can't find
any dictionary with "signedin" or "signin" as a single word (in fact we have it
as two word on https://accounts.google.com/Login).

Developers should be able to expect to camel case at word boundaries. If they
can't, we're making life harder for them. Internal implementation details and
code conventions don't seem like a good reason to do that.

[Matt Perry, 2013-08-15 22:25:19]

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
File chrome/common/extensions/api/signedin_devices.idl (right):

https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/signedin_devices.idl:8: namespace signedinDevices {
On 2013/08/15 22:12:04, dharcourt wrote:
> On 2013/08/13 22:42:37, lipalani1 wrote:
> > I decided on one word after looking at the word Signin in chromium code base
> > which has a wide usage. Let me know what you think.
> > (I have a couple of helper functions from previous check ins that used
"signed
> > in"(2 words), but I plan to fix that in a future commit. Rest of the code is
> > consistently using the single word "Signedin".)
> > On 2013/08/12 22:21:01, Matt Perry wrote:
> > > Is "signed in" one word or two? I think two. So this should be
> singedInDevices
> > > (and likewise everywhere - the permission, the namespace, the code
> directory).
> > > Ditto for hacker style: signed_in_devices.
> > 
> 
> Came across this in passing: Since API callers won't be familiar with the
> Chromium code base, it doesn't seem that code base usage should be a
determining
> factor in API choice... do we have any public APIs with "signedin" as a single
> word?
> 
> If not, it seems we should follow english language conventions, and I can't
find
> any dictionary with "signedin" or "signin" as a single word (in fact we have
it
> as two word on https://accounts.google.com/Login).
> 
> Developers should be able to expect to camel case at word boundaries. If they
> can't, we're making life harder for them. Internal implementation details and
> code conventions don't seem like a good reason to do that.

Yeah, I agree. Sorry I didn't push back more. The API really should be
signedInDevices (capital I). But that will also affect the C++ code - extension
API file, class, and directory names.

[commit-bot: I haz the power, 2013-08-15 22:38:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/22706006/69001

[lipalani1, 2013-08-15 22:40:29]

On 2013/08/15 22:25:19, Matt Perry wrote:
>
https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
> File chrome/common/extensions/api/signedin_devices.idl (right):
> 
>
https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
> chrome/common/extensions/api/signedin_devices.idl:8: namespace signedinDevices
{
> On 2013/08/15 22:12:04, dharcourt wrote:
> > On 2013/08/13 22:42:37, lipalani1 wrote:
> > > I decided on one word after looking at the word Signin in chromium code
base
> > > which has a wide usage. Let me know what you think.
> > > (I have a couple of helper functions from previous check ins that used
> "signed
> > > in"(2 words), but I plan to fix that in a future commit. Rest of the code
is
> > > consistently using the single word "Signedin".)
> > > On 2013/08/12 22:21:01, Matt Perry wrote:
> > > > Is "signed in" one word or two? I think two. So this should be
> > singedInDevices
> > > > (and likewise everywhere - the permission, the namespace, the code
> > directory).
> > > > Ditto for hacker style: signed_in_devices.
> > > 
> > 
> > Came across this in passing: Since API callers won't be familiar with the
> > Chromium code base, it doesn't seem that code base usage should be a
> determining
> > factor in API choice... do we have any public APIs with "signedin" as a
single
> > word?
> > 
> > If not, it seems we should follow english language conventions, and I can't
> find
> > any dictionary with "signedin" or "signin" as a single word (in fact we have
> it
> > as two word on https://accounts.google.com/Login).
> > 
> > Developers should be able to expect to camel case at word boundaries. If
they
> > can't, we're making life harder for them. Internal implementation details
and
> > code conventions don't seem like a good reason to do that.
> 
> Yeah, I agree. Sorry I didn't push back more. The API really should be
> signedInDevices (capital I). But that will also affect the C++ code -
extension
> API file, class, and directory names.
(Just saw this)
Agree with you folks.

I have had some trouble pushing this patch in. In order not to delay further
would you mind if I did this change as a new patch. Thanks!

[lipalani, 2013-08-15 22:45:33]

also FWIW I am pretty sure CQ would fail :) and I will have to commit
manually. If there is a good reason to do it right away I can do it before
committing manually otherwise I will do it on monday.

Thanks!


On Thu, Aug 15, 2013 at 3:40 PM, <lipalani@chromium.org> wrote:

> On 2013/08/15 22:25:19, Matt Perry wrote:
>
> https://codereview.chromium.**org/22706006/diff/5001/chrome/**
>
common/extensions/api/**signedin_devices.idl<https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/api/signedin_devices.idl>
>
>> File chrome/common/extensions/api/**signedin_devices.idl (right):
>>
>
>
> https://codereview.chromium.**org/22706006/diff/5001/chrome/**
>
common/extensions/api/**signedin_devices.idl#newcode8<https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/api/signedin_devices.idl#newcode8>
>
>> chrome/common/extensions/api/**signedin_devices.idl:8: namespace
>> signedinDevices
>>
> {
>
>> On 2013/08/15 22:12:04, dharcourt wrote:
>> > On 2013/08/13 22:42:37, lipalani1 wrote:
>> > > I decided on one word after looking at the word Signin in chromium
>> code
>>
> base
>
>> > > which has a wide usage. Let me know what you think.
>> > > (I have a couple of helper functions from previous check ins that used
>> "signed
>> > > in"(2 words), but I plan to fix that in a future commit. Rest of the
>> code
>>
> is
>
>> > > consistently using the single word "Signedin".)
>> > > On 2013/08/12 22:21:01, Matt Perry wrote:
>> > > > Is "signed in" one word or two? I think two. So this should be
>> > singedInDevices
>> > > > (and likewise everywhere - the permission, the namespace, the code
>> > directory).
>> > > > Ditto for hacker style: signed_in_devices.
>> > >
>> >
>> > Came across this in passing: Since API callers won't be familiar with
>> the
>> > Chromium code base, it doesn't seem that code base usage should be a
>> determining
>> > factor in API choice... do we have any public APIs with "signedin" as a
>>
> single
>
>> > word?
>> >
>> > If not, it seems we should follow english language conventions, and I
>> can't
>> find
>> > any dictionary with "signedin" or "signin" as a single word (in fact we
>> have
>> it
>> > as two word on
https://accounts.google.com/**Login<https://accounts.google.com/Login>
>> ).
>> >
>> > Developers should be able to expect to camel case at word boundaries. If
>>
> they
>
>> > can't, we're making life harder for them. Internal implementation
>> details
>>
> and
>
>> > code conventions don't seem like a good reason to do that.
>>
>
>  Yeah, I agree. Sorry I didn't push back more. The API really should be
>> signedInDevices (capital I). But that will also affect the C++ code -
>>
> extension
>
>> API file, class, and directory names.
>>
> (Just saw this)
> Agree with you folks.
>
> I have had some trouble pushing this patch in. In order not to delay
> further
> would you mind if I did this change as a new patch. Thanks!
>
>
https://codereview.chromium.**org/22706006/<https://codereview.chromium.org/2...
>

[Matt Perry, 2013-08-15 22:47:25]

On 2013/08/15 22:40:29, lipalani1 wrote:
> On 2013/08/15 22:25:19, Matt Perry wrote:
> >
>
https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
> > File chrome/common/extensions/api/signedin_devices.idl (right):
> > 
> >
>
https://codereview.chromium.org/22706006/diff/5001/chrome/common/extensions/a...
> > chrome/common/extensions/api/signedin_devices.idl:8: namespace
signedinDevices
> {
> > On 2013/08/15 22:12:04, dharcourt wrote:
> > > On 2013/08/13 22:42:37, lipalani1 wrote:
> > > > I decided on one word after looking at the word Signin in chromium code
> base
> > > > which has a wide usage. Let me know what you think.
> > > > (I have a couple of helper functions from previous check ins that used
> > "signed
> > > > in"(2 words), but I plan to fix that in a future commit. Rest of the
code
> is
> > > > consistently using the single word "Signedin".)
> > > > On 2013/08/12 22:21:01, Matt Perry wrote:
> > > > > Is "signed in" one word or two? I think two. So this should be
> > > singedInDevices
> > > > > (and likewise everywhere - the permission, the namespace, the code
> > > directory).
> > > > > Ditto for hacker style: signed_in_devices.
> > > > 
> > > 
> > > Came across this in passing: Since API callers won't be familiar with the
> > > Chromium code base, it doesn't seem that code base usage should be a
> > determining
> > > factor in API choice... do we have any public APIs with "signedin" as a
> single
> > > word?
> > > 
> > > If not, it seems we should follow english language conventions, and I
can't
> > find
> > > any dictionary with "signedin" or "signin" as a single word (in fact we
have
> > it
> > > as two word on https://accounts.google.com/Login).
> > > 
> > > Developers should be able to expect to camel case at word boundaries. If
> they
> > > can't, we're making life harder for them. Internal implementation details
> and
> > > code conventions don't seem like a good reason to do that.
> > 
> > Yeah, I agree. Sorry I didn't push back more. The API really should be
> > signedInDevices (capital I). But that will also affect the C++ code -
> extension
> > API file, class, and directory names.
> (Just saw this)
> Agree with you folks.
> 
> I have had some trouble pushing this patch in. In order not to delay further
> would you mind if I did this change as a new patch. Thanks!

Sure, SGTM.

[commit-bot: I haz the power, 2013-08-15 23:10:56]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[lipalani1, 2013-08-16 21:09:00]

Committed patchset #13 manually as r218087 (presubmit successful).

[not at google - send to devlin, 2013-08-16 22:03:47]

https://codereview.chromium.org/22706006/diff/103001/chrome/common/extensions...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/22706006/diff/103001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:472: }.
This should be a , not a . Please don't dcommit when a try run has never
succeeded.

[tim (not reviewing), 2013-08-16 22:19:39]

On 2013/08/16 22:03:47, kalman wrote:
>
https://codereview.chromium.org/22706006/diff/103001/chrome/common/extensions...
> File chrome/common/extensions/api/_api_features.json (right):
> 
>
https://codereview.chromium.org/22706006/diff/103001/chrome/common/extensions...
> chrome/common/extensions/api/_api_features.json:472: }.
> This should be a , not a . Please don't dcommit when a try run has never
> succeeded.

Additionally, it looks like some of the Apps sync_integration_tests failed on
win_rel.

[lipalani1, 2013-08-16 22:40:07]

On 2013/08/16 22:19:39, timsteele wrote:
> On 2013/08/16 22:03:47, kalman wrote:
> >
>
https://codereview.chromium.org/22706006/diff/103001/chrome/common/extensions...
> > File chrome/common/extensions/api/_api_features.json (right):
> > 
> >
>
https://codereview.chromium.org/22706006/diff/103001/chrome/common/extensions...
> > chrome/common/extensions/api/_api_features.json:472: }.
> > This should be a , not a . Please don't dcommit when a try run has never
> > succeeded.
> 
> Additionally, it looks like some of the Apps sync_integration_tests failed on
> win_rel.

My bad. I think its all the same issue. I am running a new try job. 

In the past I used to look for the same failure across multiple platforms to
determine if it is an issue with my change or determine it is a transient error
if it occurred in only one platform. Apparently that method does not work any
more as each platform seems to run something different. Will change my approach
:)

[lipalani1, 2013-08-20 01:04:35]

Committed patchset #19 manually as r218374 (presubmit successful).

[not at google - send to devlin, 2013-08-20 22:25:32]

On 2013/08/20 01:04:35, lipalani1 wrote:
> Committed patchset #19 manually as r218374 (presubmit successful).

Please rename signedin_devices to signed_in_devices.

[not at google - send to devlin, 2013-08-20 23:12:34]

and please add documentation.

[not at google - send to devlin, 2013-08-21 00:27:14]

and your changes to _api_features.json have gone. This API won't work.

I think this API needs a sample extension.

[lipalani, 2013-08-21 23:28:58]

Ben - I checked in the idl files and the implementation to unblock Kristen.

(I also updated the issue description, before committing, to say it is not
yet exposed to the dev channel).

I have 3 cls in pipeline:
1. Finish the plumbing necessary to do device info change notifications.
2. Renamed signedin to signed_in
3. add the permissions back and a sample extension.

Hopefully this answers your qn.
thanks!


On Tue, Aug 20, 2013 at 5:27 PM, <kalman@chromium.org> wrote:

> and your changes to _api_features.json have gone. This API won't work.
>
> I think this API needs a sample extension.
>
>
https://codereview.chromium.**org/22706006/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.