Bug 112314

Created attachment 193229 [details] Patch

tommyw: would you take a look?

Comment on attachment 193229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193229&action=review It looks really good but maybe you should try to get someone from e.g. WebSocket to have a look as well. > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:172 > { Please check that the data channel isn't closed first. > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:299 > + // FIXME: Will determine if raising exception should be necessary. It isn't possible to throw an exception here so you can remove the comment. > LayoutTests/fast/mediastream/RTCPeerConnection-datachannel.html:87 > +shouldBe("dc.binaryType", "'blob'"); Nit: You can use shouldBeEqualToString here instead.

Hi Takashi Toyoshima, Could you please review this patch? The implementation looks like WebSocket. Thanks in advance.

Created attachment 193273 [details] Patch

(In reply to comment #3) > (From update of attachment 193229 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193229&action=review > > It looks really good but maybe you should try to get someone from e.g. WebSocket to have a look as well. > > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:172 > > { > > Please check that the data channel isn't closed first. > > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:299 > > + // FIXME: Will determine if raising exception should be necessary. > > It isn't possible to throw an exception here so you can remove the comment. > > > LayoutTests/fast/mediastream/RTCPeerConnection-datachannel.html:87 > > +shouldBe("dc.binaryType", "'blob'"); > > Nit: You can use shouldBeEqualToString here instead. Fixed. Thanks for your review.

+tkent Hi Li, I'm familiar with WebSocket, but not a WebKit reviewer yet. tkent or ap looks proper reviewers in this case. Kent-san, can you take a look?

Created attachment 193549 [details] Patch

Update the patch for handling failed blob loading. When the blob data can't be loaded correctly (e.g. the File or Blob resource could not be found at the time the read was processed), skip the blob, and continue to send the next.

Comment on attachment 193549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193549&action=review > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:278 > + String errorMessage("Failed to load Blob: error code = " + String::number(errorCode)); Nit: There's no need for a temporary String here; you can simply add the string directly to the call to 'addConsoleMessage'. Relatedly, is there any more detail we can provide here beyond an opaque number? Does "1" in the error code that shows up in the tests actually mean anything? As a developer, I'm not sure what I'd do with this information.

Comment on attachment 193549 [details] Patch Attachment 193549 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17066817

Created attachment 193800 [details] Patch

(In reply to comment #9) > Update the patch for handling failed blob loading. > When the blob data can't be loaded correctly (e.g. the File or Blob resource could not be found at the time the read was processed), skip the blob, and continue to send the next. When the blob data can't be loaded successfully, user agent must close the channel, like what WebSocket is doing. And add the sub test for failed blob loading.

(In reply to comment #13) > (In reply to comment #9) > > Update the patch for handling failed blob loading. > > When the blob data can't be loaded correctly (e.g. the File or Blob resource could not be found at the time the read was processed), skip the blob, and continue to send the next. > > When the blob data can't be loaded successfully, user agent must close the channel, like what WebSocket is doing. > And add the sub test for failed blob loading. Actually it isn't specified in the WebRTC spec what should happen if a blob fails to load but after discussing it with hta we agree that this is probably the best thing to do.

I think this (closing) is the right way - but can you point me at the place in the WebSocket API spec that says what to do when the data can't be loaded correctly? I couldn't find it on a quick read - the send data methods all say "fail with prejudice if the data can't be sent" - is that the one you're thinking of?

(In reply to comment #15) > I think this (closing) is the right way - but can you point me at the place in the WebSocket API spec that says what to do when the data can't be loaded correctly? I couldn't find it on a quick read - the send data methods all say "fail with prejudice if the data can't be sent" - is that the one you're thinking of? Yes, and see the discussion https://bugs.webkit.org/show_bug.cgi?id=112534#c5

Comment on attachment 193800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193800&action=review > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:178 > + m_outgoingBlobQueue.append(Blob::create(data->url(), data->type(), data->size())); Why do we need to create a new blob here? > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:229 > + OwnPtr<Vector<char> > binaryData = adoptPtr(new Vector<char>(dataLength)); > + memcpy(binaryData->data(), data, dataLength); > + RefPtr<RawData> rawData = RawData::create(); > + binaryData->swap(*rawData->mutableData()); > + OwnPtr<BlobData> blobData = BlobData::create(); > + blobData->appendData(rawData.release(), 0, BlobDataItem::toEndOfFile); > + RefPtr<Blob> blob = Blob::create(blobData.release(), dataLength); This all seems really low-level for this file. Does Blob not know how to create itself from a sequence of bytes? The benefit of using a blob is that you don't need to bring all the data into memory. If you're going to bring all the data into memory anyway, you might as well use an array buffer. To have a high-quality blob implementation, we'd need to pass an opaque handle to the data from its source all the way here.

(In reply to comment #17) > (From update of attachment 193800 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193800&action=review > > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:178 > > + m_outgoingBlobQueue.append(Blob::create(data->url(), data->type(), data->size())); > > Why do we need to create a new blob here? > > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:229 > > + OwnPtr<Vector<char> > binaryData = adoptPtr(new Vector<char>(dataLength)); > > + memcpy(binaryData->data(), data, dataLength); > > + RefPtr<RawData> rawData = RawData::create(); > > + binaryData->swap(*rawData->mutableData()); > > + OwnPtr<BlobData> blobData = BlobData::create(); > > + blobData->appendData(rawData.release(), 0, BlobDataItem::toEndOfFile); > > + RefPtr<Blob> blob = Blob::create(blobData.release(), dataLength); > > This all seems really low-level for this file. Does Blob not know how to create itself from a sequence of bytes? Blob has no direct method to do that, it can be created through BlobData. > > The benefit of using a blob is that you don't need to bring all the data into memory. If you're going to bring all the data into memory anyway, you might as well use an array buffer. To have a high-quality blob implementation, we'd need to pass an opaque handle to the data from its source all the way here. The data has been in memory already, I encapsulated the raw data into a blob object, meet the requirement of RTCDataChannel API. And WebSocket is doing the same thing. Please See: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/websockets/WebSocket.cpp#L531

> Blob has no direct method to do that, it can be created through BlobData. Yes, we would need to change that. > The data has been in memory already, I encapsulated the raw data into a blob object, meet the requirement of RTCDataChannel API. I understand. I'm saying that's a poor implementation. > And WebSocket is doing the same thing. WebSockets has a different architecture. In WebSockets, we need to bring all the data into memory because the protocol handling is done in WebCore. For WebRTC, the protocol handling is done outside of WebCore. That means the handle to the data should be created outside of WebCore.

(In reply to comment #19) > > Blob has no direct method to do that, it can be created through BlobData. > > Yes, we would need to change that. I agree with you, we can move the implementation to Blob.cpp, avoid code copies in WebSocket and RTCDataChannel. > > > The data has been in memory already, I encapsulated the raw data into a blob object, meet the requirement of RTCDataChannel API. > > I understand. I'm saying that's a poor implementation. > > > And WebSocket is doing the same thing. > > WebSockets has a different architecture. In WebSockets, we need to bring all the data into memory because the protocol handling is done in WebCore. For WebRTC, the protocol handling is done outside of WebCore. That means the handle to the data should be created outside of WebCore. Do you mean we save the raw data into a file and return the file object to Web Developers when onmessage event handle was triggered? If that is true, the data will be copied from memory to disk at first, and then copied from disk to memory when Web developers read the blob(in fact File object). It will have two extra functions, reading and writing file, I think there will be a lower performance.

(In reply to comment #17) > (From update of attachment 193800 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193800&action=review > > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:178 > > + m_outgoingBlobQueue.append(Blob::create(data->url(), data->type(), data->size())); > > Why do we need to create a new blob here? Because blob will be handled asynchronously, we need create a new Blob object. The original blob may be destructed because of gc when the loading resource function is invoked.

(In reply to comment #19) > WebSockets has a different architecture. In WebSockets, we need to bring all the data into memory because the protocol handling is done in WebCore. For WebRTC, the protocol handling is done outside of WebCore. That means the handle to the data should be created outside of WebCore. I don't think so, there are two reasons as followed: 1. the binary data will be interpreted to be arraybuffer or blob, which is determined by m_binaryType, and m_binaryType variable is only accessed in WebCore. 2. If we move this code away from WebCore, all other ports will implement the same codes like chromium.

Hi abarth, Do you think my clarification is reasonable? Could you please have a look again?

Closing as this is no longer relevant.