New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 749917 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[Media Router] Make CastSinkExtraData take net::IPEndPoint instead of net::IPAddress

Project Member Reported by zhaobin@chromium.org, Jul 28 2017

Issue description

In media_sink_internal.h, we have

// Extra data for Cast media sink.
struct CastSinkExtraData {
  net::IPAddress ip_address;
  ...
}

When implementing CastMediaSinkService to move Cast discovery to browser side, it seems useful to include port information as well and use net::IPEndPoint to simplify code. 

To resolve code review comments for: https://chromium-review.googlesource.com/c/575247/
 

Comment 1 by sko...@chromium.org, Jul 31 2017

Labels: Hotlist-CodeHealth
Status: Assigned (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6de7a9c9891246e21f57e58e2a4426f1c250baf7

commit 6de7a9c9891246e21f57e58e2a4426f1c250baf7
Author: Bin Zhao <zhaobin@chromium.org>
Date: Fri Aug 04 21:40:34 2017

[net_interfaces] Extract ip_endpoint.mojom from host_resolver_service.mojom

Media Router needs ip_endpoint mojo struct to send data to MRP. ip_endpoint mojo struct is
currently defined in host_resolver_service.mojom. Seperate it out so that media_router.mojom
can include it.

Bug:  749917 
Change-Id: I2a4ab93e1b2bd40a3579aa26930e393c12d5d942
Reviewed-on: https://chromium-review.googlesource.com/600747
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Commit-Queue: Bin Zhao <zhaobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492130}
[modify] https://crrev.com/6de7a9c9891246e21f57e58e2a4426f1c250baf7/net/dns/mojo_host_struct_traits.cc
[modify] https://crrev.com/6de7a9c9891246e21f57e58e2a4426f1c250baf7/net/dns/mojo_host_struct_traits.h
[modify] https://crrev.com/6de7a9c9891246e21f57e58e2a4426f1c250baf7/net/interfaces/BUILD.gn
[modify] https://crrev.com/6de7a9c9891246e21f57e58e2a4426f1c250baf7/net/interfaces/host_resolver_service.mojom
[add] https://crrev.com/6de7a9c9891246e21f57e58e2a4426f1c250baf7/net/interfaces/ip_endpoint.mojom
[add] https://crrev.com/6de7a9c9891246e21f57e58e2a4426f1c250baf7/net/interfaces/ip_endpoint.typemap
[add] https://crrev.com/6de7a9c9891246e21f57e58e2a4426f1c250baf7/net/interfaces/ip_endpoint_struct_traits.cc
[add] https://crrev.com/6de7a9c9891246e21f57e58e2a4426f1c250baf7/net/interfaces/ip_endpoint_struct_traits.h
[modify] https://crrev.com/6de7a9c9891246e21f57e58e2a4426f1c250baf7/net/interfaces/typemaps.gni

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bef0b41d0e1e7d8fc139c002312660c23eaf3b0a

commit bef0b41d0e1e7d8fc139c002312660c23eaf3b0a
Author: Bin Zhao <zhaobin@chromium.org>
Date: Tue Aug 08 23:00:39 2017

[net_interfaces] rename address to address_bytes in ip_address.mojom

ip_endpoint.mojom has field "IPAddress address". When referencing underlying IPAddress in .js file,
we use: ip_endpoint.address.address.join('.'); The naming is a little confusing. Change address to
address_bytes in ip_address.mojom instead.

Extension side change: https://critique.corp.google.com/#review/164510538

Bug:  749917 
Change-Id: I4158a867303449c9f21b7e1e1c1792d9c2ccabd6
Reviewed-on: https://chromium-review.googlesource.com/604407
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Commit-Queue: Bin Zhao <zhaobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492764}
[modify] https://crrev.com/bef0b41d0e1e7d8fc139c002312660c23eaf3b0a/net/interfaces/ip_address.mojom
[modify] https://crrev.com/bef0b41d0e1e7d8fc139c002312660c23eaf3b0a/net/interfaces/ip_address_struct_traits.cc
[modify] https://crrev.com/bef0b41d0e1e7d8fc139c002312660c23eaf3b0a/net/interfaces/ip_address_struct_traits.h

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/272ca40c665c7a3ad8d210aac4e12a43197dad29

commit 272ca40c665c7a3ad8d210aac4e12a43197dad29
Author: Bin Zhao <zhaobin@chromium.org>
Date: Tue Aug 15 19:37:07 2017

[Media Router] Make CastSinkExtraData take net::IPEndPoint instead of net::IPAddress

When implementing CastMediaSinkService and dual discovery, we decide to pass
MediaSinkInternal objects around in thread hopping methods. It will simplify
code if CastSinkExtraData takes one IPEndPoint field instead of both
IPAddress and port fields.

It seems unnecessary to update media_router.mojom since MRP does not need
port field. We need to introduce ip_endpoint.mojom and update extension code
if we would like to add port field to mojo.

To resolve code review comments in:
https://chromium-review.googlesource.com/c/575247/

Extension side change: https://critique.corp.google.com/#review/164215565

Bug:  749917 
Change-Id: I318fa43ce876439555b571f0e1bf9516e9f785d5
Reviewed-on: https://chromium-review.googlesource.com/590830
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Commit-Queue: Bin Zhao <zhaobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494499}
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.cc
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl_unittest.cc
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/browser/media/router/mojo/media_router_mojo_test.cc
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/common/media_router/discovery/media_sink_internal.cc
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/common/media_router/discovery/media_sink_internal.h
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/common/media_router/discovery/media_sink_internal_unittest.cc
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/common/media_router/mojo/media_router.mojom
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/common/media_router/mojo/media_router_struct_traits.cc
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/common/media_router/mojo/media_router_struct_traits.h
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/common/media_router/mojo/media_router_struct_traits_unittest.cc
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/renderer/resources/extensions/media_router_bindings.js
[modify] https://crrev.com/272ca40c665c7a3ad8d210aac4e12a43197dad29/chrome/renderer/resources/renderer_resources.grd

Status: Fixed (was: Assigned)

Sign in to add a comment