Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1131463006:
[PresentationAPI] Plumbing send(Blob) from PresentationSession IDL to platform/. (Closed)
|
Created:
5 years, 7 months ago by USE s.singapati at gmail.com Modified:
5 years, 6 months ago CC:
blink-reviews, dglazkov+blink, mark a. foltz Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Description[PresentationAPI] Plumbing send(Blob) from PresentationSession IDL to platform/.
All messages are queued in PresentationSession. Blob messages are loaded as
array buffer asynchronously and then sent in order.
This is part of a three-side change:
1. This (Blink) CL
2. Chromium CL: https://codereview.chromium.org/1140713005/
3. Cleanup: Make sendBlobData() pure virtual in WebPresentationClient.
BUG=459008
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196668
Patch Set 1 #
Total comments: 21
Patch Set 2 : #
Total comments: 5
Patch Set 3 : Refactor and queue all messages. #
Total comments: 2
Patch Set 4 : #
Total comments: 1
Messages
Total messages: 29 (10 generated)
s.singapati@samsung.com changed reviewers: + avayvod@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org
PTAL. Here is initial patch. Blob items (only) are queued and loaded as array buffer and then sent. This will make the order of sending messages incorrect as String and ArrayBuffer are sent immediately. Probably other messages should also be queued to preserve the order.
s.singapati@samsung.com changed reviewers: + imcheng@chromium.org
avayvod@chromium.org changed reviewers: + haibinlu@chromium.org - mfoltz@chromium.org
Mark and me will be at the Second Screens WG F2F meeting this week so will be slow to respond. Adding Haibin as the implementor of |onmessage|. Seems like WebRTC RTCDataChannel doesn't support Blob in Chromium. WebSocket does though, here's the implementation for the inspiration: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... The queue is indeed implemented in Blink so I think we should follow this pattern and have the common message queue here (e.g in PresentationSession, dequeue it only when the dispatcher reports sending the next message?). That might or might not simplify the dispatcher implementation of send then.
Yes, in overall this looks like need some re-factoring. 1st thing is it needs to queue String & ArrayBuffer in addition to Blob to preserve order of messages. (same as in WebSocket). Need to decide on having / not having the queue in PresentationDispatcher. But with current approach (two queues), It can have simultaneous blob loading in PresentationSession and message delivery from dispatcher to mojo service. Any proposals please?
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.cpp:60: virtual ~BlobLoader() { } override maybe? https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.cpp:65: virtual void didFinishLoading() override No need for virtual if you already have override. https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.cpp:194: if (!m_messages.isEmpty() && !m_blobLoader) { if (m_messages.isEmpty() || m_blobLoader) return; https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.cpp:255: ASSERT(m_messages.size() > 0); You probably just want to check that it is not empty. You might also want to move the check at the beginning of this method? https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.cpp:263: // FIXME: generate error message? Yes, probably. Could .send() return a Promise that you could reject? https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... File Source/modules/presentation/PresentationSession.h (right): https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.h:73: // TODO(): Check the need for queuing String and ArrayBuffer also. TODO(<handle>), plz https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.h:89: // Methods for BlobLoader. s/Methods/Callbacks/ https://codereview.chromium.org/1131463006/diff/1/public/platform/modules/pre... File public/platform/modules/presentation/WebPresentationClient.h (right): https://codereview.chromium.org/1131463006/diff/1/public/platform/modules/pre... public/platform/modules/presentation/WebPresentationClient.h:50: virtual void sendBlobData(const WebString& presentationUrl, const WebString& presentationId, const uint8_t* data, size_t length) Could you say in the comment what should be done with |data| with regards to pointer ownership?
mfoltz@chromium.org changed reviewers: + mfoltz@chromium.org
Concerns: - This seems not very efficient. It's reading file blobs into the renderer process only to turn around and pass the contents back to the presentation service in the browser process. Can we push a file handle onto the message queue instead and do the reading in the presentation service side? - How does this work with Blobs that are not backed by files (i.e. created using the Blob constructor)? https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... File Source/modules/presentation/PresentationSession.h (right): https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.h:73: // TODO(): Check the need for queuing String and ArrayBuffer also. What does this mean? String and ArrayBuffer should be available immediately as their data content is part of the script value. https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.h:88: class BlobLoader; Should this be class BlobLoader { void didFinishLoadingBlob() void didFailLoadingBlob() } Otherwise I don't follow how the methods below are related to the BlobLoader forward declaration. https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.h:88: class BlobLoader; Forward declarations should go immediately after private (like typedefs): http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
Addressed current review comments.
RE: About concerns:
- This seems not very efficient. It's reading file blobs into the renderer
process only to turn around and pass the contents back to the presentation
service in the browser process. Can we push a file handle onto the message queue
instead and do the reading in the presentation service side?
- How does this work with Blobs that are not backed by files (i.e. created using
the Blob constructor)?
I will to check the possibility of sending BlobDataHandle (uuid, type, size) to
the presentation service in browser process instead of reading it in renderer.
And I have tested current implementation using for eg., new Blob(['foo']),
Blob([xhr.response], {type: "image/png"}) etc. Need to do more testing with File
/ FileReader apis.
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationSession.cpp (right):
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:60: virtual ~BlobLoader() {
}
On 2015/05/19 13:08:55, Mounir Lamouri wrote:
> override maybe?
Done.
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:65: virtual void
didFinishLoading() override
On 2015/05/19 13:08:54, Mounir Lamouri wrote:
> No need for virtual if you already have override.
Done.
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:194: if
(!m_messages.isEmpty() && !m_blobLoader) {
On 2015/05/19 13:08:54, Mounir Lamouri wrote:
> if (m_messages.isEmpty() || m_blobLoader)
> return;
Done.
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:255:
ASSERT(m_messages.size() > 0);
On 2015/05/19 13:08:55, Mounir Lamouri wrote:
> You probably just want to check that it is not empty. You might also want to
> move the check at the beginning of this method?
Done.
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.cpp:263: // FIXME: generate
error message?
On 2015/05/19 13:08:54, Mounir Lamouri wrote:
> Yes, probably. Could .send() return a Promise that you could reject?
Seems to be API change.
Mark: Do you have any comments?
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
File Source/modules/presentation/PresentationSession.h (right):
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.h:73: // TODO(): Check the need
for queuing String and ArrayBuffer also.
On 2015/05/20 15:40:04, mfoltz_ooo_until_5-26 wrote:
> What does this mean? String and ArrayBuffer should be available immediately
as
> their data content is part of the script value.
Updated comments. Currently String and ArrayBuffer data is sent immediately to
the client, but Blob data is loaded as array buffer and then sent. This makes
message sending order incorrect. If we go with current approach (loading Blobs
here), then we probably need to queue String & ArrayBuffer also to preserve the
order.
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.h:88: class BlobLoader;
On 2015/05/20 15:40:04, mfoltz_ooo_until_5-26 wrote:
> Forward declarations should go immediately after private (like typedefs):
>
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
Done.
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.h:88: class BlobLoader;
On 2015/05/20 15:40:04, mfoltz_ooo_until_5-26 wrote:
> Should this be class BlobLoader {
> void didFinishLoadingBlob()
> void didFailLoadingBlob()
> }
>
> Otherwise I don't follow how the methods below are related to the BlobLoader
> forward declaration.
Done. Updated comment. didFinishLoadingBlob() & didFailLoadingBlob() are invoked
from BlobLoader.
https://codereview.chromium.org/1131463006/diff/1/Source/modules/presentation...
Source/modules/presentation/PresentationSession.h:89: // Methods for BlobLoader.
On 2015/05/19 13:08:55, Mounir Lamouri wrote:
> s/Methods/Callbacks/
Done.
https://codereview.chromium.org/1131463006/diff/1/public/platform/modules/pre...
File public/platform/modules/presentation/WebPresentationClient.h (right):
https://codereview.chromium.org/1131463006/diff/1/public/platform/modules/pre...
public/platform/modules/presentation/WebPresentationClient.h:50: virtual void
sendBlobData(const WebString& presentationUrl, const WebString& presentationId,
const uint8_t* data, size_t length)
On 2015/05/19 13:08:55, Mounir Lamouri wrote:
> Could you say in the comment what should be done with |data| with regards to
> pointer ownership?
Done. Updated comments.
On 2015/05/20 15:40:04, mfoltz_ooo_until_5-26 wrote: > Concerns: > > - This seems not very efficient. It's reading file blobs into the renderer > process only to turn around and pass the contents back to the presentation > service in the browser process. Can we push a file handle onto the message > queue instead and do the reading in the presentation service side? > How about for eg., if Android has it's own implementation (eg., PresentationServiceImpl.java) implementing src/content/common/presentation/presentation_service.mojom? I think Blob reading in Browser process should be common for all platforms. Any ideas please?
On 2015/05/25 at 19:22:33, s.singapati wrote: > On 2015/05/20 15:40:04, mfoltz_ooo_until_5-26 wrote: > > Concerns: > > > > - This seems not very efficient. It's reading file blobs into the renderer > > process only to turn around and pass the contents back to the presentation > > service in the browser process. Can we push a file handle onto the message > > queue instead and do the reading in the presentation service side? > > > How about for eg., if Android has it's own implementation (eg., PresentationServiceImpl.java) implementing src/content/common/presentation/presentation_service.mojom? > I think Blob reading in Browser process should be common for all platforms. Any ideas please? The upside of not doing a round trip to the browser twice for one thing outweigh any potential code redundancy. Android implementation will likely reuse the C++ PresentationServiceImpl implementation using JNI further down the pipe to talk to the Android APIs instead of the component extension on desktop.
First, about loading Blobs in the browser. I don't see a good way of doing that at the moment, since the logic for constructing Blobs from file contents lives in Blink. I think it does make sense to pass handles for the PresentationSession/RTCDataChannel/WebSocket and similar messaging APIs, but we would need to consult with the Blink owners about the right way to do this to share code and handle any security concerns. Second, I think the change looks good in terms of constructing and sending Blobs. However I don't see how we will be able to maintain message order with a queue on the PresentationSession side for Blob messages and a queue in the PresentationDispatcher for non-Blob messages. Options that come to mind: (1) Send a signal to the PresentationDispatcher that there is a blob read in progress and pause forwarding messages to the PresentationService. (2) Move the queue from PresentationDispatcher to PresentationSession and queue all messages in PresentationSession. https://codereview.chromium.org/1131463006/diff/20001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1131463006/diff/20001/Source/modules/presenta... Source/modules/presentation/PresentationSession.cpp:198: ASSERT(!m_blobLoader); I don't think this is necessary because of the return at L195. https://codereview.chromium.org/1131463006/diff/20001/Source/modules/presenta... Source/modules/presentation/PresentationSession.cpp:265: // FIXME: generate error message? There's currently no clean way to signal to the page that message sending failed. We could just close the session, but I would rather come up with an API for signaling errors (i.e., PresentationSession.onerror). Anton, WDYT? https://codereview.chromium.org/1131463006/diff/20001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.h (right): https://codereview.chromium.org/1131463006/diff/20001/Source/modules/presenta... Source/modules/presentation/PresentationSession.h:74: // TODO(s.singapati): Currently String and ArrayBuffer data is sent It looks like the way that WebSocket implements send(Blob) is to: 1. keep the blob handle at the head of the queue & start a BlobLoader 2. blocks further sends 3. when loading is complete, replace the handle with the blob contents 4. resumes the queue https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Can we do something similar here?
PTAL. RE: On 2015/05/26 20:41:48, mark a. foltz wrote: > First, about loading Blobs in the browser. I don't see a good way of doing that > at the moment, since the logic for constructing Blobs from file contents lives > in Blink. I think it does make sense to pass handles for the > PresentationSession/RTCDataChannel/WebSocket and similar messaging APIs, but we > would need to consult with the Blink owners about the right way to do this to > share code and handle any security concerns. I have tried to use storage::BlobDataHandle / BlobStorageContext classes in browser process, but it didn't seem simple. (Didn't get much time to work on it though) > Second, I think the change looks good in terms of constructing and sending > Blobs. However I don't see how we will be able to maintain message order with a > queue on the PresentationSession side for Blob messages and a queue in the > PresentationDispatcher for non-Blob messages. > > Options that come to mind: > > (1) Send a signal to the PresentationDispatcher that there is a blob read in > progress and pause forwarding messages to the PresentationService. > > (2) Move the queue from PresentationDispatcher to PresentationSession and queue > all messages in PresentationSession. PatchSet#3: 1. Queue all messages in PresentationSession 2. If Blob, queue the handle and start loading immediately 3. But, block the sends while Blob is loading 4. send the loaded blob 5. resume the queue. I have done manual testing (eg., from Presentation Class) from idl to PresentationServiceImpl. Without any other changes in Chromium implementation, messages are queued in PresentationDispacther and works as it is now. This approach looks simple and does not need extra notifications from PresentationDispacther to PresentationSession. Also, sending from PresentationDispacther queue to PresentationService can be simultaneous. https://codereview.chromium.org/1131463006/diff/20001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1131463006/diff/20001/Source/modules/presenta... Source/modules/presentation/PresentationSession.cpp:198: ASSERT(!m_blobLoader); On 2015/05/26 20:41:48, mark a. foltz wrote: > I don't think this is necessary because of the return at L195. Done. https://codereview.chromium.org/1131463006/diff/20001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.h (right): https://codereview.chromium.org/1131463006/diff/20001/Source/modules/presenta... Source/modules/presentation/PresentationSession.h:74: // TODO(s.singapati): Currently String and ArrayBuffer data is sent On 2015/05/26 20:41:48, mark a. foltz wrote: > It looks like the way that WebSocket implements send(Blob) is to: > > 1. keep the blob handle at the head of the queue & start a BlobLoader > 2. blocks further sends > 3. when loading is complete, replace the handle with the blob contents > 4. resumes the queue > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Can we do something similar here? Done. One difference is, this patch does not replace the handle with the loaded blob contents, But sends it immediately, then continue processing the queue.
LGTM, but I'm not sure how to respond to your question in the comments. https://codereview.chromium.org/1131463006/diff/40001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1131463006/diff/40001/Source/modules/presenta... Source/modules/presentation/PresentationSession.cpp:279: // TODO(s.singapati): Is it more safe to call m_blobLoader.clear() here? Where is clear() defined? I don't see it here, or on GarbageCollectedFinalized<> or FileReaderLoaderClient.
https://codereview.chromium.org/1131463006/diff/40001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1131463006/diff/40001/Source/modules/presenta... Source/modules/presentation/PresentationSession.cpp:279: // TODO(s.singapati): Is it more safe to call m_blobLoader.clear() here? On 2015/06/01 17:39:28, mark a. foltz wrote: > Where is clear() defined? I don't see it here, or on > GarbageCollectedFinalized<> or FileReaderLoaderClient. Member::clear(), https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I was thinking that between the execution of lines 269 and 278, is there any possibility that handleMessageQueue() is invoked and lead to incorrect behavior. well at least there won't be any problem moving m_blobLoader.clear() here. I will update the patch if there is no other concerns with this.
PTAL. Mounir Lamouri: Any comments please? https://codereview.chromium.org/1131463006/diff/60001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1131463006/diff/60001/Source/modules/presenta... Source/modules/presentation/PresentationSession.cpp:277: m_blobLoader.clear(); Did manual testing. Tried to but could not simulate the scenario that handleMessageQueue() is invoked before removing the first item (current blob) in the queue. But just to be more strict, moved m_blobLoader.clear() here.
The CQ bit was checked by s.singapati@samsung.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/1131463006/#ps60001 (title: " ")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1131463006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by s.singapati@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1131463006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/65617)
The CQ bit was checked by s.singapati@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1131463006/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196668 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
