[Background Sync] Add proper error status to API calls and callbacks

[Background Sync] Add proper error status to API calls and callbacks

BUG=484306
Committed: https://crrev.com/93178c5d47daee8abfd78652f70f0cb73ea8e516
Cr-Commit-Position: refs/heads/master@{#329625}

[iclelland, 2015-05-12 20:32:17]

iclelland@chromium.org changed reviewers:
+ jkarlin@chromium.org, tsepez@chromium.org

[iclelland, 2015-05-12 20:53:53]

tsepez: PTAL at the changes to the mojo definitions

jkarlin: Everything, especially the mapping of manager error codes to callback
behaviour.

Thanks

[Tom Sepez, 2015-05-12 21:29:15]

.mojom LGTM

[jkarlin, 2015-05-13 11:19:47]

LGTM with minor changes

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_provider.cc:128: result =
Might as well declare result here as it's not used anywhere else.

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_provider.cc:133:
callbacks->onSuccess(nullptr);
This should be a NOTREACHED() case

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_provider.cc:181:
registrations.size());
Results should be declared here

[iclelland, 2015-05-13 11:48:37]

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_provider.cc:128: result =
On 2015/05/13 11:19:46, jkarlin wrote:
> Might as well declare result here as it's not used anywhere else.

clang doesn't allow variable declaration from within a switch case :( It's not a
scope.

I could either introduce an anonymous scope just for the case, or, if null is
never a valid possibility for an ERROR_NONE result, I could check for that
explicitly, and declare result in the scope of the if clause.

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_provider.cc:133:
callbacks->onSuccess(nullptr);
On 2015/05/13 11:19:46, jkarlin wrote:
> This should be a NOTREACHED() case

Perhaps I should rename the method instead; the same callback is used for both
register() and getRegistrations(), where NOT_FOUND is expected.

[jkarlin, 2015-05-13 12:01:56]

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_provider.cc:128: result =
On 2015/05/13 11:48:36, iclelland wrote:
> On 2015/05/13 11:19:46, jkarlin wrote:
> > Might as well declare result here as it's not used anywhere else.
> 
> clang doesn't allow variable declaration from within a switch case :( It's not
a
> scope.
> 
> I could either introduce an anonymous scope just for the case, or, if null is
> never a valid possibility for an ERROR_NONE result, I could check for that
> explicitly, and declare result in the scope of the if clause.

My bad, what you have is fine.

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_provider.cc:133:
callbacks->onSuccess(nullptr);
On 2015/05/13 11:48:36, iclelland wrote:
> On 2015/05/13 11:19:46, jkarlin wrote:
> > This should be a NOTREACHED() case
> 
> Perhaps I should rename the method instead; the same callback is used for both
> register() and getRegistrations(), where NOT_FOUND is expected.

I see. Yes, I'd split them into different callbacks as they expect different
errors.

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_provider.cc:181:
registrations.size());
On 2015/05/13 11:19:46, jkarlin wrote:
> Results should be declared here

I retract this comment, you can't declare it within the switch.

[iclelland, 2015-05-13 12:18:32]

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1131943005/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_provider.cc:133:
callbacks->onSuccess(nullptr);
On 2015/05/13 12:01:56, jkarlin wrote:
> On 2015/05/13 11:48:36, iclelland wrote:
> > On 2015/05/13 11:19:46, jkarlin wrote:
> > > This should be a NOTREACHED() case
> > 
> > Perhaps I should rename the method instead; the same callback is used for
both
> > register() and getRegistrations(), where NOT_FOUND is expected.
> 
> I see. Yes, I'd split them into different callbacks as they expect different
> errors.

Done.

[iclelland, 2015-05-13 12:19:27]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-05-13 12:19:28]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1131943005/#ps20001
(title: "Split RegisterCallback into two methods")

[commit-bot: I haz the power, 2015-05-13 12:19:35]

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

[commit-bot: I haz the power, 2015-05-13 13:11:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-13 13:12:04]

Patchset 2 (id:??) landed as
https://crrev.com/93178c5d47daee8abfd78652f70f0cb73ea8e516
Cr-Commit-Position: refs/heads/master@{#329625}