Bug 180488 – Start writing ServiceWorker registrations to disk

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Start writing ServiceWorker registrations to disk

[Brady Eidson]

Created attachment 328595 [details]
Patch

[Chris Dumez]

Comment on attachment 328595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328595&action=review

r=me

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:89
> +    std::unique_ptr<String> errorMessage;

Wh a pointer? string::isNull() not enough?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:154
> +static String updateViaCacheToString(ServiceWorkerUpdateViaCache update)

Why are we storing them as Strings?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:168
> +static String workerTypeToString(WorkerType workerType)

Why are we storing them as Strings?

> Source/WebCore/workers/service/server/RegistrationDatabase.h:39
> +class RegistrationDatabase : public CrossThreadTaskHandler {

FAST_MALLOC?

> Source/WebCore/workers/service/server/RegistrationStore.h:42
> +class RegistrationStore {

FAST_MALLOC?

> Source/WebCore/workers/service/server/RegistrationStore.h:44
> +    RegistrationStore(const String& databaseDirectory);

explicit?

[Brady Eidson]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 328595 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328595&action=review
> 
> r=me
> 
> > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:89
> > +    std::unique_ptr<String> errorMessage;
> 
> Wh a pointer? string::isNull() not enough?

Yah, I overthought this.

> 
> > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:154
> > +static String updateViaCacheToString(ServiceWorkerUpdateViaCache update)
> 
> Why are we storing them as Strings?
> 
> > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:168
> > +static String workerTypeToString(WorkerType workerType)
> 
> Why are we storing them as Strings?

We've learned in the past - Storing enum-like things in databases is fragile, as changes to their numeric values in code happens without consideration to their persistency on disk.

We could *manually* convert enums to integers, and convert them back, for example - but that still suffers from similar fragility.


> 
> > Source/WebCore/workers/service/server/RegistrationDatabase.h:39
> > +class RegistrationDatabase : public CrossThreadTaskHandler {
> 
> FAST_MALLOC?
> 
> > Source/WebCore/workers/service/server/RegistrationStore.h:42
> > +class RegistrationStore {
> 
> FAST_MALLOC?

Sure.

> 
> > Source/WebCore/workers/service/server/RegistrationStore.h:44
> > +    RegistrationStore(const String& databaseDirectory);
> 
> explicit?

Sure.

[EWS Watchlist]

Comment on attachment 328595 [details]
Patch

Attachment 328595 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5518240

New failing tests:
loader/go-back-cached-main-resource.html

[EWS Watchlist]

Created attachment 328610 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Brady Eidson]

Created attachment 328611 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 328611 [details]
Patch

Clearing flags on attachment: 328611

Committed r225610: <https://trac.webkit.org/changeset/225610>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35896479>

[Saam Barati]

Comment on attachment 328611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328611&action=review

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:90
> +    auto scopeExit = makeScopeExit([&, errorMessage = &errorMessage] {

Why do you do errorMessage = &errorMessage here if you're already capturing by reference?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:91
> +        ASSERT(!errorMessage->isNull());

This causes a build failure for me when building on iOS. I'm going to commit to make this ASSERT_UNUSED.

[Saam Barati]

Comment on attachment 328611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328611&action=review

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:99
> +        errorMessage = "Failed to open registration database";

nit: I believe this is more efficient if it were ASCIILiteral

[Saam Barati]

Comment on attachment 328611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328611&action=review

>> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:91
>> +        ASSERT(!errorMessage->isNull());
> 
> This causes a build failure for me when building on iOS. I'm going to commit to make this ASSERT_UNUSED.

Landed in:
https://trac.webkit.org/changeset/225621/webkit