Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1118613003:
Add UI for cast system tray integration (Closed)
|
Created:
5 years, 7 months ago by jdufault Modified:
5 years, 7 months ago CC:
chromium-reviews, kalyank, stevenjb+watch_chromium.org, sadrul, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UI for cast system tray integration
- TrayCast is the central injection point for the UI code
- TrayCast itself just delegates between two other views, of which only one is active at a time depending on if the device is currently casting
- ScreenCaptureTrayItem is disabled so it will not be active when the device is casting (otherwise there would be two very similar tray items that have the same functionality but slightly different appearance)
BUG=433140
Committed: https://crrev.com/ba5bc09201e5cfa59fc392b87fb184a77b587654
Cr-Commit-Position: refs/heads/master@{#330398}
Patch Set 1 #
Total comments: 58
Patch Set 2 : Show different active cast titles based on if we are casting a tab or the desktop #
Total comments: 1
Patch Set 3 : Remove accidental comment #
Total comments: 49
Patch Set 4 : Address changes #
Total comments: 10
Patch Set 5 : Fix comment #Patch Set 6 : Fix style issues #Patch Set 7 : Fix bad fix #Patch Set 8 : Fix broken unit tests #
Messages
Total messages: 30 (9 generated)
jdufault@chromium.org changed reviewers: + achuith@chromium.org
https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.cc File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:47: auto activity = item.second.activity; Let's spell out the type of activity here https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:48: if (activity.allow_stop && activity.id.empty() == false) { Don't need {} here https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:55: auto cast_config = Let's spell out the type of cast_config here https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:63: class CastSelectDefaultView : public TrayItemMore { Class comment here https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:70: void UpdateLabel(); Function comment for all public functions https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:80: class CastCastView : public views::View, public views::ButtonListener { Class comment https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:82: CastCastView(CastConfigDelegate* cast_config_delegate); explicit https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:85: void UpdateLabel(); Comment https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:134: auto stop_button_text = Spell out the type here https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:164: // Center the label Period at the end of comment. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:182: auto receiver = i.second.receiver; Spell out the types of receiver and activity here. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:184: if (activity.id.empty() == false) { if (!activity.id.empty()) is a more common construct https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:188: unnecessary newline. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:190: return; You don't need this return https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:201: class CastDuplexView : public views::View { class comment. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:215: // Overridden from views::View. Make this private https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:261: class CastTrayView : public TrayItemView { class comment https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:265: void UpdateAlignment(ShelfAlignment alignment); Add new line and function comment https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:275: auto& rb = ui::ResourceBundle::GetSharedInstance(); Spell out type of rb https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:300: CastSelectDefaultView::CastSelectDefaultView( Shouldn't this be right after the class definition of CastSelectDefaultView? https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:341: class CastDetailedView : public TrayDetailsView, public ViewClickListener { class comment https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:348: void Update(); function comment https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:425: // Remove all of the existing views period at end of comment. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:429: // Add a view for each receiver same https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:433: auto container = AddReceiverToList(receiver_activity); spell out type https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:443: auto container = new HoverHighlightView(this); spell out type https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:449: auto& name = receiverActivity.receiver.name; spell out type https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:459: bool userAddingRunning = Shell::GetInstance() const bool https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.h File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.h... ash/system/cast/tray_cast.h:29: // Overridden from CastObserver. Make this private
https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.cc File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:47: auto activity = item.second.activity; On 2015/04/30 00:03:31, achuithb wrote: > Let's spell out the type of activity here Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:48: if (activity.allow_stop && activity.id.empty() == false) { On 2015/04/30 00:03:32, achuithb wrote: > Don't need {} here Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:55: auto cast_config = On 2015/04/30 00:03:32, achuithb wrote: > Let's spell out the type of cast_config here Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:63: class CastSelectDefaultView : public TrayItemMore { On 2015/04/30 00:03:31, achuithb wrote: > Class comment here Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:70: void UpdateLabel(); On 2015/04/30 00:03:32, achuithb wrote: > Function comment for all public functions Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:80: class CastCastView : public views::View, public views::ButtonListener { On 2015/04/30 00:03:32, achuithb wrote: > Class comment Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:82: CastCastView(CastConfigDelegate* cast_config_delegate); On 2015/04/30 00:03:32, achuithb wrote: > explicit Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:85: void UpdateLabel(); On 2015/04/30 00:03:32, achuithb wrote: > Comment Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:134: auto stop_button_text = On 2015/04/30 00:03:32, achuithb wrote: > Spell out the type here Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:164: // Center the label On 2015/04/30 00:03:31, achuithb wrote: > Period at the end of comment. Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:182: auto receiver = i.second.receiver; On 2015/04/30 00:03:32, achuithb wrote: > Spell out the types of receiver and activity here. Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:184: if (activity.id.empty() == false) { On 2015/04/30 00:03:32, achuithb wrote: > if (!activity.id.empty()) is a more common construct Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:188: On 2015/04/30 00:03:32, achuithb wrote: > unnecessary newline. Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:190: return; On 2015/04/30 00:03:32, achuithb wrote: > You don't need this return Changed to break; it expresses intent that we only look at the first receiver. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:201: class CastDuplexView : public views::View { On 2015/04/30 00:03:32, achuithb wrote: > class comment. Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:215: // Overridden from views::View. On 2015/04/30 00:03:32, achuithb wrote: > Make this private Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:261: class CastTrayView : public TrayItemView { On 2015/04/30 00:03:32, achuithb wrote: > class comment Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:265: void UpdateAlignment(ShelfAlignment alignment); On 2015/04/30 00:03:31, achuithb wrote: > Add new line and function comment Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:275: auto& rb = ui::ResourceBundle::GetSharedInstance(); On 2015/04/30 00:03:32, achuithb wrote: > Spell out type of rb Removed |rb|, folded into the call https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:300: CastSelectDefaultView::CastSelectDefaultView( On 2015/04/30 00:03:31, achuithb wrote: > Shouldn't this be right after the class definition of CastSelectDefaultView? Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:341: class CastDetailedView : public TrayDetailsView, public ViewClickListener { On 2015/04/30 00:03:32, achuithb wrote: > class comment Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:348: void Update(); On 2015/04/30 00:03:32, achuithb wrote: > function comment Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:425: // Remove all of the existing views On 2015/04/30 00:03:32, achuithb wrote: > period at end of comment. Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:429: // Add a view for each receiver On 2015/04/30 00:03:32, achuithb wrote: > same Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:433: auto container = AddReceiverToList(receiver_activity); On 2015/04/30 00:03:31, achuithb wrote: > spell out type Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:443: auto container = new HoverHighlightView(this); On 2015/04/30 00:03:32, achuithb wrote: > spell out type It is within the line, clearly HoverHighlightView https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:449: auto& name = receiverActivity.receiver.name; On 2015/04/30 00:03:32, achuithb wrote: > spell out type Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:459: bool userAddingRunning = Shell::GetInstance() On 2015/04/30 00:03:32, achuithb wrote: > const bool Done. https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.h File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1118613003/diff/1/ash/system/cast/tray_cast.h... ash/system/cast/tray_cast.h:29: // Overridden from CastObserver. On 2015/04/30 00:03:33, achuithb wrote: > Make this private Done.
https://codereview.chromium.org/1118613003/diff/20001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/20001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:624: // This view acts as the interface between the system tray and Oops, this shouldn't have been added. I'll delete it.
Please flesh out the description to describe everything that's going on with this CL https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:36: namespace { Move this anonymous scope below into namespace ash::tray https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:43: static void StopCastCallback( fn comment Also, move these functions to anonymous scope rather than make them static. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:53: static void StopCast() { fn comment https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:91: auto& rb = ui::ResourceBundle::GetSharedInstance(); Do not use auto here https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:97: // TODO(jdufault): What should the default label be? Did you resolve this TODO? https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:164: icon_ = new FixedSizedImageView(0, kTrayPopupItemHeight); Perhaps add a few comments in this ctor since it's long and doing a few different things. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:218: auto extra_height = height() - label_container_->GetPreferredSize().height(); Don't use auto here https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:234: CastConfigDelegate::Receiver receiver = i.second.receiver; const https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:235: CastConfigDelegate::Activity activity = i.second.activity; const https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:236: if (!activity.id.empty()) { Add a comment about what you're displaying https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:378: void Update(); Maybe call this GetReceiversAndActivities? The comment isn't quite accurate. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:390: // Add settings entries. Don't think this comment adds much value https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:392: // Overridden from ViewClickListener. newline before this https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:473: auto container = new HoverHighlightView(this); Remove auto here https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:486: // Add settings entries. Don't need this comment https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:515: // TODO(jdufault): Should this cast the desktop instead of showing a remove this TODO I think this should be handled by CastToReceiver and the js rather than here. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:518: // TODO(jdufault): Should we transition to the default view? Remove this TODO. We can address it based on feedback. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:543: void TrayCast::OnCastRefresh() { Is this in use? https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:18: } // namespace tray https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:47: void UpdatePrimaryView(); Fn comment https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:49: bool is_casting_; default https://codereview.chromium.org/1118613003/diff/40001/ash/system/chromeos/scr... File ash/system/chromeos/screen_security/screen_capture_tray_item.cc (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/chromeos/scr... ash/system/chromeos/screen_security/screen_capture_tray_item.cc:90: // TODO(jdufault): Does this break casting and capturing the screen at the Remove TODO? https://codereview.chromium.org/1118613003/diff/40001/ash/system/chromeos/scr... File ash/system/chromeos/screen_security/screen_capture_tray_item.h (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/chromeos/scr... ash/system/chromeos/screen_security/screen_capture_tray_item.h:44: bool is_casting_; default https://codereview.chromium.org/1118613003/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray_item.h (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray_item.h:38: // Returns a view for the item to be displayed in the popup list after Pull this file out into a separate CL
https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:36: namespace { On 2015/05/06 20:02:32, achuithb wrote: > Move this anonymous scope below into namespace ash::tray Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:43: static void StopCastCallback( On 2015/05/06 20:02:32, achuithb wrote: > fn comment > > Also, move these functions to anonymous scope rather than make them static. Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:53: static void StopCast() { On 2015/05/06 20:02:31, achuithb wrote: > fn comment Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:91: auto& rb = ui::ResourceBundle::GetSharedInstance(); On 2015/05/06 20:02:32, achuithb wrote: > Do not use auto here Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:97: // TODO(jdufault): What should the default label be? On 2015/05/06 20:02:32, achuithb wrote: > Did you resolve this TODO? It was more of a marker to think about what the default should be and NO_DEVICE makes sense; I removed the TODO. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:164: icon_ = new FixedSizedImageView(0, kTrayPopupItemHeight); On 2015/05/06 20:02:31, achuithb wrote: > Perhaps add a few comments in this ctor since it's long and doing a few > different things. Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:218: auto extra_height = height() - label_container_->GetPreferredSize().height(); On 2015/05/06 20:02:31, achuithb wrote: > Don't use auto here Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:234: CastConfigDelegate::Receiver receiver = i.second.receiver; On 2015/05/06 20:02:32, achuithb wrote: > const Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:235: CastConfigDelegate::Activity activity = i.second.activity; On 2015/05/06 20:02:31, achuithb wrote: > const Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:236: if (!activity.id.empty()) { On 2015/05/06 20:02:32, achuithb wrote: > Add a comment about what you're displaying Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:378: void Update(); On 2015/05/06 20:02:32, achuithb wrote: > Maybe call this GetReceiversAndActivities? The comment isn't quite accurate. I've renamed/reorganized Update, UpdateCastReceiverList, UpdateReceiverScrollList, and AddReceiverToList https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:390: // Add settings entries. On 2015/05/06 20:02:31, achuithb wrote: > Don't think this comment adds much value Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:392: // Overridden from ViewClickListener. On 2015/05/06 20:02:31, achuithb wrote: > newline before this Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:473: auto container = new HoverHighlightView(this); On 2015/05/06 20:02:31, achuithb wrote: > Remove auto here It is very clearly HoverHighlightView https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:486: // Add settings entries. On 2015/05/06 20:02:31, achuithb wrote: > Don't need this comment Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:515: // TODO(jdufault): Should this cast the desktop instead of showing a On 2015/05/06 20:02:32, achuithb wrote: > remove this TODO > > I think this should be handled by CastToReceiver and the js rather than here. Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:518: // TODO(jdufault): Should we transition to the default view? On 2015/05/06 20:02:31, achuithb wrote: > Remove this TODO. We can address it based on feedback. It currently "reverts" to the default view since clicking anywhere else will hide the tray - doing the transition just makes the behavior more explicit. Removed the comment. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:543: void TrayCast::OnCastRefresh() { On 2015/05/06 20:02:32, achuithb wrote: > Is this in use? From CastObserver, but it never got called. Removed https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:18: } On 2015/05/06 20:02:32, achuithb wrote: > // namespace tray Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:47: void UpdatePrimaryView(); On 2015/05/06 20:02:32, achuithb wrote: > Fn comment Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:49: bool is_casting_; On 2015/05/06 20:02:32, achuithb wrote: > default Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/chromeos/scr... File ash/system/chromeos/screen_security/screen_capture_tray_item.cc (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/chromeos/scr... ash/system/chromeos/screen_security/screen_capture_tray_item.cc:90: // TODO(jdufault): Does this break casting and capturing the screen at the On 2015/05/06 20:02:32, achuithb wrote: > Remove TODO? Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/chromeos/scr... File ash/system/chromeos/screen_security/screen_capture_tray_item.h (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/chromeos/scr... ash/system/chromeos/screen_security/screen_capture_tray_item.h:44: bool is_casting_; On 2015/05/06 20:02:32, achuithb wrote: > default Done. https://codereview.chromium.org/1118613003/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray_item.h (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray_item.h:38: // Returns a view for the item to be displayed in the popup list after On 2015/05/06 20:02:32, achuithb wrote: > Pull this file out into a separate CL Done.
https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:473: auto container = new HoverHighlightView(this); On 2015/05/06 21:17:31, jdufault wrote: > On 2015/05/06 20:02:31, achuithb wrote: > > Remove auto here > > It is very clearly HoverHighlightView auto instead of HoverHighlightView* doesn't seem like a worthy saving imo, but ok.
Thanks for taking care of the feedback. Please let me know when you've had a chance to update the description - Jenny has agreed to take a look at the CLs. https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:52: // Invoking this function will cause the device to stop casting. Maybe // Stops currently casting device.
https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:52: // Invoking this function will cause the device to stop casting. On 2015/05/06 21:28:15, achuithb wrote: > Maybe > // Stops currently casting device. Done.
jdufault@chromium.org changed reviewers: + jennyz@chromium.org
On 2015/05/06 22:26:16, jdufault wrote: > https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... > File ash/system/cast/tray_cast.cc (right): > > https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... > ash/system/cast/tray_cast.cc:52: // Invoking this function will cause the device > to stop casting. > On 2015/05/06 21:28:15, achuithb wrote: > > Maybe > > // Stops currently casting device. > > Done. Ping jennyz
LGTM with nits. https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:482: auto container = new HoverHighlightView(this); nit: Prefer using explicit type HoverHighlightView* instead of auto, like line 506 does. At least, you should be consistent through. https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:597: if (is_casting_ == false) { nit, only one line in bracket, don't need "{...}". https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:604: if (tray_) { ditto.
https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:482: auto container = new HoverHighlightView(this); On 2015/05/12 21:10:21, jennyz wrote: > nit: Prefer using explicit type HoverHighlightView* instead of auto, like line > 506 does. At least, you should be consistent through. +1 https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:597: if (is_casting_ == false) { On 2015/05/12 21:10:21, jennyz wrote: > nit, only one line in bracket, don't need "{...}". Thanks for pointing this out - I glossed over it. Actually, let's re-write this as: if (is_casting_) default_->ActivateCastView(); else default_->ActivateSelectView();
https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:482: auto container = new HoverHighlightView(this); On 2015/05/12 21:13:39, achuithb wrote: > On 2015/05/12 21:10:21, jennyz wrote: > > nit: Prefer using explicit type HoverHighlightView* instead of auto, like line > > 506 does. At least, you should be consistent through. > > +1 Done. https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:597: if (is_casting_ == false) { On 2015/05/12 21:13:39, achuithb wrote: > On 2015/05/12 21:10:21, jennyz wrote: > > nit, only one line in bracket, don't need "{...}". > > Thanks for pointing this out - I glossed over it. > > Actually, let's re-write this as: > > if (is_casting_) > default_->ActivateCastView(); > else > default_->ActivateSelectView(); Done. https://codereview.chromium.org/1118613003/diff/60001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:604: if (tray_) { On 2015/05/12 21:10:21, jennyz wrote: > ditto. Done.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jennyz@chromium.org Link to the patchset: https://codereview.chromium.org/1118613003/#ps120001 (title: "Fix bad fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118613003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jdufault@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118613003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jennyz@chromium.org Link to the patchset: https://codereview.chromium.org/1118613003/#ps140001 (title: "Fix broken unit tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118613003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ba5bc09201e5cfa59fc392b87fb184a77b587654 Cr-Commit-Position: refs/heads/master@{#330398} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
