ServiceWorker: DB functions should return status code instead of boolean (3)

ServiceWorker: DB functions should return status code instead of boolean (3)

Part1: https://codereview.chromium.org/275103004/
Part2: https://codereview.chromium.org/287843002/

This change makes all remaining functions return status code instead of boolean.


BUG=372704
TEST=should pass all existing tests

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

[nhiroki, 2014-05-15 11:31:10]

Hi,

If the part2 patch looks good to you, can you review this, too?
I believe that the next one will be the last.

[michaeln, 2014-05-15 20:44:57]

lgtm (i just have some nits to consider or not)

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:455: if (status !=
SERVICE_WORKER_ERROR_NOT_FOUND)
nit: could be restructured to only utilize early return instead of the } else {
clause

if (x != 1 && x != b)
  return z;

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:524: (status ==
SERVICE_WORKER_OK && !is_initialized_)) {
this test is a recurring theme, stuffing it in a helper method might be nice,
not sure what to name such a method?

if (IsNewOrNonexistentDatabase(status))
  return xxx;

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:880: return true;
nit: return WriteBatch() == OK;

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:908: return true;
ditto

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.h:170: void
DidDatabaseOperation(
Maybe you have in mind to use this generically named method to complete other
operations beside UpdateToActive, and to trigger the deleteAndStartOver logic in
here?

Until there are more uses of this method, might be good to name it
DidUpdateToActiveState.

[michaeln, 2014-05-15 20:45:53]

and this CL descript comment seems out of place

"Other remaining functions will be fixed in the following CLs."

should that be removed?

[nhiroki, 2014-05-19 04:58:03]

Thank you for your comments! Updated.

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:455: if (status !=
SERVICE_WORKER_ERROR_NOT_FOUND)
On 2014/05/15 20:44:58, michaeln wrote:
> nit: could be restructured to only utilize early return instead of the } else
{
> clause
> 
> if (x != 1 && x != b)
>   return z;

Done.

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:524: (status ==
SERVICE_WORKER_OK && !is_initialized_)) {
On 2014/05/15 20:44:58, michaeln wrote:
> this test is a recurring theme, stuffing it in a helper method might be nice,
> not sure what to name such a method?
> 
> if (IsNewOrNonexistentDatabase(status))
>   return xxx;

Done.

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:880: return true;
On 2014/05/15 20:44:58, michaeln wrote:
> nit: return WriteBatch() == OK;

Removed this branch.

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:908: return true;
On 2014/05/15 20:44:58, michaeln wrote:
> ditto

Done.

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/284123003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.h:170: void
DidDatabaseOperation(
On 2014/05/15 20:44:58, michaeln wrote:
> Maybe you have in mind to use this generically named method to complete other
> operations beside UpdateToActive, and to trigger the deleteAndStartOver logic
in
> here?

You are right.

> Until there are more uses of this method, might be good to name it
> DidUpdateToActiveState.

Okay, renamed it.

[nhiroki, 2014-05-19 04:58:15]

On 2014/05/15 20:45:53, michaeln wrote:
> and this CL descript comment seems out of place
> 
> "Other remaining functions will be fixed in the following CLs."
> 
> should that be removed?

Done.

[nhiroki, 2014-05-19 04:58:21]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-05-19 04:58:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/284123003/80001

[commit-bot: I haz the power, 2014-05-19 06:05:02]

Change committed as 271342