[Media Router] Dedup RouteMessage and SessionMessage mojom definitions |
||||
Issue description
During the mojo IPC review it was suggested that we merge the definitions of RouteMessage and SessionMessage since they are very similar:
media_router.mojom:
struct RouteMessage {
enum Type {
TEXT,
BINARY
};
// The type of this message.
Type type;
// Used when the |type| is TEXT.
string? message;
// Used when the |type| is BINARY.
array<uint8>? data;
};
presentation.mojom:
enum PresentationMessageType {
TEXT,
ARRAY_BUFFER,
BLOB,
};
struct SessionMessage {
PresentationMessageType type;
// Used when message type is TEXT.
string? message;
// Used when message type is ARRAY_BUFFER or BLOB.
array<uint8>? data;
};
The only difference here is the type (BLOB + ARRAY_BUFFER / BINARY). In our content code (PresentationDispatcher / PresentationServiceImpl) I don't see difference in treatment between BLOB and ARRAY_BUFFER, so we can probably safety collapse the two into one. So the work would be:
- Replace BLOB / ARRAY_BUFFER with BINARY
- Replace RouteMessage with SessionMessage
,
Dec 6 2016
,
Dec 6 2016
,
Dec 6 2016
Do we want all functions associated with RouteMessage to also be changed to SessionMessage?
,
Dec 6 2016
SessionMessage is old terminology, we should use ConnectionMessage instead.
,
Dec 6 2016
So to clarify, since I'm new to this part of the code, this bug is asking to replace BLOB/ARRAY_BUFFER with BINARY and Route/Session Message with Connection Message?
,
Dec 6 2016
I believe so. imcheng@ ?
,
Dec 6 2016
Yeah, renaming to ConnectionMessage SGTM. Functions that had the name "SessionMessage" should be changed to "ConnectionMessage". Functions that had the name "RouteMessage" (e.g., MediaRouter) can stay the same, since technically the message is associated with a route at that point. But I don't feel strongly. Mark WDYT?
,
Dec 6 2016
I'd have to see the context. Right now the only source of messages in the browser is via PresentationConnection, although the media remoting project may change that.
,
Dec 6 2016
The api is defined here: https://cs.chromium.org/chromium/src/chrome/browser/media/router/media_router.h?q=f:media_router.h&sq=package:chromium&dr&l=137 which is called from here: https://cs.chromium.org/chromium/src/chrome/browser/media/router/presentation_service_delegate_impl.cc?type=cs&q=f:presentation_service_delegate_impl.cc&sq=package:chromium&l=894
,
Dec 7 2016
From conversations with you and Bin, the direction we are heading is to get rid of the messaging APIs from the MR and instead pass Mojo connection handles (or connection proxies). So keeping the API as "RouteMessage" in the MR is fine with me. Again, you may want to check with miu@ to see how they might be using them as well.
,
Dec 7 2016
So to summarize, I should just be worrying about SessionMessage name change then?
,
Dec 8 2016
Yes. Let's leave RouteMessage alone for now.
,
Dec 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/44204ce5f5a841d13d26e89abbc93cd4db181425 commit 44204ce5f5a841d13d26e89abbc93cd4db181425 Author: lethalantidote <lethalantidote@chromium.org> Date: Wed Dec 14 03:11:47 2016 Updates SessionMessage to ConnectionMessage. Merges PresentationMessageTypes ARRAY_BUFFER/BLOB into BINARY. BUG= 629151 Review-Url: https://codereview.chromium.org/2562603002 Cr-Commit-Position: refs/heads/master@{#438409} [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/chrome/browser/media/router/presentation_service_delegate_impl.cc [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/chrome/browser/media/router/presentation_service_delegate_impl.h [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/browser/presentation/presentation_service_impl.cc [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/browser/presentation/presentation_service_impl.h [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/browser/presentation/presentation_service_impl_unittest.cc [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/public/browser/BUILD.gn [add] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/public/browser/presentation_connection_message.cc [add] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/public/browser/presentation_connection_message.h [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/public/browser/presentation_service_delegate.h [delete] https://crrev.com/865c534bef93f875acf0073efcbbf79e201339fd/content/public/browser/presentation_session_message.cc [delete] https://crrev.com/865c534bef93f875acf0073efcbbf79e201339fd/content/public/browser/presentation_session_message.h [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/public/common/presentation_constants.cc [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/public/common/presentation_constants.h [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/renderer/presentation/presentation_dispatcher.cc [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/content/renderer/presentation/presentation_dispatcher.h [modify] https://crrev.com/44204ce5f5a841d13d26e89abbc93cd4db181425/third_party/WebKit/public/platform/modules/presentation/presentation.mojom
,
Dec 14 2016
|
||||
►
Sign in to add a comment |
||||
Comment 1 by sko...@chromium.org
, Jul 27 2016Status: Available (was: Untriaged)