Make NetworkSendQueue use Cstring instead of String for UTF-8 data
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>