Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1268553002:
Reflect the current state in Media Router Action icon. (Closed)
DescriptionReflect the current state in Media Router Action icon.
The four states and scenarios are as following:
- Idle (Not locally casting to a sink).
- Active (Currently locally casting to a sink).
- Warning (Current issue shown to user is of type 'warning').
- Error (Current issue shown to user is of type 'fatal').
This change also adds more Media Router Action icons, as well as update the current idle icons.
Committed: https://crrev.com/4eebb13efbe6b3304bc501da6331e64e05d716f9
Cr-Commit-Position: refs/heads/master@{#342841}
Patch Set 1 : #
Total comments: 24
Patch Set 2 : Rebase. #Patch Set 3 : Changes per kmarshall@'s comments. #
Total comments: 10
Patch Set 4 : Rebase. #Patch Set 5 : Changes per kmarshall@'s comments. #
Total comments: 34
Patch Set 6 : Rebase. #Patch Set 7 : Changes per pkasting@'s comments. #
Total comments: 12
Patch Set 8 : Rebase. #Patch Set 9 : Changes per kmarshall@'s comments. #
Total comments: 24
Patch Set 10 : Changes per pkasting@'s comments. #Patch Set 11 : Rebase. #Patch Set 12 : Fix tests (asan). #Patch Set 13 : Fix tests (lsan). #Messages
Total messages: 43 (23 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
apacible@chromium.org changed reviewers: + kmarshall@chromium.org, oshima@chromium.org, pkasting@chromium.org
Patchset #1 (id:120001) has been deleted
PTAL, thanks! kmarshall for everything. oshima for chrome/app/theme/. pkasting for chrome/browser/ui/toolbar/. Screenshots: https://screenshot.googleplex.com/rDPGdvcW1O0.png https://screenshot.googleplex.com/8SHovFpTED2.png https://screenshot.googleplex.com/c6Tco5WurpD.png https://screenshot.googleplex.com/KAFsu9VkrBL.png
c/a/theme lgtm
https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:26: MediaRouterAction* action) alignment & DCHECK the ptrs for non-nullness https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:72: routes_observer_.reset(); Unnecessary https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:204: return web_contents ? static_cast<media_router::MediaRouterMojoImpl*>( No need to static_cast here https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:20: class Issue; Sorting https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:25: // Observes when there is an updated issue and passes it to |action_|. You can create a single Observer class that implements IssuesObserver and MediaRoutesObserver. Is there a reason why the MediaRouterAction can't also be an observer? The instances are bound one-to-one with an Action object. It would simplify test code, because you could just trigger the Observer methods on the Action directly. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:104: void UpdateCurrentIssue(const media_router::Issue* issue); I think "SetCurrentIssue" works better here, because this function does not modify the issue object itself. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:108: void UpdateCurrentRoutes(const std::vector<media_router::MediaRoute>& routes); Suggestion: IMO it makes more sense for this to take a bool for local_active_route_exists vs. taking the vector of routes - let the caller do the computation. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:125: MediaRouterActionState GetUpdatedMediaRouterActionState(); No "updated"? https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:140: scoped_ptr<const media_router::Issue> issue_; Why have a const here? Callers will wrap this pointer with constness as needed. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:155: FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, Initialization); These go at the top of the private: section https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:35: ActionIssuesObserver::OnIssueUpdated(issue); If this method just delegates to the superclass, then you can just omit it entirely. I'm not quite sure what the purpose of these test observer classes are? https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:53: class TestMediaRouterAction : public MediaRouterAction { It would be better to test this class directly instead of subclassing it. Hooks introduced by test subclasses can obfuscate real bugs.
Patchset #3 (id:180001) has been deleted
https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:26: MediaRouterAction* action) On 2015/07/31 18:18:39, Kevin M wrote: > alignment & DCHECK the ptrs for non-nullness Done. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:72: routes_observer_.reset(); On 2015/07/31 18:18:39, Kevin M wrote: > Unnecessary Done. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:204: return web_contents ? static_cast<media_router::MediaRouterMojoImpl*>( On 2015/07/31 18:18:39, Kevin M wrote: > No need to static_cast here Done. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:20: class Issue; On 2015/07/31 18:18:40, Kevin M wrote: > Sorting Done. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:25: // Observes when there is an updated issue and passes it to |action_|. On 2015/07/31 18:18:39, Kevin M wrote: > You can create a single Observer class that implements IssuesObserver and > MediaRoutesObserver. > > Is there a reason why the MediaRouterAction can't also be an observer? The > instances are bound one-to-one with an Action object. It would simplify test > code, because you could just trigger the Observer methods on the Action > directly. Switching to single Observer class. We don't always have a MediaRouter* we can get within the entire lifetime of MediaRouterAction, but both IssuesObserver and MediaRoutesObserver require MediaRouter* to exist. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:104: void UpdateCurrentIssue(const media_router::Issue* issue); On 2015/07/31 18:18:40, Kevin M wrote: > I think "SetCurrentIssue" works better here, because this function does not > modify the issue object itself. Done. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:108: void UpdateCurrentRoutes(const std::vector<media_router::MediaRoute>& routes); On 2015/07/31 18:18:39, Kevin M wrote: > Suggestion: IMO it makes more sense for this to take a bool for > local_active_route_exists vs. taking the vector of routes - let the caller do > the computation. Done. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:125: MediaRouterActionState GetUpdatedMediaRouterActionState(); On 2015/07/31 18:18:40, Kevin M wrote: > No "updated"? Done. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:140: scoped_ptr<const media_router::Issue> issue_; On 2015/07/31 18:18:39, Kevin M wrote: > Why have a const here? Callers will wrap this pointer with constness as needed. Removed. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:155: FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, Initialization); On 2015/07/31 18:18:39, Kevin M wrote: > These go at the top of the private: section Done. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:35: ActionIssuesObserver::OnIssueUpdated(issue); On 2015/07/31 18:18:40, Kevin M wrote: > If this method just delegates to the superclass, then you can just omit it > entirely. > > I'm not quite sure what the purpose of these test observer classes are? Removed. https://codereview.chromium.org/1268553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:53: class TestMediaRouterAction : public MediaRouterAction { On 2015/07/31 18:18:40, Kevin M wrote: > It would be better to test this class directly instead of subclassing it. Hooks > introduced by test subclasses can obfuscate real bugs. I need a valid MediaRouter* so I subclassed it to set MediaRouterMojoImpl* as a mock Media Router. I'm open to alternate ways to do this.
https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:104: friend class ::TestMediaRouterAction; Is the :: needed here? https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:84: if (media_router) You can DCHECK for this, but it's a pretty sure thing that the MediaRouter is available. :) https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:25: class MediaRouterActionObserver : public media_router::IssuesObserver, Add a comment about the class' function? https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:28: MediaRouterActionObserver(media_router::MediaRouter* router, As per our discussion, the MediaRouter* will not change under a given BrowserContext. You can fold the Observer functionality into the MediaRouterAction and just share the MediaRouter* getter there. https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:137: // scoped_ptr<media_router::IssuesObserver> issues_observer_; Remove these?
Patchset #5 (id:240001) has been deleted
https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_mojo_impl.h (right): https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_mojo_impl.h:104: friend class ::TestMediaRouterAction; On 2015/08/03 17:15:37, Kevin M wrote: > Is the :: needed here? Yes, this is to specify TestMediaRouterAction isn't declared in a namespace, whereas MediaRouterMojoImpl is in media_router namespace. https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:84: if (media_router) On 2015/08/03 17:15:37, Kevin M wrote: > You can DCHECK for this, but it's a pretty sure thing that the MediaRouter is > available. :) Acknowledged. https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:25: class MediaRouterActionObserver : public media_router::IssuesObserver, On 2015/08/03 17:15:37, Kevin M wrote: > Add a comment about the class' function? Acknowledged. https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:28: MediaRouterActionObserver(media_router::MediaRouter* router, On 2015/08/03 17:15:37, Kevin M wrote: > As per our discussion, the MediaRouter* will not change under a given > BrowserContext. You can fold the Observer functionality into the > MediaRouterAction and just share the MediaRouter* getter there. Done. https://codereview.chromium.org/1268553002/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:137: // scoped_ptr<media_router::IssuesObserver> issues_observer_; On 2015/08/03 17:15:37, Kevin M wrote: > Remove these? Done.
https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:68: default: Don't add a default case when your switch handles all values of an enum. Instead put the code here outside the switch. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:137: issue_.reset(); Nit: Simpler: issue.reset(issue ? new media_router::Issue(*issue) : nullptr); https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:150: has_local_routes_ = it != routes.end(); Nit: Simpler: has_local_routes_ = std::find_if(routes.begin(), routes.end(), [](const auto& route) { return route.is_local(); }) != routes.end(); https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:165: (content::BrowserContext*) browser->profile()); C++-style casts, please https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:184: if (issue_.get()->severity() == media_router::Issue::FATAL) Don't use .get() just to call ->. Use -> directly without the .get(). (2x) https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:186: else if (issue_.get()->severity() == media_router::Issue::WARNING) No else after return https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:193: return MEDIA_ROUTER_ACTION_IDLE; Nit: Simpler: return has_local_routes_ ? MEDIA_ROUTER_ACTION_ACTIVE : MEDIA_ROUTER_ACTION_IDLE; https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:22: enum MediaRouterActionState { Why not make this public inside MediaRouterAction (so MediaRouterAction::State)? That would scope all these to that class, meaning you could remove the MEDIA_ROUTER_ACTION_ from each, and wouldn't seem to restrict the kind of usage you're doing in this CL AFAICT. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:79: FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateIssuesAndRoutes); I think in this case these friend declarations signal that parts of the tests are too low-level. They look at things like the |state_| value directly when ideally they would look at externally-visible data (e.g. the result of GetIcon()). This means that if someone modifies the implementation of the class in a way that doesn't affect the externally-visible behavior, they still might break all the tests and have to fix. In many cases, the tests access members that have direct accessors, e.g. |id_| instead of GetId(), or |name_| instead of GetActionName(). In other cases, like the |state_| case mentioned above, perhaps the tests should be looking at something other than the actual underlying |state_|. If these sorts of issues are fixed correctly, it will hopefully be possible to remove most or all of the friend and FRIEND_TEST declarations here as well as in the unittest code. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:86: // Marked virtual to use in tests. Nit: "Overridden by tests." would probably be clearer, since the test code overrides this, it doesn't call it https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:90: // |state_|. If |state_| has changed, update |state_| and then update Nit: update -> updates (2x) https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:91: // MediaRouterAction's icon. This updates |state_|, not just the icon. So either it should be called MaybeUpdateState(), or perhaps we should eliminate the concept of the "current state" entirely and deal directly in what the "current icon" is: replace |state_| with a gfx::Image* that points at one of the member icons, and replace GetMediaRouterActionState() with something like GetCurrentIcon() that returns the correct pointer. Then the implementation of the public GetIcon() method becomes trivial, and the way to update test code which looks at the state becomes obvious (it should look at the result of GetIcon() instead). https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:94: // Called when |issue_| or |local_active_route_exists_| may have changed. This comment is confusing, since it's not really part of the function's contract, and it actually better describes when people call MaybeUpdateIcon() (which in turn calls this unconditionally). Either describe what this function does instead, or eliminate the comment. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:109: // Used to determine current state of the MediaRouterAction. This doesn't actually say what |issue_| is, how it's set, etc. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:48: }; Nit: DISALLOW_COPY_AND_ASSIGN for all these classes https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:79: false, "")), Nit: std::string() in place of "" everywhere https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:136: media_router::MediaRoute fake_route_remote_; Nit: Even in test code, we're not supposed to have protected members if possible. Some ways to help implement this: * Instead of making the fakes be test members, make them local to the test function in question if used once, or if used in multiple places make a helper that returns the correct object either by value or scoped_ptr. * If helpful you could have helper functions which do things like update the routes with a local or remote route (or both, or neither; maybe a function which takes two bools or an enum or bitfield to say what to do), which would let the actual test functions below avoid locals and minimize boilerplate.
Patchset #7 (id:300001) has been deleted
Patchset #7 (id:320001) has been deleted
Patchset #7 (id:340001) has been deleted
https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:68: default: On 2015/08/03 21:28:51, Peter Kasting wrote: > Don't add a default case when your switch handles all values of an enum. > > Instead put the code here outside the switch. Acknowledged; ended up removing enum. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:137: issue_.reset(); On 2015/08/03 21:28:51, Peter Kasting wrote: > Nit: Simpler: > > issue.reset(issue ? new media_router::Issue(*issue) : nullptr); Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:150: has_local_routes_ = it != routes.end(); On 2015/08/03 21:28:51, Peter Kasting wrote: > Nit: Simpler: > > has_local_routes_ = > std::find_if(routes.begin(), routes.end(), > [](const auto& route) { return route.is_local(); }) != > routes.end(); Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:165: (content::BrowserContext*) browser->profile()); On 2015/08/03 21:28:51, Peter Kasting wrote: > C++-style casts, please Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:184: if (issue_.get()->severity() == media_router::Issue::FATAL) On 2015/08/03 21:28:51, Peter Kasting wrote: > Don't use .get() just to call ->. Use -> directly without the .get(). (2x) Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:186: else if (issue_.get()->severity() == media_router::Issue::WARNING) On 2015/08/03 21:28:51, Peter Kasting wrote: > No else after return Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:193: return MEDIA_ROUTER_ACTION_IDLE; On 2015/08/03 21:28:51, Peter Kasting wrote: > Nit: Simpler: > > return has_local_routes_ ? > MEDIA_ROUTER_ACTION_ACTIVE : MEDIA_ROUTER_ACTION_IDLE; Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:22: enum MediaRouterActionState { On 2015/08/03 21:28:51, Peter Kasting wrote: > Why not make this public inside MediaRouterAction (so MediaRouterAction::State)? > That would scope all these to that class, meaning you could remove the > MEDIA_ROUTER_ACTION_ from each, and wouldn't seem to restrict the kind of usage > you're doing in this CL AFAICT. Acknowledged; ended up removing the enum. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:79: FRIEND_TEST_ALL_PREFIXES(MediaRouterActionUnitTest, UpdateIssuesAndRoutes); On 2015/08/03 21:28:51, Peter Kasting wrote: > I think in this case these friend declarations signal that parts of the tests > are too low-level. They look at things like the |state_| value directly when > ideally they would look at externally-visible data (e.g. the result of > GetIcon()). This means that if someone modifies the implementation of the class > in a way that doesn't affect the externally-visible behavior, they still might > break all the tests and have to fix. > > In many cases, the tests access members that have direct accessors, e.g. |id_| > instead of GetId(), or |name_| instead of GetActionName(). In other cases, like > the |state_| case mentioned above, perhaps the tests should be looking at > something other than the actual underlying |state_|. > > If these sorts of issues are fixed correctly, it will hopefully be possible to > remove most or all of the friend and FRIEND_TEST declarations here as well as in > the unittest code. Thanks for bringing this up; from the changes (based on this and other comments), I ended up removing all the friend class/test related declarations. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:86: // Marked virtual to use in tests. On 2015/08/03 21:28:51, Peter Kasting wrote: > Nit: "Overridden by tests." would probably be clearer, since the test code > overrides this, it doesn't call it Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:90: // |state_|. If |state_| has changed, update |state_| and then update On 2015/08/03 21:28:51, Peter Kasting wrote: > Nit: update -> updates (2x) Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:91: // MediaRouterAction's icon. On 2015/08/03 21:28:51, Peter Kasting wrote: > This updates |state_|, not just the icon. So either it should be called > MaybeUpdateState(), or perhaps we should eliminate the concept of the "current > state" entirely and deal directly in what the "current icon" is: replace > |state_| with a gfx::Image* that points at one of the member icons, and replace > GetMediaRouterActionState() with something like GetCurrentIcon() that returns > the correct pointer. Then the implementation of the public GetIcon() method > becomes trivial, and the way to update test code which looks at the state > becomes obvious (it should look at the result of GetIcon() instead). Switched to keeping track of the current icon rather than a state. We won't be using the state for anything else. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:94: // Called when |issue_| or |local_active_route_exists_| may have changed. On 2015/08/03 21:28:51, Peter Kasting wrote: > This comment is confusing, since it's not really part of the function's > contract, and it actually better describes when people call MaybeUpdateIcon() > (which in turn calls this unconditionally). > > Either describe what this function does instead, or eliminate the comment. Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:109: // Used to determine current state of the MediaRouterAction. On 2015/08/03 21:28:51, Peter Kasting wrote: > This doesn't actually say what |issue_| is, how it's set, etc. Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:48: }; On 2015/08/03 21:28:51, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN for all these classes Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:79: false, "")), On 2015/08/03 21:28:51, Peter Kasting wrote: > Nit: std::string() in place of "" everywhere Done. https://codereview.chromium.org/1268553002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:136: media_router::MediaRoute fake_route_remote_; On 2015/08/03 21:28:51, Peter Kasting wrote: > Nit: Even in test code, we're not supposed to have protected members if > possible. > > Some ways to help implement this: > > * Instead of making the fakes be test members, make them local to the test > function in question if used once, or if used in multiple places make a helper > that returns the correct object either by value or scoped_ptr. > * If helpful you could have helper functions which do things like update the > routes with a local or remote route (or both, or neither; maybe a function which > takes two bools or an enum or bitfield to say what to do), which would let the > actual test functions below avoid locals and minimize boilerplate. Thanks for the info! Added helpers for these.
https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:164: gfx::Image* MediaRouterAction::GetCurrentIcon() { Consider returning a const reference if this can't return null. https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:71: gfx::Image* GetCurrentIcon(); const https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:71: gfx::Image* GetCurrentIcon(); Mark this function as const. (inspired by a miu code review; he luvs him some constses) https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:77: gfx::Image media_router_active_icon_; Make these const? https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:91: gfx::Image* current_icon_; const ref? https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:226: routes = { fake_route_remote() }; This type of assignment isn't allowed :( see "(Uniform) Initialization Syntax" in https://chromium-cpp.appspot.com/
https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:164: gfx::Image* MediaRouterAction::GetCurrentIcon() { On 2015/08/05 20:20:27, Kevin M wrote: > Consider returning a const reference if this can't return null. Done. https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:71: gfx::Image* GetCurrentIcon(); On 2015/08/05 20:20:27, Kevin M wrote: > const Done. https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:71: gfx::Image* GetCurrentIcon(); On 2015/08/05 20:20:27, Kevin M wrote: > Mark this function as const. > > (inspired by a miu code review; he luvs him some constses) Done. https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:77: gfx::Image media_router_active_icon_; On 2015/08/05 20:20:27, Kevin M wrote: > Make these const? Done. https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:91: gfx::Image* current_icon_; On 2015/08/05 20:20:27, Kevin M wrote: > const ref? Talked offline, const pointer instead. https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/1268553002/diff/360001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:226: routes = { fake_route_remote() }; On 2015/08/05 20:20:28, Kevin M wrote: > This type of assignment isn't allowed :( > > see "(Uniform) Initialization Syntax" in https://chromium-cpp.appspot.com/ Updated. Thanks!
LGTM https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:40: issue_(nullptr), Nit: No need for this line https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:134: != routes.end(); Nit: != should be on end of previous line https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:155: if (&new_icon != current_icon_) { Nit: It seems arguably better if GetCurrentIcon() returned a const gfx::Image*, as that's really how this method wants to consume its output. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:158: // Update the icon to reflect the new icon. Nit: Maybe clearer: "Tell the associated view to update its icon to reflect the change made above." https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:68: // MediaRouterAction's icon. This comment is out of date. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:84: // The possible icons and their behaviors are: You might want to put the icon descriptions above the actual |_icon_| members. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:88: // device. Tiny nit: Indent 2 more https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:94: // in |OnIssueUpdated|, which is called by the IssueManager. Nit: Use () rather than || to denote function names https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:97: // Whether there exists a local active route. Nit: Whether a local active route exists. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:98: bool has_local_routes_; Tiny nit: If this is true if "a route" exists, should it be |has_local_route_|? https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:17: : fake_issue_notification_(media_router::Issue("title notification", Nit: All lines of args must be aligned (so, probably, break before "title notification") (several places) https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:122: // Don't update |state_| since the issue is only a notification. All these comments about |state_| are out of date.
lgtm
https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:40: issue_(nullptr), On 2015/08/07 17:49:27, Peter Kasting wrote: > Nit: No need for this line Done. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:134: != routes.end(); On 2015/08/07 17:49:27, Peter Kasting wrote: > Nit: != should be on end of previous line Done. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:155: if (&new_icon != current_icon_) { On 2015/08/07 17:49:27, Peter Kasting wrote: > Nit: It seems arguably better if GetCurrentIcon() returned a const gfx::Image*, > as that's really how this method wants to consume its output. Switched back to returning const gfx::Image*. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:158: // Update the icon to reflect the new icon. On 2015/08/07 17:49:27, Peter Kasting wrote: > Nit: Maybe clearer: "Tell the associated view to update its icon to reflect the > change made above." Done. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:68: // MediaRouterAction's icon. On 2015/08/07 17:49:28, Peter Kasting wrote: > This comment is out of date. Updated. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:84: // The possible icons and their behaviors are: On 2015/08/07 17:49:27, Peter Kasting wrote: > You might want to put the icon descriptions above the actual |_icon_| members. Done. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:88: // device. On 2015/08/07 17:49:27, Peter Kasting wrote: > Tiny nit: Indent 2 more Acknowledged. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:94: // in |OnIssueUpdated|, which is called by the IssueManager. On 2015/08/07 17:49:28, Peter Kasting wrote: > Nit: Use () rather than || to denote function names Done. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:97: // Whether there exists a local active route. On 2015/08/07 17:49:27, Peter Kasting wrote: > Nit: Whether a local active route exists. Done. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:98: bool has_local_routes_; On 2015/08/07 17:49:27, Peter Kasting wrote: > Tiny nit: If this is true if "a route" exists, should it be |has_local_route_|? Done. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:17: : fake_issue_notification_(media_router::Issue("title notification", On 2015/08/07 17:49:28, Peter Kasting wrote: > Nit: All lines of args must be aligned (so, probably, break before "title > notification") (several places) Done. https://codereview.chromium.org/1268553002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:122: // Don't update |state_| since the issue is only a notification. On 2015/08/07 17:49:28, Peter Kasting wrote: > All these comments about |state_| are out of date. Done.
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, pkasting@chromium.org, kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/1268553002/#ps440001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268553002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268553002/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #12 (id:450001) has been deleted
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, pkasting@chromium.org, kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/1268553002/#ps470001 (title: "Fix tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268553002/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268553002/470001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #13 (id:490001) has been deleted
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, pkasting@chromium.org, kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/1268553002/#ps510001 (title: "Fix tests (lsan).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268553002/510001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268553002/510001
Message was sent while issue was closed.
Committed patchset #13 (id:510001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/4eebb13efbe6b3304bc501da6331e64e05d716f9 Cr-Commit-Position: refs/heads/master@{#342841} |
