Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1002293005:
[PresentationAPI] Plumbing send() from PresentationSession IDL to platform/. (Closed)
|
Created:
5 years, 9 months ago by USE s.singapati at gmail.com Modified:
5 years, 8 months ago CC:
blink-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Description[PresentationAPI] Plumbing send() from PresentationSession IDL to platform/.
This patch adds plumbing for DOMString, DOMArrayBuffer and DOMArrayBufferView. This is part of a three-side change:
1. This (Blink) CL
2. Chromium CL: https://codereview.chromium.org/1037483003
3. Cleanup: Making the send methods pure virtual in WebPresentationClient.
BUG=459008
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194302
Patch Set 1 #
Total comments: 9
Patch Set 2 : Defined an empty method instead of pure virtual and fixed other review comments. #Patch Set 3 : Added BLINK_ASSERT_NOT_REACHED in WebPresentationClient. #Patch Set 4 : Modified postMessage idl to take SerializedScriptValue argument #Patch Set 5 : Added plumbing for ArrayBuffer and ArrayBufferView. #
Total comments: 14
Patch Set 6 : Renamed postMessage() api to send(), other review fixes. #
Total comments: 6
Patch Set 7 : Added ASSERTs. #
Total comments: 2
Patch Set 8 : Forward declare the Blob class. #
Total comments: 12
Patch Set 9 : Refactored send for ArrayBuffer and ArrayBufferView. Removed send(Blob) for now. #Patch Set 10 : Blob cleanup. #
Total comments: 22
Patch Set 11 : Renamed url argument and other fixes. #Patch Set 12 : change char* to uint8* for ArrayBuffer data for mojo purpose. #
Total comments: 2
Patch Set 13 : Allow sending ArrayBuffer with zero bytes of data. #Patch Set 14 : Fix layout test: change postMessage to send. #Messages
Total messages: 51 (10 generated)
s.singapati@samsung.com changed reviewers: + avayvod@chromium.org, mfoltz@chromium.org
Hi, Please provide your comments. This patch only covers postMessage(DOMString) case. But I am working on ArrayBuffer & Blob. Is it ok to provide a separate CL for this? Also, i think |onmessage| event implementation can be done as separate CL. Thank you.
On 2015/03/25 11:34:44, s.singapati wrote: > Hi, Please provide your comments. > > This patch only covers postMessage(DOMString) case. But I am working on > ArrayBuffer & Blob. Is it ok to provide a separate CL for this? Also, i think > |onmessage| event implementation can be done as separate CL. > > Thank you. Looks okay to me, but I would recommend adding a reviewer more familiar with Blink. peter@ and mlamouri@ have been reviewing most of the Blink patches relating to Presentation API.
s.singapati@samsung.com changed reviewers: + mlamouri@chromium.org, peter@chromium.org
Anton should take a look at this. Nonetheless, thank you for the patch!! :-) https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.cpp:117: { Why is it OK for these methods to be empty? Can we at least have TODOs? https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... File Source/modules/presentation/PresentationSession.idl (right): https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.idl:22: [RaisesException] void postMessage(ArrayBufferView data); I don't see why we need these overloads. I would consider postMessage(DOMString) to be a mere placeholder? Other postMessage implementations in Blink follow this signature: void postMessage(any message, optional sequence<> transferables); Since a session is a remote target I can understand that you don't want the transferables, but rather than having all these overloads can we use "any data" or the newer "SerializedScriptValue data"? Do we envision this could cause issues deserializing by the recipient? In that case I would be curious to learn how this would work well for Blobs. https://codereview.chromium.org/1002293005/diff/1/public/platform/modules/pre... File public/platform/modules/presentation/WebPresentationClient.h (right): https://codereview.chromium.org/1002293005/diff/1/public/platform/modules/pre... public/platform/modules/presentation/WebPresentationClient.h:44: virtual void postMessage(const WebString& url, const WebString& presentationId, const WebString& message) = 0; This won't compile because the postMessage() method does not yet exist in //content's PresentationDispatcher. You'll need to define an empty method inline. (void ..() {})
lgtm % my & Peter's comments re: signatures, I think we should try to be consistent with WebRTC as much as possible I think it's okay to delete the unimplemented methods for now for faster progress https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.cpp:42: void throwInvalidStateError(ExceptionState& exceptionState) Rename to something reflecting what kind of error it represents (e.g. throwPresentationDisconnectedError() ) ?
https://codereview.chromium.org/1002293005/diff/1/public/platform/modules/pre... File public/platform/modules/presentation/WebPresentationClient.h (right): https://codereview.chromium.org/1002293005/diff/1/public/platform/modules/pre... public/platform/modules/presentation/WebPresentationClient.h:44: virtual void postMessage(const WebString& url, const WebString& presentationId, const WebString& message) = 0; On 2015/03/25 20:20:29, Peter Beverloo wrote: > This won't compile because the postMessage() method does not yet exist in > //content's PresentationDispatcher. You'll need to define an empty method > inline. (void ..() {}) I think it might work actually if Chromium change is submitted (since there's no override used in the implementation of the interface by Blink tradition). Am I right?
On 2015/03/26 00:19:22, whywhat wrote: > https://codereview.chromium.org/1002293005/diff/1/public/platform/modules/pre... > File public/platform/modules/presentation/WebPresentationClient.h (right): > > https://codereview.chromium.org/1002293005/diff/1/public/platform/modules/pre... > public/platform/modules/presentation/WebPresentationClient.h:44: virtual void > postMessage(const WebString& url, const WebString& presentationId, const > WebString& message) = 0; > On 2015/03/25 20:20:29, Peter Beverloo wrote: > > This won't compile because the postMessage() method does not yet exist in > > //content's PresentationDispatcher. You'll need to define an empty method > > inline. (void ..() {}) > > I think it might work actually if Chromium change is submitted (since there's no > override used in the implementation of the interface by Blink tradition). Am I > right? Yes, but the Chromium change mentions it depends on this one.
https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.cpp:42: void throwInvalidStateError(ExceptionState& exceptionState) On 2015/03/26 00:14:19, whywhat wrote: > Rename to something reflecting what kind of error it represents (e.g. > throwPresentationDisconnectedError() ) ? Done. https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.cpp:117: { On 2015/03/25 20:20:29, Peter Beverloo wrote: > Why is it OK for these methods to be empty? Can we at least have TODOs? Done. Added TODOs for now. I think blink side implementation can be done soon for this. https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... File Source/modules/presentation/PresentationSession.idl (right): https://codereview.chromium.org/1002293005/diff/1/Source/modules/presentation... Source/modules/presentation/PresentationSession.idl:22: [RaisesException] void postMessage(ArrayBufferView data); On 2015/03/25 20:20:29, Peter Beverloo wrote: > I don't see why we need these overloads. I would consider postMessage(DOMString) > to be a mere placeholder? > > Other postMessage implementations in Blink follow this signature: > > void postMessage(any message, optional sequence<> transferables); > > Since a session is a remote target I can understand that you don't want the > transferables, but rather than having all these overloads can we use "any data" > or the newer "SerializedScriptValue data"? Do we envision this could cause > issues deserializing by the recipient? In that case I would be curious to learn > how this would work well for Blobs. Acknowledged. I think we try to be consistent with WebRTC as commented by avayvod. https://codereview.chromium.org/1002293005/diff/1/public/platform/modules/pre... File public/platform/modules/presentation/WebPresentationClient.h (right): https://codereview.chromium.org/1002293005/diff/1/public/platform/modules/pre... public/platform/modules/presentation/WebPresentationClient.h:44: virtual void postMessage(const WebString& url, const WebString& presentationId, const WebString& message) = 0; On 2015/03/26 00:19:22, whywhat wrote: > On 2015/03/25 20:20:29, Peter Beverloo wrote: > > This won't compile because the postMessage() method does not yet exist in > > //content's PresentationDispatcher. You'll need to define an empty method > > inline. (void ..() {}) > > I think it might work actually if Chromium change is submitted (since there's no > override used in the implementation of the interface by Blink tradition). Am I > right? Done. Added empty definition for now and will be cleaned-up after Chromium patch is landed.
The CQ bit was checked by s.singapati@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/1002293005/#ps40001 (title: "Added BLINK_ASSERT_NOT_REACHED in WebPresentationClient.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1002293005/40001
What part of WebRTC is this trying to align with? This does not sound OK to me - I'd consider that to be a bug. Please be sure to follow up on aligning your postMessage API with the other ones.
The CQ bit was unchecked by s.singapati@samsung.com
On 2015/03/26 16:39:41, Peter Beverloo wrote: > What part of WebRTC is this trying to align with? This does not sound OK to me - > I'd consider that to be a bug. > > Please be sure to follow up on aligning your postMessage API with the other > ones. Hey Peter, we're aligning postMessage() with RTCDataChannel send() (mostly about the data types though): https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... If you think the IDL can be implemented in a better way (since the RTCDataChannel was implemented some time ago), I agree we should do it.
On 2015/03/26 17:19:09, whywhat wrote: > On 2015/03/26 16:39:41, Peter Beverloo wrote: > > What part of WebRTC is this trying to align with? This does not sound OK to me > - > > I'd consider that to be a bug. > > > > Please be sure to follow up on aligning your postMessage API with the other > > ones. > > Hey Peter, > > we're aligning postMessage() with RTCDataChannel send() (mostly about the data > types though): > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > If you think the IDL can be implemented in a better way (since the > RTCDataChannel was implemented some time ago), I agree we should do it. Right, thank you for the explanation. You could use a union type to make the IDL cleaner, but that would make the implementation messier (more branches). The rationale in their spec is fair. Why was the name postMessage() chosen if it doesn't align with other postMessage()s in the platform? I can't immediately come up with something better either :/.
On 2015/03/26 17:31:19, Peter Beverloo wrote: > On 2015/03/26 17:19:09, whywhat wrote: > > On 2015/03/26 16:39:41, Peter Beverloo wrote: > > > What part of WebRTC is this trying to align with? This does not sound OK to > me > > - > > > I'd consider that to be a bug. > > > > > > Please be sure to follow up on aligning your postMessage API with the other > > > ones. > > > > Hey Peter, > > > > we're aligning postMessage() with RTCDataChannel send() (mostly about the data > > types though): > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > If you think the IDL can be implemented in a better way (since the > > RTCDataChannel was implemented some time ago), I agree we should do it. > > Right, thank you for the explanation. > > You could use a union type to make the IDL cleaner, but that would make the > implementation messier (more branches). The rationale in their spec is fair. > > Why was the name postMessage() chosen if it doesn't align with other > postMessage()s in the platform? I can't immediately come up with something > better either :/. Hi, Thanks for comments. I will look into following up with other postMessage APIs using SerializedScriptValue.
On 2015/03/26 21:46:16, s.singapati wrote: > On 2015/03/26 17:31:19, Peter Beverloo wrote: > > On 2015/03/26 17:19:09, whywhat wrote: > > > On 2015/03/26 16:39:41, Peter Beverloo wrote: > > > > What part of WebRTC is this trying to align with? This does not sound OK > to > > me > > > - > > > > I'd consider that to be a bug. > > > > > > > > Please be sure to follow up on aligning your postMessage API with the > other > > > > ones. > > > > > > Hey Peter, > > > > > > we're aligning postMessage() with RTCDataChannel send() (mostly about the > data > > > types though): > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > If you think the IDL can be implemented in a better way (since the > > > RTCDataChannel was implemented some time ago), I agree we should do it. > > > > Right, thank you for the explanation. > > > > You could use a union type to make the IDL cleaner, but that would make the > > implementation messier (more branches). The rationale in their spec is fair. > > > > Why was the name postMessage() chosen if it doesn't align with other > > postMessage()s in the platform? I can't immediately come up with something > > better either :/. > > Hi, Thanks for comments. I will look into following up with other postMessage > APIs using SerializedScriptValue. Anton raised the issue with postMessage() vs send() in the WG. https://github.com/w3c/presentation-api/issues/46#issuecomment-88057245 Peter, do you want to wait until that issue is resolved before proceeding with this review?
On 2015/03/31 18:07:55, mark a. foltz wrote: > On 2015/03/26 21:46:16, s.singapati wrote: > > On 2015/03/26 17:31:19, Peter Beverloo wrote: > > > On 2015/03/26 17:19:09, whywhat wrote: > > > > On 2015/03/26 16:39:41, Peter Beverloo wrote: > > > > > What part of WebRTC is this trying to align with? This does not sound OK > > to > > > me > > > > - > > > > > I'd consider that to be a bug. > > > > > > > > > > Please be sure to follow up on aligning your postMessage API with the > > other > > > > > ones. > > > > > > > > Hey Peter, > > > > > > > > we're aligning postMessage() with RTCDataChannel send() (mostly about the > > data > > > > types though): > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > If you think the IDL can be implemented in a better way (since the > > > > RTCDataChannel was implemented some time ago), I agree we should do it. > > > > > > Right, thank you for the explanation. > > > > > > You could use a union type to make the IDL cleaner, but that would make the > > > implementation messier (more branches). The rationale in their spec is fair. > > > > > > Why was the name postMessage() chosen if it doesn't align with other > > > postMessage()s in the platform? I can't immediately come up with something > > > better either :/. > > > > Hi, Thanks for comments. I will look into following up with other postMessage > > APIs using SerializedScriptValue. > > Anton raised the issue with postMessage() vs send() in the WG. > > https://github.com/w3c/presentation-api/issues/46#issuecomment-88057245 > > Peter, do you want to wait until that issue is resolved before proceeding with > this review? That's great! Since doing Blink API changes after they're implemented is a pain, I'd prefer if the rename could be done in this patch. After that it should be landed indeed. Additionally, talking to Anton today, we probably do not want to use SerializedScriptValue for this purpose and instead use the four overloads WebRTC provides - changing to be consistent with them. The reason for this is that the recipient of the message is not necessarily Chrome, so we can't use our serialization procedures. The name postMessage() threw me off track for this -- sorry!
Hi Anton and Peter, Thanks for the discussion and comments. I have left |postMessage| as it is for now. I think we need to wait for the decision and spec changes about postMessage() vs send().
On 2015/04/02 12:26:50, s.singapati wrote: > Hi Anton and Peter, Thanks for the discussion and comments. I have left > |postMessage| as it is for now. I think we need to wait for the decision and > spec changes about postMessage() vs send(). I think we can proceed with send(). This is how Mozilla implemented it and what was proposed. Leaving it as postMessage() in the spec was an oversight. I created a pull request to update the spec: https://github.com/w3c/presentation-api/pull/72 Given our implementation is behind the flag yet, I think there's no requirement to wait. The absence of this method blocks the Chromium Media Router work so we would like to have it implemented asap. Please, let me know if you don't have resources to finish at least the DOMString version of the method in the next few (working) days.
On 2015/04/02 13:18:35, whywhat wrote: > On 2015/04/02 12:26:50, s.singapati wrote: > > Hi Anton and Peter, Thanks for the discussion and comments. I have left > > |postMessage| as it is for now. I think we need to wait for the decision and > > spec changes about postMessage() vs send(). > > I think we can proceed with send(). This is how Mozilla implemented it and what > was proposed. Leaving it as postMessage() in the spec was an oversight. I > created a pull request to update the spec: > https://github.com/w3c/presentation-api/pull/72 > > Given our implementation is behind the flag yet, I think there's no requirement > to wait. The absence of this method blocks the Chromium Media Router work so we > would like to have it implemented asap. > > Please, let me know if you don't have resources to finish at least the DOMString > version of the method in the next few (working) days. Ok, I can change method names to send() and upload the patch on Tuesday (07.04) after easter holiday. Do you have any other comments in this patch? About chromium patch, could you provide your comments on new patch uploaded today?
https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... File Source/modules/presentation/PresentationController.cpp (right): https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationController.cpp:115: void PresentationController::postMessage(const String& url, const String& presentationId, const String& message) send() https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationController.cpp:122: void PresentationController::postMessage(const String& url, const String& presentationId, const char* data, size_t length) send() https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... File Source/modules/presentation/PresentationController.h (right): https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationController.h:60: void postMessage(const String& url, const String& presentationId, const String& message); Per conversation, send() https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationController.h:63: void postMessage(const String& url, const String& presentationId, const char* data, size_t length); Ditto https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationController.h:64: Are you going to add a declaration for send(Blob*) as well? https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.h (right): https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationSession.h:47: void postMessage(DOMArrayBuffer* data, ExceptionState&); The declarations in RTCDataChannel.h pass the ArrayBuffer/ArrayBufferView by reference, not as a raw ptr. This makes more sense to me as we can't have the object go out of scope while it's being sent. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Can we pass by reference here as well? https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.idl (right): https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationSession.idl:19: [RaisesException] void postMessage(DOMString message); s/postMessage/send/g
https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... File Source/modules/presentation/PresentationController.cpp (right): https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationController.cpp:115: void PresentationController::postMessage(const String& url, const String& presentationId, const String& message) On 2015/04/02 20:26:39, mark a. foltz wrote: > send() Done. https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationController.cpp:122: void PresentationController::postMessage(const String& url, const String& presentationId, const char* data, size_t length) On 2015/04/02 20:26:39, mark a. foltz wrote: > send() Done. https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... File Source/modules/presentation/PresentationController.h (right): https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationController.h:60: void postMessage(const String& url, const String& presentationId, const String& message); On 2015/04/02 20:26:39, mark a. foltz wrote: > Per conversation, send() Done. https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationController.h:63: void postMessage(const String& url, const String& presentationId, const char* data, size_t length); On 2015/04/02 20:26:39, mark a. foltz wrote: > Ditto Done. https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationController.h:64: On 2015/04/02 20:26:39, mark a. foltz wrote: > Are you going to add a declaration for send(Blob*) as well? Not started yet. I was planning to look into Blob once current blink & chromium patches are close to finalization. I think I will have time now to do Blob handling. Could we do it as new CL, just to speedup and not blocking the possible parallel implementation? https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.h (right): https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationSession.h:47: void postMessage(DOMArrayBuffer* data, ExceptionState&); On 2015/04/02 20:26:39, mark a. foltz wrote: > The declarations in RTCDataChannel.h pass the ArrayBuffer/ArrayBufferView by > reference, not as a raw ptr. This makes more sense to me as we can't have the > object go out of scope while it's being sent. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Can we pass by reference here as well? Done. https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... File Source/modules/presentation/PresentationSession.idl (right): https://codereview.chromium.org/1002293005/diff/80001/Source/modules/presenta... Source/modules/presentation/PresentationSession.idl:19: [RaisesException] void postMessage(DOMString message); On 2015/04/02 20:26:39, mark a. foltz wrote: > s/postMessage/send/g Done.
https://codereview.chromium.org/1002293005/diff/100001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/100001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:114: // TODO(): To be implemented. Throw a DOMException here for an unimplemented feature? https://codereview.chromium.org/1002293005/diff/100001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:114: // TODO(): To be implemented. Please put your @chromium.org login here and in other TODO()s. https://codereview.chromium.org/1002293005/diff/100001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:144: // TODO(): DOM exception Or ASSERT(data)? I think you should be guaranteed to get a live and valid |data| object here. One of the Blink developers can confirm this. Therefore I would lean towards ASSERT().
https://codereview.chromium.org/1002293005/diff/100001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/100001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:114: // TODO(): To be implemented. On 2015/04/08 23:14:35, mark a. foltz wrote: > Please put your @chromium.org login here and in other TODO()s. I do not have username at @chromium.org yet, but @samsung.com. Is it ok to add in TODO()s?
https://codereview.chromium.org/1002293005/diff/100001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/100001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:114: // TODO(): To be implemented. On 2015/04/08 23:14:35, mark a. foltz wrote: > Throw a DOMException here for an unimplemented feature? Done. yes, i think DOMException is ok for now. https://codereview.chromium.org/1002293005/diff/100001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:144: // TODO(): DOM exception Or ASSERT(data)? On 2015/04/08 23:14:35, mark a. foltz wrote: > I think you should be guaranteed to get a live and valid |data| object here. > One of the Blink developers can confirm this. Therefore I would lean towards > ASSERT(). Done. It is tested that data is valid. and atleast there is no major reason for DOM exception.
lgtm You will need a lgtm from one of the Blink OWNERS (Peter/Mounir). I believe that there will also need to be some layout tests updated to exercise this API. https://codereview.chromium.org/1002293005/diff/120001/Source/modules/present... File Source/modules/presentation/PresentationSession.h (right): https://codereview.chromium.org/1002293005/diff/120001/Source/modules/present... Source/modules/presentation/PresentationSession.h:9: #include "core/fileapi/Blob.h" You can forward declare this instead of including the .h.
Currently there is very basic layout test for Presentation API which tests only for presence of methods, objects, types on presentation object. session object is not available at the moment. I think detailed tests need to be added when start/joinSession apis are working fully. Do we need to create a new issue to track Layout tests? @Peter/Mounir: Do you have any comments? Thanks. https://codereview.chromium.org/1002293005/diff/120001/Source/modules/present... File Source/modules/presentation/PresentationSession.h (right): https://codereview.chromium.org/1002293005/diff/120001/Source/modules/present... Source/modules/presentation/PresentationSession.h:9: #include "core/fileapi/Blob.h" On 2015/04/10 18:31:09, mark a. foltz wrote: > You can forward declare this instead of including the .h. Done.
Re: tests - it's hard to write LayoutTests without having a mock PresentationClient interface implemented in Chromium's test_runner, however if it's implemented, it makes landing two/three side patches without breaking one of the trees nearly impossible. I've been postponing this work until we either have the plumbing finished and/or Blink/Chromium merged into one repo. https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:107: if (message.isNull() || message.isEmpty()) I wonder if sending empty messages should be okay (e.g. to ping the presentation)? It seems that WebRTC standard is not clear about this and Chromium implementation doesn't pass empty messages through. I think the way it is implemented now is fine but wanted to raise the issue. https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:136: void PresentationSession::send(PassRefPtr<DOMArrayBufferView> data, ExceptionState& exceptionState) looks like both send() methods for ArrayBufferView and ArrayBuffer could be implemented via a common private sendInternal() method: void PS::send(PRP<DAB> data, ES& eS) { ASSERT(data && data->buffer()); sendInternal(static_cast<const char*>(data->data()), data->byteLength(), eS); } void PS::send(PRP<DABV> data, ES& eS) { ASSERT(data); sendInternal(static_cast<const char*>(data->baseAddress()), data->byteLength(), eS); } void PS::sendInternal(const char* data, size_t size, ES& eS) { ASSERT(data); if (m_state != WPSS::Connected) { throwPDE(eS); return } if (!size) return; PC* controller = pC(); if (controller) controller->send(m_url, m_id, data, size); } I wonder if we could also implement sending a string the same way? Do we need to distinguish between an array of bytes and a string? https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:142: ASSERT(data); Do you need to ASSERT for data->baseAddress()? https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... File Source/modules/presentation/PresentationSession.idl (right): https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... Source/modules/presentation/PresentationSession.idl:20: [RaisesException] void send(Blob data); Either remove send(Blob) or mention it in the description of the change, noting that it's not implemented yet. https://codereview.chromium.org/1002293005/diff/140001/public/platform/module... File public/platform/modules/presentation/WebPresentationClient.h (right): https://codereview.chromium.org/1002293005/diff/140001/public/platform/module... public/platform/modules/presentation/WebPresentationClient.h:43: // TODO(): make below send() methods pure virtual once Chromium has the implementation. add your username inside the TODO https://codereview.chromium.org/1002293005/diff/140001/public/platform/module... public/platform/modules/presentation/WebPresentationClient.h:45: virtual void send(const WebString& url, const WebString& presentationId, const WebString& message) We generally don't allow overloaded methods in Chromium and I don't think we should add more in Blink either. Could you rename these to sendString/sendArray?
https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:107: if (message.isNull() || message.isEmpty()) On 2015/04/13 12:58:45, whywhat wrote: > I wonder if sending empty messages should be okay (e.g. to ping the > presentation)? > It seems that WebRTC standard is not clear about this and Chromium > implementation doesn't pass empty messages through. > I think the way it is implemented now is fine but wanted to raise the issue. Acknowledged. https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:136: void PresentationSession::send(PassRefPtr<DOMArrayBufferView> data, ExceptionState& exceptionState) On 2015/04/13 12:58:45, whywhat wrote: > looks like both send() methods for ArrayBufferView and ArrayBuffer could be > implemented via a common private sendInternal() method: > > void PS::send(PRP<DAB> data, ES& eS) { > ASSERT(data && data->buffer()); > sendInternal(static_cast<const char*>(data->data()), data->byteLength(), eS); > } > > void PS::send(PRP<DABV> data, ES& eS) { > ASSERT(data); > sendInternal(static_cast<const char*>(data->baseAddress()), > data->byteLength(), eS); > } > > void PS::sendInternal(const char* data, size_t size, ES& eS) { > ASSERT(data); > if (m_state != WPSS::Connected) { throwPDE(eS); return } > > if (!size) return; > > PC* controller = pC(); > if (controller) > controller->send(m_url, m_id, data, size); > } > > I wonder if we could also implement sending a string the same way? Do we need to > distinguish between an array of bytes and a string? Done. Re factored this. sending a string the same way is other option for eg. sending String.characters8() which is <const unsigned char*>. But sending all types in same way may need extra argument to indicate what message/binary type is sent (for the purpose of onmessage implementation) Also, array buffer data can be sent as <const unsigned char*>. And mojo does not have 'char' as basic types but, have int8, uint8. so probably char* needs conversion in presentation_dispatcher. I will check more on this and align with mojo capabilities. https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:142: ASSERT(data); On 2015/04/13 12:58:45, whywhat wrote: > Do you need to ASSERT for data->baseAddress()? Done. Now it is done in sendInternal(). https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... File Source/modules/presentation/PresentationSession.idl (right): https://codereview.chromium.org/1002293005/diff/140001/Source/modules/present... Source/modules/presentation/PresentationSession.idl:20: [RaisesException] void send(Blob data); On 2015/04/13 12:58:45, whywhat wrote: > Either remove send(Blob) or mention it in the description of the change, noting > that it's not implemented yet. Done. Removed it for now. https://codereview.chromium.org/1002293005/diff/140001/public/platform/module... File public/platform/modules/presentation/WebPresentationClient.h (right): https://codereview.chromium.org/1002293005/diff/140001/public/platform/module... public/platform/modules/presentation/WebPresentationClient.h:43: // TODO(): make below send() methods pure virtual once Chromium has the implementation. On 2015/04/13 12:58:45, whywhat wrote: > add your username inside the TODO Done. https://codereview.chromium.org/1002293005/diff/140001/public/platform/module... public/platform/modules/presentation/WebPresentationClient.h:45: virtual void send(const WebString& url, const WebString& presentationId, const WebString& message) On 2015/04/13 12:58:45, whywhat wrote: > We generally don't allow overloaded methods in Chromium and I don't think we > should add more in Blink either. Could you rename these to sendString/sendArray? Done.
lgtm with all the changes applied. https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... File Source/modules/presentation/PresentationController.cpp (right): https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationController.cpp:115: void PresentationController::send(const String& url, const String& presentationId, const String& message) ditto https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationController.cpp:122: void PresentationController::send(const String& url, const String& presentationId, const char* data, size_t length) ditto https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... File Source/modules/presentation/PresentationController.h (right): https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationController.h:60: void send(const String& url, const String& presentationId, const String& message); ditto https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationController.h:63: void send(const String& url, const String& presentationId, const char* data, size_t length); ditto https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:103: if (m_state != WebPresentationSessionState::Connected) { In order to follow the specification, could you do == Disconnected instead? https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:108: return; Why are you doing that check? If someone wants to send an empty string, why should that be forbidden? https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:130: if (m_state != WebPresentationSessionState::Connected) { ditto: use == Disconnected https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... File Source/modules/presentation/PresentationSession.idl (right): https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationSession.idl:21: [RaisesException] void send(ArrayBufferView data); Could you add a TODO pointing to a bug for Blob support? https://codereview.chromium.org/1002293005/diff/180001/public/platform/module... File public/platform/modules/presentation/WebPresentationClient.h (right): https://codereview.chromium.org/1002293005/diff/180001/public/platform/module... public/platform/modules/presentation/WebPresentationClient.h:45: virtual void sendString(const WebString& url, const WebString& presentationId, const WebString& message) What's the |url| here? presentationUrl? https://codereview.chromium.org/1002293005/diff/180001/public/platform/module... public/platform/modules/presentation/WebPresentationClient.h:50: virtual void sendArrayBuffer(const WebString& url, const WebString& presentationId, const char* data, size_t length) ditto
Now sending "const uint8_t*" instead of ""const char*" for ArrayBuffer data, to avoid possible reinterpret_casting later, before sending to mojo PresentationServiceImpl. Chromium CL will be updated according to this. https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... File Source/modules/presentation/PresentationController.cpp (right): https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationController.cpp:115: void PresentationController::send(const String& url, const String& presentationId, const String& message) On 2015/04/16 10:06:30, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationController.cpp:122: void PresentationController::send(const String& url, const String& presentationId, const char* data, size_t length) On 2015/04/16 10:06:30, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... File Source/modules/presentation/PresentationController.h (right): https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationController.h:60: void send(const String& url, const String& presentationId, const String& message); On 2015/04/16 10:06:30, Mounir Lamouri wrote: > ditto Done. renamed to presentationUrl. https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationController.h:63: void send(const String& url, const String& presentationId, const char* data, size_t length); On 2015/04/16 10:06:30, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:103: if (m_state != WebPresentationSessionState::Connected) { On 2015/04/16 10:06:30, Mounir Lamouri wrote: > In order to follow the specification, could you do == Disconnected instead? Done. https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:108: return; On 2015/04/16 10:06:30, Mounir Lamouri wrote: > Why are you doing that check? If someone wants to send an empty string, why > should that be forbidden? Done. What about byteLength() for ArrayBuffer/View? https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:130: if (m_state != WebPresentationSessionState::Connected) { On 2015/04/16 10:06:30, Mounir Lamouri wrote: > ditto: use == Disconnected Done. https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... File Source/modules/presentation/PresentationSession.idl (right): https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationSession.idl:21: [RaisesException] void send(ArrayBufferView data); On 2015/04/16 10:06:30, Mounir Lamouri wrote: > Could you add a TODO pointing to a bug for Blob support? Done. https://codereview.chromium.org/1002293005/diff/180001/public/platform/module... File public/platform/modules/presentation/WebPresentationClient.h (right): https://codereview.chromium.org/1002293005/diff/180001/public/platform/module... public/platform/modules/presentation/WebPresentationClient.h:45: virtual void sendString(const WebString& url, const WebString& presentationId, const WebString& message) On 2015/04/16 10:06:30, Mounir Lamouri wrote: > What's the |url| here? presentationUrl? Done. renamed to presentationUrl. https://codereview.chromium.org/1002293005/diff/180001/public/platform/module... public/platform/modules/presentation/WebPresentationClient.h:50: virtual void sendArrayBuffer(const WebString& url, const WebString& presentationId, const char* data, size_t length) On 2015/04/16 10:06:30, Mounir Lamouri wrote: > ditto Done.
https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:108: return; On 2015/04/16 at 16:10:02, s.singapati wrote: > On 2015/04/16 10:06:30, Mounir Lamouri wrote: > > Why are you doing that check? If someone wants to send an empty string, why > > should that be forbidden? > > Done. What about byteLength() for ArrayBuffer/View? Indeed. https://codereview.chromium.org/1002293005/diff/220001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/220001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:133: return; Could you remove that check too?
https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/180001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:108: return; On 2015/04/16 10:06:30, Mounir Lamouri wrote: > Why are you doing that check? If someone wants to send an empty string, why > should that be forbidden? Because that's how WebRTC DataChannel works in Chromium? Its spec doesn't define the behavior though.
https://codereview.chromium.org/1002293005/diff/220001/Source/modules/present... File Source/modules/presentation/PresentationSession.cpp (right): https://codereview.chromium.org/1002293005/diff/220001/Source/modules/present... Source/modules/presentation/PresentationSession.cpp:133: return; On 2015/04/16 17:14:17, Mounir Lamouri wrote: > Could you remove that check too? Done. Removed the check now, though it is not defined in the spec.
Can we proceed with latest patchset or any other comments?
On 2015/04/21 19:43:42, s.singapati wrote: > Can we proceed with latest patchset or any other comments? It's been LGTMed by Mounir so feel free to submit via the queue when you are ready. Do you need a committer to submit for you?
On 2015/04/22 16:52:25, mark a. foltz wrote: > On 2015/04/21 19:43:42, s.singapati wrote: > > Can we proceed with latest patchset or any other comments? > > It's been LGTMed by Mounir so feel free to submit via the queue when you are > ready. Do you need a committer to submit for you? Thanks Mark. I will submit this tomorrow morning Finland time.
The CQ bit was checked by s.singapati@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1002293005/#ps240001 (title: "Allow sending ArrayBuffer with zero bytes of data.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1002293005/240001
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/60447)
On 2015/04/23 12:16:09, I haz the power (commit-bot) wrote: > 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/60447) I think i need to update WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt with PresentationSession - send instead of postMessage.
The CQ bit was checked by s.singapati@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, avayvod@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1002293005/#ps260001 (title: "Fix layout test: change postMessage to send.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1002293005/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194302 |
