New issue
Advanced search Search tips

Issue 632623 link

Starred by 2 users
Status: Fixed
Owner:
mfo...@chromium.org
Closed: Mar 2017
Cc:
mfo...@chromium.org
btolsch@chromium.org
powerb@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 679075
Blocking:
issue 629372



Sign in to add a comment

Use typemapping and remove presentation_type_converters.cc

Project Member Reported by dcheng@chromium.org, Jul 29 2016 
Related to  issue 629377 . There's some limited checks now, but it would be much better if this was a typemap: then an invalid message would (or it should) result in killing the process sending the browser process the bad data, as well as closing the message pipe. From a security perspective, this is much better than keeping a process sending us invalid data alive.

Comment 1 by mfo...@chromium.org, Aug 25 2016 

Any explicit conversions between string types and GURL/KURL should also be removed as we migrate implementations to use URL types.

Comment 2 by sko...@chromium.org, Sep 9 2016

Labels: -M-54

Comment 3 by mfo...@chromium.org, Sep 10 2016

Cc: -mfo...@chromium.org
Owner: mfo...@chromium.org
Status: Started (was: Available)
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 15 2016 

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

commit 551008391f0723c846b2277048ef7ce9c61e805b
Author: mfoltz <mfoltz@chromium.org>
Date: Thu Sep 15 02:48:54 2016

Converts Presentation API to use KURL/WebURL in place of string types for availability and presentation URLs.  Removes several conversions.

BUG= 632623 

Review-Url: https://codereview.chromium.org/2327993002
Cr-Commit-Position: refs/heads/master@{#418767}

[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/content/renderer/presentation/presentation_connection_client.cc
[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/content/renderer/presentation/presentation_connection_client.h
[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/content/renderer/presentation/presentation_dispatcher.cc
[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/content/renderer/presentation/presentation_dispatcher.h
[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/third_party/WebKit/Source/modules/presentation/PresentationConnection.h
[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/third_party/WebKit/Source/modules/presentation/PresentationController.cpp
[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp
[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h
[modify] https://crrev.com/551008391f0723c846b2277048ef7ce9c61e805b/third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 8 2016 

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

commit 7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74
Author: mfoltz <mfoltz@chromium.org>
Date: Sat Oct 08 01:35:24 2016

Converts content/browser for Presentation API and Media Router APIs to use GURL for presentation URLs.  Updates many, many unit tests.

BUG= 632623 

Review-Url: https://codereview.chromium.org/2386633003
Cr-Commit-Position: refs/heads/master@{#424037}

[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/create_presentation_connection_request.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/create_presentation_connection_request.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/create_presentation_connection_request_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/media_route.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/media_route_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/media_router.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/media_sink.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/media_sinks_observer_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/media_source.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/media_source.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/media_source_helper.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/media_source_helper.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/media_source_helper_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/mock_screen_availability_listener.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/mock_screen_availability_listener.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/presentation_media_sinks_observer_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/presentation_request.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/presentation_request.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/presentation_request_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/presentation_service_delegate_impl.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/presentation_service_delegate_impl.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/media/router/test_helper.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/ui/webui/media_router/cast_modes_with_media_sources_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/ui/webui/media_router/media_router_ui.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/ui/webui/media_router/query_result_manager.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/ui/webui/media_router/query_result_manager.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/chrome/test/media_router/media_router_e2e_browsertest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/content/browser/presentation/presentation_service_impl.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/content/browser/presentation/presentation_service_impl.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/content/browser/presentation/presentation_service_impl_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/content/browser/presentation/presentation_type_converters.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/content/browser/presentation/presentation_type_converters_unittest.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/content/public/browser/presentation_screen_availability_listener.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/content/public/browser/presentation_service_delegate.h
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/content/public/browser/presentation_session.cc
[modify] https://crrev.com/7a2c823bd8f66f7ea61f8ade94f5e76f0f32ed74/content/public/browser/presentation_session.h

Comment 6 by dcheng@chromium.org, Nov 17 2016

Summary: Use typemapping and remove GetPresentationSessionMessage / ToMojoSessionMessage / media_router_type_converters.cc (was: Use typemapping and remove GetPresentationSessionMessage / ToMojoSessionMessage)
Updating this title to reflect that this issue also covers the type converters that media router uses.

Comment 7 by mfo...@chromium.org, Jan 6 2017

Blockedon: 679075

Comment 8 by mfo...@chromium.org, Jan 10 2017

Summary: Use typemapping and remove presentation_type_converters.cc (was: Use typemapping and remove GetPresentationSessionMessage / ToMojoSessionMessage / media_router_type_converters.cc)
FYI I'm using this bug to cover type conversions for presentation.mojom.
media_router.mojom is  Bug #629373 .
Project Member

Comment 9 by bugdroid1@chromium.org, Feb 3 2017 

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

commit dfbd7c3e439e77b2d685ba90e0a90178a164410c
Author: mfoltz <mfoltz@chromium.org>
Date: Fri Feb 03 20:04:19 2017

Replace type converters for presentation.mojom with typemaps.

Typemap for ConnectionMessage will come in a separate patch, which should finish the conversion for presentation.mojom, barring further refactorings.

BUG= 632623 

Review-Url: https://codereview.chromium.org/2613153003
Cr-Commit-Position: refs/heads/master@{#448057}

[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/browser/BUILD.gn
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/browser/presentation/OWNERS
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/browser/presentation/presentation_service_impl.cc
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/browser/presentation/presentation_service_impl.h
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/browser/presentation/presentation_service_impl_unittest.cc
[delete] https://crrev.com/c03dcfbe13aecc48b0b4cd0a107a8d659bdda939/content/browser/presentation/presentation_type_converters.cc
[delete] https://crrev.com/c03dcfbe13aecc48b0b4cd0a107a8d659bdda939/content/browser/presentation/presentation_type_converters.h
[delete] https://crrev.com/c03dcfbe13aecc48b0b4cd0a107a8d659bdda939/content/browser/presentation/presentation_type_converters_unittest.cc
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/common/DEPS
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/common/presentation/OWNERS
[add] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/common/presentation/presentation.typemap
[add] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/common/presentation/presentation_struct_traits.h
[add] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/common/presentation/typemaps.gni
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/public/common/presentation_session.h
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/renderer/presentation/presentation_connection_proxy.cc
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/renderer/presentation/presentation_connection_proxy.h
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/renderer/presentation/presentation_dispatcher.cc
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/renderer/presentation/presentation_dispatcher.h
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/renderer/presentation/presentation_dispatcher_unittest.cc
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/content/test/BUILD.gn
[modify] https://crrev.com/dfbd7c3e439e77b2d685ba90e0a90178a164410c/mojo/public/tools/bindings/chromium_bindings_configuration.gni
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 7 2017 

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

commit 0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a
Author: mfoltz <mfoltz@chromium.org>
Date: Tue Mar 07 18:24:19 2017

Converts presentation.mojom to use a typemap for content::PresentationConnectionMessage.

This triggered a number of knock-on refactorings:

1. PresentationConnectionMessage is now a move-only type to enforce no extraneous copies as it is passed in the browser.

2. presentation_constants.h is folded into presentation_connection_message.h as it defines a constant only for PCM.

3. The mojo message is now a union and the type field in PCM is removed.  It can be derived from which Optional field is set.

4. Tests are refactored to remove needless proxy/trampoline methods.

There are still more tangentially related cleanups to do:

- RouteMessage is not necessary and can be replaced with PCM.
- Unit tests use message loops in needlessly complicated ways.
- The message size limit needs to be more flexible and enforced downstream as Cast and non-Cast sinks have different limits.

BUG= 632623 

Review-Url: https://codereview.chromium.org/2706463002
Cr-Commit-Position: refs/heads/master@{#455143}

[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/chrome/browser/media/router/browser_presentation_connection_proxy.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/chrome/browser/media/router/browser_presentation_connection_proxy.h
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/chrome/browser/media/router/presentation_service_delegate_impl.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/chrome/browser/media/router/presentation_service_delegate_impl.h
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/chrome/browser/media/router/route_message.h
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/browser/presentation/presentation_service_impl.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/browser/presentation/presentation_service_impl.h
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/browser/presentation/presentation_service_impl_unittest.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/common/BUILD.gn
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/common/presentation/presentation.typemap
[add] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/common/presentation/presentation_struct_traits.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/common/presentation/presentation_struct_traits.h
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/public/browser/presentation_service_delegate.h
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/public/common/BUILD.gn
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/public/common/presentation_connection_message.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/public/common/presentation_connection_message.h
[delete] https://crrev.com/ff5ba024e4149f83c45711c070c2473a417b7963/content/public/common/presentation_constants.cc
[delete] https://crrev.com/ff5ba024e4149f83c45711c070c2473a417b7963/content/public/common/presentation_constants.h
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/renderer/presentation/presentation_connection_proxy.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/renderer/presentation/presentation_connection_proxy.h
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/renderer/presentation/presentation_connection_proxy_unittest.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/renderer/presentation/presentation_dispatcher.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/renderer/presentation/presentation_dispatcher.h
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/content/renderer/presentation/presentation_dispatcher_unittest.cc
[modify] https://crrev.com/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a/third_party/WebKit/public/platform/modules/presentation/presentation.mojom

Comment 11 by mfo...@chromium.org, Mar 7 2017 

This can finally be closed.  All data types in PresentationService are typemapped and manual conversions are gone.  Further cleanups are noted here:

Bug 699219 - [Presentation API] Enforce presentation message size limit downstream from Presentation API
 Bug 699225  - [PresentationAPI] Remove RunLoop::Quit() from presentation unit tests
 Bug 699230  - [Media Router] Replace media_router::RouteMessage with PresentationConnectionMessage

Comment 12 by mfo...@chromium.org, Mar 7 2017

Status: Fixed (was: Started)
►

Sign in to add a comment