Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2947403004:
[MediaRouter] Replace RouteMessage with PresentationConnectionMessage (Closed)
|
Created:
3 years, 6 months ago by imcheng Modified:
3 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, apacible+watch_chromium.org, avayvod+watch_chromium.org, chfremer+watch_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, erickung+watch_chromium.org, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, mfoltz+watch_chromium.org, miu+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, xjz+watch_chromium.org, yzshen+watch_chromium.org, xjz Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MediaRouter] Replace RouteMessage with PresentationConnectionMessage
- move ToHumanReadableString() from RouteMessage (deleted) to test_helper
- make PCM move constructor noexcept to optimize for std::move_if_noexcept in std::vector and other STL containers
TODO in future patches:
- consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API.
- replace RouteMessage mojo type with PCM
BUG=699230
Review-Url: https://codereview.chromium.org/2947403004
Cr-Commit-Position: refs/heads/master@{#484825}
Committed: https://chromium.googlesource.com/chromium/src/+/a9332ac8182d3ed2f84717e669d8a62ff8ab58ea
Patch Set 1 : . #
Total comments: 10
Patch Set 2 : Addressed Takumi's comments #
Total comments: 36
Patch Set 3 : Addressed Mark's comments #Patch Set 4 : simplified optional check + move ToString to test_helper #Patch Set 5 : emplace_back #
Total comments: 2
Patch Set 6 : addressed miu's comments #Patch Set 7 : fix android #
Total comments: 4
Patch Set 8 : Addressed dcheng's commentS #Patch Set 9 : rebase #Messages
Total messages: 72 (45 generated)
The CQ bit was checked by imcheng@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by imcheng@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.
Description was changed from ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage TODO: - consider making PCM move-only after xjz@'s remoting patch lands, if we will only ever have 1 message observer per route. - replace RouteMessage mojo type with PCM BUG=699230 ========== to ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage TODO: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 ==========
Description was changed from ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage TODO: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 ========== to ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage TODO in future patches: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage TODO in future patches: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 ========== to ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage - make PCM move constructor noexcept to optimize for std::move_if_noexcept in std::vector and other STL containers TODO in future patches: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 ==========
Description was changed from ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage - make PCM move constructor noexcept to optimize for std::move_if_noexcept in std::vector and other STL containers TODO in future patches: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 ========== to ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage - move ToHumanReadableString() from RouteMessage (deleted) to PCM - make PCM move constructor noexcept to optimize for std::move_if_noexcept in std::vector and other STL containers TODO in future patches: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 ==========
imcheng@chromium.org changed reviewers: + takumif@chromium.org
Takumi: PTAL thanks! https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:374: for (const auto& message : messages) { Note: This code will be removed in xjz@'s patch https://codereview.chromium.org/2951523002/
lgtm with minor comments. You'll still need an lgtm from a full committer as well :) https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.h (right): https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:137: // remoting route. There are two types of messages: 1) TEXT notification Nit: we should probably update the comment given we no longer use the TEXT/BINARY enum. https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:272: media_router_.TakeMessagesSentToProvider(true, &messages); Suggestion: instead of calling TakeMessagesSentToProvider() here and below, call TakeTextMessagesSentToProvider() or TakeDataMessagesSentToProvider() that call TakeMessagesSentToProvider() with true and false respectively. https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/route_message_observer.h (right): https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/route_message_observer.h:15: #include "content/public/common/presentation_connection_message.h" Can we forward-declare this instead? https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/route_message_observer.h:23: class RouteMessageObserver { Maybe we should add a TODO to rename this to PresentationConnectionMessageObserver?
The CQ bit was checked by imcheng@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.
imcheng@chromium.org changed reviewers: + mfoltz@chromium.org
+mfoltz PTAL https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.h (right): https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:137: // remoting route. There are two types of messages: 1) TEXT notification On 2017/06/26 18:50:15, takumif wrote: > Nit: we should probably update the comment given we no longer use the > TEXT/BINARY enum. I switched them to lower case so it no longer refers to the nonexistent enum. https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:272: media_router_.TakeMessagesSentToProvider(true, &messages); On 2017/06/26 18:50:15, takumif wrote: > Suggestion: instead of calling TakeMessagesSentToProvider() here and below, call > TakeTextMessagesSentToProvider() or TakeDataMessagesSentToProvider() that call > TakeMessagesSentToProvider() with true and false respectively. I am less inclined to change this since this code will soon be deleted by xjz@'s patch. https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/route_message_observer.h (right): https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/route_message_observer.h:15: #include "content/public/common/presentation_connection_message.h" On 2017/06/26 18:50:15, takumif wrote: > Can we forward-declare this instead? Done. https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/route_message_observer.h:23: class RouteMessageObserver { On 2017/06/26 18:50:15, takumif wrote: > Maybe we should add a TODO to rename this to > PresentationConnectionMessageObserver? Done.
https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/20001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:272: media_router_.TakeMessagesSentToProvider(true, &messages); On 2017/06/27 06:41:57, imcheng wrote: > On 2017/06/26 18:50:15, takumif wrote: > > Suggestion: instead of calling TakeMessagesSentToProvider() here and below, > call > > TakeTextMessagesSentToProvider() or TakeDataMessagesSentToProvider() that call > > TakeMessagesSentToProvider() with true and false respectively. > > I am less inclined to change this since this code will soon be deleted by xjz@'s > patch. Acknowledged.
Thanks - this is an excellent cleanup. Minor style comments and there is some possible simplification of unit tests (optional to fix). Do you plan on using PCM in media_router.mojom so we can end up with one typemap? https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/an... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/an... chrome/browser/media/android/router/media_router_android.cc:320: std::vector<content::PresentationConnectionMessage> messages(1); Does messages({message}) work? https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:375: if (message.data) { message.is_binary() https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:384: DCHECK(message.message); If is_binary && !active_bridge_ then this DCHECK will fail. Intentional? https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:392: if (active_bridge_ && This might be easier to follow if there was an early return at the beginning for !active_bridge_. Optional to fix https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:105: inbound_messages_.push_back(PresentationConnectionMessage()); Does emplace_back(message) work here and below? https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:127: if (entry.first.message.has_value() == text) { Did you mean !entry.first.is_binary() ? https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:169: PresentationConnectionMessage message; message(text) https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:180: PresentationConnectionMessage message; message(*data) https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:274: for (const PresentationConnectionMessage& message : messages) { const auto& ? https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:275: if (message.message && expected_message == *message.message) { if (expected_message == message.message) Here and below. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:123: content::PresentationConnectionMessage message_1; message_1("foo") etc. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1007: batch1->resize(1); I would slightly prefer to use the PCM constructors here, via emplace_back or just push_back(PCM("text1")), instead of manually assigning to the class fields. Optional to fix though (this is existing code). https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1035: messages_.insert(messages_.end(), messages.begin(), messages.end()); messages_ = messages? https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1040: ASSERT_TRUE(messages_[0].message); PCM implements == so you could store the messages added to messages_ and just: ASSERT_TRUE(messages_ == expected_messages_) ? https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... File content/public/common/presentation_connection_message.cc (right): https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... content/public/common/presentation_connection_message.cc:44: PresentationConnectionMessage& PresentationConnectionMessage::operator=( Can you add a note explaining why this isn't move-only? https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... content/public/common/presentation_connection_message.cc:54: std::string PresentationConnectionMessage::ToHumanReadableString() const { This only seems to be called from logging in unittests. Guard with #ifdef !NDEBUG?
The CQ bit was checked by imcheng@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...
https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/an... File chrome/browser/media/android/router/media_router_android.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/an... chrome/browser/media/android/router/media_router_android.cc:320: std::vector<content::PresentationConnectionMessage> messages(1); On 2017/06/28 07:29:53, mark a. foltz wrote: > Does messages({message}) work? Yeah uniform initialization should work. I will use messages = {message}; https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:375: if (message.data) { On 2017/06/28 07:29:54, mark a. foltz wrote: > message.is_binary() Done. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:384: DCHECK(message.message); On 2017/06/28 07:29:53, mark a. foltz wrote: > If is_binary && !active_bridge_ then this DCHECK will fail. Intentional? If is_binary is true then we won't reach here due to the continue; . https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:392: if (active_bridge_ && On 2017/06/28 07:29:54, mark a. foltz wrote: > This might be easier to follow if there was an early return at the beginning for > !active_bridge_. Optional to fix That might be a bit challenging to extract since there is some logic for !active_bridge_ that's dependent on other conditions (L436-437). I will leave it to xjz@ / miu@ to determine what's the best way to reorganize the logic here if needed. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:105: inbound_messages_.push_back(PresentationConnectionMessage()); On 2017/06/28 07:29:54, mark a. foltz wrote: > Does emplace_back(message) work here and below? Done. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:127: if (entry.first.message.has_value() == text) { On 2017/06/28 07:29:54, mark a. foltz wrote: > Did you mean !entry.first.is_binary() ? Done. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:169: PresentationConnectionMessage message; On 2017/06/28 07:29:54, mark a. foltz wrote: > message(text) Done. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:180: PresentationConnectionMessage message; On 2017/06/28 07:29:54, mark a. foltz wrote: > message(*data) Done. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:274: for (const PresentationConnectionMessage& message : messages) { On 2017/06/28 07:29:55, mark a. foltz wrote: > const auto& ? Done. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:275: if (message.message && expected_message == *message.message) { On 2017/06/28 07:29:54, mark a. foltz wrote: > if (expected_message == message.message) > > Here and below. I don't think you can use std::string operator== on a base::Optional<std::string>? https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:123: content::PresentationConnectionMessage message_1; On 2017/06/28 07:29:55, mark a. foltz wrote: > message_1("foo") etc. Done. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1007: batch1->resize(1); On 2017/06/28 07:29:55, mark a. foltz wrote: > I would slightly prefer to use the PCM constructors here, via emplace_back or > just push_back(PCM("text1")), instead of manually assigning to the class fields. > Optional to fix though (this is existing code). Done. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1035: messages_.insert(messages_.end(), messages.begin(), messages.end()); On 2017/06/28 07:29:55, mark a. foltz wrote: > messages_ = messages? This appends the contents of |messages| to |messages_|. https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1040: ASSERT_TRUE(messages_[0].message); On 2017/06/28 07:29:55, mark a. foltz wrote: > PCM implements == so you could store the messages added to messages_ and just: > > ASSERT_TRUE(messages_ == expected_messages_) > > ? Done. Though I find that the problem with asserting vectors is that the error message on failure is often cryptic. So this has a slight edge in terms of figuring out where exactly the the content mismatched. https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... File content/public/common/presentation_connection_message.cc (right): https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... content/public/common/presentation_connection_message.cc:44: PresentationConnectionMessage& PresentationConnectionMessage::operator=( On 2017/06/28 07:29:55, mark a. foltz wrote: > Can you add a note explaining why this isn't move-only? added comment in header file. https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... content/public/common/presentation_connection_message.cc:54: std::string PresentationConnectionMessage::ToHumanReadableString() const { On 2017/06/28 07:29:56, mark a. foltz wrote: > This only seems to be called from logging in unittests. Guard with #ifdef > !NDEBUG? Added #ifndef NDEBUG.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:275: if (message.message && expected_message == *message.message) { On 2017/06/29 at 08:21:09, imcheng wrote: > On 2017/06/28 07:29:54, mark a. foltz wrote: > > if (expected_message == message.message) > > > > Here and below. > > I don't think you can use std::string operator== on a base::Optional<std::string>? There's an operator declared in optional.h. https://cs.chromium.org/chromium/src/base/optional.h?rcl=9ebd822e4d8891a01a33... https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1040: ASSERT_TRUE(messages_[0].message); On 2017/06/29 at 08:21:09, imcheng wrote: > On 2017/06/28 07:29:55, mark a. foltz wrote: > > PCM implements == so you could store the messages added to messages_ and just: > > > > ASSERT_TRUE(messages_ == expected_messages_) > > > > ? > > Done. Though I find that the problem with asserting vectors is that the error message on failure is often cryptic. So this has a slight edge in terms of figuring out where exactly the the content mismatched. Yeah. It would be nice if there were an ASSERT_CONTAINERS_EQ() macro.
https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector_unittest.cc (right): https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector_unittest.cc:275: if (message.message && expected_message == *message.message) { On 2017/06/29 17:38:56, mark a. foltz wrote: > On 2017/06/29 at 08:21:09, imcheng wrote: > > On 2017/06/28 07:29:54, mark a. foltz wrote: > > > if (expected_message == message.message) > > > > > > Here and below. > > > > I don't think you can use std::string operator== on a > base::Optional<std::string>? > > There's an operator declared in optional.h. > > https://cs.chromium.org/chromium/src/base/optional.h?rcl=9ebd822e4d8891a01a33... Oh that's right. I tried it and it compiles and I was wondering why. I've been looking for a operator() to T which is nowhere to be found, then I realized about the operator==. https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... File content/public/common/presentation_connection_message.cc (right): https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... content/public/common/presentation_connection_message.cc:54: std::string PresentationConnectionMessage::ToHumanReadableString() const { On 2017/06/29 08:21:09, imcheng wrote: > On 2017/06/28 07:29:56, mark a. foltz wrote: > > This only seems to be called from logging in unittests. Guard with #ifdef > > !NDEBUG? > > Added #ifndef NDEBUG. It turns out we build unit tests on release builds, so the macro won't work. I will move this to test_helper.{cc,h} instead.
On 2017/06/28 07:29:56, mark a. foltz wrote: > Thanks - this is an excellent cleanup. Minor style comments and there is some > possible simplification of unit tests (optional to fix). > > Do you plan on using PCM in media_router.mojom so we can end up with one > typemap? Yep that will be the next step. After that, we can look into plumbing connection messages extension <-> renderer, but that will require some design work. > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/an... > File chrome/browser/media/android/router/media_router_android.cc (right): > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/an... > chrome/browser/media/android/router/media_router_android.cc:320: > std::vector<content::PresentationConnectionMessage> messages(1); > Does messages({message}) work? > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > File chrome/browser/media/cast_remoting_connector.cc (right): > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > chrome/browser/media/cast_remoting_connector.cc:375: if (message.data) { > message.is_binary() > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > chrome/browser/media/cast_remoting_connector.cc:384: DCHECK(message.message); > If is_binary && !active_bridge_ then this DCHECK will fail. Intentional? > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > chrome/browser/media/cast_remoting_connector.cc:392: if (active_bridge_ && > This might be easier to follow if there was an early return at the beginning for > !active_bridge_. Optional to fix > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > File chrome/browser/media/cast_remoting_connector_unittest.cc (right): > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > chrome/browser/media/cast_remoting_connector_unittest.cc:105: > inbound_messages_.push_back(PresentationConnectionMessage()); > Does emplace_back(message) work here and below? > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > chrome/browser/media/cast_remoting_connector_unittest.cc:127: if > (entry.first.message.has_value() == text) { > Did you mean !entry.first.is_binary() ? > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > chrome/browser/media/cast_remoting_connector_unittest.cc:169: > PresentationConnectionMessage message; > message(text) > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > chrome/browser/media/cast_remoting_connector_unittest.cc:180: > PresentationConnectionMessage message; > message(*data) > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > chrome/browser/media/cast_remoting_connector_unittest.cc:274: for (const > PresentationConnectionMessage& message : messages) { > const auto& ? > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > chrome/browser/media/cast_remoting_connector_unittest.cc:275: if > (message.message && expected_message == *message.message) { > if (expected_message == message.message) > > Here and below. > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... > File > chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc > (right): > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... > chrome/browser/media/router/browser_presentation_connection_proxy_unittest.cc:123: > content::PresentationConnectionMessage message_1; > message_1("foo") etc. > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... > File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc > (right): > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... > chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1007: > batch1->resize(1); > I would slightly prefer to use the PCM constructors here, via emplace_back or > just push_back(PCM("text1")), instead of manually assigning to the class fields. > Optional to fix though (this is existing code). > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... > chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1035: > messages_.insert(messages_.end(), messages.begin(), messages.end()); > messages_ = messages? > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ro... > chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1040: > ASSERT_TRUE(messages_[0].message); > PCM implements == so you could store the messages added to messages_ and just: > > ASSERT_TRUE(messages_ == expected_messages_) > > ? > > https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... > File content/public/common/presentation_connection_message.cc (right): > > https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... > content/public/common/presentation_connection_message.cc:44: > PresentationConnectionMessage& PresentationConnectionMessage::operator=( > Can you add a note explaining why this isn't move-only? > > https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... > content/public/common/presentation_connection_message.cc:54: std::string > PresentationConnectionMessage::ToHumanReadableString() const { > This only seems to be called from logging in unittests. Guard with #ifdef > !NDEBUG?
On 2017/06/29 at 17:58:23, imcheng wrote: > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > File chrome/browser/media/cast_remoting_connector_unittest.cc (right): > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > chrome/browser/media/cast_remoting_connector_unittest.cc:275: if (message.message && expected_message == *message.message) { > On 2017/06/29 17:38:56, mark a. foltz wrote: > > On 2017/06/29 at 08:21:09, imcheng wrote: > > > On 2017/06/28 07:29:54, mark a. foltz wrote: > > > > if (expected_message == message.message) > > > > > > > > Here and below. > > > > > > I don't think you can use std::string operator== on a > > base::Optional<std::string>? > > > > There's an operator declared in optional.h. > > > > https://cs.chromium.org/chromium/src/base/optional.h?rcl=9ebd822e4d8891a01a33... > > Oh that's right. I tried it and it compiles and I was wondering why. I've been looking for a operator() to T which is nowhere to be found, then I realized about the operator==. > > https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... > File content/public/common/presentation_connection_message.cc (right): > > https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... > content/public/common/presentation_connection_message.cc:54: std::string PresentationConnectionMessage::ToHumanReadableString() const { > On 2017/06/29 08:21:09, imcheng wrote: > > On 2017/06/28 07:29:56, mark a. foltz wrote: > > > This only seems to be called from logging in unittests. Guard with #ifdef > > > !NDEBUG? > > > > Added #ifndef NDEBUG. > > It turns out we build unit tests on release builds, so the macro won't work. I will move this to test_helper.{cc,h} instead. Yes, but are the logging statements debug-only (DVLOG)? If not, they should be.
On 2017/06/29 18:02:02, mark a. foltz wrote: > On 2017/06/29 at 17:58:23, imcheng wrote: > > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > > File chrome/browser/media/cast_remoting_connector_unittest.cc (right): > > > > > https://codereview.chromium.org/2947403004/diff/40001/chrome/browser/media/ca... > > chrome/browser/media/cast_remoting_connector_unittest.cc:275: if > (message.message && expected_message == *message.message) { > > On 2017/06/29 17:38:56, mark a. foltz wrote: > > > On 2017/06/29 at 08:21:09, imcheng wrote: > > > > On 2017/06/28 07:29:54, mark a. foltz wrote: > > > > > if (expected_message == message.message) > > > > > > > > > > Here and below. > > > > > > > > I don't think you can use std::string operator== on a > > > base::Optional<std::string>? > > > > > > There's an operator declared in optional.h. > > > > > > > https://cs.chromium.org/chromium/src/base/optional.h?rcl=9ebd822e4d8891a01a33... > > > > Oh that's right. I tried it and it compiles and I was wondering why. I've been > looking for a operator() to T which is nowhere to be found, then I realized > about the operator==. > > > > > https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... > > File content/public/common/presentation_connection_message.cc (right): > > > > > https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... > > content/public/common/presentation_connection_message.cc:54: std::string > PresentationConnectionMessage::ToHumanReadableString() const { > > On 2017/06/29 08:21:09, imcheng wrote: > > > On 2017/06/28 07:29:56, mark a. foltz wrote: > > > > This only seems to be called from logging in unittests. Guard with #ifdef > > > > !NDEBUG? > > > > > > Added #ifndef NDEBUG. > > > > It turns out we build unit tests on release builds, so the macro won't work. I > will move this to test_helper.{cc,h} instead. > > Yes, but are the logging statements debug-only (DVLOG)? If not, they should be. The human readable strings are streamed to gtest objects (ADD_FAILURE(), EXPECT_EQ() etc.) which don't check NDEBUG.
> > https://codereview.chromium.org/2947403004/diff/40001/content/public/common/p... > > > content/public/common/presentation_connection_message.cc:54: std::string > > PresentationConnectionMessage::ToHumanReadableString() const { > > > On 2017/06/29 08:21:09, imcheng wrote: > > > > On 2017/06/28 07:29:56, mark a. foltz wrote: > > > > > This only seems to be called from logging in unittests. Guard with #ifdef > > > > > !NDEBUG? > > > > > > > > Added #ifndef NDEBUG. > > > > > > It turns out we build unit tests on release builds, so the macro won't work. I > > will move this to test_helper.{cc,h} instead. > > > > Yes, but are the logging statements debug-only (DVLOG)? If not, they should be. > > The human readable strings are streamed to gtest objects (ADD_FAILURE(), EXPECT_EQ() etc.) which don't check NDEBUG. Ah, ok. Then a test-only compilation target is the next best thing.
lgtm
The CQ bit was checked by imcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from takumif@chromium.org Link to the patchset: https://codereview.chromium.org/2947403004/#ps80001 (title: "simplified optional check + move ToString to test_helper")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
imcheng@chromium.org changed reviewers: + avi@chromium.org, dcheng@chromium.org, miu@chromium.org
+ additional reviewers. Please review: miu: cast_remoting_connector* avi: content/public/common/* dcheng: media_router.typemap, media_router_struct_traits.h
Description was changed from ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage - move ToHumanReadableString() from RouteMessage (deleted) to PCM - make PCM move constructor noexcept to optimize for std::move_if_noexcept in std::vector and other STL containers TODO in future patches: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 ========== to ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage - move ToHumanReadableString() from RouteMessage (deleted) to test_helper - make PCM move constructor noexcept to optimize for std::move_if_noexcept in std::vector and other STL containers TODO in future patches: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 ==========
content/public/common lgtm
cast_remoting_connector* lgtm BTW, xjz@ is rewriting that code to use mojo instead of route messages anyway. Let the "rebase race" begin! :) https://codereview.chromium.org/2947403004/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2947403004/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:384: DCHECK(message.message); nit: The |*message.message| expressions below will do a DCHECK, so you don't need this here.
Friendly ping dcheng for media_router.typemap, media_router_struct_traits.h https://codereview.chromium.org/2947403004/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2947403004/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:384: DCHECK(message.message); On 2017/06/29 22:07:59, miu wrote: > nit: The |*message.message| expressions below will do a DCHECK, so you don't > need this here. Done.
The CQ bit was checked by imcheng@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by imcheng@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.
LGTM with nits (I didn't look at anything outside the struct traits) https://codereview.chromium.org/2947403004/diff/140001/chrome/common/media_ro... File chrome/common/media_router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2947403004/diff/140001/chrome/common/media_ro... chrome/common/media_router/mojo/media_router_struct_traits.h:189: if (!data.ReadMessage(&text) || !text) Here and below, just read directly into out->message and out->data (respectively). The deserialized value isn't used on failure anyway, so it's OK to build it incrementally. https://codereview.chromium.org/2947403004/diff/140001/chrome/common/media_ro... chrome/common/media_router/mojo/media_router_struct_traits.h:201: default: Also, consider removing default: that way the compiler will warn on new enum values in the future
https://codereview.chromium.org/2947403004/diff/140001/chrome/common/media_ro... File chrome/common/media_router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2947403004/diff/140001/chrome/common/media_ro... chrome/common/media_router/mojo/media_router_struct_traits.h:189: if (!data.ReadMessage(&text) || !text) On 2017/07/06 08:18:05, dcheng wrote: > Here and below, just read directly into out->message and out->data > (respectively). > > The deserialized value isn't used on failure anyway, so it's OK to build it > incrementally. Done. https://codereview.chromium.org/2947403004/diff/140001/chrome/common/media_ro... chrome/common/media_router/mojo/media_router_struct_traits.h:201: default: On 2017/07/06 08:18:05, dcheng wrote: > Also, consider removing default: that way the compiler will warn on new enum > values in the future Done.
The CQ bit was checked by imcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, takumif@chromium.org, avi@chromium.org, miu@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2947403004/#ps160001 (title: "Addressed dcheng's commentS")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by imcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, avi@chromium.org, miu@chromium.org, dcheng@chromium.org, takumif@chromium.org Link to the patchset: https://codereview.chromium.org/2947403004/#ps180001 (title: "rebase")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by imcheng@chromium.org
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": 180001, "attempt_start_ts": 1499394185722160,
"parent_rev": "4b357464fd8f1590de9493bd7a7f5be5870377ec", "commit_rev":
"a9332ac8182d3ed2f84717e669d8a62ff8ab58ea"}
Message was sent while issue was closed.
Description was changed from ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage - move ToHumanReadableString() from RouteMessage (deleted) to test_helper - make PCM move constructor noexcept to optimize for std::move_if_noexcept in std::vector and other STL containers TODO in future patches: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 ========== to ========== [MediaRouter] Replace RouteMessage with PresentationConnectionMessage - move ToHumanReadableString() from RouteMessage (deleted) to test_helper - make PCM move constructor noexcept to optimize for std::move_if_noexcept in std::vector and other STL containers TODO in future patches: - consider making PCM move-only after xjz@'s remoting patch lands, though I am less inclined to make this optimization to overly restrict the API. - replace RouteMessage mojo type with PCM BUG=699230 Review-Url: https://codereview.chromium.org/2947403004 Cr-Commit-Position: refs/heads/master@{#484825} Committed: https://chromium.googlesource.com/chromium/src/+/a9332ac8182d3ed2f84717e669d8... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/a9332ac8182d3ed2f84717e669d8... |
