Chrome Alarm Clear and ClearAll Updates

Chrome Alarm Clear and ClearAll Updates

chrome.alarms.clear will now take an optional callback indicating if the
specified alarm was cleared. It will no longer post an error.

chrome.alarms.clearAll will also behave similarly. It will provide a
callback at the completion of clearAll indicating if all alarms were
cleared successfully.

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

[robliao, 2014-03-19 21:29:24]

Reviewers: Please review this CL. Thanks

This handles the clear side of things.

As an aside, the presubmit on Windows outputs
  File "D:\src\chrome\common\extensions\docs\server2\path_util.py", line 29, in
AssertIsValid
    assert IsValid(path), 'Path "%s" is invalid' % path
AssertionError: Path "..\..\.." is invalid

...but the helppage looks file via preview.py

[not at google - send to devlin, 2014-03-20 10:52:21]

lgtm with that change that looks pretty trivial to AlarmManager. ping back if
not.

https://codereview.chromium.org/205323002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/alarms/alarms_api.cc (right):

https://codereview.chromium.org/205323002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/alarms/alarms_api.cc:203: SetResult(new
base::FundamentalValue(true));
seems more correct to set this to (num_alarms_cleared > 0) if you can plumb that
through. seems trivial.

[Matt Perry, 2014-03-20 18:53:41]

https://codereview.chromium.org/205323002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/alarms/alarms_api.cc (right):

https://codereview.chromium.org/205323002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/alarms/alarms_api.cc:203: SetResult(new
base::FundamentalValue(true));
On 2014/03/20 10:52:21, kalman wrote:
> seems more correct to set this to (num_alarms_cleared > 0) if you can plumb
that
> through. seems trivial.

hmm.. I think generally removeAll() on the empty set would result in success.

[robliao, 2014-03-20 19:01:04]

https://codereview.chromium.org/205323002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/alarms/alarms_api.cc (right):

https://codereview.chromium.org/205323002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/alarms/alarms_api.cc:203: SetResult(new
base::FundamentalValue(true));
There are two philosophies to take here:
1) We treat this as a success indicator. This means it returns true.
2) We treat this as an indicator work was done. This means it return false.

I'm not sure which is more useful, so let's go with consistency for APIs that
clear everything. chrome.storage.clear uses success. The other ones are
non-obvious at first glance.

On 2014/03/20 18:53:42, Matt Perry wrote:
> On 2014/03/20 10:52:21, kalman wrote:
> > seems more correct to set this to (num_alarms_cleared > 0) if you can plumb
> that
> > through. seems trivial.
> 
> hmm.. I think generally removeAll() on the empty set would result in success.

[Matt Perry, 2014-03-20 21:07:00]

https://codereview.chromium.org/205323002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/alarms/alarms_api.cc (right):

https://codereview.chromium.org/205323002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/alarms/alarms_api.cc:203: SetResult(new
base::FundamentalValue(true));
Yeah, #1 makes more sense IMO. I think it should only return false if
num_alarms_remaining != 0.

On 2014/03/20 19:01:05, robliao wrote:
> There are two philosophies to take here:
> 1) We treat this as a success indicator. This means it returns true.
> 2) We treat this as an indicator work was done. This means it return false.
> 
> I'm not sure which is more useful, so let's go with consistency for APIs that
> clear everything. chrome.storage.clear uses success. The other ones are
> non-obvious at first glance.
> 
> On 2014/03/20 18:53:42, Matt Perry wrote:
> > On 2014/03/20 10:52:21, kalman wrote:
> > > seems more correct to set this to (num_alarms_cleared > 0) if you can
plumb
> > that
> > > through. seems trivial.
> > 
> > hmm.. I think generally removeAll() on the empty set would result in
success.
>

[robliao, 2014-03-20 21:20:30]

https://codereview.chromium.org/205323002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/alarms/alarms_api.cc (right):

https://codereview.chromium.org/205323002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/alarms/alarms_api.cc:203: SetResult(new
base::FundamentalValue(true));
We do a hard CHECK if that happens, so we should be good to go for checkin as
is.

If there are no other objections, I'll commit this later today.
On 2014/03/20 21:07:00, Matt Perry wrote:
> Yeah, #1 makes more sense IMO. I think it should only return false if
> num_alarms_remaining != 0.
> 
> On 2014/03/20 19:01:05, robliao wrote:
> > There are two philosophies to take here:
> > 1) We treat this as a success indicator. This means it returns true.
> > 2) We treat this as an indicator work was done. This means it return false.
> > 
> > I'm not sure which is more useful, so let's go with consistency for APIs
that
> > clear everything. chrome.storage.clear uses success. The other ones are
> > non-obvious at first glance.
> > 
> > On 2014/03/20 18:53:42, Matt Perry wrote:
> > > On 2014/03/20 10:52:21, kalman wrote:
> > > > seems more correct to set this to (num_alarms_cleared > 0) if you can
> plumb
> > > that
> > > > through. seems trivial.
> > > 
> > > hmm.. I think generally removeAll() on the empty set would result in
> success.
> > 
>

[Matt Perry, 2014-03-20 21:21:31]

lgtm

[not at google - send to devlin, 2014-03-20 22:25:05]

always returning true sounds fine. lgtm.

[robliao, 2014-03-20 22:25:44]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2014-03-20 22:27:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/205323002/1

[commit-bot: I haz the power, 2014-03-20 23:50:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 23:50:04]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[robliao, 2014-03-21 00:02:00]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2014-03-21 00:03:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/205323002/1

[commit-bot: I haz the power, 2014-03-21 11:31:16]

Change committed as 258526