Bug 176845 – Add Cache API support of records persistency

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

[youenn fablet]

Add persistency on disk for regular sessions and in-memory for private sessions

[youenn fablet]

Created attachment 320646 [details]
Patch

[youenn fablet]

Created attachment 320701 [details]
Patch

[Build Bot]

Attachment 320701 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:238:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 320706 [details]
Patch

[Build Bot]

Attachment 320706 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:237:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:426:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 320709 [details]
Patch

[Build Bot]

Attachment 320709 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:244:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:433:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 320710 [details]
Patch

[Build Bot]

Attachment 320710 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:244:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:434:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 320710 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:61
> +    if (keyURL.hasQuery())
> +        keyURL.setQuery({ });

Seems like there should be a removeQuery(), just like there is a removeFragmentIdentifier().

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:105
> +    auto key = Key { ASCIILiteral("record"), String { m_uniqueName }, String(), createCanonicalUUIDString(), m_caches.salt() };

I would do Key key { ... }; rather than the auto key = Key { ... };

If m_uniqueName is already a String, why the need to copy-construct?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:107
> +    RecordInformation recordInformation = { WTFMove(key), insertionTime, record.identifier, 0 , record.request.url(), false, { } };

I don't think the = should be necessary.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:140
> +struct TraversalResult {
> +    Ref<Caches> caches;
> +    uint64_t cacheIdentifier;
> +    CompletionCallback callback;
> +    HashMap<String, Vector<RecordInformation>> records;
> +    Vector<Key> failedRecords;
> +};

Probably want to wrap this in an anonymous namespace to static linkage.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:153
> +    TraversalResult traversalResult = { makeRef(m_caches), m_identifier, WTFMove(callback), { }, { } };

Should not need =.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:180
> +        RecordInformation recordInformation = { storageRecord->key, insertionTime, 0, 0, record.request.url(), false, { } };

Ditto.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:220
> +    using ReadRecordsCallback = WTF::Function<void(Vector<Record>&&, Vector<uint64_t>&&)>;
> +    static RefPtr<ReadRecordTaskCounter> create(ReadRecordsCallback&& callback) { return adoptRef(new ReadRecordTaskCounter(WTFMove(callback))); }

Why RefPtr<ReadRecordTaskCounter>? Can this return a Ref<>?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:244
> +    explicit ReadRecordTaskCounter(ReadRecordsCallback&& callback) : m_callback(WTFMove(callback)) { }

We usually write this as: 

explicit ReadRecordTaskCounter(ReadRecordsCallback&& callback)
    : m_callback(WTFMove(callback)) 
{
}

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:317
> +    static RefPtr<AsynchronousPutTaskCounter> create() { return adoptRef(new AsynchronousPutTaskCounter()); }

Ref?  Also, you don't need the () in new AsynchronousPutTaskCounter()

[Alex Christensen]

Comment on attachment 320710 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:61
>> +        keyURL.setQuery({ });
> 
> Seems like there should be a removeQuery(), just like there is a removeFragmentIdentifier().

Since we're removing the fragment right after it, I think this needs a removeQueryAndFragmentIdentifier.  I've seen this pattern before, too.

[youenn fablet]

Created attachment 320773 [details]
Patch

[youenn fablet]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 320710 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320710&action=review
> 
> >> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:61
> >> +        keyURL.setQuery({ });
> > 
> > Seems like there should be a removeQuery(), just like there is a removeFragmentIdentifier().
> 
> Since we're removing the fragment right after it, I think this needs a
> removeQueryAndFragmentIdentifier.  I've seen this pattern before, too.

Right, there are at least two places in CacheStorage API where we would benefit from that pattern. This is on my TODO list.

[Build Bot]

Attachment 320773 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:243:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:433:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Thanks for the review.

(In reply to Sam Weinig from comment #10)
> Comment on attachment 320710 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320710&action=review
> 
> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:61
> > +    if (keyURL.hasQuery())
> > +        keyURL.setQuery({ });
> 
> Seems like there should be a removeQuery(), just like there is a
> removeFragmentIdentifier().
> 
> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:105
> > +    auto key = Key { ASCIILiteral("record"), String { m_uniqueName }, String(), createCanonicalUUIDString(), m_caches.salt() };
> 
> I would do Key key { ... }; rather than the auto key = Key { ... };
> 
> If m_uniqueName is already a String, why the need to copy-construct?

Fixed.

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:107
> > +    RecordInformation recordInformation = { WTFMove(key), insertionTime, record.identifier, 0 , record.request.url(), false, { } };
> 
> I don't think the = should be necessary.

Fixed.

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:140
> > +struct TraversalResult {
> > +    Ref<Caches> caches;
> > +    uint64_t cacheIdentifier;
> > +    CompletionCallback callback;
> > +    HashMap<String, Vector<RecordInformation>> records;
> > +    Vector<Key> failedRecords;
> > +};
> 
> Probably want to wrap this in an anonymous namespace to static linkage.

I am not sure what you are proposing, can you elaborate?


> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:153
> > +    TraversalResult traversalResult = { makeRef(m_caches), m_identifier, WTFMove(callback), { }, { } };
> 
> Should not need =.

Fixed.

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:180
> > +        RecordInformation recordInformation = { storageRecord->key, insertionTime, 0, 0, record.request.url(), false, { } };
> 
> Ditto.

Fixed.

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:220
> > +    using ReadRecordsCallback = WTF::Function<void(Vector<Record>&&, Vector<uint64_t>&&)>;
> > +    static RefPtr<ReadRecordTaskCounter> create(ReadRecordsCallback&& callback) { return adoptRef(new ReadRecordTaskCounter(WTFMove(callback))); }
> 
> Why RefPtr<ReadRecordTaskCounter>? Can this return a Ref<>?

It makes capturing the counter in the lambda quicker to write.
[taskCounter] vs [taskCounter = taskeCounter.copyRef()].

Maybe being explicit about the copyRef is a good thing though?

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:244
> > +    explicit ReadRecordTaskCounter(ReadRecordsCallback&& callback) : m_callback(WTFMove(callback)) { }
> 
> We usually write this as: 
> 
> explicit ReadRecordTaskCounter(ReadRecordsCallback&& callback)
>     : m_callback(WTFMove(callback)) 
> {
> }

Would it make sense to change the code style here?
I forgot to change this in the patch, will do it asap.

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:317
> > +    static RefPtr<AsynchronousPutTaskCounter> create() { return adoptRef(new AsynchronousPutTaskCounter()); }
> 
> Ref?  Also, you don't need the () in new AsynchronousPutTaskCounter()

[youenn fablet]

Created attachment 320774 [details]
Patch

[youenn fablet]

Filed bug 176911 for the URL helper method.

[Build Bot]

Attachment 320774 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:436:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

(In reply to youenn fablet from comment #15)
> Thanks for the review.
> 
> 
> > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:140
> > > +struct TraversalResult {
> > > +    Ref<Caches> caches;
> > > +    uint64_t cacheIdentifier;
> > > +    CompletionCallback callback;
> > > +    HashMap<String, Vector<RecordInformation>> records;
> > > +    Vector<Key> failedRecords;
> > > +};
> > 
> > Probably want to wrap this in an anonymous namespace to static linkage.
> 
> I am not sure what you are proposing, can you elaborate?
> 

When you have a function in a cpp file that you want to constrain to that translation unit, you mark that by saying static void functionName()... For a struct or class, you can do the same thing by wrapping it in an anonymous namespace.  In this case:


namespace {

struct TraversalResult {
    Ref<Caches> caches;
    uint64_t cacheIdentifier;
    CompletionCallback callback;
    HashMap<String, Vector<RecordInformation>> records;
    Vector<Key> failedRecords;
};

}

> 
> > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:220
> > > +    using ReadRecordsCallback = WTF::Function<void(Vector<Record>&&, Vector<uint64_t>&&)>;
> > > +    static RefPtr<ReadRecordTaskCounter> create(ReadRecordsCallback&& callback) { return adoptRef(new ReadRecordTaskCounter(WTFMove(callback))); }
> > 
> > Why RefPtr<ReadRecordTaskCounter>? Can this return a Ref<>?
> 
> It makes capturing the counter in the lambda quicker to write.
> [taskCounter] vs [taskCounter = taskeCounter.copyRef()].
> 
> Maybe being explicit about the copyRef is a good thing though?

I think so. It's yet another think in my list of things that makes me wonder if Ref<> should be copyable.

[youenn fablet]

> When you have a function in a cpp file that you want to constrain to that
> translation unit, you mark that by saying static void functionName()... For
> a struct or class, you can do the same thing by wrapping it in an anonymous
> namespace.  In this case:

We could do that for all classes declared in the cpp file.
I guess the point is that TraversalResult name is generic enough that it could collide.

> > Maybe being explicit about the copyRef is a good thing though?

Will move to this then.
 
> I think so. It's yet another think in my list of things that makes me wonder
> if Ref<> should be copyable.

That is a vast discussion indeed.

[youenn fablet]

Created attachment 320793 [details]
Patch

[Build Bot]

Attachment 320793 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:438:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 320793 [details]
Patch

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

r=me other than these comments.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:157
> +            RunLoop::main().dispatch([traversalResult = WTFMove(traversalResult)]() mutable {

An isolatedCopy would protect this from future changes accidentally introducing multiply-reffed strings going across threads here.  The key is questionable already.  I think until we have thread safe refcounted strings we need to isolatedCopy things when going across threads at places like this.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:218
> +class ReadRecordTaskCounter : public ThreadSafeRefCounted<ReadRecordTaskCounter> {

This doesn't need to be ThreadSafeRefCounted.  RefCounted is fine because all operations of this class are on the same thread.  Add assertions.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:319
> +class AsynchronousPutTaskCounter : public RefCounted<AsynchronousPutTaskCounter> {

This is a strange abstraction.  I think it would be better to reorganize the code that uses it.

[youenn fablet]

Created attachment 320852 [details]
Patch for landing

[Build Bot]

Attachment 320852 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:473:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 320863 [details]
Patch for landing

[Build Bot]

Attachment 320863 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:474:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 320863 [details]
Patch for landing

Clearing flags on attachment: 320863

Committed r222073: <http://trac.webkit.org/changeset/222073>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693326>