[Cache API] Add a WK2 implementation of CacheStorageConnection

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

[Cache API] Add a WK2 implementation of CacheStorageConnection

[youenn fablet]

Created attachment 318294 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:84:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:153:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:253:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 42 files


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

[youenn fablet]

Created attachment 318297 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:84:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:153:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:253:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 42 files


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

[youenn fablet]

Created attachment 318301 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:84:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:153:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:253:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 42 files


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

[youenn fablet]

Created attachment 318303 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:212:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:84:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:153:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:253:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 42 files


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

[youenn fablet]

Created attachment 318326 [details]
Patch

[youenn fablet]

Created attachment 318329 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:84:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:170:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:271:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 43 files


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

[youenn fablet]

Created attachment 318331 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:84:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:170:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:271:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 45 files


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

[Alex Christensen]

Comment on attachment 318331 [details]
Patch

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

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.cpp:43
> +    if (!WebProcess::singleton().networkConnection().connection().sendSync(Messages::NetworkConnectionToWebProcess::CreateCacheStorageConnection(sessionID), Messages::NetworkConnectionToWebProcess::CreateCacheStorageConnection::Reply(m_identifier), 0)) {

Does this need to be synchronous?

[youenn fablet]

Comment on attachment 318331 [details]
Patch

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

>> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.cpp:43
>> +    if (!WebProcess::singleton().networkConnection().connection().sendSync(Messages::NetworkConnectionToWebProcess::CreateCacheStorageConnection(sessionID), Messages::NetworkConnectionToWebProcess::CreateCacheStorageConnection::Reply(m_identifier), 0)) {
> 
> Does this need to be synchronous?

Given a connection is created once per process in most cases, being synchronous is fine here seems acceptable.
If we do not want to make it synchronous, we have two options:
- Buffer the requests (open/remove...) that will be flowing as soon as the connection is created. Does not seem worth it.
- Pass the sessionID instead of the identifier. This is doable but is not a usual pattern and would not interact well with IPC identifier thing.

[youenn fablet]

> Given a connection is created once per process in most cases, being
> synchronous is fine here seems acceptable.
> If we do not want to make it synchronous, we have two options:
> - Buffer the requests (open/remove...) that will be flowing as soon as the
> connection is created. Does not seem worth it.
> - Pass the sessionID instead of the identifier. This is doable but is not a
> usual pattern and would not interact well with IPC identifier thing.

Talking with Alex, let's go with option 2.

[Chris Dumez]

Comment on attachment 318331 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:454
> +    m_cacheStorageConnections.take(identifier);

remove() since you're not using the value returned by take().

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:37
> +static std::unique_ptr<CacheStorageEngine>& defaultCacheStorageEngine()

Should be merged into CacheStorageEngine::defaultEngine(), there is no need for 2 functions.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:40
> +    static NeverDestroyed<std::unique_ptr<CacheStorageEngine>> engine;

You can initialize right away: = std::make_unique<CacheStorageEngine>();

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:75
> +    readCaches([this, origin, cacheName, callback = WTFMove(callback)](std::optional<Error>&& error) mutable {

Based on the readCaches() name and the fact that it takes in a lambda, it is not clear to me:
1. What readCaches() actually does
2. What the passed lambda is for

Should readCaches() pass what was read to the lambda?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:77
> +            callback(UnexpectedType<Error> { error.value() });

makeUnexpected() ?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:82
> +        auto& caches = result.iterator->value;

Could be merged into the previous line.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:90
> +                    callback(UnexpectedType<Error> { error.value() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:113
> +        callback(UnexpectedType<Error> { Error::Internal });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:119
> +            callback(UnexpectedType<Error> { error.value() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:130
> +            callback(UnexpectedType<Error> { error.value() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:141
> +            callback(UnexpectedType<Error> { result.error() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:151
> +    readCache(cacheIdentifier, [this, cacheIdentifier, records = WTFMove(records), callback = WTFMove(callback)](CacheOrError&& result) mutable {

We have used the opposite naming so far:
ExceptionOr<>
ResourceErrorOr<>

Also usually templated so this could be ErrorOr<Cache> maybe?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:153
> +            callback(UnexpectedType<Error> { result.error() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:159
> +        WebCore::CacheQueryOptions options;

using namespace WebCore; ?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:164
> +            if (!matchingRecords.size()) {

matchingRecords.isEmpty()

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:180
> +        writeCacheRecords(cacheIdentifier, WTFMove(recordIdentifiers), [cacheIdentifier, callback = WTFMove(callback)](RecordIdentifiersOrError&& result) mutable {

ErrorOr<RecordIdentifier> ?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:190
> +            callback(UnexpectedType<Error> { result.error() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:197
> +        if (!recordsToRemove.size()) {

recordsToRemove.isEmpty()

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:209
> +                callback(UnexpectedType<Error> { result.error() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:215
> +                callback(UnexpectedType<Error> { Error::Internal });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:227
> +    // FIXME: Implement writing

Missing period at the end.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:233
> +    // FIXME: Implement reading

Missing period at the end.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:239
> +    // FIXME: Implement reading

Missing period at the end.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:242
> +        callback(UnexpectedType<Error> { Error::Internal });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:250
> +    // FIXME: Implement writing

Missing period at the end.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:251
> +    callback(WTFMove(recordsIdentifiers));

What's the point of passing the records identifiers to the callback if they are passed in by the client anyway?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:256
> +    // FIXME: Implement writing

Missing period at the end.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:257
> +    callback(WTFMove(recordsIdentifiers));

What's the point of passing the records identifiers to the callback if they are passed in by the client anyway?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:260
> +CacheStorageEngine::Cache* CacheStorageEngine::cache(uint64_t cacheIdentifier)

We often write:
auto CacheStorageEngine::cache(uint64_t cacheIdentifier) -> Cache*

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:57
> +    CacheStorageEngine() = default;

Why is this needed?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:59
> +    using CacheIdentifierOrError = Expected<uint64_t, Error>;

See earlier comment about using ErrorOr<> for consistency with the rest of the code base.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:62
> +    using CacheInfosOrError = Expected<Vector<WebCore::CacheStorageConnection::CacheInfo>, Error>;

ditto

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:65
> +    using RecordsOrError = Expected<Vector<Record>, Error>;

ditto.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:68
> +    using RecordIdentifiersOrError = Expected<Vector<uint64_t>, Error>;

ditto.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:73
> +    void open(const String& /* origin */, const String& /* cacheName */, CacheIdentifierCallback&&);

Why the /* */? You don't need them. Since the parameter names are useful, just have them. The style checker only complains about redundant parameter names.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:74
> +    void remove(uint64_t /* cacheIdentifier */, CacheIdentifierCallback&&);

ditto.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:84
> +    void writeCaches(CompletionCallback&&);

ToDisk?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:85
> +    void readCaches(CompletionCallback&&);

FromDisk?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:94
> +    using CacheOrError = Expected<std::reference_wrapper<Cache>, Error>;

ErrorOr<>?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:109
> +    uint64_t m_nextCacheIdentifier { 0 };

Any reason this cannot be a static in the cpp? Is extCacheIdentifier going to be used from several threads for e.g.?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineConnection.cpp:1
> +

Blank line?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineConnection.h:63
> +    uint64_t m_identifier { 0 };

Doesn't seem like this needs a default value?

> Source/WebKit/Platform/IPC/ArgumentCoders.h:406
> +            expected = WTF::UnexpectedType<ErrorType> { WTFMove(error) };

makeUnexpected()?

> Source/WebKit/Platform/IPC/ArgumentCoders.h:407
> +        }

Seems like you should return true here? Otherwise you go on to try and read a value :(

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:180
> +    options.cacheName = cacheName;

WTFMove(cacheName)?

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:200
> +    if (!decoder.decode(type))

I am fuzzy on the details but don't we usually use encodeEnum() / decodeEnum() for enum types?

> Source/WebKit/Shared/WebCoreArgumentCoders.h:792
> +template<> struct EnumTraits<WebCore::CacheStorageConnection::Error> {

Not familiar with this part of the code but do we really need those if we use encodeEnum() / decodeEnum()? IDB's KeyType uses encodeEnum() / decodeEnum() and I don't see such EnumTraits.

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.cpp:1
> +

Blank line?

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.cpp:93
> +    CacheStorageConnection::openCompleted(requestIdentifier, result.hasValue() ? result.value() : 0, !result.hasValue() ? Error::Internal : Error::None);

It is really unfortunate we are passing both a value and an error here (and below). I already commented about this in the previous patch.

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.h:59
> +    void doOpen(uint64_t requestIdentifier, const String& /* origin */, const String& /* cacheName */) final;

/* */ are not needed.

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.h:60
> +    void doRemove(uint64_t requestIdentifier, uint64_t /* cacheIdentifier */) final;

/* */ are not needed.

[Chris Dumez]

In the future, it'd be nice to upload smaller patches. This is really hard to review properly.

[youenn fablet]

Created attachment 318448 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:79:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:164:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:265:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 44 files


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

[youenn fablet]

Thanks for the review.
Patch is big but I hope this is the last one.

I fixed according your comments except for ErrorOr<> and related aliases.
It was hard to make it look good, the names becoming quite big.

As of encodeEnum/decodeEnum, the trait adds a check that the enum value is valid.


> Based on the readCaches() name and the fact that it takes in a lambda, it is
> not clear to me:
> 1. What readCaches() actually does
> 2. What the passed lambda is for
> 
> Should readCaches() pass what was read to the lambda?

Changed it to readCachesFromDisk.
When implementing readCaches, I think it might take an Origin, populate the HashMap and pass the Caches object directly to the lambda.

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:251
> > +    callback(WTFMove(recordsIdentifiers));
> 
> What's the point of passing the records identifiers to the callback if they
> are passed in by the client anyway?

The ids are useful for the future processing of the method.
And the callback needs it also.
The callback could have it but this would end up making an extra copy.

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:109
> > +    uint64_t m_nextCacheIdentifier { 0 };
> 
> Any reason this cannot be a static in the cpp? Is extCacheIdentifier going
> to be used from several threads for e.g.?

Not sure about it right now.
I can add a FIXME.

> > Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.cpp:93
> > +    CacheStorageConnection::openCompleted(requestIdentifier, result.hasValue() ? result.value() : 0, !result.hasValue() ? Error::Internal : Error::None);
> 
> It is really unfortunate we are passing both a value and an error here (and
> below). I already commented about this in the previous patch.

This will be fixed in a follow-up

[youenn fablet]

Comment on attachment 318448 [details]
Patch

Some troubles with PAL/identifier/SessionID.h

[youenn fablet]

Created attachment 318465 [details]
Patch

[youenn fablet]

Created attachment 318466 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:162:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:263:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 44 files


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

[Alex Christensen]

Comment on attachment 318466 [details]
Patch

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

> Source/WebKit/ChangeLog:81
> +        * Shared/WebCoreArgumentCoders.h:

I think we should move away from WebCoreArgumentCoders and towards template encoders in WebCore headers.

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:43
> +    default:

This should be case Error::Internal: instead of default: so that if we add a new error type in the future it will cause a compiler error if we forget to update this, too.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:103
> +    Cache* cache(uint64_t cacheIdentifier);

I think it would make sense to pass a PAL::SessionID here instead of a uint64_t with a name of cacheIdentifier to make it clear that the SessionID is the cacheIdentifier.  Same with other functions in this class, and even the functions called through IPC.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:116
> +    WebKit::CacheStorageEngine::Error,
> +    WebKit::CacheStorageEngine::Error::Internal

Elsewhere in the patch you indent these more.

> Source/WebKit/Platform/IPC/ArgumentCoders.h:384
> +template<typename ValueType, typename ErrorType> struct ArgumentCoder<WTF::Expected<ValueType, ErrorType>> {

Cool!

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.cpp:1
> +

Extra space

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.h:55
> +    void doOpen(uint64_t requestIdentifier, const String& /* origin */, const String& /* cacheName */) final;

I think the /* */ are unnecessary.

[youenn fablet]

Thanks for the review.

> > Source/WebKit/ChangeLog:81
> > +        * Shared/WebCoreArgumentCoders.h:
> 
> I think we should move away from WebCoreArgumentCoders and towards template
> encoders in WebCore headers.

I can see the point. I went the usual way here.

> > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:43
> > +    default:
> 
> This should be case Error::Internal: instead of default: so that if we add a
> new error type in the future it will cause a compiler error if we forget to
> update this, too.

Win bot has difficulties with that when returning in switch statements and not returning anything after the switch statement, hence the current design.

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:103
> > +    Cache* cache(uint64_t cacheIdentifier);
> 
> I think it would make sense to pass a PAL::SessionID here instead of a
> uint64_t with a name of cacheIdentifier to make it clear that the SessionID
> is the cacheIdentifier.  Same with other functions in this class, and even
> the functions called through IPC.

The CacheStorageEngine is session specific.
Each cache inside the CacheStorageEngine is session ID specific.
CacheStorageEngineConnection is responsible to do the selection of the CacheStorageEngine according the session ID.

I am not exactly sure how to test that.
I guess there might be internals to open pages/iframes in private mode.

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:116
> > +    WebKit::CacheStorageEngine::Error,
> > +    WebKit::CacheStorageEngine::Error::Internal
> 
> Elsewhere in the patch you indent these more.

OK

> > Source/WebKit/Platform/IPC/ArgumentCoders.h:384
> > +template<typename ValueType, typename ErrorType> struct ArgumentCoder<WTF::Expected<ValueType, ErrorType>> {
> 
> Cool!
> 
> > Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.cpp:1
> > +
> 
> Extra space

OK 

> > Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.h:55
> > +    void doOpen(uint64_t requestIdentifier, const String& /* origin */, const String& /* cacheName */) final;
> 
> I think the /* */ are unnecessary.

OK

[youenn fablet]

Created attachment 318496 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:162:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:263:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 44 files


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

[WebKit Commit Bot]

Comment on attachment 318496 [details]
Patch for landing

Rejecting attachment 318496 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 318496, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4337222

[youenn fablet]

Created attachment 318499 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:162:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:263:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 44 files


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

[WebKit Commit Bot]

Comment on attachment 318499 [details]
Patch for landing

Clearing flags on attachment: 318499

Committed r220917: <http://trac.webkit.org/changeset/220917>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33963325>