Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1194123003:
[PresentationAPI] on-session-message handler for ArrayBuffer and Blob messages. (Closed)
|
Created:
5 years, 6 months ago by USE s.singapati at gmail.com Modified:
5 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[PresentationAPI] on-session-message handler for ArrayBuffer and Blob messages.
Depends on the Blink CL: https://codereview.chromium.org/1206513004/
BUG=459008
Committed: https://crrev.com/09f3699d4c094c327b2a70ae6a9229092207c982
Cr-Commit-Position: refs/heads/master@{#338263}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 2
Patch Set 3 : wrap PresentationSessionClient in scoped_ptr. #
Total comments: 1
Patch Set 4 : generic interface to receive a binary messages. #Patch Set 5 : rebase #Messages
Total messages: 23 (8 generated)
s.singapati@samsung.com changed reviewers: + habib.virji@samsung.com
https://codereview.chromium.org/1194123003/diff/1/content/renderer/presentati... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1194123003/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.cc:376: new PresentationSessionClient(messages[i]->presentation_url, This can be handled prior to the switch statement as it is getting replicated in each case.
Hi Habib, Thanks for review. Is it ok to proceed? https://codereview.chromium.org/1194123003/diff/1/content/renderer/presentati... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1194123003/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.cc:376: new PresentationSessionClient(messages[i]->presentation_url, On 2015/06/23 13:04:23, Habib Virji wrote: > This can be handled prior to the switch statement as it is getting replicated in > each case. Done.
Yes please proceed.
s.singapati@samsung.com changed reviewers: + haibinlu@chromium.org, imcheng@chromium.org, mfoltz@chromium.org
PTAL.
imcheng@google.com changed reviewers: + imcheng@google.com
https://codereview.chromium.org/1194123003/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1194123003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:372: PresentationSessionClient* session_client = new PresentationSessionClient( Since you moved the allocation of PresentationSessionClient out-of-line, I would suggest wrapped it in a scoped_ptr, and then invoking session_client.Pass() in the non-default cases. Then you won't have to manually call delete in the default case. I believe this would be less error prone than manually managing new'ed memory.
PTaL. https://codereview.chromium.org/1194123003/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1194123003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:372: PresentationSessionClient* session_client = new PresentationSessionClient( On 2015/06/23 22:08:31, imcheng wrote: > Since you moved the allocation of PresentationSessionClient out-of-line, I would > suggest wrapped it in a scoped_ptr, and then invoking session_client.Pass() in > the non-default cases. Then you won't have to manually call delete in the > default case. I believe this would be less error prone than manually managing > new'ed memory. Done. Used session_client.release() as it takes WebPresentationSessionClient*.
LGTM https://codereview.chromium.org/1194123003/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1194123003/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:373: new PresentationSessionClient(messages[i]->presentation_url, It appears that this ctor will copy the contents of presentation_url and presentation_id into a blink::WebString each time through this loop. Then look up the PresentationSession to dispatch the event in Presentation.cpp. This is not ideal from an efficiency point of view, but it's likely that there will only be one message in the queue, and there is only one active session per frame. As this can be optimized later, can you add a NOTE that passing batches of messages to the Blink layer would be more efficient?
PTAL.
s.singapati@samsung.com changed reviewers: + avayvod@chromium.org
On 2015/06/30 at 14:32:47, s.singapati wrote: > PTAL. lgtm If you've got lgtm already with a minor nit and fixed the nit, you can usually bypass an extra review and submit.
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 Link to the patchset: https://codereview.chromium.org/1194123003/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1194123003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) (exceeded global retry quota)
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/1194123003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/09f3699d4c094c327b2a70ae6a9229092207c982 Cr-Commit-Position: refs/heads/master@{#338263} |
