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

Issue 664326 link

Starred by 0 users

Issue metadata

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



Sign in to add a comment

[Media Router] PresentationRequest should be a move-only type

Project Member Reported by mfo...@chromium.org, Nov 10 2016

Issue description

There is a bunch of code in the flow to pass ownership of a presentation request from PSImpl -> PSDImpl -> MRDC -> MRUI.  It would streamline this code path to make the request a move-capable or move-only type instead of transferring it via unique_ptr<> a bunch of places.


 

Comment 1 by sko...@chromium.org, Nov 17 2016

Status: Available (was: Untriaged)

Comment 2 by mfo...@chromium.org, May 10 2017

Labels: Hotlist-Fixit-PE2017
Owner: imch...@chromium.org
Status: Started (was: Available)
Not sure whether the original bug description is still relevant since PresentationRequest behaves more like a data struct, and that we no longer pass it around with unique_ptr (instead, const ref is used). That said, keeping this bug open since I am doing a round of code cleanup right now involving it. (Related:  bug 708209 )

Comment 5 by mfo...@chromium.org, Jul 19 2017

I think a const ref is fine, although IIUC the PresentationRequest is bound to a number of callbacks.  In that case, would prefer to either copy it or add a move constructor depending on the semantics, instead of moving it around via std::unique_ptr.

Yep, enabling move for it would be useful in some scenarios.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 24 2017

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

commit 008d9944bd493e04153cd1812d52b6709d036d5d
Author: Derek Cheng <imcheng@chromium.org>
Date: Mon Jul 24 20:21:49 2017

[Media Router] PresentationRequest cleanup round 1.

Move PresentationRequest to content/browser and use it in
PresentationServiceDelegate where applicable.
- Replace GetMediaSources() method with helper function in c/b/m/r.
- Replace Equals() method with a helper function in PSDImpl.
- PresentationRequest is now a struct.


Other things to consider after this patch:
- Move PresentationRequest out of CPCR, maybe make
  ShowMediaRouterDialogForPresentation take a PR and 2 callbacks?
-- the callbacks can be wrapped in a struct internal to MRDC.
- Remove the PR equality checking - we don't need this if we know
the PresentationRequest has not been reset.
- Support moves for PresentationRequest. Right now it's effective in
only one place but further cleanups may find it useful.


Bug:  664326 ,  708209 
Change-Id: I0ead3d0c2a8cc73849a6d007369e2ab13f1467d2
Reviewed-on: https://chromium-review.googlesource.com/578528
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489064}
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/media/android/router/media_router_dialog_controller_android.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/media/router/BUILD.gn
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/media/router/create_presentation_connection_request.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/media/router/create_presentation_connection_request.h
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/media/router/create_presentation_connection_request_unittest.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/media/router/media_router_dialog_controller_unittest.cc
[delete] https://crrev.com/b6326dbba2a5b7e2b1ce7979031149dd8013ad20/chrome/browser/media/router/presentation_request.cc
[delete] https://crrev.com/b6326dbba2a5b7e2b1ce7979031149dd8013ad20/chrome/browser/media/router/presentation_request.h
[delete] https://crrev.com/b6326dbba2a5b7e2b1ce7979031149dd8013ad20/chrome/browser/media/router/presentation_request_unittest.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/media/router/presentation_service_delegate_impl.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/media/router/presentation_service_delegate_impl.h
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_unittest.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/ui/webui/media_router/media_router_ui.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/ui/webui/media_router/media_router_ui.h
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/common/media_router/media_source_helper.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/common/media_router/media_source_helper.h
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/chrome/test/BUILD.gn
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/content/browser/presentation/presentation_service_impl.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/content/browser/presentation/presentation_service_impl.h
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/content/browser/presentation/presentation_service_impl_unittest.cc
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/content/public/browser/BUILD.gn
[add] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/content/public/browser/presentation_request.cc
[add] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/content/public/browser/presentation_request.h
[modify] https://crrev.com/008d9944bd493e04153cd1812d52b6709d036d5d/content/public/browser/presentation_service_delegate.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 24 2017

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

commit 66d441e7c4fc2e6a41be8a10f6974d026388138e
Author: Fady Samuel <fsamuel@chromium.org>
Date: Mon Jul 24 21:07:10 2017

Revert "[Media Router] PresentationRequest cleanup round 1."

This reverts commit 008d9944bd493e04153cd1812d52b6709d036d5d.

Reason for revert:

This seems to have broken the build on Chrome for Chrome OS.

../../chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:417:57: error: chosen constructor is explicit in copy-initialization
      {main_frame_process_id_, main_frame_routing_id_}, {}, frame_origin_);
                                                        ^~
../../build/linux/debian_jessie_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/vector:79:7: note: explicit constructor declared here
      vector(const _Allocator& __a = _Allocator())
      ^
../../content/public/browser/presentation_request.h:23:48: note: passing argument to parameter 'presentation_urls' here
                      const std::vector<GURL>& presentation_urls,
                                               ^



Original change's description:
> [Media Router] PresentationRequest cleanup round 1.
> 
> Move PresentationRequest to content/browser and use it in
> PresentationServiceDelegate where applicable.
> - Replace GetMediaSources() method with helper function in c/b/m/r.
> - Replace Equals() method with a helper function in PSDImpl.
> - PresentationRequest is now a struct.
> 
> 
> Other things to consider after this patch:
> - Move PresentationRequest out of CPCR, maybe make
>   ShowMediaRouterDialogForPresentation take a PR and 2 callbacks?
> -- the callbacks can be wrapped in a struct internal to MRDC.
> - Remove the PR equality checking - we don't need this if we know
> the PresentationRequest has not been reset.
> - Support moves for PresentationRequest. Right now it's effective in
> only one place but further cleanups may find it useful.
> 
> 
> Bug:  664326 ,  708209 
> Change-Id: I0ead3d0c2a8cc73849a6d007369e2ab13f1467d2
> Reviewed-on: https://chromium-review.googlesource.com/578528
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: mark a. foltz <mfoltz@chromium.org>
> Commit-Queue: Derek Cheng <imcheng@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#489064}

TBR=avi@chromium.org,mfoltz@chromium.org,imcheng@chromium.org

Change-Id: I5f4dde8982386104081caba7cbac91339e930129
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  664326 ,  708209 
Reviewed-on: https://chromium-review.googlesource.com/583908
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489073}
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/android/router/media_router_dialog_controller_android.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/BUILD.gn
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/create_presentation_connection_request.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/create_presentation_connection_request.h
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/create_presentation_connection_request_unittest.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/media_router_dialog_controller_unittest.cc
[add] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/presentation_request.cc
[add] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/presentation_request.h
[add] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/presentation_request_unittest.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/presentation_service_delegate_impl.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/presentation_service_delegate_impl.h
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_unittest.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/ui/webui/media_router/media_router_ui.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/ui/webui/media_router/media_router_ui.h
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/common/media_router/media_source_helper.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/common/media_router/media_source_helper.h
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/chrome/test/BUILD.gn
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/content/browser/presentation/presentation_service_impl.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/content/browser/presentation/presentation_service_impl.h
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/content/browser/presentation/presentation_service_impl_unittest.cc
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/content/public/browser/BUILD.gn
[delete] https://crrev.com/f54c590ab470764e7051f910eb743c9668754528/content/public/browser/presentation_request.cc
[delete] https://crrev.com/f54c590ab470764e7051f910eb743c9668754528/content/public/browser/presentation_request.h
[modify] https://crrev.com/66d441e7c4fc2e6a41be8a10f6974d026388138e/content/public/browser/presentation_service_delegate.h

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 25 2017

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

commit 1071a2399769677b68b43ec1289efca24fb53979
Author: Derek Cheng <imcheng@chromium.org>
Date: Tue Jul 25 22:23:56 2017

[Reland] [Media Router] PresentationRequest cleanup round 1.

In C++11, STL containers cannot be initialized with {} due to
http://cplusplus.github.io/LWG/lwg-defects.html#2193. This was fixed in
C++14, but for now we will spell out the constructor.

Original patch: https://chromium-review.googlesource.com/c/578528/

Original description:

[Media Router] PresentationRequest cleanup round 1.

Move PresentationRequest to content/browser and use it in
PresentationServiceDelegate where applicable.
- Replace GetMediaSources() method with helper function in c/b/m/r.
- Replace Equals() method with a helper function in PSDImpl.
- PresentationRequest is now a struct.


Other things to consider after this patch:
- Move PresentationRequest out of CPCR, maybe make
  ShowMediaRouterDialogForPresentation take a PR and 2 callbacks?
-- the callbacks can be wrapped in a struct internal to MRDC.
- Remove the PR equality checking - we don't need this if we know
the PresentationRequest has not been reset.
- Support moves for PresentationRequest. Right now it's effective in
only one place but further cleanups may find it useful.


Bug:  664326 ,  708209 
Change-Id: I2afb8f3946f5b5fb1832e5789bb9dc987d57f135
Reviewed-on: https://chromium-review.googlesource.com/583870
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489463}
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/media/android/router/media_router_dialog_controller_android.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/media/router/BUILD.gn
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/media/router/create_presentation_connection_request.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/media/router/create_presentation_connection_request.h
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/media/router/create_presentation_connection_request_unittest.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/media/router/media_router_dialog_controller_unittest.cc
[delete] https://crrev.com/f71b90f3e2816bd84bd35bf02d918995c99ddad2/chrome/browser/media/router/presentation_request.cc
[delete] https://crrev.com/f71b90f3e2816bd84bd35bf02d918995c99ddad2/chrome/browser/media/router/presentation_request.h
[delete] https://crrev.com/f71b90f3e2816bd84bd35bf02d918995c99ddad2/chrome/browser/media/router/presentation_request_unittest.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/media/router/presentation_service_delegate_impl.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/media/router/presentation_service_delegate_impl.h
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_unittest.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/ui/webui/media_router/media_router_ui.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/ui/webui/media_router/media_router_ui.h
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/common/media_router/media_source_helper.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/common/media_router/media_source_helper.h
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/chrome/test/BUILD.gn
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/content/browser/presentation/presentation_service_impl.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/content/browser/presentation/presentation_service_impl.h
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/content/browser/presentation/presentation_service_impl_unittest.cc
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/content/public/browser/BUILD.gn
[add] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/content/public/browser/presentation_request.cc
[add] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/content/public/browser/presentation_request.h
[modify] https://crrev.com/1071a2399769677b68b43ec1289efca24fb53979/content/public/browser/presentation_service_delegate.h

Status: Fixed (was: Started)

Sign in to add a comment