Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2147143002:
[Chrome OS MD] Draw a 1px separator between 2 tray items (Closed)
DescriptionDraw a 1px(hardware pixel) separator between 2 tray items
When the shelf is semi-transparent or opaque, draw a 1*32px separator line
between 2 tray items except before and after the logout button (currently,
it is off by default, behind --ash-md=experimental). After tray's visibility
changes, OnTrayVisibilityChanged in StatusAreaWidget will be called to compute
the visibility of the separator accordingly.
TEST=manual:
- Change device screen factor to different number, check if the separator width
stays at 1px and separator height is updated proportionally.
- Check if the separator shows if and only if when shelf is semi-transparent
(by open a window overlapping with the shelf) or opaque (by open a maximized
window).
- Change shelf alignment from bottom to left to right, check if the separator
updates its orientation.
- Change the visibility of logout button by switching between the public account
and the personal account, check if the separator hides both before and after
the logout button if it is visible and separator hide when it is not visible.
BUG=605993
Committed: https://crrev.com/6cf3f3eac174b448c518483ddbc1ae7dc47b2b2e
Cr-Commit-Position: refs/heads/master@{#413220}
Patch Set 1 #Patch Set 2 : merge #
Total comments: 58
Patch Set 3 : new approach #
Total comments: 1
Patch Set 4 : update class names #Patch Set 5 : nits #Patch Set 6 : update observer #Patch Set 7 : merge #
Total comments: 66
Patch Set 8 : address comments #
Total comments: 38
Patch Set 9 : merge conflicts + comments #
Total comments: 30
Patch Set 10 : fix ways to calculate x position for separator + fix comments #
Total comments: 20
Patch Set 11 : address nits #Messages
Total messages: 93 (63 generated)
Description was changed from ========== implemented BUG= ========== to ========== Draw a 1dp separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md). TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent or opaque. - Change shelf alignment from bottom to left to right, check it the separator updates its orientation. - Change the visibility of logout button, check if the separator hides both before and after the button. BUG=605993 ==========
yiyix@chromium.org changed reviewers: + varkha@chromium.org
@varkha, Could you please take look?
The CQ bit was checked by yiyix@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 yiyix@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.
In the CL description: behind --ash-md --> behind --ash-md=experimental Clarify 1px (1 dp or 1 hardware pixel) Clarify how to make shelf transparent "check it the separator" -> "check if the separator" Clarify how to change logout button visibility The separator looks very dim and barely visible on black background (see also comment about this in code). Side issue: Feels that the dimmer (lighting up / shading down the main shelf on mouse hover) should also affect the status tray. Maybe raise this with sgabriel@. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:121: // requires in front of Logout Button because it is red and appealing by How about: Paints a line to separate from the subsequent tray items. No leading separator is required before the Logout button because that button's red background is distinctive on its own. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:21: namespace ash { Don't need it second time. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:34: // Calls TrayBackgroundView::Initialize() and save a pointer to logout_button. nit: How about // Sets |logout_button_tray_| to |logout_button_tray| and calls TrayBackgroundView::Initialize(). https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:35: void InitializeTrayItems(LogoutButtonTray* logout_button_tray); Hmm. Not sure if you need this method. Maybe simply add a set_logout_button_tray() and call it where you would normally call Initialize (before or after because as far as I can tell OnPaint() won't get called until later). https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... File ash/common/system/overview/overview_button_tray.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... ash/common/system/overview/overview_button_tray.cc:36: // Padding used to adjust the user-visible size of overview dark background. nit: s/overview/overview tray https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... ash/common/system/overview/overview_button_tray.cc:117: AdjustOverviewBackground(); Do you need to call those 2 methods that create borders here in MD case given that the padding is not dependent on |alignment|? https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... ash/common/system/overview/overview_button_tray.cc:144: tray_container()->SetBorder(views::Border::CreateEmptyBorder( nit: simpler to use gfx::Insets(kBackgroundTrayPadding). Bonus points for fixing that elsewhere in this file (there is a Insets ctor that takes just 2 values so no need to repeat all 4). https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... File ash/common/system/overview/overview_button_tray.h (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... ash/common/system/overview/overview_button_tray.h:60: // Additional padding are used to adjust the user-visible size of overview nit: s/are/is More importantly the comment should say what the method does so how about: Sets an empty border around the overview tray to change user-visible size of the dark background. Doesn't do anything in non-MD. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... ash/common/system/overview/overview_button_tray.h:62: void AdjustOverviewBackground(); Maybe SetOverviewTrayBorder() ? https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... ash/common/system/status_area_widget.cc:70: virtual_keyboard_tray_->InitializeTrayItems(logout_button_tray_); See my comment about VKT::InitializeTrayIcon. Maybe just call vkt_->Initialize() here and then vkt_->set_logout_button_tray(logout_button_tray_) https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... ash/common/system/status_area_widget.cc:89: delete logout_button_tray_; nit: in a separate CL can you maybe make those owned items use auto pointers to manage lifetime with less code? https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... ash/common/system/status_area_widget.cc:90: logout_button_tray_ = NULL; Is the changed order significant? https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... File ash/common/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... ash/common/system/status_area_widget_delegate.cc:15: #include "ash/common/system/tray/tray_background_view.h" Do you still need this? https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/system_tray.cc:77: const int kBackgroundTrayPadding = 3; Should this live in a common place? https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/system_tray.cc:615: nit: no ws. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/system_tray.cc:617: AdjustStatusTrayBackground(); Same questions / comments as in overview_button_tray. Do you need to call this every time alignment changes in MD given the padding is same on all sides? https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.cc:382: } else { nit: no need for 'else' with early return above. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.cc:528: MaterialDesignController::IsShelfMaterial()) { How about if (!MaterialDesignController::IsShelfMaterial() || shelf()->GetBackgroundType() == ShelfBackgroundType::SHELF_BACKGROUND_DEFAULT) { return; } instead? https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.cc:535: const int height = GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY); If you move this up you can use |height| instead of calling GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY). I would move both width and height above together. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.cc:542: gfx::Rect bounds = horizontal_shelf ? gfx::Rect(x, y, width, height) nit: const. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.h:27: // his shown/hidden. It also inherits from ActionableView so that the tray nit: s/his/is https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.h:139: // In the given |canvas|, draw a 1px wide and 32dp high separator line to the nit: s/draw/draws nit: s/high/tall https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.h:140: // right of the tray item. nit: clarify which tray item. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.h:154: // For Material Design, some extra padding is added to tray items so there is nit: drop "some" https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.h:157: // separator needed. No border is set for non-MD. nit: s/separator/a separator is https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/tray_constants.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_constants.cc:88: const SkColor kSeparatorColor = SkColorSetA(SK_ColorWHITE, 0x29); Opacity seems quite low (16%?) so it is barely visible on black background on hi-dpi screens. I may want to look at it on a device. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/tray_constants.h (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_constants.h:78: // Separator is line of 1px wide and it is used to separate 2 tray items in the How about: // Color and width of a line used to separate tray items in the shelf. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/web_n... File ash/common/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/web_n... ash/common/system/web_notification/web_notification_tray.cc:523: // Paint a 1px separator to separate the notification button and status tray. nit: I think this comment is not necessary. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/web_n... File ash/common/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/web_n... ash/common/system/web_notification/web_notification_tray.h:173: I think it would be better to make it protected because you are hiding it from possible overrides in subclasses. That or move down since overrides should be the last methods declared (a pattern I observe even though I could not find it mentioned in the style guide).
@varkha, could you please look at the new approach. Please find the design doc here: https://docs.google.com/document/d/1oDtCqwSJdaBtAfBWcQOjKnHZ6o_ysd4LlDI-vl4uj... https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:121: // requires in front of Logout Button because it is red and appealing by On 2016/07/14 21:34:46, varkha wrote: > How about: > Paints a line to separate from the subsequent tray items. No leading separator > is required before the Logout button because that button's red background is > distinctive on its own. No need to emphasize that the separator is 1px? I like your wording better thou. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:21: namespace ash { On 2016/07/14 21:34:47, varkha wrote: > Don't need it second time. Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:34: // Calls TrayBackgroundView::Initialize() and save a pointer to logout_button. On 2016/07/14 21:34:47, varkha wrote: > nit: How about > // Sets |logout_button_tray_| to |logout_button_tray| and calls > TrayBackgroundView::Initialize(). As we are using the new approach. we don't need to modify the initialize method. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:35: void InitializeTrayItems(LogoutButtonTray* logout_button_tray); On 2016/07/14 21:34:47, varkha wrote: > Hmm. Not sure if you need this method. Maybe simply add a > set_logout_button_tray() and call it where you would normally call Initialize > (before or after because as far as I can tell OnPaint() won't get called until > later). Per our discussion offline, I will use observer design pattern to avoid that one tray knows about another tray. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... File ash/common/system/overview/overview_button_tray.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... ash/common/system/overview/overview_button_tray.cc:36: // Padding used to adjust the user-visible size of overview dark background. On 2016/07/14 21:34:47, varkha wrote: > nit: s/overview/overview tray Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... ash/common/system/overview/overview_button_tray.cc:117: AdjustOverviewBackground(); On 2016/07/14 21:34:47, varkha wrote: > Do you need to call those 2 methods that create borders here in MD case given > that the padding is not dependent on |alignment|? true, I will add a condition. thanks https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... ash/common/system/overview/overview_button_tray.cc:144: tray_container()->SetBorder(views::Border::CreateEmptyBorder( On 2016/07/14 21:34:47, varkha wrote: > nit: simpler to use gfx::Insets(kBackgroundTrayPadding). > Bonus points for fixing that elsewhere in this file (there is a Insets ctor that > takes just 2 values so no need to repeat all 4). Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... File ash/common/system/overview/overview_button_tray.h (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... ash/common/system/overview/overview_button_tray.h:60: // Additional padding are used to adjust the user-visible size of overview On 2016/07/14 21:34:47, varkha wrote: > nit: s/are/is > More importantly the comment should say what the method does so how about: > Sets an empty border around the overview tray to change user-visible size of the > dark background. Doesn't do anything in non-MD. Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/overv... ash/common/system/overview/overview_button_tray.h:62: void AdjustOverviewBackground(); On 2016/07/14 21:34:47, varkha wrote: > Maybe SetOverviewTrayBorder() ? Sure, SetOverviewTrayBorder() is more about what the function does. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... ash/common/system/status_area_widget.cc:70: virtual_keyboard_tray_->InitializeTrayItems(logout_button_tray_); On 2016/07/14 21:34:47, varkha wrote: > See my comment about VKT::InitializeTrayIcon. Maybe just call vkt_->Initialize() > here and then vkt_->set_logout_button_tray(logout_button_tray_) N/A to the new approach https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... ash/common/system/status_area_widget.cc:89: delete logout_button_tray_; On 2016/07/14 21:34:47, varkha wrote: > nit: in a separate CL can you maybe make those owned items use auto pointers to > manage lifetime with less code? N/A to the new approach https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... ash/common/system/status_area_widget.cc:90: logout_button_tray_ = NULL; On 2016/07/14 21:34:47, varkha wrote: > Is the changed order significant? N/A to the new approach https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... File ash/common/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/statu... ash/common/system/status_area_widget_delegate.cc:15: #include "ash/common/system/tray/tray_background_view.h" On 2016/07/14 21:34:47, varkha wrote: > Do you still need this? Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/system_tray.cc:77: const int kBackgroundTrayPadding = 3; On 2016/07/14 21:34:47, varkha wrote: > Should this live in a common place? I was thinking that the overview button padding could be different from the status tray padding. They should be independent. So I choose to list these 2 constant in the status_tray.cc and overview_button_tray.cc file separately. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/system_tray.cc:615: On 2016/07/14 21:34:47, varkha wrote: > nit: no ws. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/system_tray.cc:617: AdjustStatusTrayBackground(); On 2016/07/14 21:34:47, varkha wrote: > Same questions / comments as in overview_button_tray. Do you need to call this > every time alignment changes in MD given the padding is same on all sides? Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.cc:382: } else { On 2016/07/14 21:34:48, varkha wrote: > nit: no need for 'else' with early return above. Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.cc:528: MaterialDesignController::IsShelfMaterial()) { On 2016/07/14 21:34:47, varkha wrote: > How about > > if (!MaterialDesignController::IsShelfMaterial() || > shelf()->GetBackgroundType() == > ShelfBackgroundType::SHELF_BACKGROUND_DEFAULT) { > return; > } > > instead? It is better for readers to follow https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.cc:535: const int height = GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY); On 2016/07/14 21:34:47, varkha wrote: > If you move this up you can use |height| instead of calling > GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY). > I would move both width and height above together. Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.cc:542: gfx::Rect bounds = horizontal_shelf ? gfx::Rect(x, y, width, height) On 2016/07/14 21:34:47, varkha wrote: > nit: const. Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.h:27: // his shown/hidden. It also inherits from ActionableView so that the tray On 2016/07/14 21:34:48, varkha wrote: > nit: s/his/is Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.h:139: // In the given |canvas|, draw a 1px wide and 32dp high separator line to the On 2016/07/14 21:34:48, varkha wrote: > nit: s/draw/draws > nit: s/high/tall Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.h:140: // right of the tray item. On 2016/07/14 21:34:48, varkha wrote: > nit: clarify which tray item. Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.h:154: // For Material Design, some extra padding is added to tray items so there is On 2016/07/14 21:34:48, varkha wrote: > nit: drop "some" Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_background_view.h:157: // separator needed. No border is set for non-MD. On 2016/07/14 21:34:48, varkha wrote: > nit: s/separator/a separator is Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/tray_constants.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_constants.cc:88: const SkColor kSeparatorColor = SkColorSetA(SK_ColorWHITE, 0x29); On 2016/07/14 21:34:48, varkha wrote: > Opacity seems quite low (16%?) so it is barely visible on black background on > hi-dpi screens. I may want to look at it on a device. The spec is changed to 0.24 instead. ref: https://folio.googleplex.com/cros-core-ui/spec#%2F1.1%20-%20Shelf%20general%2... https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/tray_constants.h (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_constants.h:78: // Separator is line of 1px wide and it is used to separate 2 tray items in the On 2016/07/14 21:34:48, varkha wrote: > How about: > // Color and width of a line used to separate tray items in the shelf. Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/web_n... File ash/common/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/web_n... ash/common/system/web_notification/web_notification_tray.cc:523: // Paint a 1px separator to separate the notification button and status tray. On 2016/07/14 21:34:48, varkha wrote: > nit: I think this comment is not necessary. Done. https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/web_n... File ash/common/system/web_notification/web_notification_tray.h (right): https://codereview.chromium.org/2147143002/diff/20001/ash/common/system/web_n... ash/common/system/web_notification/web_notification_tray.h:173: On 2016/07/14 21:34:48, varkha wrote: > I think it would be better to make it protected because you are hiding it from > possible overrides in subclasses. That or move down since overrides should be > the last methods declared (a pattern I observe even though I could not find it > mentioned in the style guide). N/A with new approach
Description was changed from ========== Draw a 1dp separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md). TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent or opaque. - Change shelf alignment from bottom to left to right, check it the separator updates its orientation. - Change the visibility of logout button, check if the separator hides both before and after the button. BUG=605993 ========== to ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ==========
The CQ bit was checked by yiyix@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...
Description was changed from ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ========== to ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). Introduced a new observer, ViewObserver, to observe how the visibility of TrayBackgroundView changes, so the visibility of the separator can be updated accordingly. TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ==========
https://codereview.chromium.org/2147143002/diff/40001/ash/common/system/view_... File ash/common/system/view_observer.h (right): https://codereview.chromium.org/2147143002/diff/40001/ash/common/system/view_... ash/common/system/view_observer.h:15: class ASH_EXPORT ViewObserver { This name seems too generic. Maybe TrayBackgroundViewVisibilityObserver?
I would include a link to the design doc in the CL description and a link to this CL in the design doc to make navigation easier.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). Introduced a new observer, ViewObserver, to observe how the visibility of TrayBackgroundView changes, so the visibility of the separator can be updated accordingly. TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ========== to ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). Introduced a new observer, TrayBackgroundViewVisibilityObserver, to observe how the visibility of TrayBackgroundView changes, so the visibility of the separator can be updated accordingly. Design doc: https://docs.google.com/document/d/1oDtCqwSJdaBtAfBWcQOjKnHZ6o_ysd4LlDI-vl4ujHw/ TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ==========
The CQ bit was checked by yiyix@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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 yiyix@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...
yiyix@chromium.org changed reviewers: + jamescook@chromium.org
On 2016/07/27 16:01:57, yiyix wrote: @jamescook, Could you please take a look? Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Rather than introducing a new observer, what about adding a method to WmShelfObserver and having TrayBackgroundView become a WmShelfObserver? It could add/remove itself to the wm_shelf passed in its constructor.
The CQ bit was checked by yiyix@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...
Patchset #6 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Patchset #6 (id:140001) has been deleted
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: 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_...)
Description was changed from ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). Introduced a new observer, TrayBackgroundViewVisibilityObserver, to observe how the visibility of TrayBackgroundView changes, so the visibility of the separator can be updated accordingly. Design doc: https://docs.google.com/document/d/1oDtCqwSJdaBtAfBWcQOjKnHZ6o_ysd4LlDI-vl4ujHw/ TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ========== to ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). Introduced a new method in wm_shelf_observer, to observe how the visibility of tray items (TrayBackgroundView) changes, so the visibility of the separator can be updated accordingly. Design doc: https://docs.google.com/document/d/1oDtCqwSJdaBtAfBWcQOjKnHZ6o_ysd4LlDI-vl4ujHw/ TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ==========
The CQ bit was checked by yiyix@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.
@jamescook, I updated the observer. Could you take another look at this code change? Thank you.
I'm sorry I didn't make this comment before, but this CL seems to do a lot of work passing observer pointers around just to allow TrayBackgroundView to invoke a single method on StatusAreaWidget. There's only ever a single observer, and this code is all in the same module, so it feels like overkill to me. It might be simpler to expose StatusAreaWidget on WmRootWindowController, then do something like this in TrayBackgroundView: wm_shelf_->GetWindow()->GetRootWindowController()->GetStatusAreaWidget()->UpdateSeparatorVisibility(); What do you think? https://codereview.chromium.org/2147143002/diff/180001/ash/common/shelf/wm_sh... File ash/common/shelf/wm_shelf_observer.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/shelf/wm_sh... ash/common/shelf/wm_shelf_observer.h:24: virtual void OnVisibilityChange(TrayBackgroundView* tray) {} nit: Document what this does / when it will be called. Also, I would name the method something that has "tray" in it, like OnTrayVisibilityChanging() or similar. Also, if it's called after the change, call it "Changed". If before, call it "Changing". https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:31: explicit ImeMenuTray(WmShelf* wm_shelf, WmShelfObserver* wm_shelf_observer); no explicit We only use explicit for single-parameter constructors https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tray.h:38: explicit PaletteTray(WmShelf* wm_shelf, WmShelfObserver* wm_shelf_observer); ditto https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/session/logout_button_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/session/logout_button_tray.h:21: class ViewObserver; not needed https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:18: class LogoutButtonTray; not needed https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:26: explicit VirtualKeyboardTray(WmShelf* wm_shelf, no explicit https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/over... File ash/common/system/overview/overview_button_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/over... ash/common/system/overview/overview_button_tray.h:30: explicit OverviewButtonTray(WmShelf* wm_shelf, ditto https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:248: if (ash::MaterialDesignController::IsShelfMaterial()) { nit: early exit to avoid indenting below? https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:255: } nit: I would do something like: const bool show_separator = !logout_button_tray_->visible(); tray->SetSeparatorVisibility(show_separator); also below This also gives you a place to put comments about why you are showing/hiding it https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:256: nit: no blank line https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:264: } Is there a way to do this based on getting the tray item by index, rather than hard-coding the knowledge about the order of the items into this function? You need to hard-code knowledge about the logout button, since it is bright red, but could you figure out information about its neighbor based on the index? I'm worried this will break if someone reorders the items, and they won't notice because the logout button is not commonly seen by developers. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... File ash/common/system/status_area_widget.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.h:95: void OnVisibilityChange(TrayBackgroundView* tray) override; nit: put "WmShelfObserver:" above this line and move the rest of the comment into the .cc file https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/system_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/system_tray.h:45: explicit SystemTray(WmShelf* wm_shelf, WmShelfObserver* wm_shelf_observer); no explicit https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/system_tray.h:222: // of the dark background. Doesn't do anything in MD. nice documentation, esp. noting the MD vs non-MD status https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:231: wm_shelf_observer_ = wm_shelf_observer; nit: initialize in the member list above Also, DCHECK(wm_shelf_observer_) since it must not be null (which makes the code self-documenting) https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:267: wm_shelf_observer_->OnVisibilityChange(this); so I think this should end with "Changing", since it's before the change https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:378: if (IsHorizontalAlignment(shelf()->GetAlignment())) { optional nit: I'm about to land a CL that will let you call wm_shelf_->IsHorizontalAlignment() instead of the utility function. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:526: void TrayBackgroundView::SetSeparatorVisibility(bool is_show) { nit: is_shown or just shown https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:527: if (is_separator_visible_ != is_show) I don't think you need this if() https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:537: if (is_separator_visible_) { nit: put !is_separator_visible_ into the early-exit above https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:540: const int x = height + kHitRegionPadding + kHitRegionPadding; x is based on the height? https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:542: const float scale = canvas->UndoDeviceScaleFactor(); I think you need to do this inside the scoped_canvas so you don't change the state of |canvas| for code outside this function https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:554: } I find this function a little hard to read. It seems odd to construct a bounds rect just to draw a line from one point to another. Maybe it would be clearer if it computed 2 points, or 1 point and x/y offsets? https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:69: explicit TrayBackgroundView(WmShelf* wm_shelf, no explicit https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:149: // right of this TrayBackgeoundView item. nit: TrayBackgroundView, or just say "item" https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/tray_constants.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_constants.h:79: extern const int kHitRegionPadding; nit: document this
I would also be OK with adding WmShelf::GetStatusAreaWidget() and using that. (Long term I want StatusAreaWidget access via WmRootWindowController, but in the short term getting it through WmShelf would be easier to build.)
https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/session/logout_button_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/session/logout_button_tray.h:30: explicit LogoutButtonTray(WmShelf* wm_shelf, nit: no need for explicit. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:252: tray->SetSeparatorVisibility(false); nit: collapse into a single method invocation, i.e. tray->SetSeparatorVisibility(!logout_button_tray_->visible()); This could avoid code bloat. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:260: tray->SetSeparatorVisibility(false); nit: collapse into a single method invocation, i.e. tray->SetSeparatorVisibility(!logout_button_tray_->visible() || virtual_keyboard_tray_->visible()) https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... File ash/common/system/status_area_widget.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.h:91: // Based on visibility of |tray| and tray item on its right, it sets |tray|'s nit: remove "it" (in "it sets |tray|'s ..."). https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:636: if (!ash::MaterialDesignController::IsShelfMaterial()) { nit: no need for {}. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:381: 0, kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth))); Looks like you can simplify this similar to: // Extend hit region horizontally or vertically depending on shelf alignment. const gfx::Insets insets(IsHorizontalAlignment(shelf()->GetAlignment()) ? gfx::Insets(0, kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth) : gfx::Insets(kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth, 0); tray_container()->SetBorder(views::Border::CreateEmptyBorder(insets)); https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:526: void TrayBackgroundView::SetSeparatorVisibility(bool is_show) { On 2016/08/09 00:25:18, James Cook wrote: > nit: is_shown or just shown ... or just "visibility". https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:534: ShelfBackgroundType::SHELF_BACKGROUND_DEFAULT) nit - {} for multi-line condition.
https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/session/logout_button_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/session/logout_button_tray.h:30: explicit LogoutButtonTray(WmShelf* wm_shelf, nit: no need for explicit. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:252: tray->SetSeparatorVisibility(false); nit: collapse into a single method invocation, i.e. tray->SetSeparatorVisibility(!logout_button_tray_->visible()); This could avoid code bloat. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:260: tray->SetSeparatorVisibility(false); nit: collapse into a single method invocation, i.e. tray->SetSeparatorVisibility(!logout_button_tray_->visible() || virtual_keyboard_tray_->visible()) https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... File ash/common/system/status_area_widget.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.h:91: // Based on visibility of |tray| and tray item on its right, it sets |tray|'s nit: remove "it" (in "it sets |tray|'s ..."). https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:636: if (!ash::MaterialDesignController::IsShelfMaterial()) { nit: no need for {}. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:381: 0, kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth))); Looks like you can simplify this similar to: // Extend hit region horizontally or vertically depending on shelf alignment. const gfx::Insets insets(IsHorizontalAlignment(shelf()->GetAlignment()) ? gfx::Insets(0, kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth) : gfx::Insets(kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth, 0); tray_container()->SetBorder(views::Border::CreateEmptyBorder(insets)); https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:526: void TrayBackgroundView::SetSeparatorVisibility(bool is_show) { On 2016/08/09 00:25:18, James Cook wrote: > nit: is_shown or just shown ... or just "visibility". https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:534: ShelfBackgroundType::SHELF_BACKGROUND_DEFAULT) nit - {} for multi-line condition.
Patchset #8 (id:200001) has been deleted
I am using WmShelf::GetStatusAreaWidget() to expose the StatusAreaWidget to TrayBackgroundView. Thank you for the detailed review. https://codereview.chromium.org/2147143002/diff/180001/ash/common/shelf/wm_sh... File ash/common/shelf/wm_shelf_observer.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/shelf/wm_sh... ash/common/shelf/wm_shelf_observer.h:24: virtual void OnVisibilityChange(TrayBackgroundView* tray) {} On 2016/08/09 00:25:17, James Cook wrote: > nit: Document what this does / when it will be called. > > Also, I would name the method something that has "tray" in it, like > OnTrayVisibilityChanging() or similar. Also, if it's called after the change, > call it "Changed". If before, call it "Changing". Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:31: explicit ImeMenuTray(WmShelf* wm_shelf, WmShelfObserver* wm_shelf_observer); On 2016/08/09 00:25:17, James Cook wrote: > no explicit > > We only use explicit for single-parameter constructors Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tray.h:38: explicit PaletteTray(WmShelf* wm_shelf, WmShelfObserver* wm_shelf_observer); On 2016/08/09 00:25:17, James Cook wrote: > ditto Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/session/logout_button_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/session/logout_button_tray.h:21: class ViewObserver; On 2016/08/09 00:25:17, James Cook wrote: > not needed Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/session/logout_button_tray.h:30: explicit LogoutButtonTray(WmShelf* wm_shelf, On 2016/08/10 23:27:24, varkha wrote: > nit: no need for explicit. After changing the observer, there will be only 1 parameter https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:18: class LogoutButtonTray; On 2016/08/09 00:25:17, James Cook wrote: > not needed It is from the 1st approach. I forget to clean it. Thanks. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/chro... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.h:26: explicit VirtualKeyboardTray(WmShelf* wm_shelf, On 2016/08/09 00:25:17, James Cook wrote: > no explicit Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/over... File ash/common/system/overview/overview_button_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/over... ash/common/system/overview/overview_button_tray.h:30: explicit OverviewButtonTray(WmShelf* wm_shelf, On 2016/08/09 00:25:17, James Cook wrote: > ditto Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:248: if (ash::MaterialDesignController::IsShelfMaterial()) { On 2016/08/09 00:25:17, James Cook wrote: > nit: early exit to avoid indenting below? Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:255: } On 2016/08/09 00:25:17, James Cook wrote: > nit: I would do something like: > const bool show_separator = !logout_button_tray_->visible(); > tray->SetSeparatorVisibility(show_separator); > > also below > > This also gives you a place to put comments about why you are showing/hiding it Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:256: On 2016/08/09 00:25:17, James Cook wrote: > nit: no blank line Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:260: tray->SetSeparatorVisibility(false); On 2016/08/10 23:27:24, varkha wrote: > nit: collapse into a single method invocation, i.e. > tray->SetSeparatorVisibility(!logout_button_tray_->visible() || > virtual_keyboard_tray_->visible()) This function is re-write with indices. I will keep in mind how to simplify my bool evaluations. Thanks https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.cc:264: } On 2016/08/09 00:25:17, James Cook wrote: > Is there a way to do this based on getting the tray item by index, rather than > hard-coding the knowledge about the order of the items into this function? You > need to hard-code knowledge about the logout button, since it is bright red, but > could you figure out information about its neighbor based on the index? > > I'm worried this will break if someone reorders the items, and they won't notice > because the logout button is not commonly seen by developers. I totally agree. I don't know anything about the logout button before starting this task. I re-write this function based on the index. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... File ash/common/system/status_area_widget.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.h:91: // Based on visibility of |tray| and tray item on its right, it sets |tray|'s On 2016/08/10 23:27:24, varkha wrote: > nit: remove "it" (in "it sets |tray|'s ..."). Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/stat... ash/common/system/status_area_widget.h:95: void OnVisibilityChange(TrayBackgroundView* tray) override; On 2016/08/09 00:25:17, James Cook wrote: > nit: put "WmShelfObserver:" above this line and move the rest of the comment > into the .cc file Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:636: if (!ash::MaterialDesignController::IsShelfMaterial()) { On 2016/08/10 23:27:24, varkha wrote: > nit: no need for {}. Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/system_tray.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/system_tray.h:45: explicit SystemTray(WmShelf* wm_shelf, WmShelfObserver* wm_shelf_observer); On 2016/08/09 00:25:17, James Cook wrote: > no explicit Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/system_tray.h:222: // of the dark background. Doesn't do anything in MD. On 2016/08/09 00:25:17, James Cook wrote: > nice documentation, esp. noting the MD vs non-MD status Thank you :D https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:231: wm_shelf_observer_ = wm_shelf_observer; On 2016/08/09 00:25:18, James Cook wrote: > nit: initialize in the member list above > > Also, DCHECK(wm_shelf_observer_) since it must not be null (which makes the code > self-documenting) After removing the observer, this is not applicable. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:267: wm_shelf_observer_->OnVisibilityChange(this); On 2016/08/09 00:25:17, James Cook wrote: > so I think this should end with "Changing", since it's before the change After using index, the method is called after visibility change, so I updated it to OnTrayVisibilityChanged. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:378: if (IsHorizontalAlignment(shelf()->GetAlignment())) { On 2016/08/09 00:25:17, James Cook wrote: > optional nit: I'm about to land a CL that will let you call > wm_shelf_->IsHorizontalAlignment() instead of the utility function. Thank you for head up. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:381: 0, kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth))); On 2016/08/10 23:27:25, varkha wrote: > Looks like you can simplify this similar to: > > // Extend hit region horizontally or vertically depending on shelf alignment. > const gfx::Insets insets(IsHorizontalAlignment(shelf()->GetAlignment()) > ? gfx::Insets(0, kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth) > : gfx::Insets(kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth, 0); > tray_container()->SetBorder(views::Border::CreateEmptyBorder(insets)); Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:526: void TrayBackgroundView::SetSeparatorVisibility(bool is_show) { On 2016/08/09 00:25:18, James Cook wrote: > nit: is_shown or just shown Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:527: if (is_separator_visible_ != is_show) On 2016/08/09 00:25:18, James Cook wrote: > I don't think you need this if() Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:534: ShelfBackgroundType::SHELF_BACKGROUND_DEFAULT) On 2016/08/10 23:27:25, varkha wrote: > nit - {} for multi-line condition. Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:537: if (is_separator_visible_) { On 2016/08/09 00:25:17, James Cook wrote: > nit: put !is_separator_visible_ into the early-exit above Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:540: const int x = height + kHitRegionPadding + kHitRegionPadding; On 2016/08/09 00:25:18, James Cook wrote: > x is based on the height? The application icons have the same height and width, so I wanted to use GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY) as the width to calculate the x location of the separator. I can see how it is confusing to read now. I will use GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY) instead of height for the readability. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:542: const float scale = canvas->UndoDeviceScaleFactor(); On 2016/08/09 00:25:18, James Cook wrote: > I think you need to do this inside the scoped_canvas so you don't change the > state of |canvas| for code outside this function There is no drawing functions inside of ScopedCanvas. ScopeCanvas is there to restore the Canvas if anything goes out of scope. I took this file as an example to write this function. https://cs.chromium.org/chromium/src/ui/views/painter.cc?sq=package:chromium&... https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:554: } On 2016/08/09 00:25:17, James Cook wrote: > I find this function a little hard to read. It seems odd to construct a bounds > rect just to draw a line from one point to another. Maybe it would be clearer if > it computed 2 points, or 1 point and x/y offsets? I was thinking that i need to construct a rect to draw the line, so it would be easier if the line and the rect have the same size. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:69: explicit TrayBackgroundView(WmShelf* wm_shelf, On 2016/08/09 00:25:18, James Cook wrote: > no explicit Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:149: // right of this TrayBackgeoundView item. On 2016/08/09 00:25:18, James Cook wrote: > nit: TrayBackgroundView, or just say "item" Done. https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... File ash/common/system/tray/tray_constants.h (right): https://codereview.chromium.org/2147143002/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_constants.h:79: extern const int kHitRegionPadding; On 2016/08/09 00:25:18, James Cook wrote: > nit: document this Done.
The CQ bit was checked by yiyix@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...
Patchset #8 (id:220001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Description was changed from ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). Introduced a new method in wm_shelf_observer, to observe how the visibility of tray items (TrayBackgroundView) changes, so the visibility of the separator can be updated accordingly. Design doc: https://docs.google.com/document/d/1oDtCqwSJdaBtAfBWcQOjKnHZ6o_ysd4LlDI-vl4ujHw/ TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ========== to ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). After tray's visibility changes, OnTrayVisibilityChanged in StatusAreaWidget will be called to compute the visibility of the separator accordingly. TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ==========
@jamescook and @varkha: Could you review this change? Thank you.
https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:263: // If |logout_button_tray_| is visible, then no leading separator is required nit: indents. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... File ash/common/system/status_area_widget.h (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.h:52: // Based on visibility of |tray| and tray item on its right, sets |tray|'s nit: s/and tray item/and a tray item
Almost there! https://codereview.chromium.org/2147143002/diff/240001/ash/aura/wm_shelf_aura.h File ash/aura/wm_shelf_aura.h (right): https://codereview.chromium.org/2147143002/diff/240001/ash/aura/wm_shelf_aura... ash/aura/wm_shelf_aura.h:19: class StatusAreaWidget; nit: not needed (in general, if an interface from which you inherit makes a reference to a type you don't need to forward declare it yourself) https://codereview.chromium.org/2147143002/diff/240001/ash/common/shelf/wm_sh... File ash/common/shelf/wm_shelf.h (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/shelf/wm_sh... ash/common/shelf/wm_shelf.h:26: class StatusAreaWidget; nit: alphabetize https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/chro... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (left): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/chro... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:103: tray_container()->SetBorder(views::Border::NullBorder()); Q: Are you sure this isn't needed anymore? I thought mohsen just added it? https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/chro... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/chro... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:75: horizontal_padding += std::max(0, vertical_padding - horizontal_padding); horizontal_padding isn't used after this line https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/chro... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:125: } There seems to be some duplication between this code and the SetShelfAlignment() code above. Do you need both? If so, can it be shared? https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:206: } nit: I think this would be clearer if you did something like: if (!logout->visible()) return false; int logout_button_index = delegate_->GetIndexOf(logout_button_tray_); // Logout button should always exist. DCHECK_NE(-1, logout_button_index); Then loop through looking for the next visible child. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:253: void StatusAreaWidget::OnTrayVisibilityChanged(TrayBackgroundView* tray) { nit: move up to match function order in header https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:262: } else { nit: early return above this line might make it easier to read with the OS_CHROMEOS ifdef. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:266: #if defined(OS_CHROMEOS) nit: move this above the comment https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:271: tray->SetSeparatorVisibility(true); maybe put this in an #else block? https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... File ash/common/system/status_area_widget.h (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.h:102: // Check if |tray| is next visible tray to the |logout_button_tray_|. If nit: By "next visible tray" do you mean next in the view child list? Or on the right? On the left? https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:229: is_separator_visible_ = false; can this be initialized in the member list above? https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:307: // OnTrayVisibilityChanged will only be called if the visibility of the tray Is this still true? I don't understand what you're saying here. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:23: class WmShelfObserver; not needed https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:66: // Creates a TrayBackgroundView and adds a |tray_background_view_observer| to Remove this comment. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:147: // In the given |canvas|, draws a 1px wide and 32dp tall separator line to the super nit: I would mention the dimensions here, since they might change in the future
https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:536: const float scale = canvas->UndoDeviceScaleFactor(); Also, I still think this should go inside the scoped_canvas below. It affects the scaling of the canvas, which should affect its matrix. Per SkCanvas.h, the canvas->save() call inside ScopedCanvas should preserve the matrix. https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkCanvas.h... Also, the example you cite puts the UndoDeviceScaleFactor inside the ScopedCanvas. https://cs.chromium.org/chromium/src/ui/views/painter.cc?sq=package:chromium&... Maybe I'm wrong? I'm not super-familiar with SkCanvas.
yoshiki@chromium.org changed reviewers: + yoshiki@chromium.org
The CQ bit was checked by yiyix@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...
Merged with changes made by @yoshiki. re-worked the logics of adding insets to traybackgroundview. @jamescook + varkha, could you review this changes again? thanks @yoshiki, Could you also review this change, so I am sure that I did not break the logic in arc++ environment. Thank you very much https://codereview.chromium.org/2147143002/diff/240001/ash/aura/wm_shelf_aura.h File ash/aura/wm_shelf_aura.h (right): https://codereview.chromium.org/2147143002/diff/240001/ash/aura/wm_shelf_aura... ash/aura/wm_shelf_aura.h:19: class StatusAreaWidget; On 2016/08/11 20:36:54, James Cook wrote: > nit: not needed (in general, if an interface from which you inherit makes a > reference to a type you don't need to forward declare it yourself) Done. https://codereview.chromium.org/2147143002/diff/240001/ash/common/shelf/wm_sh... File ash/common/shelf/wm_shelf.h (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/shelf/wm_sh... ash/common/shelf/wm_shelf.h:26: class StatusAreaWidget; On 2016/08/11 20:36:54, James Cook wrote: > nit: alphabetize Done. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/chro... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (left): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/chro... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:103: tray_container()->SetBorder(views::Border::NullBorder()); On 2016/08/11 20:36:54, James Cook wrote: > Q: Are you sure this isn't needed anymore? I thought mohsen just added it? One of the change I made it to initialize the border as null in the traybackgroundview, then add the padding around SystemTray and OverviewTray; where as before we initialize the padding in traybackgroundview and change the border to null in notification, virtual_keyboard (the one @mohsen just added), etc. I think it is better to add them as needed; and it also gives the flexibility of adjusting the padding size. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/chro... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/chro... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:75: horizontal_padding += std::max(0, vertical_padding - horizontal_padding); On 2016/08/11 20:36:54, James Cook wrote: > horizontal_padding isn't used after this line Sorry, I had these from merge, I did not check carefully. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/chro... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:125: } On 2016/08/11 20:36:54, James Cook wrote: > There seems to be some duplication between this code and the SetShelfAlignment() > code above. Do you need both? If so, can it be shared? The earlier one means needs to be deleted. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:206: } On 2016/08/11 20:36:55, James Cook wrote: > nit: I think this would be clearer if you did something like: > > if (!logout->visible()) > return false; > int logout_button_index = delegate_->GetIndexOf(logout_button_tray_); > // Logout button should always exist. > DCHECK_NE(-1, logout_button_index); > > Then loop through looking for the next visible child. Yes, it makes the code easier to understand. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:253: void StatusAreaWidget::OnTrayVisibilityChanged(TrayBackgroundView* tray) { On 2016/08/11 20:36:55, James Cook wrote: > nit: move up to match function order in header I moved up SetShelfAlignment, UpdateAfterLoginStatusChange and OnTrayVisibilityChanged to match the header file. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:262: } else { On 2016/08/11 20:36:55, James Cook wrote: > nit: early return above this line might make it easier to read with the > OS_CHROMEOS ifdef. Done. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:263: // If |logout_button_tray_| is visible, then no leading separator is required On 2016/08/11 20:01:03, varkha wrote: > nit: indents. Done. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:266: #if defined(OS_CHROMEOS) On 2016/08/11 20:36:54, James Cook wrote: > nit: move this above the comment Done. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.cc:271: tray->SetSeparatorVisibility(true); On 2016/08/11 20:36:55, James Cook (slow reviews) wrote: > maybe put this in an #else block? Done. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... File ash/common/system/status_area_widget.h (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.h:52: // Based on visibility of |tray| and tray item on its right, sets |tray|'s On 2016/08/11 20:01:03, varkha wrote: > nit: s/and tray item/and a tray item Done. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/stat... ash/common/system/status_area_widget.h:102: // Check if |tray| is next visible tray to the |logout_button_tray_|. If On 2016/08/11 20:36:55, James Cook wrote: > nit: By "next visible tray" do you mean next in the view child list? Or on the > right? On the left? I mean it is next in the view child list, so it displays as left view to logout_button_tray_ in chrome os system. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:229: is_separator_visible_ = false; On 2016/08/11 20:36:55, James Cook (slow reviews) wrote: > can this be initialized in the member list above? Done. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:307: // OnTrayVisibilityChanged will only be called if the visibility of the tray On 2016/08/11 20:36:55, James Cook (slow reviews) wrote: > Is this still true? I don't understand what you're saying here. If the visibility is the same as before, then if statement in front of the function returns true (if (visible == layer()->GetTargetVisibility())) so my function will be called if the visibility is changed. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:536: const float scale = canvas->UndoDeviceScaleFactor(); On 2016/08/11 22:39:13, James Cook (slow reviews) wrote: > Also, I still think this should go inside the scoped_canvas below. It affects > the scaling of the canvas, which should affect its matrix. Per SkCanvas.h, the > canvas->save() call inside ScopedCanvas should preserve the matrix. > > https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkCanvas.h... > > Also, the example you cite puts the UndoDeviceScaleFactor inside the > ScopedCanvas. > > https://cs.chromium.org/chromium/src/ui/views/painter.cc?sq=package:chromium&... > > Maybe I'm wrong? I'm not super-familiar with SkCanvas. Do you mean change the order of const float scale = canvas->UndoDeviceScaleFactor(); and gfx::ScopedCanvas scoped_canvas(canvas); so everything happens in the saved canvas? I can do that. I am not familiar with this part of code too. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:23: class WmShelfObserver; On 2016/08/11 20:36:55, James Cook wrote: > not needed Done. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:66: // Creates a TrayBackgroundView and adds a |tray_background_view_observer| to On 2016/08/11 20:36:55, James Cook wrote: > Remove this comment. Done. https://codereview.chromium.org/2147143002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:147: // In the given |canvas|, draws a 1px wide and 32dp tall separator line to the On 2016/08/11 20:36:55, James Cook (slow reviews) wrote: > super nit: I would mention the dimensions here, since they might change in the > future Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
I tested this on ARC++ environment. It's almost good but the separator position looks wrong when there is multiple small notification icons in the notification tray. Could you adjust position of separator? Thanks. https://codereview.chromium.org/2147143002/diff/260001/ash/common/shelf/wm_sh... File ash/common/shelf/wm_shelf.h (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/shelf/wm_sh... ash/common/shelf/wm_shelf.h:115: StatusAreaWidget* GetStatusAreaWidget(); nit: Please make this const method. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... ash/common/system/status_area_widget.cc:145: if (!ash::MaterialDesignController::IsShelfMaterial()) { nit: no need brackets, since there is only a line in the block. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... ash/common/system/status_area_widget.cc:254: bool StatusAreaWidget::IsNextVisibleTrayToLogout(TrayBackgroundView* tray) { Shouldn't we return false immediately if logout_button_tray_ == nullptr? https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... File ash/common/system/status_area_widget.h (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... ash/common/system/status_area_widget.h:105: bool IsNextVisibleTrayToLogout(TrayBackgroundView* tray); nit: can we make this const? https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:632: // AdjustStatusTrayBackground(); nit: Please remove the unnecessary code. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:228: views::BoxLayout* layout; nit: I think we don't need to move it to here. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:243: SetBorder(views::Border::CreateEmptyBorder(insets)); nit: I think adding |margin_| at last is simpler. eg. const gfx::Insets insets( ... ...); SetBorder(views::Border::CreateEmptyBorder(margin_ + insets)); https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:542: is_separator_visible_ = is_shown; Shouldn't we call SchedulePaint() after setting the flag? https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:555: const int x = kTrayItemSize + kHitRegionPadding + kHitRegionPadding; Using kTrayItemSize is wrong. The width (or height if vertical) of the tray may expand if there are multiple icons in a single tray. Please retrieve the actual size from WebNotificationTray (=TrayBackgroundView) or it's container. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:148: void DrawSeparator(gfx::Canvas* canvas); Is this method declaration necessary? I can't find the method definition. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:178: bool is_separator_visible_; nit: could you add a brief comment for this member variable? https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/web_... File ash/common/system/web_notification/web_notification_tray.cc (left): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/web_... ash/common/system/web_notification/web_notification_tray.cc:180: if (!animation_.get() || !animation_->is_animating()) Please don't remove this. This is a short circuit for the non-animated case.
tdanderson@chromium.org changed reviewers: + tdanderson@chromium.org
I'm not 100% sure why PanelLayoutManagerTest.TouchHitTestPanel is failing on Win trybots for Patch Set 9 (but not Patch Set 8). Nothing jumps out at me when looking at the diff between PS8 and PS9 (note this also looks like it includes some of yoshiki@'s work for the notification tray, and I'm not sure if that would have anything to do with the failure). Looking at the git logs for recently-landed things which may have triggered some change between the PS8 trybot run and the PS9 trybot run, I found https://codereview.chromium.org/2254603002. Though again I'm not sure offhand how or if that patch would make any difference. Unless James sees something I haven't, I'd be inclined to suggest we disable the test on Windows (like what is done for PanelLayoutManagerTest.SplitViewOverlapWhenLarge).
https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... File ash/common/system/status_area_widget.h (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... ash/common/system/status_area_widget.h:102: // Check if |tray| is next visible view in the children() list, i.e. check if nit: is this accurate? // Checks if |tray| is the tray to the left of |logout_button_tray_|. Returns true if both are visible, false otherwise. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:243: SetBorder(views::Border::CreateEmptyBorder(insets)); I think you can conditionally calculate insets and then create and set border in one place to avoid code bloat. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:345: // Update the separator visibility accordingly. nit: Please consider dropping this comment. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:146: // In the given |canvas|, draws a 1x32px separator line and it is 4 pixel to nit: no need for "and it is".
@yoshiki, I think I have fixed the problem with the separator showing in the wrong place in web notification tray consisting 2 or 3 item. Could you test it again? Thank you very much for the help. Thank you for making such a detailed review on my cl. I totally missed the places where the function is defined the header file but not in the .cc file. I can stay late and talk to you today (Aug. 19 in tokyo time) to make sure it passes the arc++ test. @jamescook + verkha, could you review this CL? Thank you all. https://codereview.chromium.org/2147143002/diff/260001/ash/common/shelf/wm_sh... File ash/common/shelf/wm_shelf.h (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/shelf/wm_sh... ash/common/shelf/wm_shelf.h:115: StatusAreaWidget* GetStatusAreaWidget(); On 2016/08/18 08:39:08, yoshiki wrote: > nit: Please make this const method. Done. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... ash/common/system/status_area_widget.cc:145: if (!ash::MaterialDesignController::IsShelfMaterial()) { On 2016/08/18 08:39:08, yoshiki wrote: > nit: no need brackets, since there is only a line in the block. Thanks, i missed it. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... ash/common/system/status_area_widget.cc:254: bool StatusAreaWidget::IsNextVisibleTrayToLogout(TrayBackgroundView* tray) { On 2016/08/18 08:39:08, yoshiki wrote: > Shouldn't we return false immediately if logout_button_tray_ == nullptr? |logout_button_tray_| should not be nullptr. It always get added to the children list. I have adde this check to make sure |logout_button_tray_| exists DCHECK_NE(-1, logout_button_index); I think it is better to add this check if (!logout_button_tray_->visible()) after the DCHECK, so we are sure that |logout_button_tray_| is not nullptr. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... File ash/common/system/status_area_widget.h (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... ash/common/system/status_area_widget.h:102: // Check if |tray| is next visible view in the children() list, i.e. check if On 2016/08/18 16:33:24, varkha wrote: > nit: is this accurate? > // Checks if |tray| is the tray to the left of |logout_button_tray_|. Returns > true if both are visible, false otherwise. |tray| may not be the tray after |logout_button_tray_| in the list of children. I want to be clear that |tray| is the next visible tray after |logout_button_tray_|. I can update to: // Checks if |tray| is the next visible tray to the left of |logout_button_tray_|. Returns true if both are visible, false otherwise. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/stat... ash/common/system/status_area_widget.h:105: bool IsNextVisibleTrayToLogout(TrayBackgroundView* tray); On 2016/08/18 08:39:08, yoshiki wrote: > nit: can we make this const? Done. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:632: // AdjustStatusTrayBackground(); On 2016/08/18 08:39:08, yoshiki wrote: > nit: Please remove the unnecessary code. sorry, I forget to remove these dead codes from my previous versions. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:228: views::BoxLayout* layout; On 2016/08/18 08:39:08, yoshiki wrote: > nit: I think we don't need to move it to here. Done. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:243: SetBorder(views::Border::CreateEmptyBorder(insets)); On 2016/08/18 16:33:24, varkha wrote: > I think you can conditionally calculate insets and then create and set border in > one place to avoid code bloat. Done. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:345: // Update the separator visibility accordingly. On 2016/08/18 16:33:24, varkha wrote: > nit: Please consider dropping this comment. Done. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:542: is_separator_visible_ = is_shown; On 2016/08/18 08:39:08, yoshiki wrote: > Shouldn't we call SchedulePaint() after setting the flag? Done. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:555: const int x = kTrayItemSize + kHitRegionPadding + kHitRegionPadding; On 2016/08/18 08:39:08, yoshiki wrote: > Using kTrayItemSize is wrong. > > The width (or height if vertical) of the tray may expand if there are multiple > icons in a single tray. Please retrieve the actual size from WebNotificationTray > (=TrayBackgroundView) or it's container. In material design the tray has a fixed size is 32 by 32. So i used this knowledge to find the place to draw the separator. I agree that this makes this the drawing not as flexible and it is the reason that the line is drawn at the wrong place in notification tray in the arc++ machine when you have 2 or 3 icons. I will fix it. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:148: void DrawSeparator(gfx::Canvas* canvas); On 2016/08/18 08:39:08, yoshiki wrote: > Is this method declaration necessary? I can't find the method definition. Good catch! Thank you! I had this function from one of the earlier solutions. I forget to remove it from the header as the function is removed. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:178: bool is_separator_visible_; On 2016/08/18 08:39:08, yoshiki wrote: > nit: could you add a brief comment for this member variable? Done. https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/web_... File ash/common/system/web_notification/web_notification_tray.cc (left): https://codereview.chromium.org/2147143002/diff/260001/ash/common/system/web_... ash/common/system/web_notification/web_notification_tray.cc:180: if (!animation_.get() || !animation_->is_animating()) On 2016/08/18 08:39:08, yoshiki wrote: > Please don't remove this. This is a short circuit for the non-animated case. I am so sorry, i removed it as I tray different solutions to update the background and as i review my own cl, i missed this change. So sorry.
LGTM with nits https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:113: bounds = gfx::Rect(view->GetLocalBounds().x() + kHitRegionPadding, nit: cache the result of view->GetLocalBounds() in a local variable so you don't call it so many times https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:554: const bool horizontal_shelf = IsHorizontalAlignment(shelf()->GetAlignment()); I would use IsHorizontalAlignment(shelf_alignment_) here. It's bad that this class caches its own copy of the shelf alignment (I would like to eliminate that) but I think the same value should be used everywhere for consistency. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:68: TrayBackgroundView(WmShelf* wm_shelf); nit: keep explicit, since there is one parameter
As I tested, it looks working well with arc apps. Thank you for your hard work. LGTM. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:9: #include "ash/common/material_design/material_design_controller.h" nit: is this change necessary? https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:631: nit: I think we shouldn't have this clean up in this CL, since this CL doesn't change code around here. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:174: bool is_separator_visible_; nit: please add a blank line after this to keep consistency with above variables.
The CQ bit was checked by yiyix@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...
LGTM with nits. Thanks for pulling this through with all the small details! https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/over... File ash/common/system/overview/overview_button_tray.cc (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/over... ash/common/system/overview/overview_button_tray.cc:130: kVerticalShelfVerticalPadding, kVerticalShelfHorizontalPadding))); nit: Can you calculate insets in all 3 cases and then icon_->SetBorder(views::Border::CreateEmptyBorder(insets)) only once? https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:233: const int padding = 3; nit: can you declare the constant in anonymous namespace above and then use something like: const Insets insets(MDC::IsShelfMaterial ? gfx::Insets(...) : gfx::Insets(kStatusTrayBackgroundPadding); ? https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:558: : GetLocalBounds().height() - kSeparatorWidth; nit: can you shorten code to (pseudocode): const int x = (horizontal ? width : height) - kSeparatorWidth; ? https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:173: // This variable stores the visibility of this tray's separator. nit: Maybe just "Visibility of this tray's separator." or include where this separator is placed (i.e. is it on the leading or trailing edge).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Address all nits. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/over... File ash/common/system/overview/overview_button_tray.cc (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/over... ash/common/system/overview/overview_button_tray.cc:130: kVerticalShelfVerticalPadding, kVerticalShelfHorizontalPadding))); On 2016/08/19 16:33:39, varkha wrote: > nit: Can you calculate insets in all 3 cases and then > icon_->SetBorder(views::Border::CreateEmptyBorder(insets)) only once? Done. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:9: #include "ash/common/material_design/material_design_controller.h" On 2016/08/19 03:45:31, yoshiki wrote: > nit: is this change necessary? Done. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:631: On 2016/08/19 03:45:31, yoshiki wrote: > nit: I think we shouldn't have this clean up in this CL, since this CL doesn't > change code around here. Done. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:113: bounds = gfx::Rect(view->GetLocalBounds().x() + kHitRegionPadding, On 2016/08/19 00:55:50, James Cook (slow reviews) wrote: > nit: cache the result of view->GetLocalBounds() in a local variable so you don't > call it so many times Thanks! After i finally get it working after many many trials, I somehow failed to see this repetitive calling. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:233: const int padding = 3; On 2016/08/19 16:33:39, varkha wrote: > nit: can you declare the constant in anonymous namespace above and then use > something like: > const Insets insets(MDC::IsShelfMaterial ? gfx::Insets(...) : > gfx::Insets(kStatusTrayBackgroundPadding); > ? Done. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:554: const bool horizontal_shelf = IsHorizontalAlignment(shelf()->GetAlignment()); On 2016/08/19 00:55:50, James Cook (slow reviews) wrote: > I would use IsHorizontalAlignment(shelf_alignment_) here. It's bad that this > class caches its own copy of the shelf alignment (I would like to eliminate > that) but I think the same value should be used everywhere for consistency. Done. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.cc:558: : GetLocalBounds().height() - kSeparatorWidth; On 2016/08/19 16:33:39, varkha wrote: > nit: can you shorten code to (pseudocode): > const int x = (horizontal ? width : height) - kSeparatorWidth; > ? Done. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... File ash/common/system/tray/tray_background_view.h (right): https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:68: TrayBackgroundView(WmShelf* wm_shelf); On 2016/08/19 00:55:50, James Cook (slow reviews) wrote: > nit: keep explicit, since there is one parameter Done. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:173: // This variable stores the visibility of this tray's separator. On 2016/08/19 16:33:39, varkha wrote: > nit: Maybe just "Visibility of this tray's separator." or include where this > separator is placed (i.e. is it on the leading or trailing edge). Done. https://codereview.chromium.org/2147143002/diff/270001/ash/common/system/tray... ash/common/system/tray/tray_background_view.h:174: bool is_separator_visible_; On 2016/08/19 03:45:31, yoshiki wrote: > nit: please add a blank line after this to keep consistency with above > variables. Done.
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org, jamescook@chromium.org, yoshiki@chromium.org Link to the patchset: https://codereview.chromium.org/2147143002/#ps290001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). After tray's visibility changes, OnTrayVisibilityChanged in StatusAreaWidget will be called to compute the visibility of the separator accordingly. TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ========== to ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). After tray's visibility changes, OnTrayVisibilityChanged in StatusAreaWidget will be called to compute the visibility of the separator accordingly. TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:290001)
Message was sent while issue was closed.
Description was changed from ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). After tray's visibility changes, OnTrayVisibilityChanged in StatusAreaWidget will be called to compute the visibility of the separator accordingly. TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 ========== to ========== Draw a 1px(hardware pixel) separator between 2 tray items When the shelf is semi-transparent or opaque, draw a 1*32px separator line between 2 tray items except before and after the logout button (currently, it is off by default, behind --ash-md=experimental). After tray's visibility changes, OnTrayVisibilityChanged in StatusAreaWidget will be called to compute the visibility of the separator accordingly. TEST=manual: - Change device screen factor to different number, check if the separator width stays at 1px and separator height is updated proportionally. - Check if the separator shows if and only if when shelf is semi-transparent (by open a window overlapping with the shelf) or opaque (by open a maximized window). - Change shelf alignment from bottom to left to right, check if the separator updates its orientation. - Change the visibility of logout button by switching between the public account and the personal account, check if the separator hides both before and after the logout button if it is visible and separator hide when it is not visible. BUG=605993 Committed: https://crrev.com/6cf3f3eac174b448c518483ddbc1ae7dc47b2b2e Cr-Commit-Position: refs/heads/master@{#413220} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6cf3f3eac174b448c518483ddbc1ae7dc47b2b2e Cr-Commit-Position: refs/heads/master@{#413220} |
