Bug 213714

Created attachment 403040 [details]
Patch

Created attachment 403053 [details]
Patch

Created attachment 403059 [details]
Patch

Comment on attachment 403059 [details]
Patch

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

Test failures look like they might be caused by the refactoring.

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:124
> +    // FIXME: We might want to use strict conversion like WebSocket.

Maybe test coverage would help us decide this?

> Source/WebCore/fileapi/NetworkSendQueue.h:49
> +    using WriteString = Function<void(const CString&)>;

Might be helpful to make it clear that this is UTF-8 encoded. In WTF::String that’s included in the definition of the class, but not WTF::CString, so we might need an argument name?

> Source/WebCore/fileapi/NetworkSendQueue.h:56
> +    void enqueue(const CString&);

Would be nice to take a CString&&, add some WTFMove at call sites, and save a little bit of reference count churn.

Might also be helpful to make it clear that this is UTF-8 encoded. In WTF::String that’s included in the definition of the class, but not WTF::CString, so we might need an argument name?

> Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:120
> +    auto text = adoptNS([[NSString alloc] initWithBytes:utf8String.data() length:utf8String.size() encoding:NSUTF8StringEncoding]);

Do we have a guarantee that this won’t fail UTF-8 decoding? If not, might need a null check.

> Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:121
> +    auto message = adoptNS([[NSURLSessionWebSocketMessage alloc] initWithString: text.get()]);

Stray space after the colon.

(In reply to Darin Adler from comment #4)
> Comment on attachment 403059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403059&action=review
> 
> Test failures look like they might be caused by the refactoring.
> 
> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:124
> > +    // FIXME: We might want to use strict conversion like WebSocket.
> 
> Maybe test coverage would help us decide this?

Right, this patch keeps the status quo and I plan to validate one way or the other as a follow-up.

> > Source/WebCore/fileapi/NetworkSendQueue.h:49
> > +    using WriteString = Function<void(const CString&)>;
> 
> Might be helpful to make it clear that this is UTF-8 encoded. In WTF::String
> that’s included in the definition of the class, but not WTF::CString, so we
> might need an argument name?

OK

> > Source/WebCore/fileapi/NetworkSendQueue.h:56
> > +    void enqueue(const CString&);
> 
> Would be nice to take a CString&&, add some WTFMove at call sites, and save
> a little bit of reference count churn.

I'll look into that.
This is not done as in most cases, no count churn will anyway happen (it only happens in case of mixing blob calls with string calls, which is unusual). 

But, since the call sites create the Ref and do not use after...

> Might also be helpful to make it clear that this is UTF-8 encoded. In
> WTF::String that’s included in the definition of the class, but not
> WTF::CString, so we might need an argument name?

OK.

> > Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:120
> > +    auto text = adoptNS([[NSString alloc] initWithBytes:utf8String.data() length:utf8String.size() encoding:NSUTF8StringEncoding]);
> 
> Do we have a guarantee that this won’t fail UTF-8 decoding? If not, might
> need a null check.

This comes straight from IPC so we cannot be sure.
Will fix this.

> > Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:121
> > +    auto message = adoptNS([[NSURLSessionWebSocketMessage alloc] initWithString: text.get()]);
> 
> Stray space after the colon.

OK

Created attachment 403186 [details]
Patch

Windows crash seems related?

Comment on attachment 403186 [details]
Patch

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

r=me assuming Windows failures are not due to these changes

> Source/WebCore/fileapi/NetworkSendQueue.h:49
> +    using WriteString = Function<void(const CString& /* utf8 */)>;

Could use an argument name. It does not need to be commented out. Also, in a comment I would spell it UTF-8, not utf8.

> Source/WebCore/fileapi/NetworkSendQueue.h:56
> +    void enqueue(CString&& /* utf8 */);

Ditto.

Win failures seem to be due to http://trac.webkit.org/changeset/263700/webkit as per https://results.webkit.org/?suite=layout-tests&test=http%2Fwpt%2Ffetch%2Frequest-stream-disturbed-1.html.

(In reply to youenn fablet from comment #9)
> Win failures seem to be due to
> http://trac.webkit.org/changeset/263700/webkit as per
> https://results.webkit.org/?suite=layout-
> tests&test=http%2Fwpt%2Ffetch%2Frequest-stream-disturbed-1.html.

https://bugs.webkit.org/show_bug.cgi?id=213792

Created attachment 403277 [details]
Patch for landing

Committed r263797: <https://trac.webkit.org/changeset/263797>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403277 [details].

<rdar://problem/64986255>