Bug 176845 - Add Cache API support of records persistency
Summary: Add Cache API support of records persistency
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 176818
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-13 09:14 PDT by youenn fablet
Modified: 2022-09-07 08:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch (53.99 KB, patch)
2017-09-13 09:41 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (54.03 KB, patch)
2017-09-13 15:41 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (54.09 KB, patch)
2017-09-13 16:23 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (54.49 KB, patch)
2017-09-13 16:40 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (54.52 KB, patch)
2017-09-13 16:50 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (54.45 KB, patch)
2017-09-14 09:00 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (54.46 KB, patch)
2017-09-14 09:05 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (54.55 KB, patch)
2017-09-14 11:09 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (56.36 KB, patch)
2017-09-14 17:33 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (56.41 KB, patch)
2017-09-14 19:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-09-13 09:14:37 PDT
Add persistency on disk for regular sessions and in-memory for private sessions
Comment 1 youenn fablet 2017-09-13 09:41:44 PDT
Created attachment 320646 [details]
Patch
Comment 2 youenn fablet 2017-09-13 15:41:28 PDT
Created attachment 320701 [details]
Patch
Comment 3 Build Bot 2017-09-13 15:43:24 PDT
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.
Comment 4 youenn fablet 2017-09-13 16:23:11 PDT
Created attachment 320706 [details]
Patch
Comment 5 Build Bot 2017-09-13 16:24:31 PDT
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.
Comment 6 youenn fablet 2017-09-13 16:40:12 PDT
Created attachment 320709 [details]
Patch
Comment 7 Build Bot 2017-09-13 16:42:04 PDT
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.
Comment 8 youenn fablet 2017-09-13 16:50:36 PDT
Created attachment 320710 [details]
Patch
Comment 9 Build Bot 2017-09-13 16:53:09 PDT
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.
Comment 10 Sam Weinig 2017-09-13 21:57:44 PDT
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()
Comment 11 Alex Christensen 2017-09-13 23:22:30 PDT
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.
Comment 12 youenn fablet 2017-09-14 09:00:19 PDT
Created attachment 320773 [details]
Patch
Comment 13 youenn fablet 2017-09-14 09:01:05 PDT
(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.
Comment 14 Build Bot 2017-09-14 09:02:54 PDT
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.
Comment 15 youenn fablet 2017-09-14 09:04:02 PDT
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()
Comment 16 youenn fablet 2017-09-14 09:05:04 PDT
Created attachment 320774 [details]
Patch
Comment 17 youenn fablet 2017-09-14 09:06:19 PDT
Filed bug 176911 for the URL helper method.
Comment 18 Build Bot 2017-09-14 09:07:46 PDT
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.
Comment 19 Sam Weinig 2017-09-14 09:19:37 PDT
(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.
Comment 20 youenn fablet 2017-09-14 10:21:20 PDT
> 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.
Comment 21 youenn fablet 2017-09-14 11:09:48 PDT
Created attachment 320793 [details]
Patch
Comment 22 Build Bot 2017-09-14 11:12:25 PDT
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.
Comment 23 Alex Christensen 2017-09-14 16:14:07 PDT
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.
Comment 24 youenn fablet 2017-09-14 17:33:19 PDT
Created attachment 320852 [details]
Patch for landing
Comment 25 Build Bot 2017-09-14 17:36:15 PDT
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.
Comment 26 youenn fablet 2017-09-14 19:03:10 PDT
Created attachment 320863 [details]
Patch for landing
Comment 27 Build Bot 2017-09-14 19:05:19 PDT
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.
Comment 28 WebKit Commit Bot 2017-09-14 20:56:53 PDT
Comment on attachment 320863 [details]
Patch for landing

Clearing flags on attachment: 320863

Committed r222073: <http://trac.webkit.org/changeset/222073>
Comment 29 WebKit Commit Bot 2017-09-14 20:56:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2017-09-27 12:27:57 PDT
<rdar://problem/34693326>