New issue
Advanced search Search tips

Issue 867935 link

Starred by 1 user
Status: Verified
Owner:
mfo...@chromium.org
Closed: Jan 16
Cc:
mfo...@chromium.org
taku...@chromium.org
powerb@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug


Participants' hotlists:
media-router-fixit


Sign in to add a comment

[MediaRouter] Replace MediaRoute::Equals() with operator==

Project Member Reported by imch...@chromium.org, Jul 26 
1) Equals() only compare IDs and not other fields.
2) Equals() is only used in tests. Calls of EXPECT_TRUE(route.Equals(expected)) could be replaced with EXPECT_EQ(expected, route), which is the preferred style, if operator== is implemented instead.

Tasks:
- Replace Equals() with operator==. Ideally, operator== will compare all MediaRoute fields, as this gives us stronger guarantees in tests
- Replace all (test) usages of Equals() with operator== as describe in #2 above

Comment 1 by mfo...@chromium.org, Jan 14

Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: mfo...@chromium.org
Status: Started (was: Available)
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 16 

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

commit 566c6efdbc5396c9d684c19a68d56360572cae7f
Author: mark a. foltz <mfoltz@chromium.org>
Date: Wed Jan 16 00:15:16 2019

[Media Router] Replace Equals() with ==.

Replace Equals() with operator== for MediaRoute and MediaSink.

Also:
 - Ensure operator== compares all fields.
 - Remove custom matchers in unit tests for these objects.
 - Update unit tests that failed because not all fields
   were compared.

(operator== is used only in test code as far as I can tell.)

Bug:  867935 
Change-Id: I48dcc9ddeb132e4e2db5c3b0fc235bfcfa8f9be9
Reviewed-on: https://chromium-review.googlesource.com/c/1407919
Commit-Queue: mark a. foltz <mfoltz@chromium.org>
Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622908}
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/media/router/media_router_base_unittest.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/media/router/media_sinks_observer_unittest.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/media/router/presentation/local_presentation_manager_unittest.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/media/router/presentation/presentation_service_delegate_impl_unittest.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/media/router/providers/dial/dial_activity_manager_unittest.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/media/router/providers/dial/dial_media_route_provider_unittest.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/media/router/test/media_router_mojo_test.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/media/router/test/test_helper.h
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/ui/media_router/media_sink_with_cast_modes.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/common/media_router/media_route.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/common/media_router/media_route.h
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/common/media_router/media_route_unittest.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/common/media_router/media_sink.cc
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/common/media_router/media_sink.h
[modify] https://crrev.com/566c6efdbc5396c9d684c19a68d56360572cae7f/chrome/common/media_router/media_sink_unittest.cc

Comment 3 by mfo...@chromium.org, Jan 16 (6 days ago)

Status: Verified (was: Started)
►

Sign in to add a comment