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

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

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

This change makes the following functions return status code instead of boolean.
- GetRegistrationsForOrigin()
- GetAllRegistrations()
- LazyOpen()
- ReadDatabaseVersion()

Other remaining functions will be fixed in the following CLs.


BUG=372704
TEST=should pass all existing tests

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

[nhiroki, 2014-05-15 07:12:08]

PTAL, thanks!

[michaeln, 2014-05-15 19:59:29]

lgtm

If we end up translating from the error returned by the DB class to some other
error code in the Storage class, we might want to define a set of specific
ServiceWorkerDatabase { enum ErrorCodes {}; }

NOT_FOUND is kind of overloaded, dbfile not found or what you were looking for
in the db is not there?

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:300: origins->clear();
if origins needs to be cleared here, then it also needs to be cleared in the
case below prior to reading from the db

i think we could just DCHECK(origins->empty()) up top

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:332:
registrations->clear();
ditto clearing the collection

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:373:
registrations->clear();
ditt

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:794: ids->clear();
ditto clearing collection

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:537:
callback.Run(SERVICE_WORKER_ERROR_FAILED,
should |status| be passed in here?

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:575:
callback.Run(SERVICE_WORKER_ERROR_FAILED,
ditto |status|? dunno if it should just asking

[nhiroki, 2014-05-16 02:11:11]

Thank you! Addressed comments partially for now.

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:300: origins->clear();
On 2014/05/15 19:59:30, michaeln wrote:
> if origins needs to be cleared here, then it also needs to be cleared in the
> case below prior to reading from the db
> 
> i think we could just DCHECK(origins->empty()) up top

Done.

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:332:
registrations->clear();
On 2014/05/15 19:59:30, michaeln wrote:
> ditto clearing the collection

Done.

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:373:
registrations->clear();
On 2014/05/15 19:59:30, michaeln wrote:
> ditt

Done.

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:794: ids->clear();
On 2014/05/15 19:59:30, michaeln wrote:
> ditto clearing collection

Done.

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/287843002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:537:
callback.Run(SERVICE_WORKER_ERROR_FAILED,
On 2014/05/15 19:59:30, michaeln wrote:
> should |status| be passed in here?

If we handle DB related errors like DB_CORRUPTED in this class, I think it would
be better not to propagate such errors to the upper class. Adding DB specific
error codes that you mentioned looks good. I'll try it in this CL.

[nhiroki, 2014-05-16 02:48:20]

https://codereview.chromium.org/287843002/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/287843002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:785:
DCHECK(ids->clear());
Oops... will fix this.

[nhiroki, 2014-05-16 05:16:42]

Updated. Can you take another look? Thanks!

[michaeln, 2014-05-16 21:52:12]

lgtm!

[nhiroki, 2014-05-18 13:05:32]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-05-18 13:05:40]

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

[commit-bot: I haz the power, 2014-05-18 14:58:30]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-18 15:55:33]

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

[commit-bot: I haz the power, 2014-05-18 15:55:33]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[nhiroki, 2014-05-19 01:21:40]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-05-19 01:21:51]

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

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

Change committed as 271329