Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
231643002:
Added labels under the windows in OverviewMode displaying their current name. (Closed)
|
Created:
6 years, 8 months ago by Nina Modified:
6 years, 8 months ago CC:
chromium-reviews, kalyank, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded labels under the windows in OverviewMode displaying their current name.
Labels will show the title of the active tab or selected panel. They have a blurred shadow and are animated similarly to the close buttons.
See the bug for some screenshots.
BUG=361181
TEST=WindowSelectorTest.CreateLabelUnderWindow,WindowSelectorTest.CreateLabelUnderPanel
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264602
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fixed some issues regarding code style #
Total comments: 7
Patch Set 3 : Upgrade to label aesthetics, now also working on panels, code cleanup #Patch Set 4 : Added animations to labels :) #
Total comments: 24
Patch Set 5 : Fixed aspect ratio issue when animating label movement #Patch Set 6 : Code cleanup according to latest feedback #Patch Set 7 : Added relevant tests #Patch Set 8 : Added blurred shadows to labels #
Total comments: 18
Patch Set 9 : Fixed nits, added transparency #
Total comments: 31
Patch Set 10 : Refactored the code to take advantage of blur mechanics, minor fixes #
Total comments: 8
Patch Set 11 : Follow-up fixes #
Total comments: 1
Patch Set 12 : Fixed tests, code polishing #
Total comments: 22
Patch Set 13 : Fixed latest issues #
Total comments: 2
Patch Set 14 : Fixed nits #
Messages
Total messages: 35 (0 generated)
Terry, can you please take a look at this? Thanks!
Looks great so far! I've left a few comments inline. (If anyone else is following along, Nicolas has attached some screenshots to the bug: https://code.google.com/p/chromium/issues/detail?id=361181). https://codereview.chromium.org/231643002/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector_window.cc (right): https://codereview.chromium.org/231643002/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_window.cc:150: // TODO use constant for box height? Calculate dynamically? I suspect that we'll eventually want to calculate it as a function of the window dimensions, which is something that can be decided based on review from the designer. It's OK to leave this as-is for the time being. https://codereview.chromium.org/231643002/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector_window.h (right): https://codereview.chromium.org/231643002/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_window.h:14: #include "ui/views/controls/label.h" This #include can be moved to the .cc file. https://codereview.chromium.org/231643002/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_window.h:52: const static int kLabelColor = 0xFFFFFFFF; static const instead of const static. Also, declare and define these in window_selector_window.cc near the top of the file (see the ordering in ui/views/controls/button/menu_button.cc as an example).
Thanks for the quick feedback! I did a code cleanup with the proposed changes.
https://codereview.chromium.org/231643002/diff/20001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_window.cc (right): https://codereview.chromium.org/231643002/diff/20001/ash/wm/overview/window_s... ash/wm/overview/window_selector_window.cc:25: static const int kLabelColor = 0xFFFFFFFF; Use SK_ColorWHITE, the type should be SkColor https://codereview.chromium.org/231643002/diff/20001/ash/wm/overview/window_s... ash/wm/overview/window_selector_window.cc:28: static const int kLabelBackground = 0x000000; Ditto, use SkColor type and color constant SK_ColorBLACK https://codereview.chromium.org/231643002/diff/20001/ash/wm/overview/window_s... ash/wm/overview/window_selector_window.cc:153: // TODO Animate the labels with some cute effects TODO(nsatragno). That being said, given everything else in overview is animated and this feature will be immediately visible I'd prefer it to be done in this CL. Probably use the same animation as the close button. https://codereview.chromium.org/231643002/diff/20001/ash/wm/overview/window_s... ash/wm/overview/window_selector_window.cc:154: base::string16 window_title = transform_window_.window()->title(); nit, move this to the scope where it's used, i.e. in if. https://codereview.chromium.org/231643002/diff/20001/ash/wm/overview/window_s... ash/wm/overview/window_selector_window.cc:157: // TODO use constant for box height? Calculate dynamically? Seems like it already uses a constant. That being said, it should likely be based on the font metrics. nit: TODO(nsatragno) https://codereview.chromium.org/231643002/diff/20001/ash/wm/overview/window_s... ash/wm/overview/window_selector_window.cc:161: 40); nit: this height should be defined as a constant. https://codereview.chromium.org/231643002/diff/20001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_window.h (right): https://codereview.chromium.org/231643002/diff/20001/ash/wm/overview/window_s... ash/wm/overview/window_selector_window.h:55: // IDEM UpdateCloseButtonBounds, creates a label to display under the window IDEM?
OK I moved forward and added the transitions. If everything looks good, I'll start working on tests.
Some more comments, mostly nits: https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/scoped_t... File ash/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/scoped_t... ash/wm/overview/scoped_transform_overview_window.cc:90: nit: No blank line between 89 and 91. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/scoped_t... File ash/wm/overview/scoped_transform_overview_window.h (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/scoped_t... ash/wm/overview/scoped_transform_overview_window.h:39: // labels and close buttons I'd reword to be more like the old comment to make it clear this is specific to entering overview mode: "The time for the close buttons and labels to fade in when initially shown on entering overview mode." https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:8: #include "ash/wm/overview/window_selector_item.h" It is proper form for the first #include in a .cc file to be its corresponding .h file, followed by a blank line. So here you should have: #include "ash/wm/overview/window_selector_item.h" <blank line> <all other #includes in lexicographical order> https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:44: params.parent = Let's also explicitly say params.accept_events = false; here too since we're not doing any event-handling on the label. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:58: 0, kVerticalLabelPadding, 0); My preference here would be to have one argument per line, with the first argument on line 56 (assuming it will fit within the 80 char limit). See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:119: { I am not 100% sure I understand why lines 120-125 need to be in this new scope. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.h:11: #include "ui/views/widget/widget.h" You can forward-declare the Widget class here instead of #including it. See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.h:82: // Returns the label name nit: . at end of comment. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.h:101: // Title to show under the label nit: . at end of comment https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.h:102: base::string16 item_title_; It doesn't look like this member variable is being used anywhere. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_panels.h (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_panels.h:41: virtual base::string16 GetLabelName(); OVERRIDE https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_window.h (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_window.h:44: virtual base::string16 GetLabelName(); OVERRIDE
Hi Terry, can you please look at the new update? Thanks! https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/scoped_t... File ash/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/scoped_t... ash/wm/overview/scoped_transform_overview_window.cc:90: On 2014/04/10 22:37:12, tdanderson wrote: > nit: No blank line between 89 and 91. Done. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/scoped_t... File ash/wm/overview/scoped_transform_overview_window.h (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/scoped_t... ash/wm/overview/scoped_transform_overview_window.h:39: // labels and close buttons On 2014/04/10 22:37:12, tdanderson wrote: > I'd reword to be more like the old comment to make it clear this is specific to > entering overview mode: > > "The time for the close buttons and labels to fade in when initially shown on > entering overview mode." Done. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:8: #include "ash/wm/overview/window_selector_item.h" On 2014/04/10 22:37:12, tdanderson wrote: > It is proper form for the first #include in a .cc file to be its corresponding > .h file, followed by a blank line. So here you should have: > > #include "ash/wm/overview/window_selector_item.h" > <blank line> > <all other #includes in lexicographical order> Done. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:44: params.parent = On 2014/04/10 22:37:12, tdanderson wrote: > Let's also explicitly say params.accept_events = false; here too since we're not > doing any event-handling on the label. Done. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:58: 0, kVerticalLabelPadding, 0); On 2014/04/10 22:37:12, tdanderson wrote: > My preference here would be to have one argument per line, with the first > argument on line 56 (assuming it will fit within the 80 char limit). See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:119: { On 2014/04/10 22:37:12, tdanderson wrote: > I am not 100% sure I understand why lines 120-125 need to be in this new scope. I don't know either, but that's how the animation works in the close button. Apparently removing it doesn't cause any issues. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.h:11: #include "ui/views/widget/widget.h" On 2014/04/10 22:37:12, tdanderson wrote: > You can forward-declare the Widget class here instead of #including it. See > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts. Done. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.h:82: // Returns the label name On 2014/04/10 22:37:12, tdanderson wrote: > nit: . at end of comment. Done. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.h:101: // Title to show under the label On 2014/04/10 22:37:12, tdanderson wrote: > nit: . at end of comment Done. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.h:102: base::string16 item_title_; On 2014/04/10 22:37:12, tdanderson wrote: > It doesn't look like this member variable is being used anywhere. Done. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_panels.h (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_panels.h:41: virtual base::string16 GetLabelName(); On 2014/04/10 22:37:12, tdanderson wrote: > OVERRIDE Done. https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_window.h (right): https://codereview.chromium.org/231643002/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_window.h:44: virtual base::string16 GetLabelName(); On 2014/04/10 22:37:12, tdanderson wrote: > OVERRIDE Done.
LGTM with the nits addressed below. Good job! Also be sure to update the issue description by removing [WIP] and adding a few sentences to explain what this patch does. Once you've made those changes, feel free to add flackr (for overview mode) and sadrul (for ui/views) as reviewers. https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:15: } newline after line 15 https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:104: // Label under the window with the tab name. Specify this is the name of the window's active tab. https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1093: TEST_F(WindowSelectorTest, CreateLabelUnderWindow) { Be sure to put these new test names in the codereview issue description underneath BUG= as follows: TEST=WindowSelectorTest.CreateLabelUnderWindow,WindowSelectorTest.CreateLabelUnderPanel https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1105: EXPECT_TRUE(children + 2 == root_window->children().size()) Use EXPECT_EQ here instead of EXPECT_TRUE. https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1134: EXPECT_TRUE(children + 1 == root_window->children().size()) EXPECT_EQ https://codereview.chromium.org/231643002/diff/140001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/231643002/diff/140001/ui/views/controls/label... ui/views/controls/label.h:97: // Sets the shadow blur. Default is zero. What is the allowable range for shadow_blur_? 0.0 to 1.0? https://codereview.chromium.org/231643002/diff/140001/ui/views/controls/label... ui/views/controls/label.h:101: // If true, let the shadow surrender the text. Default is false. surrender->surround https://codereview.chromium.org/231643002/diff/140001/ui/views/controls/label... ui/views/controls/label.h:103: void SetMirrorShadow(bool mirror_shadow); Since the mutators just have simple one-line implementations, I think the preferred way is to use unix_hacker function naming and to just implement them here in the header (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...). I do see many instances where this is not followed, though, so leaving it like this may be fine. https://codereview.chromium.org/231643002/diff/140001/ui/views/controls/label... ui/views/controls/label.h:278: double shadow_blur_; Add brief comments explaining what shadow_blur_ and mirror_shadow_ represent (similar to what is done for some of the other members here).
Hello, Please review the CL. I've addressed all nits. You can see a screenshot at the bug page. If you want to check the animations, please come by my desktop and I'll be glad to show a demo in a Pixel. https://map.googleplex.com/?q=googlerId:nsatragno Thanks! https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:15: } On 2014/04/14 16:58:39, tdanderson wrote: > newline after line 15 Done. https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:104: // Label under the window with the tab name. On 2014/04/14 16:58:39, tdanderson wrote: > Specify this is the name of the window's active tab. Done. https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1093: TEST_F(WindowSelectorTest, CreateLabelUnderWindow) { On 2014/04/14 16:58:39, tdanderson wrote: > Be sure to put these new test names in the codereview issue description > underneath BUG= as follows: > > TEST=WindowSelectorTest.CreateLabelUnderWindow,WindowSelectorTest.CreateLabelUnderPanel Done. https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1105: EXPECT_TRUE(children + 2 == root_window->children().size()) On 2014/04/14 16:58:39, tdanderson wrote: > Use EXPECT_EQ here instead of EXPECT_TRUE. Done. https://codereview.chromium.org/231643002/diff/140001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1134: EXPECT_TRUE(children + 1 == root_window->children().size()) On 2014/04/14 16:58:39, tdanderson wrote: > EXPECT_EQ Done. https://codereview.chromium.org/231643002/diff/140001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/231643002/diff/140001/ui/views/controls/label... ui/views/controls/label.h:97: // Sets the shadow blur. Default is zero. On 2014/04/14 16:58:39, tdanderson wrote: > What is the allowable range for shadow_blur_? 0.0 to 1.0? It's a bit more complicated. Please see the following link. https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/shadow_valu... https://codereview.chromium.org/231643002/diff/140001/ui/views/controls/label... ui/views/controls/label.h:101: // If true, let the shadow surrender the text. Default is false. On 2014/04/14 16:58:39, tdanderson wrote: > surrender->surround Done. https://codereview.chromium.org/231643002/diff/140001/ui/views/controls/label... ui/views/controls/label.h:103: void SetMirrorShadow(bool mirror_shadow); On 2014/04/14 16:58:39, tdanderson wrote: > Since the mutators just have simple one-line implementations, I think the > preferred way is to use unix_hacker function naming and to just implement them > here in the header > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...). > I do see many instances where this is not followed, though, so leaving it like > this may be fine. I'm using the same logic as in SetShadowOffset and SetShadowColors… does it make sense to change it if any of the reviewers have a say in it? https://codereview.chromium.org/231643002/diff/140001/ui/views/controls/label... ui/views/controls/label.h:278: double shadow_blur_; On 2014/04/14 16:58:39, tdanderson wrote: > Add brief comments explaining what shadow_blur_ and mirror_shadow_ represent > (similar to what is done for some of the other members here). Done.
https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:34: // Label height, ie. distance from the border of the window to the label bottom. nit: i.e. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:87: virtual base::string16 GetLabelName() = 0; This should probably be a const method, and return a const base::string16. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:105: // Label under the window its active tab name. nit s/its/displaying its https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_panels.cc (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_panels.cc:93: return SelectionWindow()->title(); SelectionWindow is overridden by WindowSelectorPanels and WindowSelectorWindow to return exactly the window you want, I don't think we need GetLabelName to be overridden as both can just call SelectionWindow to get the right window. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1097: aura::Window* root_window = Shell::GetContainer( This isn't really the root window, call this overlay_container. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1102: FireOverviewStartTimer(); I'd prefer that tests which aren't about alt tab cycling use ToggleOverview(). https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1109: // vector. This is very specific to the implementation. I.e. this test will need to be updated anytime we change what visuals are created by overview mode. This should use friend access to the WindowSelectorItem class's member to fetch the widget directly. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1117: EXPECT_EQ(label->text(), window_title); I'd expect that verifying the label is under the window would verify the position. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1122: TEST_F(WindowSelectorTest, CreateLabelUnderPanel) { Same comments as above, though since the position is already verified it may only be necessary to verify the most recently active (i.e. frontmost) panel title is used. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_window.cc (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_window.cc:108: set_bounds(window_bounds); This change seems like a noop. Revert to old code since window_bounds is not used? https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_window.cc:112: UpdateCloseButtonBounds(); nit: Can you add a TODO to move the close button management to WindowSelectorItem to be alongside the label management? It can reference http://crbug.com/352143. https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label... ui/views/controls/label.cc:346: enabled() ? enabled_shadow_color_ : disabled_shadow_color_)); I'm concerned that this effect may not be necessary at all, if you have the ability to set the blur and offset you should be able to get pretty much exactly what was asked for by using a large enough blur shouldn't you? https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label... ui/views/controls/label.h:99: void SetShadowBlur(double shadow_blur_); Trivial plain old data type setters should be inlined in .h and use unix_hacker_style(), i.e. set_mirror_shadow(bool mirror_shadow) { mirror_shadow_ = mirror_shadow; } https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label... ui/views/controls/label.h:103: void SetMirrorShadow(bool mirror_shadow); ditto. https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label... ui/views/controls/label.h:304: // If true, surrender the text with the shadow. Default is false. What do you mean by surrender the text with the shadow?
Thanks for the feedback, although I need some more information regarding the test fixes (as detailed below). Also, after today's git rebase, overview mode tests no longer compile: ../../ash/wm/overview/window_selector_unittest.cc: In member function 'virtual void ash::WindowSelectorTest_DISABLED_DragDropInProgress_Test::TestBody()': ../../ash/wm/overview/window_selector_unittest.cc:1040:44: error: variable 'ash::test::ShellTestApi shell_test_api' has initializer but incomplete type Any ideas? https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:34: // Label height, ie. distance from the border of the window to the label bottom. On 2014/04/14 20:31:18, flackr wrote: > nit: i.e. Done. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:87: virtual base::string16 GetLabelName() = 0; On 2014/04/14 20:31:18, flackr wrote: > This should probably be a const method, and return a const base::string16. Refactored the code so that we don't need this anymore https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:105: // Label under the window its active tab name. On 2014/04/14 20:31:18, flackr wrote: > nit s/its/displaying its Done. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_panels.cc (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_panels.cc:93: return SelectionWindow()->title(); On 2014/04/14 20:31:18, flackr wrote: > SelectionWindow is overridden by WindowSelectorPanels and WindowSelectorWindow > to return exactly the window you want, I don't think we need GetLabelName to be > overridden as both can just call SelectionWindow to get the right window. Refactored the code to take advantage of this. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1097: aura::Window* root_window = Shell::GetContainer( On 2014/04/14 20:31:18, flackr wrote: > This isn't really the root window, call this overlay_container. Could you please elaborate a bit more on how to call overlay_container? https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1102: FireOverviewStartTimer(); On 2014/04/14 20:31:18, flackr wrote: > I'd prefer that tests which aren't about alt tab cycling use ToggleOverview(). Done. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1109: // vector. On 2014/04/14 20:31:18, flackr wrote: > This is very specific to the implementation. I.e. this test will need to be > updated anytime we change what visuals are created by overview mode. This should > use friend access to the WindowSelectorItem class's member to fetch the widget > directly. I understand it's very dependent on the implementation. However, I don't see how we can use a friend function to solve this. The only thing that comes to my mind is making WindowSelectorItem hold a static pointer to the last label created... which is kinda ugly isn't it? How would you suggest doing it? https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1117: EXPECT_EQ(label->text(), window_title); On 2014/04/14 20:31:18, flackr wrote: > I'd expect that verifying the label is under the window would verify the > position. True, I'll work on this. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_window.cc (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_window.cc:108: set_bounds(window_bounds); On 2014/04/14 20:31:18, flackr wrote: > This change seems like a noop. Revert to old code since window_bounds is not > used? Done. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_window.cc:112: UpdateCloseButtonBounds(); On 2014/04/14 20:31:18, flackr wrote: > nit: Can you add a TODO to move the close button management to > WindowSelectorItem to be alongside the label management? It can reference > http://crbug.com/352143. Done. https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label... ui/views/controls/label.cc:346: enabled() ? enabled_shadow_color_ : disabled_shadow_color_)); On 2014/04/14 20:31:18, flackr wrote: > I'm concerned that this effect may not be necessary at all, if you have the > ability to set the blur and offset you should be able to get pretty much exactly > what was asked for by using a large enough blur shouldn't you? Good point! Yes, it works that way, so I removed this ugly code. https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label... ui/views/controls/label.h:99: void SetShadowBlur(double shadow_blur_); On 2014/04/14 20:31:18, flackr wrote: > Trivial plain old data type setters should be inlined in .h and use > unix_hacker_style(), i.e. set_mirror_shadow(bool mirror_shadow) { mirror_shadow_ > = mirror_shadow; } Done. https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label... ui/views/controls/label.h:103: void SetMirrorShadow(bool mirror_shadow); On 2014/04/14 20:31:18, flackr wrote: > ditto. Done. Actually, removed. https://codereview.chromium.org/231643002/diff/160001/ui/views/controls/label... ui/views/controls/label.h:304: // If true, surrender the text with the shadow. Default is false. On 2014/04/14 20:31:18, flackr wrote: > What do you mean by surrender the text with the shadow? This was removed, but anyway... I mean that if this is the shadow point: (x, y) instead of having only that linear shadow, you get: (x, y), (-x, y), (-x, -y), (x, -y) In plain words, that the shadow now surrounds the text from all sides instead of just one.
https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1097: aura::Window* root_window = Shell::GetContainer( On 2014/04/15 15:15:03, nsatragno wrote: > On 2014/04/14 20:31:18, flackr wrote: > > This isn't really the root window, call this overlay_container. > > Could you please elaborate a bit more on how to call overlay_container? I simply mean call this variable overlay_container instead of root_window since it is the overlay container and not the root window. https://codereview.chromium.org/231643002/diff/160001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1109: // vector. On 2014/04/15 15:15:03, nsatragno wrote: > On 2014/04/14 20:31:18, flackr wrote: > > This is very specific to the implementation. I.e. this test will need to be > > updated anytime we change what visuals are created by overview mode. This > should > > use friend access to the WindowSelectorItem class's member to fetch the widget > > directly. > > I understand it's very dependent on the implementation. However, I don't see how > we can use a friend function to solve this. The only thing that comes to my mind > is making WindowSelectorItem hold a static pointer to the last label created... > which is kinda ugly isn't it? How would you suggest doing it? WindowSelector has friend class WindowSelectorTest which means the base test class can directly access the windows_ vector. I'm proposing directly accessing the windows_ vector to get the WindowSelectorItem and directly accessing the label widget in that item. https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/scoped_... File ash/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/scoped_... ash/wm/overview/scoped_transform_overview_window.cc:90: const int ScopedTransformOverviewWindow::kFadeInMilliseconds = 80; This constant probably belongs in WindowSelectorItem since it isn't used by the ScopedTransformOverviewWindow. Eventually the close button handling should be in WindowSelectorItem too and this can be private to the WindowSelectorItem class. https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:56: label->set_shadow_blur(10); Seems like these should be constants as well, i.e. shadow blur and offset. https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:47: virtual aura::Window* SelectionWindow() const = 0; I think in general const functions should return const objects. You can make a second SelectionWindow method which returns a const aura::Window* similar to Window::GetRootWindow(): https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1130: FireOverviewStartTimer(); Same as above, please use ToggleOverview.
https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:14: #include "ash/test/shelf_view_test_api.h" You're getting the compile error because you removed the include of "ash/test/shell_test_api.h" which is still referenced by the DragDropInProgress test.
I fixed most of the issues detailed, except for the tests (which'll come in the next patchset). Also, fixed the problems related to multi-monitor on labels: now they show up correctly, and also move to the right monitor on alt-tabbing. Could you please take a look at it? Thanks! https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/scoped_... File ash/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/scoped_... ash/wm/overview/scoped_transform_overview_window.cc:90: const int ScopedTransformOverviewWindow::kFadeInMilliseconds = 80; On 2014/04/16 02:37:10, flackr wrote: > This constant probably belongs in WindowSelectorItem since it isn't used by the > ScopedTransformOverviewWindow. Eventually the close button handling should be in > WindowSelectorItem too and this can be private to the WindowSelectorItem class. Done. https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:56: label->set_shadow_blur(10); On 2014/04/16 02:37:10, flackr wrote: > Seems like these should be constants as well, i.e. shadow blur and offset. Done. https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/231643002/diff/180001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:47: virtual aura::Window* SelectionWindow() const = 0; On 2014/04/16 02:37:10, flackr wrote: > I think in general const functions should return const objects. You can make a > second SelectionWindow method which returns a const aura::Window* similar to > Window::GetRootWindow(): > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... > Done.
https://codereview.chromium.org/231643002/diff/200001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/231643002/diff/200001/ui/views/controls/label... ui/views/controls/label.h:98: double shadow_blur() const { return shadow_blur_; } Doesn't look like this is used anywhere?
On 2014/04/16 21:18:33, sadrul wrote: > https://codereview.chromium.org/231643002/diff/200001/ui/views/controls/label.h > File ui/views/controls/label.h (right): > > https://codereview.chromium.org/231643002/diff/200001/ui/views/controls/label... > ui/views/controls/label.h:98: double shadow_blur() const { return shadow_blur_; > } > Doesn't look like this is used anywhere? True, right now it isn't being used, it's just a simple accessor. Should I remove it, then?
On 2014/04/16 21:46:30, nsatragno wrote: > On 2014/04/16 21:18:33, sadrul wrote: > > > https://codereview.chromium.org/231643002/diff/200001/ui/views/controls/label.h > > File ui/views/controls/label.h (right): > > > > > https://codereview.chromium.org/231643002/diff/200001/ui/views/controls/label... > > ui/views/controls/label.h:98: double shadow_blur() const { return > shadow_blur_; > > } > > Doesn't look like this is used anywhere? > > True, right now it isn't being used, it's just a simple accessor. Should I > remove it, then? Yeah. If something needs this, we can add it back. For now, if there are no users, let's remove.
Okay, theoretically this should be it. I hope took care of all the issues, and revamped how the tests work. Would you mind giving it a look? Thanks!
https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:10: #include "ash/wm/overview/scoped_transform_overview_window.h" Don't think you need this anymore. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:12: #include "base/memory/scoped_ptr.h" Don't need this here, as you don't create any scoped_ptr's in the .cc, only use the one defined in the class header which already has to include scoped_ptr. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:37: static const int kLabelHeight = 20; To the label bottom? How is this the same as the vertical label padding? Shouldn't this be kVerticalLabelPadding + the font height, which we should be getting dynamically from the font? https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:96: UpdateWindowLabels(target_bounds, root_window); TODO(nsatragno): Handle window title updates. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:110: window_bounds.y() + window_bounds.height(), nit: window_bounds.bottom(). https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:113: label_bounds = ScreenUtil::ConvertRectFromScreen(root_window, label_bounds); nit: I'd prefer instead converting the window bounds to root window coordinates and only working in root window coordinates rather than working in screen coordinates and converting part-way through. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:27: friend class WindowSelectorTest; This belongs at the beginning of private: section. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:54: virtual const aura::Window* SelectionWindow() const = 0; I don't think you need the const version of this anymore since removing the GetLabelName function.. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1127: // Test that a label is created under the panel on entering overview mode. I think the only thing that's special about panels is that there could be more than one and only the active one will have a title listed. Could you change this test to create 2 panels and verify that the active one is the one whose title is used? https://codereview.chromium.org/231643002/diff/220001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/231643002/diff/220001/ui/views/controls/label... ui/views/controls/label.h:300: bool mirror_shadow_; unused, please remove.
Hi Rob, Thanks for the feedback, I took care of the issues. No features were added on this patch. Could you take another look? Thanks! https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:10: #include "ash/wm/overview/scoped_transform_overview_window.h" On 2014/04/17 03:44:29, flackr wrote: > Don't think you need this anymore. Done. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:10: #include "ash/wm/overview/scoped_transform_overview_window.h" On 2014/04/17 03:44:29, flackr wrote: > Don't think you need this anymore. We're using it for the milliseconds constant. When we move the close button code to WindowSelectorItem, we'll be able to remove it, I think. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:12: #include "base/memory/scoped_ptr.h" On 2014/04/17 03:44:29, flackr wrote: > Don't need this here, as you don't create any scoped_ptr's in the .cc, only use > the one defined in the class header which already has to include scoped_ptr. Done. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:37: static const int kLabelHeight = 20; On 2014/04/17 03:44:29, flackr wrote: > To the label bottom? How is this the same as the vertical label padding? > Shouldn't this be kVerticalLabelPadding + the font height, which we should be > getting dynamically from the font? Done. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:96: UpdateWindowLabels(target_bounds, root_window); On 2014/04/17 03:44:29, flackr wrote: > TODO(nsatragno): Handle window title updates. Added TODO comment https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:110: window_bounds.y() + window_bounds.height(), On 2014/04/17 03:44:29, flackr wrote: > nit: window_bounds.bottom(). Done. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:113: label_bounds = ScreenUtil::ConvertRectFromScreen(root_window, label_bounds); On 2014/04/17 03:44:29, flackr wrote: > nit: I'd prefer instead converting the window bounds to root window coordinates > and only working in root window coordinates rather than working in screen > coordinates and converting part-way through. Done. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:27: friend class WindowSelectorTest; On 2014/04/17 03:44:29, flackr wrote: > This belongs at the beginning of private: section. Done. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:54: virtual const aura::Window* SelectionWindow() const = 0; On 2014/04/17 03:44:29, flackr wrote: > I don't think you need the const version of this anymore since removing the > GetLabelName function.. Done. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1127: // Test that a label is created under the panel on entering overview mode. On 2014/04/17 03:44:29, flackr wrote: > I think the only thing that's special about panels is that there could be more > than one and only the active one will have a title listed. Could you change this > test to create 2 panels and verify that the active one is the one whose title is > used? Done. https://codereview.chromium.org/231643002/diff/220001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/231643002/diff/220001/ui/views/controls/label... ui/views/controls/label.h:300: bool mirror_shadow_; On 2014/04/17 03:44:29, flackr wrote: > unused, please remove. Snap, I though I had removed it. Done.
LGTM with nits. https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/231643002/diff/220001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:10: #include "ash/wm/overview/scoped_transform_overview_window.h" On 2014/04/17 15:02:19, nsatragno wrote: > On 2014/04/17 03:44:29, flackr wrote: > > Don't think you need this anymore. > > We're using it for the milliseconds constant. When we move the close button code > to WindowSelectorItem, we'll be able to remove it, I think. Oops, yes of course. I was thinking of the other constant. No problem. https://codereview.chromium.org/231643002/diff/240001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/231643002/diff/240001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:91: // TODO (nsatragno): Handle window title updates. nit: no space, TODO(nsatragno) https://codereview.chromium.org/231643002/diff/240001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/231643002/diff/240001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1127: // Test that a label is created under the panel on entering overview mode. nit: Update comment here: Tests that a label is created for the active panel in a group of panels in overview mode.
Nits taken care of. Sadrul, can you take a look? Thanks!
lgtm
The CQ bit was checked by tdanderson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/231643002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_dbg_simulator for step(s) base_unittests, compile, components_unittests, content_unittests, crypto_unittests, net_unittests, sql_unittests, ui_unittests, url_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
The CQ bit was checked by tdanderson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/231643002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_dbg_simulator for step(s) base_unittests, compile, components_unittests, content_unittests, crypto_unittests, net_unittests, sql_unittests, ui_unittests, url_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
The CQ bit was checked by tdanderson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/231643002/250001
Message was sent while issue was closed.
Change committed as 264602
Message was sent while issue was closed.
Failed to apply patch for ash/wm/overview/window_selector_item.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file ash/wm/overview/window_selector_item.cc
Hunk #1 FAILED at 3.
Hunk #2 FAILED at 24.
Hunk #3 succeeded at 152 with fuzz 1 (offset 118 lines).
2 out of 3 hunks FAILED -- saving rejects to file
ash/wm/overview/window_selector_item.cc.rej
Patch: ash/wm/overview/window_selector_item.cc
Index: ash/wm/overview/window_selector_item.cc
diff --git a/ash/wm/overview/window_selector_item.cc
b/ash/wm/overview/window_selector_item.cc
index
a2c25b617e9c872b1c2849209cc383ff59a14657..9866185cd9ca4487170bd80234c3fa7e483f7cc0
100644
--- a/ash/wm/overview/window_selector_item.cc
+++ b/ash/wm/overview/window_selector_item.cc
@@ -3,11 +3,75 @@
// found in the LICENSE file.
#include "ash/wm/overview/window_selector_item.h"
+
+#include "ash/screen_util.h"
+#include "ash/shell.h"
+#include "ash/shell_window_ids.h"
+#include "ash/wm/overview/scoped_transform_overview_window.h"
#include "base/auto_reset.h"
+#include "third_party/skia/include/core/SkColor.h"
#include "ui/aura/window.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/compositor/scoped_layer_animation_settings.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/layout/box_layout.h"
+#include "ui/views/widget/widget.h"
namespace ash {
+// Foreground label color.
+static const SkColor kLabelColor = SK_ColorWHITE;
+
+// Background label color.
+static const SkColor kLabelBackground = SK_ColorTRANSPARENT;
+
+// Label shadow color.
+static const SkColor kLabelShadow = 0xB0000000;
+
+// Vertical padding for the label, both over and beneath it.
+static const int kVerticalLabelPadding = 20;
+
+// Solid shadow length from the label
+static const int kVerticalShadowOffset = 1;
+
+// Amount of blur applied to the label shadow
+static const int kShadowBlur = 10;
+
+views::Widget* CreateWindowLabel(aura::Window* root_window,
+ const base::string16 title) {
+ views::Widget* widget = new views::Widget;
+ views::Widget::InitParams params;
+ params.type = views::Widget::InitParams::TYPE_POPUP;
+ params.can_activate = false;
+ params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+ params.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW;
+ params.parent =
+ Shell::GetContainer(root_window, ash::kShellWindowId_OverlayContainer);
+ params.accept_events = false;
+ params.visible_on_all_workspaces = true;
+ widget->set_focus_on_creation(false);
+ widget->Init(params);
+ views::Label* label = new views::Label;
+ label->SetEnabledColor(kLabelColor);
+ label->SetBackgroundColor(kLabelBackground);
+ label->SetShadowColors(kLabelShadow, kLabelShadow);
+ label->SetShadowOffset(0, kVerticalShadowOffset);
+ label->set_shadow_blur(kShadowBlur);
+ ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
+ label->SetFontList(bundle.GetFontList(ui::ResourceBundle::BoldFont));
+ label->SetText(title);
+ views::BoxLayout* layout = new views::BoxLayout(views::BoxLayout::kVertical,
+ 0,
+ kVerticalLabelPadding,
+ 0);
+ label->SetLayoutManager(layout);
+ widget->SetContentsView(label);
+ widget->Show();
+ return widget;
+}
+
+const int WindowSelectorItem::kFadeInMilliseconds = 80;
+
WindowSelectorItem::WindowSelectorItem()
: root_window_(NULL),
in_bounds_update_(false) {
@@ -24,6 +88,8 @@ void WindowSelectorItem::SetBounds(aura::Window* root_window,
root_window_ = root_window;
target_bounds_ = target_bounds;
SetItemBounds(root_window, target_bounds, true);
+ // TODO(nsatragno): Handle window title updates.
+ UpdateWindowLabels(target_bounds, root_window);
}
void WindowSelectorItem::RecomputeWindowTransforms() {
@@ -34,4 +100,56 @@ void WindowSelectorItem::RecomputeWindowTransforms() {
SetItemBounds(root_window_, target_bounds_, false);
}
+void WindowSelectorItem::UpdateWindowLabels(const gfx::Rect& window_bounds,
+ aura::Window* root_window) {
+ gfx::Rect converted_bounds = ScreenUtil::ConvertRectFromScreen(root_window,
+
window_bounds);
+ gfx::Rect label_bounds(converted_bounds.x(),
+ converted_bounds.bottom(),
+ converted_bounds.width(),
+ 0);
+
+ // If the root window has changed, force the window label to be recreated
+ // and faded in on the new root window.
+ if (window_label_ &&
+ window_label_->GetNativeWindow()->GetRootWindow() != root_window) {
+ window_label_.reset();
+ }
+
+ if (!window_label_) {
+ window_label_.reset(CreateWindowLabel(root_window,
+ SelectionWindow()->title()));
+ label_bounds.set_height(window_label_->
+ GetContentsView()->GetPreferredSize().height());
+ window_label_->GetNativeWindow()->SetBounds(label_bounds);
+ ui::Layer* layer = window_label_->GetNativeWindow()->layer();
+
+ layer->SetOpacity(0);
+ layer->GetAnimator()->StopAnimating();
+
+ layer->GetAnimator()->SchedulePauseForProperties(
+ base::TimeDelta::FromMilliseconds(
+ ScopedTransformOverviewWindow::kTransitionMilliseconds),
+ ui::LayerAnimationElement::OPACITY);
+
+ ui::ScopedLayerAnimationSettings settings(layer->GetAnimator());
+ settings.SetPreemptionStrategy(
+ ui::LayerAnimator::REPLACE_QUEUED_ANIMATIONS);
+ settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(
+ kFadeInMilliseconds));
+ layer->SetOpacity(1);
+ } else {
+ ui::ScopedLayerAnimationSettings settings(
+ window_label_->GetNativeWindow()->layer()->GetAnimator());
+ settings.SetPreemptionStrategy(
+ ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
+ settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(
+ ScopedTransformOverviewWindow::kTransitionMilliseconds));
+ label_bounds.set_height(window_label_->
+ GetContentsView()->GetPreferredSize().height());
+ window_label_->GetNativeWindow()->SetBounds(label_bounds);
+ }
+
+}
+
} // namespace ash
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
