Implement InstanceID API functions.

Implement InstanceID API functions.

BUG=477084
TEST=new tests
TBR=kalman@chromium.org

Committed: https://crrev.com/1d6740d0b6232669b21daeee36aedf049f9cd378
Cr-Commit-Position: refs/heads/master@{#328877}

[jianli, 2015-05-07 00:01:56]

jianli@chromium.org changed reviewers:
+ fgorski@chromium.org

[jianli, 2015-05-07 00:44:14]

Patchset #2 (id:20001) has been deleted

[jianli, 2015-05-07 00:44:31]

Patchset #2 (id:20001) has been deleted

[fgorski, 2015-05-07 19:59:54]

https://codereview.chromium.org/1128123003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/instance_id/instance_id_api.cc (right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/instance_id/instance_id_api.cc:131:
instance_id::InstanceID* instance_id,
you are completely ignoring this one. Would it make sense to dcheck against or
remove this parameter form the callback if it is not needed?

This question applies to other callbacks.

https://codereview.chromium.org/1128123003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/instance_id/instance_id_api.h (right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/instance_id/instance_id_api.h:14: class
InstanceID;
either this or the include above is not necessary.

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/instance_id/delete_id/delete_id.js
(right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_id/delete_id.js:30: if
(!id) {
nit: does it make sense to add || chrome.runtime.lastError?

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_id/delete_id.js:31:
chrome.test.fail("ID should not be zero.");
nit: s/zero/empty/

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js
(right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js:27:
chrome.instanceID.deleteToken({"scope": "GCM"});
applies elsewhere: add callback here, as you don't know why this thing failed
otherwise.

also, it seems to me that things your are testing for here are enforced by IDL
and you could skip these tests.

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js:67:
chrome.instanceID.deleteToken(
shouldn't you get the token first here?

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js:93:
//deleteToken,
test the following: getToken->Delete->getToken

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/instance_id/get_creation_time/get_creation_time.js
(right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/get_creation_time/get_creation_time.js:56:
getCreationTimeAfterGetID,
how about a test for getting creation time after ID was deleted?

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/instance_id/get_token/get_token.js
(right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/get_token/get_token.js:132:
//getTokenWithValidOptions,
add test placeholder for getToken with combination with deleteID

[jianli, 2015-05-07 20:47:02]

https://codereview.chromium.org/1128123003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/instance_id/instance_id_api.cc (right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/instance_id/instance_id_api.cc:131:
instance_id::InstanceID* instance_id,
On 2015/05/07 19:59:53, fgorski wrote:
> you are completely ignoring this one. Would it make sense to dcheck against or
> remove this parameter form the callback if it is not needed?
> 
> This question applies to other callbacks.

Indeed I has removed this from my other patch that is still in progress. Let me
do it now.

https://codereview.chromium.org/1128123003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/instance_id/instance_id_api.h (right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/instance_id/instance_id_api.h:14: class
InstanceID;
On 2015/05/07 19:59:53, fgorski wrote:
> either this or the include above is not necessary.

Removed the forward declaration since now we really need the header file.

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/instance_id/delete_id/delete_id.js
(right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_id/delete_id.js:30: if
(!id) {
On 2015/05/07 19:59:53, fgorski wrote:
> nit: does it make sense to add || chrome.runtime.lastError?

Done.

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_id/delete_id.js:31:
chrome.test.fail("ID should not be zero.");
On 2015/05/07 19:59:53, fgorski wrote:
> nit: s/zero/empty/

Done.

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js
(right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js:27:
chrome.instanceID.deleteToken({"scope": "GCM"});
On 2015/05/07 19:59:53, fgorski wrote:
> applies elsewhere: add callback here, as you don't know why this thing failed
> otherwise.

Good catch. Added.

> 
> also, it seems to me that things your are testing for here are enforced by IDL
> and you could skip these tests.

I think it is probably better to do our own validation in case that we might
change to use other definition.

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js:67:
chrome.instanceID.deleteToken(
On 2015/05/07 19:59:53, fgorski wrote:
> shouldn't you get the token first here?

This is to test calling deleteToken without getToken.

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/delete_token/delete_token.js:93:
//deleteToken,
On 2015/05/07 19:59:53, fgorski wrote:
> test the following: getToken->Delete->getToken

Done.

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/instance_id/get_creation_time/get_creation_time.js
(right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/get_creation_time/get_creation_time.js:56:
getCreationTimeAfterGetID,
On 2015/05/07 19:59:53, fgorski wrote:
> how about a test for getting creation time after ID was deleted?

ditto.

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/instance_id/get_token/get_token.js
(right):

https://codereview.chromium.org/1128123003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/instance_id/get_token/get_token.js:132:
//getTokenWithValidOptions,
On 2015/05/07 19:59:54, fgorski wrote:
> add test placeholder for getToken with combination with deleteID

Good idea. We have some more combo to cover. Rather putting here, I will add a
new test directory for all combo testing.

[fgorski, 2015-05-07 22:04:31]

LGTM

[jianli, 2015-05-07 22:05:06]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2015-05-07 22:06:34]

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

[commit-bot: I haz the power, 2015-05-07 23:29:54]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-05-07 23:30:48]

Patchset 4 (id:??) landed as
https://crrev.com/1d6740d0b6232669b21daeee36aedf049f9cd378
Cr-Commit-Position: refs/heads/master@{#328877}