Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2727123002:
[Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces (Closed)
|
Created:
3 years, 9 months ago by takumif Modified:
3 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, chfremer+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and
mojo interfaces
This CL adds MediaRouteController that will forward media controller commands
from the custom controls WebUI to the Media Router component extension, and
will receive MediaStatus updates from the extension which it then will forward
to its Observer(s) (= to the WebUI).
This patch includes mojo interfaces/structs that were first reviewed in this
patch [1]. The MediaController mojo interface will be implemented in the
component extension (not a part of Chromium), and will receive commands from
MediaRouteController. MediaRouteController implements the MediaStatusObserver
mojo interface and receives updates from the extension. Typemapping between
media_router::mojom::MediaStatus and media_router::MediaStatus is also
included in this CL.
Changes in extensions/renderer/ are for exposing the new mojo interfaces to
the component extension.
The Chromium-side implementation of custom controls redesign will be done in
these patches:
1. Mojo/MediaStatus/MediaRouteController: this patch
2. MediaRouter::GetRouteController(): http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002
4. Custom controls WebUI: http://crrev/2725503002
Custom controls redesign design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4G0/edit
[1] https://codereview.chromium.org/2674363002/
BUG=684636, 684635
Review-Url: https://codereview.chromium.org/2727123002
Cr-Commit-Position: refs/heads/master@{#458922}
Committed: https://chromium.googlesource.com/chromium/src/+/67a8f64435d0a1c3ae9d04e33f965f885b9cb8b7
Patch Set 1 : . #
Total comments: 86
Patch Set 2 : Address Mark, Devlin, Derek, and Daniel's comments #
Total comments: 2
Patch Set 3 : Rebase #Patch Set 4 : Make MediaRouteController ref-counted, move MediaStatus typemapping into its own files #Patch Set 5 : Add OnControllerBound() #
Total comments: 33
Patch Set 6 : Address Derek's and Daniel's comments #
Total comments: 42
Patch Set 7 : Remvoe OnControllerBound() #Patch Set 8 : Address Daniel's, Mark's, and Derek's comments #
Total comments: 30
Patch Set 9 : Address Mark's comments #Patch Set 10 : Add CreateObserver() to MRController unit test #
Total comments: 6
Patch Set 11 : Address Derek's comments #
Total comments: 7
Patch Set 12 : Address Mark's and Daniel's comments #Patch Set 13 : Rebase #Patch Set 14 : Change MediaStatus::status to MediaStatus::description. Attempt to fix buildbot failure #Patch Set 15 : Change comment #
Total comments: 2
Messages
Total messages: 89 (54 generated)
Description was changed from ========== [Media Router] Add MediaStatus, MediaRouteController, and mojo/typemapping BUG= ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus and MediaRouteController BUG=684636,684635 ==========
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus and MediaRouteController BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus and MediaRouteController crrev/2727123002 BUG=684636,684635 ==========
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus and MediaRouteController crrev/2727123002 BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus and MediaRouteController https://crrev/2727123002 BUG=684636,684635 ==========
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus and MediaRouteController https://crrev/2727123002 BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus and MediaRouteController http://crrev/2727123002 BUG=684636,684635 ==========
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus and MediaRouteController http://crrev/2727123002 BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus and MediaRouteController 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): TBD 3. MRUI/MRWebUIMessageHandler: TBD 4. Custom controls WebUI: http://crrev/2725503002 BUG=684636,684635 ==========
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus and MediaRouteController 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): TBD 3. MRUI/MRWebUIMessageHandler: TBD 4. Custom controls WebUI: http://crrev/2725503002 BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that forwards media controller commands from the custom controls WebUI to the Media Router component extension, and receives MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and MediaStatus is also included. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): TBD 3. MRUI/MRWebUIMessageHandler: TBD 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that forwards media controller commands from the custom controls WebUI to the Media Router component extension, and receives MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and MediaStatus is also included. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): TBD 3. MRUI/MRWebUIMessageHandler: TBD 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that forwards media controller commands from the custom controls WebUI to the Media Router component extension, and receives MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and MediaStatus is also included. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): TBD 3. MRUI/MRWebUIMessageHandler: TBD 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that forwards media controller commands from the custom controls WebUI to the Media Router component extension, and receives MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and MediaStatus is also included. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): TBD 3. MRUI/MRWebUIMessageHandler: TBD 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that forwards media controller commands from the custom controls WebUI to the Media Router component extension, and receives MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and MediaStatus is also included. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that forwards media controller commands from the custom controls WebUI to the Media Router component extension, and receives MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and MediaStatus is also included. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that forwards media controller commands from the custom controls WebUI to the Media Router component extension, and receives MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and MediaStatus is also included. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
Patchset #1 (id:40001) has been deleted
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that forwards media controller commands from the custom controls WebUI to the Media Router component extension, and receives MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and MediaStatus is also included. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls redesign design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls redesign design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. MediaRouteController implements the MediaStatusObserver mojo interface and receives updates from the extension. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls redesign design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
takumif@chromium.org changed reviewers: + dcheng@chromium.org, imcheng@chromium.org, mfoltz@chromium.org, rdevlin.cronin@chromium.org
imcheng@/mfoltz@: overall dcheng@: *.mojom, *.typemap, *struct_traits* rdcronin@: extensions/renderer/* Please take a look. This includes mojo changes from this patch [1] -- as recommended by dcheng@, I've added classes that use the mojo changes. Thanks! [1] https://codereview.chromium.org/2674363002/
Looks good overall. Most comments about clarifying the role of MediaRouteController. Killing the controller when observers go away is still a little head scratching to me - I would really prefer that clients be explicit about destroying the controller. Feel free to discuss offline if there's something I forgot from our last chat about that. I think I don't have much to add on the mojo changes themselves at this point. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_route_status_observer.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_route_status_observer.h:12: // An interface for observing updates to the status of a Media Route. How is this interface specific to Media Routes? Can it just be MediaStatusObserver? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_status.cc (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.cc:9: MediaStatus::MediaStatus() {} Style nit: using = default allows the compiler to generate a default move constructor even if a custom constructor is defined later. http://en.cppreference.com/w/cpp/language/default_constructor http://en.cppreference.com/w/cpp/language/move_constructor https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:24: // A controller for a MediaRoute. Forwards commands for controlling the route to This only controls media routes that are owned out-of-process by a MediaRouteProvider (the actual controller implementation lives in the provider). As far as I can tell, this class applies equally whether the MRP is hosted in the event page or in a different process type. Suggested text: "A controller for a MediaRoute hosted out-of-process in a Media Route Provider (e.g., in the Media Router component extension)." Also, I would suggest naming this e.g. ProviderMediaRouteController and making the connection to the MRP clearer in the documentation. imcheng@ may also have naming suggestions. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:27: // connection to the extension-side controller is invalidated, the route is Will this prevent the controller from persisting after the event page suspends? How would the caller/UX know when this has happened? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:28: // terminated, or the last observer of |this| is removed. I still find the lifetime dependency between the controller and its observers unintuitive from a design pattern point of view. If I have a media route with ephemeral UX for control, do I have to create and destroy the controller each time the UX appears/disappears? We might want the controller to persist during the entire time the dialog is open (even if the controls are not visible). See my earlier comments about adding a disposal API to give the browser the ability to be explicit about when the controller is no longer needed. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:29: class MediaRouteController : public mojom::MediaStatusObserver { Shouldn't this implement mojo::MediaController? Will an in-browser implementation of mojo::MediaController be possible to migrate media controller implementations out of the extension in the future? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:31: // Attempts to establish a mojo connection with the extension-side controller s/extension-side controller/Media Route Provider/ https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:43: // Media controller methods for forwarding commands to the extension-side ...to the Media Route Provider. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:51: void AddObserver(MediaRouteStatusObserver* observer); Please document which of the API override/implement mojo::MediaStatusObserver via // comment https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:62: // The extension-side controller that |this| forwards commands to. Media Route Provider https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:65: // The binding to observe the extension-side provider of status updates. observe status updates from the Media Route Provider https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:69: std::unordered_set<MediaRouteStatusObserver*> observers_; base::ObserverList https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:71: mojom::MediaRouteProvider* const provider_; Who owns this? Please document https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:20: class MockMediaController : public mojom::MediaController { Can these mocks be added to the library in mock_media_router.h? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:70: std::unique_ptr<MediaRouteController> controller_; Declare members in the order they are initialized. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:71: std::unique_ptr<MockMediaRouteProvider> mock_provider_; Can this just be declared as a plain member? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:99: MockMediaRouteStatusObserver observer1; Nit: Declare these as StrictMock<> to enforce strict cardinality on the number of calls (no need for .Times()). https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:136: if (!data.ReadTitle(&title)) Check that title and status are valid UTF-8.
https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... extensions/renderer/dispatcher.cc:745: {"chrome/browser/media/router/mojo/media_controller.mojom", Where are these used? I can't find them being required anywhere.
https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_route_status_observer.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_route_status_observer.h:15: MediaRouteStatusObserver() {} Constructors aren't required for interfaces. Also similar comment to mfoltz@'s, it doesn't seem specific to a MediaRoute. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_status.cc (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.cc:9: MediaStatus::MediaStatus() {} Please also make sure member variables with primitive types are initialized. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_controller.mojom (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_controller.mojom:17: // Pauses the media if it is playing. Is a no-op if not supported by the media nit: add a line break between each method for better readability. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:23: provider_->CreateMediaRouteController( A couple of questions. 1) Can the initialization of the mojo controller and mojo observer be one step? 2) Can the calls to MediaRouteProvider to initialize the controller be moved to MediaRouterMojoImpl? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:24: // A controller for a MediaRoute. Forwards commands for controlling the route to On 2017/03/06 20:06:19, mark a. foltz wrote: > This only controls media routes that are owned out-of-process by a > MediaRouteProvider (the actual controller implementation lives in the provider). > > As far as I can tell, this class applies equally whether the MRP is hosted in > the event page or in a different process type. Suggested text: > > "A controller for a MediaRoute hosted out-of-process in a Media Route Provider > (e.g., in the Media Router component extension)." > > Also, I would suggest naming this e.g. ProviderMediaRouteController and making > the connection to the MRP clearer in the documentation. imcheng@ may also have > naming suggestions. I think MediaRouteController is fine. The only part that is specific to the MediaRouteProvider is the setup of the controller / observer bindings, i.e. if we extracted the setup logic to elsewhere then this class won't need to have a direct reference to a MediaRouteProvider. What if the initialization of this object is done in MediaRouterMojoImpl? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:27: // connection to the extension-side controller is invalidated, the route is On 2017/03/06 20:06:19, mark a. foltz wrote: > Will this prevent the controller from persisting after the event page suspends? > How would the caller/UX know when this has happened? The event page does not suspend when there is at least one controller active. Ideally, we can get away with not forcing the extension to stay awake, by having this class use the same event page wakeup / queuing logic used by MediaRouterMojoImpl. This requires some refactoring of the latter so I don't think it's necessary for v1. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:28: // terminated, or the last observer of |this| is removed. On 2017/03/06 20:06:19, mark a. foltz wrote: > I still find the lifetime dependency between the controller and its observers > unintuitive from a design pattern point of view. If I have a media route with > ephemeral UX for control, do I have to create and destroy the controller each > time the UX appears/disappears? We might want the controller to persist during > the entire time the dialog is open (even if the controls are not visible). > > See my earlier comments about adding a disposal API to give the browser the > ability to be explicit about when the controller is no longer needed. This is a bit tricky because the controller can become invalid from multiple sources. There are a few possible approaches. For example, the users can maintain a scoped_refptr<MediaRouteController>. But they would also need a way to tell if the controller is no longer valid due to route termination in order to discard the reference. I think it would make more sense if we defined ownership of this class and the API the caller can use to obtain a reference to an instance. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:29: class MediaRouteController : public mojom::MediaStatusObserver { On 2017/03/06 20:06:19, mark a. foltz wrote: > Shouldn't this implement mojo::MediaController? > > Will an in-browser implementation of mojo::MediaController be possible to > migrate media controller implementations out of the extension in the future? The extension side implements mojo::MediaController. A browser implementation of mojo::MediaController is theoretically possible. This class would talk to it via the same MediaControllerPtr.
https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_status.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:17: virtual ~MediaStatus(); No virtual. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:54: uint32_t duration; base::TimeDelta? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_controller.mojom (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_controller.mojom:30: Seek(uint32 time); base::TimeDelta here as well, if possible https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:35: mojom::MediaRouteProvider* provider, It's a bit unusual to pass around raw pointers to Mojo interfaces. What keeps the interface pointer alive? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:60: base::Closure destroyer_callback_; This is pretty unusual. Where's the code that actually uses this interface? Why do we need to indirect this through a callback? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:66: std::unique_ptr<mojo::Binding<mojom::MediaStatusObserver>> binding_; unique_ptr is unnecessary here. In the initializer list, you can just construct it with |this|, and then connect it later by calling Bind() on a InterfaceRequest. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:344: SetMediaRouteStatusObserver(string route_id, MediaStatusObserver observer); Is there a time when we'd want to create a media router without an observer? Since this is 1:1 atm, we should just bind it as part CreateMediaRouteController() by passing MediaStatusObserver. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:136: if (!data.ReadTitle(&title)) Just read it directly into out->title, rather than constructing a temporary) https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:325: static std::string title(const media_router::MediaStatus& status) { const std::string& here and below, to avoid temporaries https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_status.mojom (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_status.mojom:38: uint32 duration; Can we use mojo.common.mojom.TimeDelta from https://cs.chromium.org/chromium/src/mojo/common/time.mojom?rcl=17218793515d6... https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_status.mojom:41: uint32 current_time; Same here.
Thanks for the reviews! https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_route_status_observer.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_route_status_observer.h:12: // An interface for observing updates to the status of a Media Route. On 2017/03/06 20:06:19, mark a. foltz wrote: > How is this interface specific to Media Routes? Can it just be > MediaStatusObserver? No, it's not really specific to media routes, but I didn't want to name it MediaStatusObserver because it doesn't implement media_router::mojom::MediaStatusObserver. Also, when I named this class MediaStatusObserver I had the compiler interpret it as the mojo variation in some instances that led to weird errors, so I thought it'd be better to make it less ambiguous. The same for MediaRouteController vs mojom::MediaController. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_route_status_observer.h:15: MediaRouteStatusObserver() {} On 2017/03/06 23:11:41, imcheng wrote: > Constructors aren't required for interfaces. Also similar comment to mfoltz@'s, > it doesn't seem specific to a MediaRoute. Removing the ctor, but I'm a bit hesitant to name this MediaStatusObserver. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_status.cc (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.cc:9: MediaStatus::MediaStatus() {} On 2017/03/06 23:11:41, imcheng wrote: > Please also make sure member variables with primitive types are initialized. Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.cc:9: MediaStatus::MediaStatus() {} On 2017/03/06 20:06:19, mark a. foltz wrote: > Style nit: using = default allows the compiler to generate a default move > constructor even if a custom constructor is defined later. > > http://en.cppreference.com/w/cpp/language/default_constructor > http://en.cppreference.com/w/cpp/language/move_constructor Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_status.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:17: virtual ~MediaStatus(); On 2017/03/08 01:08:19, dcheng wrote: > No virtual. Sorry for my lack of C++ knowledge, but would you mind explaining why it'd be okay not to mark the dtor virtual? If we created sub-structs with extra data, shouldn't the dtor be virtual? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:54: uint32_t duration; On 2017/03/08 01:08:19, dcheng wrote: > base::TimeDelta? Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_controller.mojom (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_controller.mojom:17: // Pauses the media if it is playing. Is a no-op if not supported by the media On 2017/03/06 23:11:41, imcheng wrote: > nit: add a line break between each method for better readability. Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_controller.mojom:30: Seek(uint32 time); On 2017/03/08 01:08:19, dcheng wrote: > base::TimeDelta here as well, if possible Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:23: provider_->CreateMediaRouteController( On 2017/03/06 23:11:41, imcheng wrote: > A couple of questions. > > 1) Can the initialization of the mojo controller and mojo observer be one step? > 2) Can the calls to MediaRouteProvider to initialize the controller be moved to > MediaRouterMojoImpl? Yes, both are possible. Doing so. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:24: // A controller for a MediaRoute. Forwards commands for controlling the route to On 2017/03/06 23:11:41, imcheng wrote: > On 2017/03/06 20:06:19, mark a. foltz wrote: > > This only controls media routes that are owned out-of-process by a > > MediaRouteProvider (the actual controller implementation lives in the > provider). > > > > As far as I can tell, this class applies equally whether the MRP is hosted in > > the event page or in a different process type. Suggested text: > > > > "A controller for a MediaRoute hosted out-of-process in a Media Route Provider > > (e.g., in the Media Router component extension)." > > > > Also, I would suggest naming this e.g. ProviderMediaRouteController and making > > the connection to the MRP clearer in the documentation. imcheng@ may also > have > > naming suggestions. > > I think MediaRouteController is fine. The only part that is specific to the > MediaRouteProvider is the setup of the controller / observer bindings, i.e. if > we extracted the setup logic to elsewhere then this class won't need to have a > direct reference to a MediaRouteProvider. What if the initialization of this > object is done in MediaRouterMojoImpl? We can move the initialization to MRMojoImpl, and have MediaRouteController not have to refer to MRP. At that point there is nothing MR-specific about MediaRouteController, so we could even call this MediaController (but I'm hesitant to do so because of confusion with mojom::MediaController). https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:28: // terminated, or the last observer of |this| is removed. On 2017/03/06 20:06:19, mark a. foltz wrote: > I still find the lifetime dependency between the controller and its observers > unintuitive from a design pattern point of view. If I have a media route with > ephemeral UX for control, do I have to create and destroy the controller each > time the UX appears/disappears? We might want the controller to persist during > the entire time the dialog is open (even if the controls are not visible). > > See my earlier comments about adding a disposal API to give the browser the > ability to be explicit about when the controller is no longer needed. The current design is that the event page is awake as long as there is a MediaRouteController, so it's probably better to tie its lifetime just to route details view, than any dialog view. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:29: class MediaRouteController : public mojom::MediaStatusObserver { On 2017/03/06 20:06:19, mark a. foltz wrote: > Shouldn't this implement mojo::MediaController? > > Will an in-browser implementation of mojo::MediaController be possible to > migrate media controller implementations out of the extension in the future? Given that this only receives commands from within the browser process (mostly MediaRouterUI), I don't think there's a need to implement mojo::MediaController for now. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:31: // Attempts to establish a mojo connection with the extension-side controller On 2017/03/06 20:06:19, mark a. foltz wrote: > s/extension-side controller/Media Route Provider/ Removing mentions of extension/MRP. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:35: mojom::MediaRouteProvider* provider, On 2017/03/08 01:08:20, dcheng wrote: > It's a bit unusual to pass around raw pointers to Mojo interfaces. What keeps > the interface pointer alive? Removing this in the new patch. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:43: // Media controller methods for forwarding commands to the extension-side On 2017/03/06 20:06:19, mark a. foltz wrote: > ...to the Media Route Provider. Removing mentions of extension/MRP. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:51: void AddObserver(MediaRouteStatusObserver* observer); On 2017/03/06 20:06:19, mark a. foltz wrote: > Please document which of the API override/implement mojo::MediaStatusObserver > via // comment OnMediaStatusUpdated() is the only method. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:60: base::Closure destroyer_callback_; On 2017/03/08 01:08:19, dcheng wrote: > This is pretty unusual. Where's the code that actually uses this interface? Why > do we need to indirect this through a callback? This is used by MediaRouterMojoImpl which will own this (the code will be in another CL [1]). MediaRouteController will be stored in a map, and we want to delete it from the map whenever it has no observers. [1] https://codereview.chromium.org/2728543009/diff/1/chrome/browser/media/router... https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:62: // The extension-side controller that |this| forwards commands to. On 2017/03/06 20:06:19, mark a. foltz wrote: > Media Route Provider Removing mentions of extension/MRP. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:65: // The binding to observe the extension-side provider of status updates. On 2017/03/06 20:06:19, mark a. foltz wrote: > observe status updates from the Media Route Provider Removing mentions of extension/MRP. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:66: std::unique_ptr<mojo::Binding<mojom::MediaStatusObserver>> binding_; On 2017/03/08 01:08:20, dcheng wrote: > unique_ptr is unnecessary here. In the initializer list, you can just construct > it with |this|, and then connect it later by calling Bind() on a > InterfaceRequest. Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:69: std::unordered_set<MediaRouteStatusObserver*> observers_; On 2017/03/06 20:06:19, mark a. foltz wrote: > base::ObserverList Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:71: mojom::MediaRouteProvider* const provider_; On 2017/03/06 20:06:19, mark a. foltz wrote: > Who owns this? Please document Removed the reference. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:20: class MockMediaController : public mojom::MediaController { On 2017/03/06 20:06:20, mark a. foltz wrote: > Can these mocks be added to the library in mock_media_router.h? I don't think so, since mock_media_router.h is for Android as well. Or do you mean media_router_mojo_test.h? Either way, I feel it's alright for these classes to be here until they're useful elsewhere. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:70: std::unique_ptr<MediaRouteController> controller_; On 2017/03/06 20:06:20, mark a. foltz wrote: > Declare members in the order they are initialized. Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:71: std::unique_ptr<MockMediaRouteProvider> mock_provider_; On 2017/03/06 20:06:19, mark a. foltz wrote: > Can this just be declared as a plain member? Removed. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:99: MockMediaRouteStatusObserver observer1; On 2017/03/06 20:06:19, mark a. foltz wrote: > Nit: Declare these as StrictMock<> to enforce strict cardinality on the number > of calls (no need for .Times()). Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:344: SetMediaRouteStatusObserver(string route_id, MediaStatusObserver observer); On 2017/03/08 01:08:20, dcheng wrote: > Is there a time when we'd want to create a media router without an observer? > Since this is 1:1 atm, we should just bind it as part > CreateMediaRouteController() by passing MediaStatusObserver. We originally had it that way and there was some discussion about it in the previous CL [1]. We gravitated towards having two separate methods because the MediaController implementation and the provider that MediaStatusObserver observes don't really have to be the same object. [1] https://codereview.chromium.org/2674363002/ https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:136: if (!data.ReadTitle(&title)) On 2017/03/06 20:06:20, mark a. foltz wrote: > Check that title and status are valid UTF-8. Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:136: if (!data.ReadTitle(&title)) On 2017/03/08 01:08:20, dcheng wrote: > Just read it directly into out->title, rather than constructing a temporary) Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:325: static std::string title(const media_router::MediaStatus& status) { On 2017/03/08 01:08:20, dcheng wrote: > const std::string& here and below, to avoid temporaries Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_status.mojom (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_status.mojom:38: uint32 duration; On 2017/03/08 01:08:20, dcheng wrote: > Can we use mojo.common.mojom.TimeDelta from > https://cs.chromium.org/chromium/src/mojo/common/time.mojom?rcl=17218793515d6... Done. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_status.mojom:41: uint32 current_time; On 2017/03/08 01:08:20, dcheng wrote: > Same here. Done. https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... extensions/renderer/dispatcher.cc:745: {"chrome/browser/media/router/mojo/media_controller.mojom", On 2017/03/06 22:51:05, Devlin wrote: > Where are these used? I can't find them being required anywhere. media_router.mojom includes these two files, so the Media Router component extension throws an error if they aren't included here.
https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... extensions/renderer/dispatcher.cc:745: {"chrome/browser/media/router/mojo/media_controller.mojom", On 2017/03/08 04:24:03, takumif wrote: > On 2017/03/06 22:51:05, Devlin wrote: > > Where are these used? I can't find them being required anywhere. > > media_router.mojom includes these two files, so the Media Router component > extension throws an error if they aren't included here. Why? These resources are added to the Dispatcher's source map, which is only used when require()'ing js resources. Unless we somewhere map a mojom's #import to require() (which I hope we don't), I don't see the correlation?
On 2017/03/08 19:33:24, Devlin wrote: > https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... > File extensions/renderer/dispatcher.cc (right): > > https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... > extensions/renderer/dispatcher.cc:745: > {"chrome/browser/media/router/mojo/media_controller.mojom", > On 2017/03/08 04:24:03, takumif wrote: > > On 2017/03/06 22:51:05, Devlin wrote: > > > Where are these used? I can't find them being required anywhere. > > > > media_router.mojom includes these two files, so the Media Router component > > extension throws an error if they aren't included here. > > Why? These resources are added to the Dispatcher's source map, which is only > used when require()'ing js resources. Unless we somewhere map a mojom's #import > to require() (which I hope we don't), I don't see the correlation? I believe you don't need to add those resources until you start requiring them in media_router_bindings.js. I would revert those changes for now and add it when we add support for the new APIs in the bindings code.
On 2017/03/08 20:15:41, imcheng wrote: > On 2017/03/08 19:33:24, Devlin wrote: > > > https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... > > File extensions/renderer/dispatcher.cc (right): > > > > > https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... > > extensions/renderer/dispatcher.cc:745: > > {"chrome/browser/media/router/mojo/media_controller.mojom", > > On 2017/03/08 04:24:03, takumif wrote: > > > On 2017/03/06 22:51:05, Devlin wrote: > > > > Where are these used? I can't find them being required anywhere. > > > > > > media_router.mojom includes these two files, so the Media Router component > > > extension throws an error if they aren't included here. > > > > Why? These resources are added to the Dispatcher's source map, which is only > > used when require()'ing js resources. Unless we somewhere map a mojom's > #import > > to require() (which I hope we don't), I don't see the correlation? > > I believe you don't need to add those resources until you start requiring them > in media_router_bindings.js. I would revert those changes for now and add it > when we add support for the new APIs in the bindings code. I believe the mojo import statements are calling require(). I'm getting errors like this: (BLESSED_EXTENSION context for pkedcjkdefgpdelpbcmbmeomcjbeemfm) No source for require(chrome/browser/media/router/mojo/media_controller.mojom)
https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_route_status_observer.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_route_status_observer.h:12: // An interface for observing updates to the status of a Media Route. On 2017/03/08 at 04:24:02, takumif wrote: > On 2017/03/06 20:06:19, mark a. foltz wrote: > > How is this interface specific to Media Routes? Can it just be > > MediaStatusObserver? > > No, it's not really specific to media routes, but I didn't want to name it MediaStatusObserver because it doesn't implement media_router::mojom::MediaStatusObserver. Also, when I named this class MediaStatusObserver I had the compiler interpret it as the mojo variation in some instances that led to weird errors, so I thought it'd be better to make it less ambiguous. The same for MediaRouteController vs mojom::MediaController. Only generated mojo bindings should be in the mojo:: namespace; and I believe even type-mapped types will be referenced with the proper namespace. Maybe you can show me the errors you're seeing? https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:24: // A controller for a MediaRoute. Forwards commands for controlling the route to On 2017/03/08 at 04:24:03, takumif wrote: > On 2017/03/06 23:11:41, imcheng wrote: > > On 2017/03/06 20:06:19, mark a. foltz wrote: > > > This only controls media routes that are owned out-of-process by a > > > MediaRouteProvider (the actual controller implementation lives in the > > provider). > > > > > > As far as I can tell, this class applies equally whether the MRP is hosted in > > > the event page or in a different process type. Suggested text: > > > > > > "A controller for a MediaRoute hosted out-of-process in a Media Route Provider > > > (e.g., in the Media Router component extension)." > > > > > > Also, I would suggest naming this e.g. ProviderMediaRouteController and making > > > the connection to the MRP clearer in the documentation. imcheng@ may also > > have > > > naming suggestions. > > > > I think MediaRouteController is fine. The only part that is specific to the > > MediaRouteProvider is the setup of the controller / observer bindings, i.e. if > > we extracted the setup logic to elsewhere then this class won't need to have a > > direct reference to a MediaRouteProvider. What if the initialization of this > > object is done in MediaRouterMojoImpl? > > We can move the initialization to MRMojoImpl, and have MediaRouteController not have to refer to MRP. At that point there is nothing MR-specific about MediaRouteController, so we could even call this MediaController (but I'm hesitant to do so because of confusion with mojom::MediaController). If the responsibility of the class is to implement mojom::MediaController independent of the downstream binding then MediaController (or MediaControllerImpl) is the best name for it IMO. (I haven't looked at the most recent patchset yet - was initialization moved out of this class?) https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:28: // terminated, or the last observer of |this| is removed. On 2017/03/08 at 04:24:02, takumif wrote: > On 2017/03/06 20:06:19, mark a. foltz wrote: > > I still find the lifetime dependency between the controller and its observers > > unintuitive from a design pattern point of view. If I have a media route with > > ephemeral UX for control, do I have to create and destroy the controller each > > time the UX appears/disappears? We might want the controller to persist during > > the entire time the dialog is open (even if the controls are not visible). > > > > See my earlier comments about adding a disposal API to give the browser the > > ability to be explicit about when the controller is no longer needed. > > The current design is that the event page is awake as long as there is a MediaRouteController, so it's probably better to tie its lifetime just to route details view, than any dialog view. Okay, whichever design is settled upon, please update this documentation. My opinion is that adding an observer and adding a reference count should be two separate actions, unless there's some way to make it self-descriptive and obvious that these are special observers that keep the thing they are observing alive.
https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_route_status_observer.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_route_status_observer.h:12: // An interface for observing updates to the status of a Media Route. On 2017/03/08 22:16:12, mark a. foltz wrote: > On 2017/03/08 at 04:24:02, takumif wrote: > > On 2017/03/06 20:06:19, mark a. foltz wrote: > > > How is this interface specific to Media Routes? Can it just be > > > MediaStatusObserver? > > > > No, it's not really specific to media routes, but I didn't want to name it > MediaStatusObserver because it doesn't implement > media_router::mojom::MediaStatusObserver. Also, when I named this class > MediaStatusObserver I had the compiler interpret it as the mojo variation in > some instances that led to weird errors, so I thought it'd be better to make it > less ambiguous. The same for MediaRouteController vs mojom::MediaController. > > Only generated mojo bindings should be in the mojo:: namespace; and I believe > even type-mapped types will be referenced with the proper namespace. Maybe you > can show me the errors you're seeing? There was a case where I was in the media_router namespace and the compiler confused "MediaStatusObserver" with "media_router::mojom::MediaStatusObserver" even though I wasn't in the mojom namespace. Perhaps it was because I was within a class whose superclass was in the mojom namespace? I just thought that this could cause more confusion in the future. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:24: // A controller for a MediaRoute. Forwards commands for controlling the route to On 2017/03/08 22:16:12, mark a. foltz wrote: > On 2017/03/08 at 04:24:03, takumif wrote: > > On 2017/03/06 23:11:41, imcheng wrote: > > > On 2017/03/06 20:06:19, mark a. foltz wrote: > > > > This only controls media routes that are owned out-of-process by a > > > > MediaRouteProvider (the actual controller implementation lives in the > > > provider). > > > > > > > > As far as I can tell, this class applies equally whether the MRP is hosted > in > > > > the event page or in a different process type. Suggested text: > > > > > > > > "A controller for a MediaRoute hosted out-of-process in a Media Route > Provider > > > > (e.g., in the Media Router component extension)." > > > > > > > > Also, I would suggest naming this e.g. ProviderMediaRouteController and > making > > > > the connection to the MRP clearer in the documentation. imcheng@ may also > > > have > > > > naming suggestions. > > > > > > I think MediaRouteController is fine. The only part that is specific to the > > > MediaRouteProvider is the setup of the controller / observer bindings, i.e. > if > > > we extracted the setup logic to elsewhere then this class won't need to have > a > > > direct reference to a MediaRouteProvider. What if the initialization of this > > > object is done in MediaRouterMojoImpl? > > > > We can move the initialization to MRMojoImpl, and have MediaRouteController > not have to refer to MRP. At that point there is nothing MR-specific about > MediaRouteController, so we could even call this MediaController (but I'm > hesitant to do so because of confusion with mojom::MediaController). > > If the responsibility of the class is to implement mojom::MediaController > independent of the downstream binding then MediaController (or > MediaControllerImpl) is the best name for it IMO. (I haven't looked at the most > recent patchset yet - was initialization moved out of this class?) I'm not sure if there is a need for this class to implement mojom::MediaController. Yes I moved initialization out in the next patch. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:28: // terminated, or the last observer of |this| is removed. On 2017/03/08 22:16:12, mark a. foltz wrote: > On 2017/03/08 at 04:24:02, takumif wrote: > > On 2017/03/06 20:06:19, mark a. foltz wrote: > > > I still find the lifetime dependency between the controller and its > observers > > > unintuitive from a design pattern point of view. If I have a media route > with > > > ephemeral UX for control, do I have to create and destroy the controller > each > > > time the UX appears/disappears? We might want the controller to persist > during > > > the entire time the dialog is open (even if the controls are not visible). > > > > > > See my earlier comments about adding a disposal API to give the browser the > > > ability to be explicit about when the controller is no longer needed. > > > > The current design is that the event page is awake as long as there is a > MediaRouteController, so it's probably better to tie its lifetime just to route > details view, than any dialog view. > > Okay, whichever design is settled upon, please update this documentation. > > My opinion is that adding an observer and adding a reference count should be two > separate actions, unless there's some way to make it self-descriptive and > obvious that these are special observers that keep the thing they are observing > alive. > Would it be better if we made the step to call the destroyer callback explicit? We could add some method like MediaRouteController::DestructIfNoObservers() that MRUI can call after removing itself as an observer.
https://codereview.chromium.org/2727123002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:55: } 1) If the associated MediaRoute is terminated, how do we notify the observers to clean up? 2) Have you considered other approaches to make the ownership model more clear? Here's a suggestion: - MediaRouter returns a scoped_refpr<MediaRouteController> to callers, who retains a reference to it as long as it is in use. - MediaRouter itself holds a raw pointer in the internal MediaRoute ID to controller mapping. - When the ref count reaches zero, MediaRouteController will be deleted. At that time, it should be removed from MediaRouter's mapping. - If the associated MediaRoute is terminated while the controller is in use, MediaRouter can send a signal to other observers to drop their references. Let's chat about this with Mark on Monday?
On 2017/03/08 22:02:12, takumif wrote: > On 2017/03/08 20:15:41, imcheng wrote: > > On 2017/03/08 19:33:24, Devlin wrote: > > > > > > https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... > > > File extensions/renderer/dispatcher.cc (right): > > > > > > > > > https://codereview.chromium.org/2727123002/diff/100001/extensions/renderer/di... > > > extensions/renderer/dispatcher.cc:745: > > > {"chrome/browser/media/router/mojo/media_controller.mojom", > > > On 2017/03/08 04:24:03, takumif wrote: > > > > On 2017/03/06 22:51:05, Devlin wrote: > > > > > Where are these used? I can't find them being required anywhere. > > > > > > > > media_router.mojom includes these two files, so the Media Router component > > > > extension throws an error if they aren't included here. > > > > > > Why? These resources are added to the Dispatcher's source map, which is > only > > > used when require()'ing js resources. Unless we somewhere map a mojom's > > #import > > > to require() (which I hope we don't), I don't see the correlation? > > > > I believe you don't need to add those resources until you start requiring them > > in media_router_bindings.js. I would revert those changes for now and add it > > when we add support for the new APIs in the bindings code. > > I believe the mojo import statements are calling require(). I'm getting errors > like this: > (BLESSED_EXTENSION context for pkedcjkdefgpdelpbcmbmeomcjbeemfm) No source for > require(chrome/browser/media/router/mojo/media_controller.mojom) /sigh... that's probably not the right thing for them to do, and we should revisit that. But that's not in the scope of this cl, so extensions dispatcher & resources.grd lgtm. (I don't think I'm needed for anything else; lemme know if you wanted me to look.)
Sorry to hold this up, but I still think the current design is a little over-complicated with APIs that are somewhat duplicative of what is already done for you by Mojo. Removing the wrapper classes and implementing a standard observer pattern would be a lot more obvious - unless there is a value add I am missing in upstream code. I'll try to set aside some time to review the other patches, or maybe a short huddle would clear things up. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_route_status_observer.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_route_status_observer.h:12: // An interface for observing updates to the status of a Media Route. On 2017/03/09 at 19:45:30, takumif wrote: > On 2017/03/08 22:16:12, mark a. foltz wrote: > > On 2017/03/08 at 04:24:02, takumif wrote: > > > On 2017/03/06 20:06:19, mark a. foltz wrote: > > > > How is this interface specific to Media Routes? Can it just be > > > > MediaStatusObserver? > > > > > > No, it's not really specific to media routes, but I didn't want to name it > > MediaStatusObserver because it doesn't implement > > media_router::mojom::MediaStatusObserver. Also, when I named this class > > MediaStatusObserver I had the compiler interpret it as the mojo variation in > > some instances that led to weird errors, so I thought it'd be better to make it > > less ambiguous. The same for MediaRouteController vs mojom::MediaController. > > > > Only generated mojo bindings should be in the mojo:: namespace; and I believe > > even type-mapped types will be referenced with the proper namespace. Maybe you > > can show me the errors you're seeing? > > There was a case where I was in the media_router namespace and the compiler confused "MediaStatusObserver" with "media_router::mojom::MediaStatusObserver" even though I wasn't in the mojom namespace. Perhaps it was because I was within a class whose superclass was in the mojom namespace? I just thought that this could cause more confusion in the future. Maybe you don't need this class at all? Since you are type mapping, it looks like it will be identical to mojom::MediaStatusObserver. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_status.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:17: virtual ~MediaStatus(); On 2017/03/08 at 04:24:02, takumif wrote: > On 2017/03/08 01:08:19, dcheng wrote: > > No virtual. > > Sorry for my lack of C++ knowledge, but would you mind explaining why it'd be okay not to mark the dtor virtual? If we created sub-structs with extra data, shouldn't the dtor be virtual? - By declaring this a struct you're saying it's not going to be subclassed, and I don't see a reason for it in the current design. - Also it's typemapped from Mojo so you're not going to be able to easily create subclasses. - If there are non-POD members, the default ctor will call the destructors of them without virtual. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:28: // terminated, or the last observer of |this| is removed. On 2017/03/09 at 19:45:30, takumif wrote: > On 2017/03/08 22:16:12, mark a. foltz wrote: > > On 2017/03/08 at 04:24:02, takumif wrote: > > > On 2017/03/06 20:06:19, mark a. foltz wrote: > > > > I still find the lifetime dependency between the controller and its > > observers > > > > unintuitive from a design pattern point of view. If I have a media route > > with > > > > ephemeral UX for control, do I have to create and destroy the controller > > each > > > > time the UX appears/disappears? We might want the controller to persist > > during > > > > the entire time the dialog is open (even if the controls are not visible). > > > > > > > > See my earlier comments about adding a disposal API to give the browser the > > > > ability to be explicit about when the controller is no longer needed. > > > > > > The current design is that the event page is awake as long as there is a > > MediaRouteController, so it's probably better to tie its lifetime just to route > > details view, than any dialog view. > > > > Okay, whichever design is settled upon, please update this documentation. > > > > My opinion is that adding an observer and adding a reference count should be two > > separate actions, unless there's some way to make it self-descriptive and > > obvious that these are special observers that keep the thing they are observing > > alive. > > > > Would it be better if we made the step to call the destroyer callback explicit? We could add some method like MediaRouteController::DestructIfNoObservers() that MRUI can call after removing itself as an observer. The API I was thinking of would always destroy the controller and notify remaining observers that the controller had gone away (and don't expect further status updates). https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:344: SetMediaRouteStatusObserver(string route_id, MediaStatusObserver observer); On 2017/03/08 at 04:24:03, takumif wrote: > On 2017/03/08 01:08:20, dcheng wrote: > > Is there a time when we'd want to create a media router without an observer? > > Since this is 1:1 atm, we should just bind it as part > > CreateMediaRouteController() by passing MediaStatusObserver. > > We originally had it that way and there was some discussion about it in the previous CL [1]. We gravitated towards having two separate methods because the MediaController implementation and the provider that MediaStatusObserver observes don't really have to be the same object. > > [1] https://codereview.chromium.org/2674363002/ The context might have been support for multiple observers of the same controller - but it looks like that's left as a TODO. Can you deep link to the comment thread where this was discussed so I can refresh myself? https://codereview.chromium.org/2727123002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:29: class MediaRouteController : public mojom::MediaStatusObserver { What would use this class? I thought that the controllers would be implemented as part of the component MRPs? On the browser side it seems like the UI could interact with the mojom::MediaControllerPtr directly as all this class does is forward calls to it.
Patchset #3 (id:140001) has been deleted
Patchset #4 (id:180001) has been deleted
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #4 (id:200001) has been deleted
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its MediaRouteStatusObserver(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. MediaRouteController implements the MediaStatusObserver mojo interface and receives updates from the extension. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls redesign design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its Observer(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. MediaRouteController implements the MediaStatusObserver mojo interface and receives updates from the extension. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls redesign design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
New changes: - Made MediaRouteController ref-counted - MediaRouteController is now included in Android builds as a result - MediaRouteStatusObserver is now MediaRouteController::Observer - MediaStatus typemapping is in its own files I've also updated CLs #2 and #3 so they can give you an idea of how MediaRouteController is used (those CLs still aren't really ready for review yet). Please take a look, thanks!
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Thanks. I think the ownership model is slightly more clear now. The MediaRouteController class would benefit from having more documentation regarding its usage and lifetime. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:13: : controller_(std::move(controller)) { No need to move, scoped_refptr supports copying. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:63: // |this| is deleted here! This may be true, but only if all references were held by the Observers. Regardless, it is best to assume this object is destroyed at this point. There may be cases in the future where observer isn't immediately added after obtaining a reference to it. In that case, it might be helpful to have a valid bit on the controller. (no change needed for this patch) https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:67: media_controller_.set_connection_error_handler(base::Bind( Can you set this in the ctor? IIRC, you can set the error handler as soon as the IntefacePtr is bound to a message pipe. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:72: return binding_.CreateInterfacePtrAndBind(); You can also set_connection_error_handler on the binding at this point. (I believe MediaController and the MediaStatusObserver are 2 separate message pipes) https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:19: // change in the route's MediaStatus. It is owned by its observers, each of I think it would be nice to document the life cycle of this class and add example usage here. Specific points: - How to construct and/or initialize this class - Invalidation - Destruction when all observers are removed. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:29: explicit Observer(scoped_refptr<MediaRouteController> controller); const scoped_refptr<MediaRouteController>& https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:37: virtual void OnControllerInvalidated(); Why make this virtual? Seems like you should enforce this behavior always. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:49: MediaRouteController(MediaRoute::Id route_id, const ref https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:51: base::Closure destructor_callback); const ref https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:73: mojom::MediaStatusObserverPtr GetObserverPtr(); Suggest naming this BindObserverPtr() or similar so it is more clear. See: https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati... https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:75: const MediaRoute::Id& route_id() const { return route_id_; } Return by value
https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_status.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:17: virtual ~MediaStatus(); On 2017/03/13 18:22:31, mark a. foltz wrote: > On 2017/03/08 at 04:24:02, takumif wrote: > > On 2017/03/08 01:08:19, dcheng wrote: > > > No virtual. > > > > Sorry for my lack of C++ knowledge, but would you mind explaining why it'd be > okay not to mark the dtor virtual? If we created sub-structs with extra data, > shouldn't the dtor be virtual? > > - By declaring this a struct you're saying it's not going to be subclassed, and > I don't see a reason for it in the current design. > - Also it's typemapped from Mojo so you're not going to be able to easily create > subclasses. > - If there are non-POD members, the default ctor will call the destructors of > them without virtual. +1, it doesn't seem like it's subclassed at the moment. Of course, if it changes, then virtual would make sense (modulo the difficulties in typemapping a polymorphic type) https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:60: base::Closure destroyer_callback_; On 2017/03/08 04:24:02, takumif wrote: > On 2017/03/08 01:08:19, dcheng wrote: > > This is pretty unusual. Where's the code that actually uses this interface? > Why > > do we need to indirect this through a callback? > > This is used by MediaRouterMojoImpl which will own this (the code will be in > another CL [1]). MediaRouteController will be stored in a map, and we want to > delete it from the map whenever it has no observers. > > [1] > https://codereview.chromium.org/2728543009/diff/1/chrome/browser/media/router... Unless we need to use a base::Callback for flexibility/layering reasons, I would strongly suggest that this class just talk directly to the MediaRouterMojoImpl. This adds a lot of indirection and makes the code harder to follow. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:13: : controller_(std::move(controller)) { On 2017/03/16 20:00:43, imcheng wrote: > No need to move, scoped_refptr supports copying. This is actually the preferred idiom. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:30: : destructor_callback_(destructor_callback), Pass closures by const ref (you can also pass by value and std::move() if you want, but the convention is to pass by const ref) https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:71: mojom::MediaStatusObserverPtr MediaRouteController::GetObserverPtr() { I would suggest adding this in a followup where it's actually used. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:29: explicit Observer(scoped_refptr<MediaRouteController> controller); On 2017/03/16 20:00:43, imcheng wrote: > const scoped_refptr<MediaRouteController>& Note that it is considered OK to pass by value (and std::move instead) https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:344: SetMediaRouteStatusObserver(string route_id, MediaStatusObserver observer); I suggest moving these to the followup where they're actually implemented.
https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:13: : controller_(std::move(controller)) { On 2017/03/17 06:15:14, dcheng wrote: > On 2017/03/16 20:00:43, imcheng wrote: > > No need to move, scoped_refptr supports copying. > > This is actually the preferred idiom. To clarify, are you saying that pass-by-value and moving are preferred? Is the reason for that because pass-by-value might be able to move instead of copy? (Why is pass-by-ref preferred for closures below?) https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:30: : destructor_callback_(destructor_callback), On 2017/03/17 06:15:14, dcheng wrote: > Pass closures by const ref (you can also pass by value and std::move() if you > want, but the convention is to pass by const ref) Done. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:63: // |this| is deleted here! On 2017/03/16 20:00:43, imcheng wrote: > This may be true, but only if all references were held by the Observers. > Regardless, it is best to assume this object is destroyed at this point. > > There may be cases in the future where observer isn't immediately added after > obtaining a reference to it. In that case, it might be helpful to have a valid > bit on the controller. (no change needed for this patch) Documenting in the comments that all the references must be held by the observers. I guess it's possible to make the controller's ctor private and necessary to be accessed through an observer to really enforce it? https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:67: media_controller_.set_connection_error_handler(base::Bind( On 2017/03/16 20:00:43, imcheng wrote: > Can you set this in the ctor? IIRC, you can set the error handler as soon as the > IntefacePtr is bound to a message pipe. Yes, we can set it in the ctor by enforcing that the controller ptr is already bound. Doing so. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:71: mojom::MediaStatusObserverPtr MediaRouteController::GetObserverPtr() { On 2017/03/17 06:15:14, dcheng wrote: > I would suggest adding this in a followup where it's actually used. Done. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:72: return binding_.CreateInterfacePtrAndBind(); On 2017/03/16 20:00:43, imcheng wrote: > You can also set_connection_error_handler on the binding at this point. (I > believe MediaController and the MediaStatusObserver are 2 separate message > pipes) Moving this to CL #2, but yes I think it'd make sense to destroy MRC when status updates are no longer coming in. We can notify the WebUI to go back to the sink list view when MRC is destroyed. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:19: // change in the route's MediaStatus. It is owned by its observers, each of On 2017/03/16 20:00:43, imcheng wrote: > I think it would be nice to document the life cycle of this class and add > example usage here. Specific points: > > - How to construct and/or initialize this class > - Invalidation > - Destruction when all observers are removed. Adding comments/usage. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:37: virtual void OnControllerInvalidated(); On 2017/03/16 20:00:43, imcheng wrote: > Why make this virtual? Seems like you should enforce this behavior always. Yes, the behavior should be enforced, but I also want to extend this method to notify MRUI that the controller is no longer valid, and the observer should be disposed. Commenting that this should be called by overriding methods. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:49: MediaRouteController(MediaRoute::Id route_id, On 2017/03/16 20:00:43, imcheng wrote: > const ref Done. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:51: base::Closure destructor_callback); On 2017/03/16 20:00:43, imcheng wrote: > const ref Done. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:73: mojom::MediaStatusObserverPtr GetObserverPtr(); On 2017/03/16 20:00:43, imcheng wrote: > Suggest naming this BindObserverPtr() or similar so it is more clear. See: > https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati... Got it. Also moving this method to CL #2. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:75: const MediaRoute::Id& route_id() const { return route_id_; } On 2017/03/16 20:00:43, imcheng wrote: > Return by value Done. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:344: SetMediaRouteStatusObserver(string route_id, MediaStatusObserver observer); On 2017/03/17 06:15:14, dcheng wrote: > I suggest moving these to the followup where they're actually implemented. They are implemented in the Media Router component extension which isn't in Chromium, but I'll move these methods to a later CL that uses them.
Thanks for the updated design. I think this shows the roles of the objects clearly. Most of my feedback is tactical on the API that links observers to the controller and a few minor nit picks on naming and documentation. The patch description should be updated to describe the new ref counting design. It remains not specific to the extension either IMO. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/media_status.h (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:30: // "YouTube". To me, this example reflects the application or the activity, not the media itself. Maybe title could be "media_title" and status "content_title" or "app_title"? I assume there is some mapping from the Cast SDK we need to maintain? Compare MediaMetadata in MediaSession: https://wicg.github.io/mediasession/#mediametadata https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:34: bool can_play_pause = false; can_pause? Not trying to bikeshed here. But would there ever be media that cannot be played, or paused but not played again? https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:70: destructor_callback_.Run(); What does this do? If there is some cleanup that needs to happen outside the MediaController when destroyed, at first glance, I would prefer the caller do it then destroy this vs. passing a callback to be run later. It's also dangerous: if the callback attempts to add a new scoped_refptr to this, it will result in a use-after-free. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:10: #include "base/observer_list.h" #include base/memory/ref_counted.h https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:21: // It is owned by its observers, each of which holds a scoped_refptr. All the ...scoped_refptr to it. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:22: // classes that share the ownership must inherit from the Observer class. A that hold a scoped_refptr must https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:36: // ObserverImpl observer2(observer1.controller()); How about: MRC::Observer* observer; { scoped_refptr<> controller = MediaRouteController::Create(...) // observer adds a scoped_refptr. // ObserverImpl ctor can be private and friend to MRC. observer = controller->AddObserver(); } // controller goes out of scope, observer keeps it alive Hiding the ctor means that the controller is exclusively refounted, and the API for adding the first and subsequent observers is clear and consistent. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:61: virtual void OnControllerInvalidated(); Shouldn't this be called only by the MediaController? If so, make it private and use a friend declaration. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:63: MediaRouteController* controller() { return controller_.get(); } Shouldn't this return controller_ as a scoped_refptr? If you return a raw ptr there's no guarantee of its lifetime. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:78: // Media controller methods for forwarding commands to |media_controller_|. ...forwarding commands to a mojom::MediaControllerPtr held in |media_controller_|. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:37: class MediaRouteControllerUnitTest : public ::testing::Test { Nit: drop "Unit" https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:46: mock_media_controller_ = base::MakeUnique<MockMediaController>(); This can be a class member. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:59: return observer_ ? observer_->controller() : nullptr; I think this will be non-null while the test cases execute. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:105: // TODO(takumif): Use the controller's mojo interface pointer here. Not sure I follow this comment? I didn't see a code path where the controller gets an update from Mojo. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:127: std::unique_ptr<MockMediaRouteControllerObserver> observer1 = IMO, this would be less boilerplate with: std::unique_ptr<> observer1(getController()->AddObserver())
https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:63: // |this| is deleted here! On 2017/03/17 21:48:01, takumif wrote: > On 2017/03/16 20:00:43, imcheng wrote: > > This may be true, but only if all references were held by the Observers. > > Regardless, it is best to assume this object is destroyed at this point. > > > > There may be cases in the future where observer isn't immediately added after > > obtaining a reference to it. In that case, it might be helpful to have a valid > > bit on the controller. (no change needed for this patch) > > Documenting in the comments that all the references must be held by the > observers. I guess it's possible to make the controller's ctor private and > necessary to be accessed through an observer to really enforce it? Well MediaRouteController is created by MediaRouter, but I'm not sure if you can prevent others from AddRef() to it. So either we need to document that adding the Observer immediately after getting obtaining the controller from MediaRouter is the only proper usage of this API or that we need a way to tell if the controller is still valid without using observers. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:37: virtual void OnControllerInvalidated(); On 2017/03/17 21:48:02, takumif wrote: > On 2017/03/16 20:00:43, imcheng wrote: > > Why make this virtual? Seems like you should enforce this behavior always. > > Yes, the behavior should be enforced, but I also want to extend this method to > notify MRUI that the controller is no longer valid, and the observer should be > disposed. Commenting that this should be called by overriding methods. A pattern I've seen is to make this non-virtual, and have it call into a virtual method to perform implementation-specific cleanups. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:31: // // Let ObserverImpl extend MediaRouteController::Observer. If we are calling this MediaRouteController and adding MR specific concepts like MediaRoute::Id, then we might as well be more specific about its relationship with MediaRouter: - you can specify that a MediaRouteController is created and returned by MediaRouter::GetRouteController instead of new'ing it out of nowhere (I realize you moved the API definition to the 2nd patch, but comment is probably ok) - To address dcheng's concern about destructor_callback, you can make MediaRouteController invoke MediaRouterMojoImpl to perform cleanup on destruction. IMO, not a big deal either way. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:42: // observer1.controller()->OnControllerInvalidated(); Hmm... I don't think we actually do this in any scenario? OnControllerInvalidated() should only be called as a response from an outside source, such as Mojo or Media Router. You can mention that OnControllerInvalidated() gets invoked if the route associated with the controller is terminated, or if there is a Mojo connection error. After that, controller() returns nullptr. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:95: void OnControllerBound(); This is not needed anymore.
Thanks for the reviews! https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_status.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:17: virtual ~MediaStatus(); On 2017/03/17 06:15:14, dcheng wrote: > On 2017/03/13 18:22:31, mark a. foltz wrote: > > On 2017/03/08 at 04:24:02, takumif wrote: > > > On 2017/03/08 01:08:19, dcheng wrote: > > > > No virtual. > > > > > > Sorry for my lack of C++ knowledge, but would you mind explaining why it'd > be > > okay not to mark the dtor virtual? If we created sub-structs with extra data, > > shouldn't the dtor be virtual? > > > > - By declaring this a struct you're saying it's not going to be subclassed, > and > > I don't see a reason for it in the current design. > > - Also it's typemapped from Mojo so you're not going to be able to easily > create > > subclasses. > > - If there are non-POD members, the default ctor will call the destructors of > > them without virtual. > > +1, it doesn't seem like it's subclassed at the moment. Of course, if it > changes, then virtual would make sense (modulo the difficulties in typemapping a > polymorphic type) Got it, removing. https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:60: base::Closure destroyer_callback_; On 2017/03/17 06:15:14, dcheng wrote: > On 2017/03/08 04:24:02, takumif wrote: > > On 2017/03/08 01:08:19, dcheng wrote: > > > This is pretty unusual. Where's the code that actually uses this interface? > > Why > > > do we need to indirect this through a callback? > > > > This is used by MediaRouterMojoImpl which will own this (the code will be in > > another CL [1]). MediaRouteController will be stored in a map, and we want to > > delete it from the map whenever it has no observers. > > > > [1] > > > https://codereview.chromium.org/2728543009/diff/1/chrome/browser/media/router... > > Unless we need to use a base::Callback for flexibility/layering reasons, I would > strongly suggest that this class just talk directly to the MediaRouterMojoImpl. > This adds a lot of indirection and makes the code harder to follow. Okay, passing MediaRouterBase* (superclass of MRMojoImpl, since MRMojoImpl is desktop-specific) to the ctor, and will be calling its method directly (in the next CL) instead of a callback. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:63: // |this| is deleted here! On 2017/03/18 00:11:32, imcheng wrote: > On 2017/03/17 21:48:01, takumif wrote: > > On 2017/03/16 20:00:43, imcheng wrote: > > > This may be true, but only if all references were held by the Observers. > > > Regardless, it is best to assume this object is destroyed at this point. > > > > > > There may be cases in the future where observer isn't immediately added > after > > > obtaining a reference to it. In that case, it might be helpful to have a > valid > > > bit on the controller. (no change needed for this patch) > > > > Documenting in the comments that all the references must be held by the > > observers. I guess it's possible to make the controller's ctor private and > > necessary to be accessed through an observer to really enforce it? > > Well MediaRouteController is created by MediaRouter, but I'm not sure if you can > prevent others from AddRef() to it. > > So either we need to document that adding the Observer immediately after getting > obtaining the controller from MediaRouter is the only proper usage of this API > or that we need a way to tell if the controller is still valid without using > observers. I wonder if it is sufficient to document that only the observers should hold on to a scoped_refptr. https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:37: virtual void OnControllerInvalidated(); On 2017/03/18 00:11:32, imcheng wrote: > On 2017/03/17 21:48:02, takumif wrote: > > On 2017/03/16 20:00:43, imcheng wrote: > > > Why make this virtual? Seems like you should enforce this behavior always. > > > > Yes, the behavior should be enforced, but I also want to extend this method to > > notify MRUI that the controller is no longer valid, and the observer should be > > disposed. Commenting that this should be called by overriding methods. > > A pattern I've seen is to make this non-virtual, and have it call into a virtual > method to perform implementation-specific cleanups. Done. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/media_status.h (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:30: // "YouTube". On 2017/03/17 23:55:04, mark a. foltz wrote: > To me, this example reflects the application or the activity, not the media > itself. > > Maybe title could be "media_title" and status "content_title" or "app_title"? I > assume there is some mapping from the Cast SDK we need to maintain? > > Compare MediaMetadata in MediaSession: > > https://wicg.github.io/mediasession/#mediametadata Cast MRP uses "name" and "status". Maybe status can be "secondary_title" or "description"? I feel "content_title" is a bit confusing with "title" or "media_title", and "app_title" wouldn't be applicable to non-Cast-SDK routes/other media. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:34: bool can_play_pause = false; On 2017/03/17 23:55:04, mark a. foltz wrote: > can_pause? > > Not trying to bikeshed here. But would there ever be media that cannot be > played, or paused but not played again? No, I don't think we'd have something that can be paused but not played again. Anton preferred can_play_pause in the previous code review. I don't feel strongly either way. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:70: destructor_callback_.Run(); On 2017/03/17 23:55:04, mark a. foltz wrote: > What does this do? If there is some cleanup that needs to happen outside the > MediaController when destroyed, at first glance, I would prefer the caller do it > then destroy this vs. passing a callback to be run later. > > It's also dangerous: if the callback attempts to add a new scoped_refptr to > this, it will result in a use-after-free. Replacing this line with router_->OnRouteControllerDestroyed(); in the next CL. MRController needs to notify MediaRouter because MediaRouter does not necessarily decide when the MRController is destroyed. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:10: #include "base/observer_list.h" On 2017/03/17 23:55:04, mark a. foltz wrote: > #include base/memory/ref_counted.h Done. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:21: // It is owned by its observers, each of which holds a scoped_refptr. All the On 2017/03/17 23:55:04, mark a. foltz wrote: > ...scoped_refptr to it. Done. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:22: // classes that share the ownership must inherit from the Observer class. A On 2017/03/17 23:55:04, mark a. foltz wrote: > that hold a scoped_refptr must Done. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:31: // // Let ObserverImpl extend MediaRouteController::Observer. On 2017/03/18 00:11:32, imcheng wrote: > If we are calling this MediaRouteController and adding MR specific concepts like > MediaRoute::Id, then we might as well be more specific about its relationship > with MediaRouter: > > - you can specify that a MediaRouteController is created and returned by > MediaRouter::GetRouteController instead of new'ing it out of nowhere (I realize > you moved the API definition to the 2nd patch, but comment is probably ok) Removing the example code, and mentioning that an observer should use MR::GetRouteController() to obtain a scoped_refptr. > - To address dcheng's concern about destructor_callback, you can make > MediaRouteController invoke MediaRouterMojoImpl to perform cleanup on > destruction. IMO, not a big deal either way. As we discussed offline, I'll be passing MediaRouterBase* into the ctor, and will be calling its method in the dtor in the next CL. I couldn't add an unused member to MRController, so I'll be passing MediaRouterBase* in the next CL. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:36: // ObserverImpl observer2(observer1.controller()); On 2017/03/17 23:55:04, mark a. foltz wrote: > How about: > > MRC::Observer* observer; > { > scoped_refptr<> controller = MediaRouteController::Create(...) > > // observer adds a scoped_refptr. > // ObserverImpl ctor can be private and friend to MRC. > observer = controller->AddObserver(); > } // controller goes out of scope, observer keeps it alive > > Hiding the ctor means that the controller is exclusively refounted, and the API > for adding the first and subsequent observers is clear and consistent. I think it's better if MRController didn't know about the observer implementation though. We are going to have an observer class that is specific to MediaRouterUI. ObserverImpl was a fictional implementation for Observer created for this example. Maybe that's confusing? Removing the example usage since it'd be different from the actual code in MRMojoImpl/MRUI. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:42: // observer1.controller()->OnControllerInvalidated(); On 2017/03/18 00:11:32, imcheng wrote: > Hmm... I don't think we actually do this in any scenario? > OnControllerInvalidated() should only be called as a response from an outside > source, such as Mojo or Media Router. > > You can mention that OnControllerInvalidated() gets invoked if the route > associated with the controller is terminated, or if there is a Mojo connection > error. After that, controller() returns nullptr. Right, OnControllerInvalidated() should be called only by Media Router or a Mojo error callback. Updating the comment. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:61: virtual void OnControllerInvalidated(); On 2017/03/17 23:55:04, mark a. foltz wrote: > Shouldn't this be called only by the MediaController? If so, make it private > and use a friend declaration. Done. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:63: MediaRouteController* controller() { return controller_.get(); } On 2017/03/17 23:55:04, mark a. foltz wrote: > Shouldn't this return controller_ as a scoped_refptr? If you return a raw ptr > there's no guarantee of its lifetime. The point of this getter is for MediaRouterUI to be able to call the controller's methods. MediaRouterUI will not store the pointer, and will be calling this getter on the observer it owns every time it needs the pointer. Adding a comment that the pointer should not be stored. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:78: // Media controller methods for forwarding commands to |media_controller_|. On 2017/03/17 23:55:04, mark a. foltz wrote: > ...forwarding commands to a mojom::MediaControllerPtr held in > |media_controller_|. Done. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:95: void OnControllerBound(); On 2017/03/18 00:11:32, imcheng wrote: > This is not needed anymore. Removed. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:37: class MediaRouteControllerUnitTest : public ::testing::Test { On 2017/03/17 23:55:04, mark a. foltz wrote: > Nit: drop "Unit" Done. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:46: mock_media_controller_ = base::MakeUnique<MockMediaController>(); On 2017/03/17 23:55:04, mark a. foltz wrote: > This can be a class member. Done. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:59: return observer_ ? observer_->controller() : nullptr; On 2017/03/17 23:55:05, mark a. foltz wrote: > I think this will be non-null while the test cases execute. In DestroyControllerOnNoObservers we reset |observer_| so that the controller has one fewer observer. I'm moving that test case to the next CL though. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:105: // TODO(takumif): Use the controller's mojo interface pointer here. On 2017/03/17 23:55:04, mark a. foltz wrote: > Not sure I follow this comment? I didn't see a code path where the controller > gets an update from Mojo. As seen in the patch before this one, the controller will be receiving status updates through a MediaStatusObserverPtr bound to it. The method to obtain an observer pointer was moved to the next CL. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:127: std::unique_ptr<MockMediaRouteControllerObserver> observer1 = On 2017/03/17 23:55:05, mark a. foltz wrote: > IMO, this would be less boilerplate with: > > std::unique_ptr<> observer1(getController()->AddObserver()) Is this tied to your other comment that the Observer's ctor should be private and accessible by MRController? I don't think MRController should know about specific implementations of Observer. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:112: I'll be adding the two tests that I removed here in the next CL, since we're no longer doing destructor_callback.
It was hard to follow which of my comments had been addressed (or not addressed) in the current patch. Did you reply to individual comments or just compose a single reply? https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:13: : controller_(std::move(controller)) { Why did you choose move semantics here? What if the caller wants to create multiple observers from the same controller? If this is necessary, please document it in the .h. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:27: void MediaRouteController::Observer::OnControllerDisposed() {} Maybe this should be OnControllerInvalidated() as it's a hook that subclasses use for cleanup (acting like a callback). This makes the naming consistent for this code path. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:63: void MediaRouteController::OnControllerInvalidated() { "On*" names I usually reserve for methods that must be bound or otherwise act as callbacks. If that's not the case, suggest "Invalidate()" here. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:65: observer.OnControllerInvalidated(); observer.Invalidate() https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:23: // the classes that host a scoped_refptr must inherit from the Observer class. Did you mean hold instead of host? https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:47: // The pointer returned by this getter should not be stored, as it can be Returns a reference to the observed MediaRouteController. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:49: MediaRouteController* controller() { return controller_.get(); } Adding a comment here is not sufficient. There's no reason to return a raw pointer, as constructing a copy of a scoped pointer is cheap and makes the object lifetime simpler to reason about. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:55: // overriding it. Didn't follow the second sentence - this method isn't virtual so it can't be overridden. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:59: // is disposed. Overridden by subclasses to do custom cleanup. Perhaps note that this is called immediately before |controller_| is destroyed, so that it should not retain any references to it. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:67: // |this| will forward media commands to |media_controller|. Constructs a MediaRouteController that forwards... https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:68: // |media_controller| must be bound to a message pipe. |destructor_callback| Remove |destructor_callback| comment https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:103: // The out-of-process controller that |this| forwards commands to. Technically, mojo services can be bound in-process. Maybe "Handle to the mojom::MediaController that receives media commands." https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:55: return observer_ ? observer_->controller() : nullptr; Still not fixed - observer_ is always set in SetUp(). https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:99: // TODO(takumif): Use a mojom::MediaStatusObserverPtr bound to the controller. Still don't quite follow this, but as long as it makes sense to you and is fixed in a followup patch :)
Found the rest of your replies hiding in plain sight :-P https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/media_status.h (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:30: // "YouTube". On 2017/03/20 at 22:10:45, takumif wrote: > On 2017/03/17 23:55:04, mark a. foltz wrote: > > To me, this example reflects the application or the activity, not the media > > itself. > > > > Maybe title could be "media_title" and status "content_title" or "app_title"? I > > assume there is some mapping from the Cast SDK we need to maintain? > > > > Compare MediaMetadata in MediaSession: > > > > https://wicg.github.io/mediasession/#mediametadata > > Cast MRP uses "name" and "status". Maybe status can be "secondary_title" or "description"? I feel "content_title" is a bit confusing with "title" or "media_title", and "app_title" wouldn't be applicable to non-Cast-SDK routes/other media. I like "description" as this seems to be describing a media activity, not a specific piece of content that's playing. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:34: bool can_play_pause = false; On 2017/03/20 at 22:10:45, takumif wrote: > On 2017/03/17 23:55:04, mark a. foltz wrote: > > can_pause? > > > > Not trying to bikeshed here. But would there ever be media that cannot be > > played, or paused but not played again? > > No, I don't think we'd have something that can be paused but not played again. Anton preferred can_play_pause in the previous code review. I don't feel strongly either way. OK, never mind. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:70: destructor_callback_.Run(); On 2017/03/20 at 22:10:45, takumif wrote: > On 2017/03/17 23:55:04, mark a. foltz wrote: > > What does this do? If there is some cleanup that needs to happen outside the > > MediaController when destroyed, at first glance, I would prefer the caller do it > > then destroy this vs. passing a callback to be run later. > > > > It's also dangerous: if the callback attempts to add a new scoped_refptr to > > this, it will result in a use-after-free. > > Replacing this line with > router_->OnRouteControllerDestroyed(); > in the next CL. MRController needs to notify MediaRouter because MediaRouter does not necessarily decide when the MRController is destroyed. The design based on notifying observers before the dtor is invoked is better. Thanks. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:36: // ObserverImpl observer2(observer1.controller()); On 2017/03/20 at 22:10:45, takumif wrote: > On 2017/03/17 23:55:04, mark a. foltz wrote: > > How about: > > > > MRC::Observer* observer; > > { > > scoped_refptr<> controller = MediaRouteController::Create(...) > > > > // observer adds a scoped_refptr. > > // ObserverImpl ctor can be private and friend to MRC. > > observer = controller->AddObserver(); > > } // controller goes out of scope, observer keeps it alive > > > > Hiding the ctor means that the controller is exclusively refounted, and the API > > for adding the first and subsequent observers is clear and consistent. > > I think it's better if MRController didn't know about the observer implementation though. We are going to have an observer class that is specific to MediaRouterUI. > > ObserverImpl was a fictional implementation for Observer created for this example. Maybe that's confusing? Removing the example usage since it'd be different from the actual code in MRMojoImpl/MRUI. Ok, if the Observer is implemented separately from the controller, this API seems fine. https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:127: std::unique_ptr<MockMediaRouteControllerObserver> observer1 = On 2017/03/20 at 22:10:45, takumif wrote: > On 2017/03/17 23:55:05, mark a. foltz wrote: > > IMO, this would be less boilerplate with: > > > > std::unique_ptr<> observer1(getController()->AddObserver()) > > Is this tied to your other comment that the Observer's ctor should be private and accessible by MRController? I don't think MRController should know about specific implementations of Observer. Yeah, if we are constructing the Observers separately then does not seem avoidable. Consider factoring out a utility method to shorten this: auto observer1 = CreateObserver();
https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:13: : controller_(std::move(controller)) { On 2017/03/20 23:47:03, mark a. foltz wrote: > Why did you choose move semantics here? > What if the caller wants to create multiple observers from the same controller? > > If this is necessary, please document it in the .h. My understanding is that passing-by-value then moving is similar to passing-by-const-ref then copying, in that the original instance owned by the caller is retained (when necessary). I think Daniel's comment on media_route_controller.h in patch #5 confirms this, and in media_route_controller.cc in the same patch he mentioned that this is preferable. Please let me know if you think it is confusing and I should use the latter though. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:27: void MediaRouteController::Observer::OnControllerDisposed() {} On 2017/03/20 23:47:03, mark a. foltz wrote: > Maybe this should be OnControllerInvalidated() as it's a hook that subclasses > use for cleanup (acting like a callback). This makes the naming consistent for > this code path. Done. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:63: void MediaRouteController::OnControllerInvalidated() { On 2017/03/20 23:47:03, mark a. foltz wrote: > "On*" names I usually reserve for methods that must be bound or otherwise act as > callbacks. > If that's not the case, suggest "Invalidate()" here. Changing to Invalidate(). https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:65: observer.OnControllerInvalidated(); On 2017/03/20 23:47:03, mark a. foltz wrote: > observer.Invalidate() Changing to InvaildateController(). https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:23: // the classes that host a scoped_refptr must inherit from the Observer class. On 2017/03/20 23:47:03, mark a. foltz wrote: > Did you mean hold instead of host? Yes, changed. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:47: // The pointer returned by this getter should not be stored, as it can be On 2017/03/20 23:47:03, mark a. foltz wrote: > Returns a reference to the observed MediaRouteController. Done. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:49: MediaRouteController* controller() { return controller_.get(); } On 2017/03/20 23:47:03, mark a. foltz wrote: > Adding a comment here is not sufficient. There's no reason to return a raw > pointer, as constructing a copy of a scoped pointer is cheap and makes the > object lifetime simpler to reason about. Done. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:55: // overriding it. On 2017/03/20 23:47:03, mark a. foltz wrote: > Didn't follow the second sentence - this method isn't virtual so it can't be > overridden. Sorry, it's out of date. Removed. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:59: // is disposed. Overridden by subclasses to do custom cleanup. On 2017/03/20 23:47:03, mark a. foltz wrote: > Perhaps note that this is called immediately before |controller_| is destroyed, > so that it should not retain any references to it. This is called after |controller_| is reset, so there shouldn't be any references left anyways, unless I'm misunderstanding what you're saying. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:67: // |this| will forward media commands to |media_controller|. On 2017/03/20 23:47:03, mark a. foltz wrote: > Constructs a MediaRouteController that forwards... Done. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:68: // |media_controller| must be bound to a message pipe. |destructor_callback| On 2017/03/20 23:47:03, mark a. foltz wrote: > Remove |destructor_callback| comment Done. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:103: // The out-of-process controller that |this| forwards commands to. On 2017/03/20 23:47:03, mark a. foltz wrote: > Technically, mojo services can be bound in-process. Maybe "Handle to the > mojom::MediaController that receives media commands." Done. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:55: return observer_ ? observer_->controller() : nullptr; On 2017/03/20 23:47:03, mark a. foltz wrote: > Still not fixed - observer_ is always set in SetUp(). I had this because I was resetting observer_ in one of the tests, but removing since I removed that test for now. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:99: // TODO(takumif): Use a mojom::MediaStatusObserverPtr bound to the controller. On 2017/03/20 23:47:03, mark a. foltz wrote: > Still don't quite follow this, but as long as it makes sense to you and is fixed > in a followup patch :) Here, instead of calling OnMediaStatusUpdated() on the controller directly, I want to call the method on an interface pointer bound to the controller, like the extension-side controller would.
lgtm https://codereview.chromium.org/2727123002/diff/340001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:23: controller_ = nullptr; Should we call RemoveObserver here to be safe? https://codereview.chromium.org/2727123002/diff/340001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:51: new MediaRouteController("routeId", std::move(media_controller_ptr))); nit: I find this test slightly easier to reason about if: - a scoped_refptr<MediaRouteController> is stored as a member in this test. - GetController() returns that member - observer_ is also created using CreateObserver(), if you think StrictMock also applies to tests that use it. https://codereview.chromium.org/2727123002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:102: // TODO(takumif): Use a mojom::MediaStatusObserverPtr bound to the controller. Do you plan to do this in the next patch? I see you removed the method to expose the observer mojo ptr in MediaRouteController.
https://codereview.chromium.org/2727123002/diff/340001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:23: controller_ = nullptr; On 2017/03/21 21:19:58, imcheng wrote: > Should we call RemoveObserver here to be safe? RemoveObserver() here will be necessary if the controller doesn't get deleted after InvalidateController() calls for some reason. ObserverList allows removal of observers during iteration, so I'll add RemoveObserver() here. We also don't have to worry about InvalidateController() getting called twice, since only MediaRouteController can call it. https://codereview.chromium.org/2727123002/diff/340001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:51: new MediaRouteController("routeId", std::move(media_controller_ptr))); On 2017/03/21 21:19:58, imcheng wrote: > nit: I find this test slightly easier to reason about if: > - a scoped_refptr<MediaRouteController> is stored as a member in this test. > - GetController() returns that member > - observer_ is also created using CreateObserver(), if you think StrictMock also > applies to tests that use it. Hmm I feel it's a bit nicer if we followed our rule that only observers can have scoped_refptrs, especially when we add tests for the invalidation of the controller. https://codereview.chromium.org/2727123002/diff/340001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:102: // TODO(takumif): Use a mojom::MediaStatusObserverPtr bound to the controller. On 2017/03/21 21:19:58, imcheng wrote: > Do you plan to do this in the next patch? I see you removed the method to expose > the observer mojo ptr in MediaRouteController. Yes. MRController::BindObserverPtr() will be in CL #2, and I will resolve these TODOs then.
LGTM Thanks for patience with multiple rounds of review. This shaped up nicely. https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2727123002/diff/300001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:13: : controller_(std::move(controller)) { On 2017/03/21 at 04:00:07, takumif wrote: > On 2017/03/20 23:47:03, mark a. foltz wrote: > > Why did you choose move semantics here? > > What if the caller wants to create multiple observers from the same controller? > > > > If this is necessary, please document it in the .h. > > My understanding is that passing-by-value then moving is similar to passing-by-const-ref then copying, in that the original instance owned by the caller is retained (when necessary). I think Daniel's comment on media_route_controller.h in patch #5 confirms this, and in media_route_controller.cc in the same patch he mentioned that this is preferable. Please let me know if you think it is confusing and I should use the latter though. Sorry, I missed that this was an argument passed by copy so std::move makes sense here. My bad. https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:48: // should not be stored by an object of a class that does not inherit from maybe "stored by any object that does not subclass ::Observer." https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:95: auto observer2 = CreateObserver(); Much nicer :-)
mojo lgtm https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:48: // should not be stored by an object of a class that does not inherit from On 2017/03/22 00:41:43, mark a. foltz wrote: > maybe "stored by any object that does not subclass ::Observer." One way to enforce this is to put this in the protected section of the class. (Maybe this doesn't work for other reasons though?) https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_status.mojom (right): https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_status.mojom:53: mojo.common.mojom.TimeDelta current_time; FWIW, I'm uncertain of the granularity of these updates, but for things like |current_time|, it seems like they might change pretty frequently. Perhaps it's worth having a separate observer method for it rather than bundling it in the MediaStatus struct. (this is just a thought; I'm not asking for any concrete changes in this CL)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/media_status.h (right): https://codereview.chromium.org/2727123002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_status.h:30: // "YouTube". On 2017/03/21 00:05:21, mark a. foltz wrote: > On 2017/03/20 at 22:10:45, takumif wrote: > > On 2017/03/17 23:55:04, mark a. foltz wrote: > > > To me, this example reflects the application or the activity, not the media > > > itself. > > > > > > Maybe title could be "media_title" and status "content_title" or > "app_title"? I > > > assume there is some mapping from the Cast SDK we need to maintain? > > > > > > Compare MediaMetadata in MediaSession: > > > > > > https://wicg.github.io/mediasession/#mediametadata > > > > Cast MRP uses "name" and "status". Maybe status can be "secondary_title" or > "description"? I feel "content_title" is a bit confusing with "title" or > "media_title", and "app_title" wouldn't be applicable to non-Cast-SDK > routes/other media. > > I like "description" as this seems to be describing a media activity, not a > specific piece of content that's playing. Changing to "description". https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:48: // should not be stored by an object of a class that does not inherit from On 2017/03/22 00:41:43, mark a. foltz wrote: > maybe "stored by any object that does not subclass ::Observer." Done. https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:48: // should not be stored by an object of a class that does not inherit from On 2017/03/22 01:17:10, dcheng wrote: > On 2017/03/22 00:41:43, mark a. foltz wrote: > > maybe "stored by any object that does not subclass ::Observer." > > One way to enforce this is to put this in the protected section of the class. > > (Maybe this doesn't work for other reasons though?) The controller will be used by classes that don't inherit from Observer, so it does need to be public. https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_status.mojom (right): https://codereview.chromium.org/2727123002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_status.mojom:53: mojo.common.mojom.TimeDelta current_time; On 2017/03/22 01:17:10, dcheng wrote: > FWIW, I'm uncertain of the granularity of these updates, but for things like > |current_time|, it seems like they might change pretty frequently. Perhaps it's > worth having a separate observer method for it rather than bundling it in the > MediaStatus struct. > > (this is just a thought; I'm not asking for any concrete changes in this CL) The updates will be once per second, so for now I'm thinking that it'd be okay to be bundled.
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its Observer(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. MediaRouteController implements the MediaStatusObserver mojo interface and receives updates from the extension. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls redesign design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its Observer(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. MediaRouteController implements the MediaStatusObserver mojo interface and receives updates from the extension. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls redesign design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, imcheng@chromium.org, mfoltz@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2727123002/#ps440001 (title: "Change comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1490223917727010,
"parent_rev": "5a7f45dab8372cb7b80ee24dfb137a7ced2049ea", "commit_rev":
"67a8f64435d0a1c3ae9d04e33f965f885b9cb8b7"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its Observer(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. MediaRouteController implements the MediaStatusObserver mojo interface and receives updates from the extension. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls redesign design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 ========== to ========== [Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces This CL adds MediaRouteController that will forward media controller commands from the custom controls WebUI to the Media Router component extension, and will receive MediaStatus updates from the extension which it then will forward to its Observer(s) (= to the WebUI). This patch includes mojo interfaces/structs that were first reviewed in this patch [1]. The MediaController mojo interface will be implemented in the component extension (not a part of Chromium), and will receive commands from MediaRouteController. MediaRouteController implements the MediaStatusObserver mojo interface and receives updates from the extension. Typemapping between media_router::mojom::MediaStatus and media_router::MediaStatus is also included in this CL. Changes in extensions/renderer/ are for exposing the new mojo interfaces to the component extension. The Chromium-side implementation of custom controls redesign will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: this patch 2. MediaRouter::GetRouteController(): http://crrev/2728543009 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls redesign design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... [1] https://codereview.chromium.org/2674363002/ BUG=684636,684635 Review-Url: https://codereview.chromium.org/2727123002 Cr-Commit-Position: refs/heads/master@{#458922} Committed: https://chromium.googlesource.com/chromium/src/+/67a8f64435d0a1c3ae9d04e33f96... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:440001) as https://chromium.googlesource.com/chromium/src/+/67a8f64435d0a1c3ae9d04e33f96...
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2727123002/diff/440001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2727123002/diff/440001/extensions/renderer/di... extensions/renderer/dispatcher.cc:754: {"chrome/browser/media/router/mojo/media_controller.mojom", This adds many more dependencies from extensions/ to chrome/. Since chrome already depends, that's an (implicit) dependency cycle, and this does not lgtm. Please revert this and come up with a way to handle this without this dependency cycle.
Message was sent while issue was closed.
https://codereview.chromium.org/2727123002/diff/440001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2727123002/diff/440001/extensions/renderer/di... extensions/renderer/dispatcher.cc:754: {"chrome/browser/media/router/mojo/media_controller.mojom", On 2017/03/23 18:36:49, Nico wrote: > This adds many more dependencies from extensions/ to chrome/. Since chrome > already depends, that's an (implicit) dependency cycle, and this does not lgtm. > Please revert this and come up with a way to handle this without this dependency > cycle. These dependencies need to exist for the Media Router extension to work properly. If the problem is simply where these mojom files are located in, then we can move them somewhere else to break the cycle. In https://codereview.chromium.org/1162243002/ I suggested //components/media_router., but I think //media/router (or //media/mojo/router) should work too. Please let me know if that's not the case. I don't think this warrants a revert. The implicit cycle has existed for quite some time now and this patch doesn't break anything. I do think it's a good idea to remove this cycle as soon as we figure out where to relocate the mojom files.
Message was sent while issue was closed.
Created crbug.com/704958 to track moving of mojom files.
Message was sent while issue was closed.
On 2017/03/23 19:03:16, imcheng wrote: > https://codereview.chromium.org/2727123002/diff/440001/extensions/renderer/di... > File extensions/renderer/dispatcher.cc (right): > > https://codereview.chromium.org/2727123002/diff/440001/extensions/renderer/di... > extensions/renderer/dispatcher.cc:754: > {"chrome/browser/media/router/mojo/media_controller.mojom", > On 2017/03/23 18:36:49, Nico wrote: > > This adds many more dependencies from extensions/ to chrome/. Since chrome > > already depends, that's an (implicit) dependency cycle, and this does not > lgtm. > > Please revert this and come up with a way to handle this without this > dependency > > cycle. > > These dependencies need to exist for the Media Router extension to work > properly. If the problem is simply where these mojom files are located in, then > we can move them somewhere else to break the cycle. In > https://codereview.chromium.org/1162243002/ I suggested > //components/media_router., but I think //media/router (or //media/mojo/router) > should work too. Please let me know if that's not the case. > > I don't think this warrants a revert. The implicit cycle has existed for quite > some time now and this patch doesn't break anything. I do think it's a good idea > to remove this cycle as soon as we figure out where to relocate the mojom files. Existing GN dependencies are one thing, but now we can't build extensions targets without building chrome. Is that not new as of this CL? See crbug.com/706786. |
