TabSwitcherUI Functionality to mirroring on Cast Moderator, Second CL [chromium/src : main]
Ahmed Moussa (Gerrit)
Attention is currently required from: Takumi Fujimoto.
Ahmed Moussa would like Takumi Fujimoto to review this change.
TabSwitcherUI Functionality to mirroring on Cast Moderator, Second CL
Allow, while mirroring/remoting tab on cast moderator, the ability to
switch the mirrored tab and to stop casting through a Tab Switcher UI.
This feature is available behind a flag parameter named `TabSwitchingUIForAccessCodeCast`. You can try it out by `--enable-features=TabSwitchingUIForAccessCodeCast`. It's disabled by default for now.
Second CL → mainly focus on updating data members, which keep track of
source tabs connected to different routes/sessions, with the new source tab that we switched to.
Third CL → will mainly focus on using the right text wording
for the Tab Switcher UI bar e.g. use `Casting` instead of `Sharing`.
Fourth and Final CL → will add metrics needed and remove the flag.
Bug: b/242618705
Change-Id: Ib3e9e4fe9875a027e9684d30c310f1ea53b394ba
---
M chrome/browser/media/cast_mirroring_performance_browsertest.cc
M chrome/browser/media/cast_mirroring_service_host.cc
M chrome/browser/media/cast_mirroring_service_host_browsertest.cc
M chrome/browser/media/router/providers/cast/cast_activity_manager.cc
M chrome/browser/media/router/providers/cast/cast_activity_manager.h
M chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc
M chrome/browser/media/router/providers/cast/cast_session_tracker.cc
M chrome/browser/media/router/providers/cast/cast_session_tracker.h
M chrome/browser/media/router/providers/cast/cast_session_tracker_unittest.cc
M chrome/browser/media/router/providers/cast/mirroring_activity.cc
M chrome/browser/media/router/providers/cast/mirroring_activity.h
M chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc
M components/mirroring/mojom/mirroring_service.mojom
M components/mirroring/mojom/session_observer.mojom
M components/mirroring/service/mirroring_service.cc
M components/mirroring/service/mirroring_service.h
M components/mirroring/service/openscreen_session_host.cc
M components/mirroring/service/openscreen_session_host.h
M components/mirroring/service/openscreen_session_host_unittest.cc
M components/mirroring/service/session.cc
M components/mirroring/service/session.h
M components/mirroring/service/session_unittest.cc
22 files changed, 243 insertions(+), 8 deletions(-)
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Takumi Fujimoto.
Takumi Fujimoto (Gerrit)
Attention is currently required from: Ahmed Moussa.
12 comments:
File chrome/browser/media/router/providers/cast/cast_activity_manager.h:
Patch Set #7, Line 329:
bool disable_auto_converting_to_flinging_ = false;Nit: Maybe `auto_conversion_to_flinging_enabled_ = true`
When a bool named "disable" is false it becomes a double negation, which is a bit harder to read than "enable = true"
File chrome/browser/media/router/providers/cast/cast_activity_manager.cc:
Patch Set #7, Line 648:
TerminateSession(route_it->second, base::DoNothing());Might be good to comment on why we're terminating the existing session.
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #7, Line 484:
void UpdateRouteSourceTabInRoutesMap() {Can you put 4 and 1 in variables named something like old_frame_id and new_frame_id so that the test is easier to read?
Patch Set #7, Line 488:
manager_->activities_.end());Optional nit: ASSERT_TRUE(base::ContainsKey(manager_->activities_, route_id));
Similar on below linesPatch Set #7, Line 504:
ASSERT_EQ(manager_->routes_by_frame_.find(5),Nit: `ASSERT_EQ(4, manager_->routes_by_frame_.size());`
File chrome/browser/media/router/providers/cast/cast_session_tracker.cc:
Patch Set #7, Line 60:
for (auto& observer : observers_)Nit: braces `{}` around a multi-line if body
File chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc:
Patch Set #7, Line 58:
MOCK_METHOD2(OnSessionAddedOrUpdated,Nit: When adding new mocks please use MOCK_METHOD() rather than the old MOCK_METHOD0() etc. Would appreciate it if you can convert the mocks below as well!
http://google.github.io/googletest/gmock_cook_book.htmlPatch Set #7, Line 469:
activity_->UpdateRouteSourceTab(-1);Is -1 some kind of a special number? It's often used to mean "invalid". If so please comment or consider using a different number
File components/mirroring/mojom/session_observer.mojom:
Patch Set #7, Line 48:
UpdateRouteSourceTab(int32 new_frame_tree_node_id);Nit: Maybe OnSourceTabChanged(). "Route" is not a term used in the mirroring service.
Patch Set #7, Line 48:
new_frame_tree_node_idNit: May be just `frame_tree_node_id`. I think it's more common not to put "new_" when changing to a new value. The old one can be "old_" if needed
File components/mirroring/service/openscreen_session_host_unittest.cc:
Patch Set #7, Line 459:
EXPECT_CALL(*this, UpdateRouteSourceTab(-1)).Times(1);What does it mean for this to be called with -1? Could you add a comment?
File components/mirroring/service/session_unittest.cc:
Patch Set #7, Line 410:
EXPECT_CALL(*this, UpdateRouteSourceTab(-1)).Times(1);What does it mean for this to be called with -1? Could you add a comment?
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Ahmed Moussa.
Ahmed Moussa uploaded patch set #10 to this change.
TabSwitcherUI Functionality to mirroring on Cast Moderator, Second CL
Allow, while mirroring/remoting tab on cast moderator, the ability to
switch the mirrored tab and to stop casting through a Tab Switcher UI.
This feature is available behind a flag parameter named `AccessCodeCastTabSwitchingUI`. You can try it out by `--enable-features=AccessCodeCastTabSwitchingUI`. It's disabled by default for now.
Second CL → mainly focus on updating data members, which keep track of
source tabs connected to different routes/sessions, with the new source tab that we switched to.
Third CL → will mainly focus on using the right text wording
for the Tab Switcher UI bar e.g. use `Casting` instead of `Sharing`.
Fourth and Final CL → will add metrics needed and remove the flag.
Bug: b/242618705
Change-Id: Ib3e9e4fe9875a027e9684d30c310f1ea53b394ba
---
M chrome/browser/media/cast_mirroring_performance_browsertest.cc
M chrome/browser/media/cast_mirroring_service_host_browsertest.cc
M chrome/browser/media/router/providers/cast/cast_activity_manager.cc
M chrome/browser/media/router/providers/cast/cast_activity_manager.h
M chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc
M chrome/browser/media/router/providers/cast/cast_session_tracker.cc
M chrome/browser/media/router/providers/cast/cast_session_tracker.h
M chrome/browser/media/router/providers/cast/cast_session_tracker_unittest.cc
M chrome/browser/media/router/providers/cast/mirroring_activity.cc
M chrome/browser/media/router/providers/cast/mirroring_activity.h
M chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc
M components/mirroring/mojom/session_observer.mojom
M components/mirroring/service/openscreen_session_host.cc
M components/mirroring/service/openscreen_session_host_unittest.cc
M components/mirroring/service/session.cc
M components/mirroring/service/session_unittest.cc
16 files changed, 220 insertions(+), 54 deletions(-)
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Takumi Fujimoto.
Patch set 10:Commit-Queue +1
12 comments:
File chrome/browser/media/router/providers/cast/cast_activity_manager.h:
Patch Set #7, Line 329:
bool disable_auto_converting_to_flinging_ = false;
Nit: Maybe `auto_conversion_to_flinging_enabled_ = true` […]
Done
File chrome/browser/media/router/providers/cast/cast_activity_manager.cc:
Patch Set #7, Line 648:
TerminateSession(route_it->second, base::DoNothing());Might be good to comment on why we're terminating the existing session.
Done
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #7, Line 484:
void UpdateRouteSourceTabInRoutesMap() {
Can you put 4 and 1 in variables named something like old_frame_id and new_frame_id so that the test […]
Done
Patch Set #7, Line 488:
manager_->activities_.end());
Optional nit: ASSERT_TRUE(base::ContainsKey(manager_->activities_, route_id)); […]
Done
Patch Set #7, Line 504:
ASSERT_EQ(manager_->routes_by_frame_.find(5),
Nit: `ASSERT_EQ(4, manager_->routes_by_frame_. […]
Done
File chrome/browser/media/router/providers/cast/cast_session_tracker.cc:
Patch Set #7, Line 60:
for (auto& observer : observers_)Nit: braces `{}` around a multi-line if body
Done
File chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc:
Patch Set #7, Line 58:
MOCK_METHOD2(OnSessionAddedOrUpdated,
Nit: When adding new mocks please use MOCK_METHOD() rather than the old MOCK_METHOD0() etc. […]
Done
Patch Set #7, Line 469:
activity_->UpdateRouteSourceTab(-1);
Is -1 some kind of a special number? It's often used to mean "invalid". […]
Done
File components/mirroring/mojom/session_observer.mojom:
Patch Set #7, Line 48:
new_frame_tree_node_idNit: May be just `frame_tree_node_id`. […]
Done
Patch Set #7, Line 48:
UpdateRouteSourceTab(int32 new_frame_tree_node_id);Nit: Maybe OnSourceTabChanged(). "Route" is not a term used in the mirroring service.
Done
File components/mirroring/service/openscreen_session_host_unittest.cc:
Patch Set #7, Line 459:
EXPECT_CALL(*this, UpdateRouteSourceTab(-1)).Times(1);What does it mean for this to be called with -1? Could you add a comment?
Done
File components/mirroring/service/session_unittest.cc:
Patch Set #7, Line 410:
EXPECT_CALL(*this, UpdateRouteSourceTab(-1)).Times(1);What does it mean for this to be called with -1? Could you add a comment?
Done
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Takumi Fujimoto (Gerrit)
Attention is currently required from: Ahmed Moussa.
Patch set 10:Code-Review +1
7 comments:
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #10, Line 487:
std::size_t(2)Optional nit: Can you simply do 2u?
File chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc:
Patch Set #10, Line 461:
TEST_F(MirroringActivityTest, OnSourceTabChanged) {Comments on session_unittest.cc apply here as well
File components/mirroring/service/openscreen_session_host_unittest.cc:
Patch Set #10, Line 154:
// mojom::SessionObserver implementation.Same comments for this file as session_unittest.cc
File components/mirroring/service/session_unittest.cc:
Patch Set #10, Line 131:
// Called when sends an outbound message.Nit: It seems to me that this comment isn't adding much value and can be removed
Patch Set #10, Line 400:
frame_tree_node_idNit: It's not obvious to me what this variable name is referring to. Please either omit it or indicate what method parameter it's referring to.
Patch Set #10, Line 400:
// A random int indicating the new tab source (frame_tree_node_id)Nit: Please end each comment with a period.
Patch Set #10, Line 411:
EXPECT_CALL(*this, OnSourceTabChanged(new_tab_source)).Times(1);Nit: As Jordan mentioned in another CL, Times(1) can be omitted:
https://chromium.googlesource.com/external/github.com/google/googletest/+/refs/heads/77A9B20B4C1E02FAC90D1D942E1D4C18/googlemock/docs/cheat_sheet.md#expectcall
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Takumi Fujimoto.
7 comments:
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #10, Line 487:
std::size_t(2)Optional nit: Can you simply do 2u?
Done
File chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc:
Patch Set #10, Line 461:
TEST_F(MirroringActivityTest, OnSourceTabChanged) {
Comments on session_unittest. […]
Done
File components/mirroring/service/openscreen_session_host_unittest.cc:
Patch Set #10, Line 154:
// mojom::SessionObserver implementation.
Same comments for this file as session_unittest. […]
Done
File components/mirroring/service/session_unittest.cc:
Patch Set #10, Line 131:
// Called when sends an outbound message.Nit: It seems to me that this comment isn't adding much value and can be removed
Done
Patch Set #10, Line 400:
// A random int indicating the new tab source (frame_tree_node_id)Nit: Please end each comment with a period.
Done
Patch Set #10, Line 400:
frame_tree_node_id
Nit: It's not obvious to me what this variable name is referring to. […]
Done
Patch Set #10, Line 411:
EXPECT_CALL(*this, OnSourceTabChanged(new_tab_source)).Times(1);
Nit: As Jordan mentioned in another CL, Times(1) can be omitted: […]
Done
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Mark Foltz, Takumi Fujimoto.
Ahmed Moussa would like Mark Foltz to review this change.
TabSwitcherUI Functionality to mirroring on Cast Moderator, Second CL
Allow, while mirroring/remoting tab on cast moderator, the ability to
switch the mirrored tab and to stop casting through a Tab Switcher UI.
This feature is available behind a flag parameter named `AccessCodeCastTabSwitchingUI`. You can try it out by `--enable-features=AccessCodeCastTabSwitchingUI`. It's disabled by default for now.
Second CL → mainly focus on updating data members, which keep track of
source tabs connected to different routes/sessions, with the new source tab that we switched to.
Third CL → will mainly focus on using the right text wording
for the Tab Switcher UI bar e.g. use `Casting` instead of `Sharing`.
Fourth and Final CL → will add metrics needed and remove the flag.
Bug: b/242618705
Change-Id: Ib3e9e4fe9875a027e9684d30c310f1ea53b394ba
---
M chrome/browser/media/cast_mirroring_performance_browsertest.cc
M chrome/browser/media/cast_mirroring_service_host.cc
M chrome/browser/media/cast_mirroring_service_host.h
M chrome/browser/media/cast_mirroring_service_host_browsertest.cc
M chrome/browser/media/router/providers/cast/cast_activity_manager.cc
M chrome/browser/media/router/providers/cast/cast_activity_manager.h
M chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc
M chrome/browser/media/router/providers/cast/cast_session_tracker.cc
M chrome/browser/media/router/providers/cast/cast_session_tracker.h
M chrome/browser/media/router/providers/cast/cast_session_tracker_unittest.cc
M chrome/browser/media/router/providers/cast/mirroring_activity.cc
M chrome/browser/media/router/providers/cast/mirroring_activity.h
M chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc
M components/mirroring/mojom/mirroring_service_host.mojom
M components/mirroring/mojom/session_observer.mojom
M components/mirroring/service/openscreen_session_host.cc
M components/mirroring/service/openscreen_session_host_unittest.cc
M components/mirroring/service/session.cc
M components/mirroring/service/session_unittest.cc
19 files changed, 281 insertions(+), 50 deletions(-)
Ahmed Moussa (Gerrit)
Attention is currently required from: Mark Foltz, Takumi Fujimoto.
1 comment:
Patchset:
mfoltz@: PTAL (cast_mirroring_service_host, mirroring_activity)
Addressing the comment [1] regarding updating `routes_by_frame_` without passing the new tab source to the mirroring service.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/3990531/comment/ef2d9b77_dc01baf8/
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Takumi Fujimoto (Gerrit)
Attention is currently required from: Ahmed Moussa, Mark Foltz.
6 comments:
File chrome/browser/media/cast_mirroring_service_host.h:
Patch Set #12, Line 74:
using GetTabSourceIdCallback = base::OnceCallback<void(int32_t)>;Nit: I think the callback type alias should get generated automatically (like this [1]) by the Mojo code generator, so you don't have to explicitly declare it.
File chrome/browser/media/cast_mirroring_service_host.cc:
Patch Set #12, Line 219:
if (!web_contents()) {Nit: When doing if-else, having the positive case (i.e. `if (web_contents())` here) come first makes it a bit easier to read
Patch Set #12, Line 220:
std::move(get_tab_source_id_callback).Run(-1);I'd prefer that we use `absl::optional<int32_t>` and `absl::nullopt` here rather than a magic number like -1 to indicate an invalid tab ID. It's expressed as `int32?` in Mojo
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #12, Line 485:
ASSERT_TRUE(base::Contains(manager_->activities_, route_id));It'd be best if we were testing `manager_` through its public API rather than by accessing its internal members -- the latter is more brittle and what we care about at the end of the day is its external behavior rather than its internal state. For instance, here we can use `GetRoutes()` instead of `activities_`.
Can we try to eliminate as much of the internal access as possible from this test?
File chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc:
Patch Set #12, Line 480:
ASSERT_EQ(activity_->frame_tree_node_id_, kFrameTreeNodeId);Should these be ASSERT or EXPECT? ASSERT failure halts the test there, so it should be used when there's no more info to be gained from running the rest of the test.
Patch Set #12, Line 483:
RunUntilIdle();We should call Mock::VerifyAndClearExpectations() here if we want EXPECT_CALL to be invoked before here and not after here.
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Mark Foltz, Takumi Fujimoto.
6 comments:
File chrome/browser/media/cast_mirroring_service_host.h:
Patch Set #12, Line 74:
using GetTabSourceIdCallback = base::OnceCallback<void(int32_t)>;
Nit: I think the callback type alias should get generated automatically (like this [1]) by the Mojo […]
Done
File chrome/browser/media/cast_mirroring_service_host.cc:
Patch Set #12, Line 219:
if (!web_contents()) {
Nit: When doing if-else, having the positive case (i.e. […]
Done
Patch Set #12, Line 220:
std::move(get_tab_source_id_callback).Run(-1);
I'd prefer that we use `absl::optional<int32_t>` and `absl::nullopt` here rather than a magic number […]
Currently numeric types cannot be nullable in mojo [1]. So, I would add one extra bool variable to indicate if a value exist.
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=657632
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #12, Line 485:
ASSERT_TRUE(base::Contains(manager_->activities_, route_id));
It'd be best if we were testing `manager_` through its public API rather than by accessing its inter […]
Updated `activities_` -> `GetRoute()`.
Regarding `routes_by_frame_`, I think we should keep it because
1) Since `routes_by_frame_` has no relevance outside cast_activity_manager, there is no public function that returns it.
2) `OnSourceTabChanged` function main purpose is to update `routes_by_frame_`. So, to me it makes since to check it directly.We could also create a private function to use instead of accessing `routes_by_frame_` directly e.g. `absl::optional<const MediaRoute::Id&> GetRouteIdFromFrameID(int frame_tree_node_id)`.
Let me know what do you think?
File chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc:
Patch Set #12, Line 480:
ASSERT_EQ(activity_->frame_tree_node_id_, kFrameTreeNodeId);
Should these be ASSERT or EXPECT? ASSERT failure halts the test there, so it should be used when the […]
Done
Patch Set #12, Line 483:
RunUntilIdle();
We should call Mock::VerifyAndClearExpectations() here if we want EXPECT_CALL to be invoked before h […]
Done
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Mark Foltz, Takumi Fujimoto.
Ahmed Moussa uploaded patch set #15 to this change.
TabSwitcherUI Functionality to mirroring on Cast Moderator, Second CL
Allow, while mirroring/remoting tab on cast moderator, the ability to
switch the mirrored tab and to stop casting through a Tab Switcher UI.
This feature is available behind a flag parameter named `AccessCodeCastTabSwitchingUI`. You can try it out by `--enable-features=AccessCodeCastTabSwitchingUI`. It's disabled by default for now.
Second CL → mainly focus on updating data members, which keep track of
source tabs connected to different routes/sessions, with the new source tab that we switched to.
Bug: b/242618705
Change-Id: Ib3e9e4fe9875a027e9684d30c310f1ea53b394ba
---
M chrome/browser/media/cast_mirroring_performance_browsertest.cc
M chrome/browser/media/cast_mirroring_service_host.cc
M chrome/browser/media/cast_mirroring_service_host.h
M chrome/browser/media/cast_mirroring_service_host_browsertest.cc
M chrome/browser/media/router/providers/cast/cast_activity_manager.cc
M chrome/browser/media/router/providers/cast/cast_activity_manager.h
M chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc
M chrome/browser/media/router/providers/cast/cast_session_tracker.cc
M chrome/browser/media/router/providers/cast/cast_session_tracker.h
M chrome/browser/media/router/providers/cast/cast_session_tracker_unittest.cc
M chrome/browser/media/router/providers/cast/mirroring_activity.cc
M chrome/browser/media/router/providers/cast/mirroring_activity.h
M chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc
M components/mirroring/mojom/mirroring_service_host.mojom
M components/mirroring/mojom/session_observer.mojom
M components/mirroring/service/openscreen_session_host.cc
M components/mirroring/service/openscreen_session_host_unittest.cc
M components/mirroring/service/session.cc
M components/mirroring/service/session_unittest.cc
19 files changed, 279 insertions(+), 50 deletions(-)
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Takumi Fujimoto (Gerrit)
Attention is currently required from: Ahmed Moussa.
5 comments:
File chrome/browser/media/cast_mirroring_service_host.cc:
Patch Set #12, Line 220:
std::move(get_tab_source_id_callback).Run(-1);Currently numeric types cannot be nullable in mojo [1]. […]
I didn't know that, thanks for digging up the info.
In that case I think it's cleaner to do what you were doing before, and just document in mirroring_service_host.mojom that -1 means no ID, and link to crbug.com/657632 for why we're using it. Sorry for the churn.
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #12, Line 485:
ASSERT_TRUE(base::Contains(manager_->activities_, route_id));
Updated `activities_` -> `GetRoute()`. […]
A more extreme way to put it is that if the internal state cannot be observed through the public API, then it doesn't matter what the internal state is. We only care about what's observable through the public API, so that's what we should be testing, although it often is easier to just directly observe the internal state.
So let's test the class through the public API used by other classes, unless that is prohibitively difficult. The question would be, what externally observable change do we expect as a result of the internal change?
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #16, Line 481:
manager_->routes_by_frame_[old_frame_id] = "RouteID";Can we tell the mapping between frame IDs and routes via GetRoutes()?
Patch Set #16, Line 481:
manager_->routes_by_frame_[old_frame_id] = "RouteID";Can this be achieved through the public interface e.g. LaunchSession()?
Patch Set #16, Line 487:
EXPECT_EQ(2u, manager_->routes_by_frame_.size());manager_->GetRoutes().size()?
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Takumi Fujimoto.
5 comments:
File chrome/browser/media/cast_mirroring_service_host.cc:
Patch Set #12, Line 220:
std::move(get_tab_source_id_callback).Run(-1);
I didn't know that, thanks for digging up the info. […]
😄, no worries.
Thanks Takumi.
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #12, Line 485:
ASSERT_TRUE(base::Contains(manager_->activities_, route_id));
A more extreme way to put it is that if the internal state cannot be observed through the public API […]
Thanks Takumi for the explanation.
Updated the CL and removed use of internal state.
The new patchset is using `ConvertMirrorToCast()` which is a private function. We can justify its use, as in fact it is the location where it notices the change made to `routes_by_frame_`. This function is only called when trying to `JoinSession`, which is the case mainly targeted by this CL.
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #16, Line 481:
manager_->routes_by_frame_[old_frame_id] = "RouteID";Can we tell the mapping between frame IDs and routes via GetRoutes()?
Done
Patch Set #16, Line 481:
manager_->routes_by_frame_[old_frame_id] = "RouteID";
Can this be achieved through the public interface e.g. […]
Done
Patch Set #16, Line 487:
EXPECT_EQ(2u, manager_->routes_by_frame_.size());
manager_->GetRoutes(). […]
Done
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Takumi Fujimoto (Gerrit)
Attention is currently required from: Ahmed Moussa.
6 comments:
File chrome/browser/media/router/providers/cast/cast_activity_manager.h:
Patch Set #18, Line 288:
absl::optional<MediaSinkInternal> ConvertMirrorToCast(int frame_tree_node_id);Nit: Can we make this `ConvertMirrorToCast(int frame_tree_node_id) const;` to indicate that it has no side effects?
Patch Set #18, Line 288:
absl::optional<MediaSinkInternal> ConvertMirrorToCast(int frame_tree_node_id);Given we're not actually converting anything, `GetSink()` may be a more appropriate name here.
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #12, Line 485:
ASSERT_TRUE(base::Contains(manager_->activities_, route_id));
Thanks Takumi for the explanation. […]
Thanks, it looks a lot better, especially if we can rename ConvertMirrorToCast().
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
For me a comment like this would be helpful to understand what the method does.
This method starts out with `route_id_to_move` associated with `frame_id_before`, and `route_id_to_terminate` with `frame_id_after`. We move `route_id_to_move` to `frame_id_after`, and that results in the termination of `route_id_to_terminate`.
Patch Set #18, Line 482:
const MediaRoute::Id& route_id2) {Maybe: frame_id_before, frame_id_after, route_id_to_move, route_id_to_terminate
Patch Set #18, Line 489:
ASSERT_TRUE(manager_->ConvertMirrorToCast(existing_frame_id));Nit: All the ASSERT_TRUE(ConvertMirrorToCast()) calls seem redundant given if it were to fail then it'd just crash on the next line anyways.
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Takumi Fujimoto.
5 comments:
File chrome/browser/media/router/providers/cast/cast_activity_manager.h:
Patch Set #18, Line 288:
absl::optional<MediaSinkInternal> ConvertMirrorToCast(int frame_tree_node_id);
Given we're not actually converting anything, `GetSink()` may be a more appropriate name here.
Makes sense. Just to make it more descriptive, we could use that `GetSinkIfShouldConvertMirrorToCast()`.
Patch Set #18, Line 288:
absl::optional<MediaSinkInternal> ConvertMirrorToCast(int frame_tree_node_id);
Nit: Can we make this `ConvertMirrorToCast(int frame_tree_node_id) const;` to indicate that it has n […]
Done
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
For me a comment like this would be helpful to understand what the method does. […]
Thanks Takumi, like your comment.
Patch Set #18, Line 482:
const MediaRoute::Id& route_id2) {Maybe: frame_id_before, frame_id_after, route_id_to_move, route_id_to_terminate
Done
Patch Set #18, Line 489:
ASSERT_TRUE(manager_->ConvertMirrorToCast(existing_frame_id));
Nit: All the ASSERT_TRUE(ConvertMirrorToCast()) calls seem redundant given if it were to fail then i […]
Done
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Mark Foltz (Gerrit)
Attention is currently required from: Ahmed Moussa, Takumi Fujimoto.
3 comments:
File chrome/browser/media/cast_mirroring_service_host.cc:
Patch Set #20, Line 251:
std::move(get_tab_source_id_callback)The return value is obtained synchronously, so there is not a need to have a callback here.
File components/mirroring/mojom/mirroring_service_host.mojom:
Patch Set #20, Line 29:
GetTabSourceId() => (int32 frame_tree_node_id);This is only called from the browser and is implemented in the browser, there is no reason for this to be an IPC. In fact this entire interface can be removed (as a separate cleanup).
File components/mirroring/mojom/session_observer.mojom:
Patch Set #20, Line 48:
OnSourceTabChanged();I would prefer that this be named OnSourceChanged() as the service can't always assume that it's tab-to-tab. In the future it might be tab-to-flinging or tab-to-screen-mirroring.
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Mark Foltz (Gerrit)
Attention is currently required from: Ahmed Moussa, Takumi Fujimoto.
1 comment:
File chrome/browser/media/cast_mirroring_service_host.cc:
Patch Set #20, Line 252:
.Run(web_contents()->GetPrimaryMainFrame()->GetFrameTreeNodeId());When a tab navigates, I am pretty sure the frame tree node id for the main frame can change. (Try navigating a tab being mirrored and logging the id to check.) Is the other code aware of that?
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Mark Foltz, Takumi Fujimoto.
5 comments:
Patchset:
mfoltz@: PTAL (cast_mirroring_service_host, mirroring_activity) […]
Done
File chrome/browser/media/cast_mirroring_service_host.cc:
Patch Set #20, Line 251:
std::move(get_tab_source_id_callback)
The return value is obtained synchronously, so there is not a need to have a callback here.
I believe using synchronous mojom calls is highly discouraged [1]. Correct me if I misunderstood the comment.
The callback can be removed after cleaning up by removing the mojom interface as suggested in the other comment [2].
[2] https://chromium-review.googlesource.com/c/chromium/src/+/3996083/comment/2c0ff523_3c394bc2/
Patch Set #20, Line 252:
.Run(web_contents()->GetPrimaryMainFrame()->GetFrameTreeNodeId());
When a tab navigates, I am pretty sure the frame tree node id for the main frame can change. […]
I tried it out, the frame tree node id didn't change. But still, I understand your concern. As explained here [1], prerendering might cause the id to change.
Learning that I think we need to update `routes_by_frame_` [2] and use some other variable that is expected to be sustained for the frame instead of frame id. Probably this should be addressed in a separate CL. Not sure about the priority though, AFAIK `routes_by_frame_` is mainly used for validation as to not allow two sessions to have the same source, also it plays a role in auto-conversion from mirroring to flinging.
Let me know what do think.
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=1179502
File components/mirroring/mojom/mirroring_service_host.mojom:
Patch Set #20, Line 29:
GetTabSourceId() => (int32 frame_tree_node_id);
This is only called from the browser and is implemented in the browser, there is no reason for this […]
Good point, like your suggestion.
Just one thing to keep in mind is that one of them is on UI thread and the other is on IO thread.
File components/mirroring/mojom/session_observer.mojom:
Patch Set #20, Line 48:
OnSourceTabChanged();
I would prefer that this be named OnSourceChanged() as the service can't always assume that it's tab […]
Ack
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Mark Foltz, Takumi Fujimoto.
1 comment:
File chrome/browser/media/cast_mirroring_service_host.cc:
Patch Set #20, Line 252:
.Run(web_contents()->GetPrimaryMainFrame()->GetFrameTreeNodeId());I tried it out, the frame tree node id didn't change. But still, I understand your concern. […]
I think I misunderstood the issue mentioned in the earlier comment [1].
IIUC the FrameTreeNodeID doesn't change, instead the frame itself is changing. So, I assume nothing need to be addressed.[1] https://bugs.chromium.org/p/chromium/issues/detail?id=1179502
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Takumi Fujimoto (Gerrit)
Attention is currently required from: Ahmed Moussa, Mark Foltz.
Patch set 20:Code-Review +1
4 comments:
Patchset:
LGTM once Mark's comments are addressed.
You also need a security OWNERS review for the mojom files.
File chrome/browser/media/router/providers/cast/cast_activity_manager.h:
Patch Set #20, Line 288:
absl::optional<MediaSinkInternal> GetSinkIfShouldConvertMirrorToCast(Nit: Maybe name it GetSinkForMirroringActivity(), although I don't feel strongly
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #20, Line 493:
.value()Nit: `*foo` is shorter than `foo.value()`, which may reduce the number of lines and make it a bit more readable here
File components/mirroring/mojom/session_observer.mojom:
Patch Set #20, Line 48:
OnSourceTabChanged();
Ack
Was there a reason why this was Ack'd rather than Done'd?
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Mark Foltz.
4 comments:
Patchset:
LGTM once Mark's comments are addressed. […]
Thanks Takumi
File chrome/browser/media/router/providers/cast/cast_activity_manager.h:
Patch Set #20, Line 288:
absl::optional<MediaSinkInternal> GetSinkIfShouldConvertMirrorToCast(Nit: Maybe name it GetSinkForMirroringActivity(), although I don't feel strongly
Done
File chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc:
Patch Set #20, Line 493:
.value()
Nit: `*foo` is shorter than `foo. […]
Done
File components/mirroring/mojom/session_observer.mojom:
Patch Set #20, Line 48:
OnSourceTabChanged();
Was there a reason why this was Ack'd rather than Done'd?
Done
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Mark Foltz (Gerrit)
Attention is currently required from: Ahmed Moussa.
Patch set 21:Code-Review +1
3 comments:
File chrome/browser/media/cast_mirroring_service_host.cc:
Patch Set #20, Line 251:
std::move(get_tab_source_id_callback)I believe using synchronous mojom calls is highly discouraged [1]. […]
Ack
Patch Set #20, Line 252:
.Run(web_contents()->GetPrimaryMainFrame()->GetFrameTreeNodeId());
I think I misunderstood the issue mentioned in the earlier comment [1]. […]
Thanks for that bug link. This looks like it will be OK. The capturer bootstraps itself using a GlobalRenderFrameHostId [1], which it then uses to look up the associated WebContents* and observer it for RFH changes. As long as it can find the correct WebContents* from the FrameTreeNodeId, then the capturer should be set up correctly.
File components/mirroring/mojom/mirroring_service_host.mojom:
Patch Set #20, Line 29:
GetTabSourceId() => (int32 frame_tree_node_id);
Good point, like your suggestion. […]
Can you file a cleanup bug for this in Internals>Cast>Streaming and link it here as TODO(crbug.com/XXX)?
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Attention is currently required from: Chromium IPC Reviews.
Ahmed Moussa would like Chromium IPC Reviews to review this change.
19 files changed, 325 insertions(+), 56 deletions(-)
gwsq (Gerrit)
Attention is currently required from: Chromium IPC Reviews, Matthew Denton.
gwsq would like Matthew Denton to review this change authored by Ahmed Moussa.
gwsq (Gerrit)
Attention is currently required from: Matthew Denton.
Ahmed Moussa has uploaded this change for review.
gwsq (Gerrit)
Attention is currently required from: Matthew Denton.
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): mpde...@chromium.org
Reviewer source(s):
mpde...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Ahmed Moussa (Gerrit)
Attention is currently required from: Matthew Denton.
1 comment:
File components/mirroring/mojom/mirroring_service_host.mojom:
Patch Set #20, Line 29:
GetTabSourceId() => (int32 frame_tree_node_id);
Can you file a cleanup bug for this in Internals>Cast>Streaming and link it here as TODO(crbug. […]
Done.
Thanks Mark.
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Patchset:
@mpdenton: here is the design doc, if more info is needed
https://docs.google.com/document/d/1JAqhPhdoG51o1j7ip7VxWYp1oWWSm20OmyBwnyKYsTI/edit?usp=sharing&resourcekey=0-zG7m9Z54VbRYFVpiuukeWAIn mirroring_service_host.mojom, this is all privileged to privileged communication.
In session_observer.mojom, communication is from unprivileged (mirroring service) to privileged (browser process).
Let me know if something else is needed. Thanks!!
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Denton (Gerrit)
Attention is currently required from: Ahmed Moussa.
Patch set 22:Code-Review +1
Ahmed Moussa (Gerrit)
1 comment:
Patchset:
@mpdenton: here is the design doc, if more info is needed […]
Done
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.
Ahmed Moussa (Gerrit)
Patch set 22:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
TabSwitcherUI Functionality to mirroring on Cast Moderator, Second CL
Allow, while mirroring/remoting tab on cast moderator, the ability to
switch the mirrored tab and to stop casting through a Tab Switcher UI.
This feature is available behind a flag parameter named `AccessCodeCastTabSwitchingUI`. You can try it out by `--enable-features=AccessCodeCastTabSwitchingUI`. It's disabled by default for now.
Second CL → mainly focus on updating data members, which keep track of
source tabs connected to different routes/sessions, with the new source tab that we switched to.
Bug: b/242618705
Change-Id: Ib3e9e4fe9875a027e9684d30c310f1ea53b394ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3996083
Reviewed-by: Takumi Fujimoto <tak...@chromium.org>
Reviewed-by: Matthew Denton <mpde...@chromium.org>
Commit-Queue: Ahmed Moussa <ahmed...@google.com>
Reviewed-by: Mark Foltz <mfo...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1080093}
---
M chrome/browser/media/cast_mirroring_performance_browsertest.cc
M chrome/browser/media/cast_mirroring_service_host.cc
M chrome/browser/media/cast_mirroring_service_host.h
M chrome/browser/media/cast_mirroring_service_host_browsertest.cc
M chrome/browser/media/router/providers/cast/cast_activity_manager.cc
M chrome/browser/media/router/providers/cast/cast_activity_manager.h
M chrome/browser/media/router/providers/cast/cast_activity_manager_unittest.cc
M chrome/browser/media/router/providers/cast/cast_session_tracker.cc
M chrome/browser/media/router/providers/cast/cast_session_tracker.h
M chrome/browser/media/router/providers/cast/cast_session_tracker_unittest.cc
M chrome/browser/media/router/providers/cast/mirroring_activity.cc
M chrome/browser/media/router/providers/cast/mirroring_activity.h
M chrome/browser/media/router/providers/cast/mirroring_activity_unittest.cc
M components/mirroring/mojom/mirroring_service_host.mojom
M components/mirroring/mojom/session_observer.mojom
M components/mirroring/service/openscreen_session_host.cc
M components/mirroring/service/openscreen_session_host_unittest.cc
M components/mirroring/service/session.cc
M components/mirroring/service/session_unittest.cc
19 files changed, 332 insertions(+), 56 deletions(-)
Johann Koenig (Gerrit)
Johann Koenig has created a revert of this change.
To view, visit change 3996083. To unsubscribe, or for help writing mail filters, visit settings.