Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
240163006:
Linux Aura Task Manager Frame Buttons Misaligned (Closed)
|
Created:
6 years, 8 months ago by jonross Modified:
6 years, 7 months ago CC:
chromium-reviews, tfarina, varkha Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionLinux Aura Task Manager Frame Buttons Misaligned
The task manager is a CustomFrameView. This had the logic for its button
placement hardcoded. The view now adds itself as a listener for button
configuration changes on linux. For other systems it falls back on a
default set that matches the current (minimize, maximize, close) order.
The layout of the buttons has changed to consult this order definition.
Loading the assets for the close button has been moved out of layout.
ResourceBundle only loads the asset once, so this can be done during
initialization.
The top padding for the buttons has been updated to provide more spacing. It now fits the rest of the padding in the class.
TEST=CustomFrameViewTest
BUG=351917
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269597
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Add #ifdefs #
Total comments: 8
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 14
Patch Set 5 : #Patch Set 6 : Refactor button observer out of CustomFrameView #
Total comments: 15
Patch Set 7 : Split out linux implementation #
Total comments: 2
Patch Set 8 : #
Total comments: 8
Patch Set 9 : #
Total comments: 6
Patch Set 10 : #
Total comments: 7
Patch Set 11 : . #Patch Set 12 : #
Messages
Total messages: 33 (0 generated)
https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:493: bool leading) { I don't think this wrapping conforms to the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi.... Should be wrapped to open bracket or all arguments wrapped. https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:527: button = minimize_button_; What if the button order specifies multiple of the same button? Can this happen? https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:552: switch (*it) { This duplication should be avoided. Maybe a common function to get the button? https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:595: std::max(0, maximum_title_bar_x_ - kTitleCaptionSpacing - Seems like it's very important that this is called after LayoutWindowControls, is this guaranteed? This layout order dependency should be documented in the header. https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:7: #include <vector> https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:12: #include "ui/views/linux_ui/window_button_order_observer.h" Including ui/views/linux_ui/ from all views platforms feels wrong, should this be guarded by a macro? i.e. #if defined(OS_LINUX) && !defined(OS_CHROMEOS) https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:36: public WindowButtonOrderObserver { nit: align public, which probably means wrapping the ':' https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:118: void LayoutButton(ImageButton* button, int x, int y, bool leading); I think it would be worth documenting this functionn as it's not obvious what all of these args should be. I.e. are x and y used directly or suggestions, what does leading mean?
I don't feel that I have enough understanding of this code yet. I suggest sadrul@ or sky@ (you would need one of them for the owners review).
https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:493: bool leading) { On 2014/04/22 18:36:45, flackr wrote: > I don't think this wrapping conforms to the style guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi.... > > Should be wrapped to open bracket or all arguments wrapped. Done. https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:527: button = minimize_button_; On 2014/04/22 18:36:45, flackr wrote: > What if the button order specifies multiple of the same button? Can this happen? Currently the last specified instance will determine the position of the button. The offsets then used for the title bar text will be incorrect. However only three buttons will ever be shown. The logic for loading the config is: GConfListener::ParseAndStoreButtonValue It provides no guarantees that the config file doesn't specify duplicates. However other handling code assumes that there are none. https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:552: switch (*it) { On 2014/04/22 18:36:45, flackr wrote: > This duplication should be avoided. Maybe a common function to get the button? Done. https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:595: std::max(0, maximum_title_bar_x_ - kTitleCaptionSpacing - On 2014/04/22 18:36:45, flackr wrote: > Seems like it's very important that this is called after LayoutWindowControls, > is this guaranteed? This layout order dependency should be documented in the > header. Not guaranteed by API. But it is private, and currently only called from Layout where we force the order. I've added comments to the docs about this. https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:7: On 2014/04/22 18:36:45, flackr wrote: > #include <vector> Done. https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:12: #include "ui/views/linux_ui/window_button_order_observer.h" On 2014/04/22 18:36:45, flackr wrote: > Including ui/views/linux_ui/ from all views platforms feels wrong, should this > be guarded by a macro? i.e. > #if defined(OS_LINUX) && !defined(OS_CHROMEOS) Done. https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:36: public WindowButtonOrderObserver { On 2014/04/22 18:36:45, flackr wrote: > nit: align public, which probably means wrapping the ':' Done. https://codereview.chromium.org/240163006/diff/20001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:118: void LayoutButton(ImageButton* button, int x, int y, bool leading); On 2014/04/22 18:36:45, flackr wrote: > I think it would be worth documenting this functionn as it's not obvious what > all of these args should be. I.e. are x and y used directly or suggestions, what > does leading mean? Removed the need for leading. Added comments.
Generally looks good, you should add an OWNER. https://codereview.chromium.org/240163006/diff/40001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/40001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:8: #include <vector> nit: separate system includes from the rest. i.e. blank line between this and basictypes. https://codereview.chromium.org/240163006/diff/40001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:14: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) nit: Move this to the end (i.e. after ui/views/window/non_client_view.h). https://codereview.chromium.org/240163006/diff/40001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:45: #endif I hate that we have to duplicate the '{', kind of tempted to suggest inserting WindowButtonOrderObserver before ButtonListener so that there's no #else and it's a one liner :-). https://codereview.chromium.org/240163006/diff/40001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:150: // Returns the window caption button for the given FrameButton, if it should Since FrameButton is an enum essentially specifying the type of the button, how about saying the given FrameButton type for clarity.
https://codereview.chromium.org/240163006/diff/40001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/40001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:8: #include <vector> On 2014/04/24 21:40:13, flackr wrote: > nit: separate system includes from the rest. i.e. blank line between this and > basictypes. Done. https://codereview.chromium.org/240163006/diff/40001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:14: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) On 2014/04/24 21:40:13, flackr wrote: > nit: Move this to the end (i.e. after ui/views/window/non_client_view.h). Done. https://codereview.chromium.org/240163006/diff/40001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:45: #endif On 2014/04/24 21:40:13, flackr wrote: > I hate that we have to duplicate the '{', kind of tempted to suggest inserting > WindowButtonOrderObserver before ButtonListener so that there's no #else and > it's a one liner :-). Done. https://codereview.chromium.org/240163006/diff/40001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:150: // Returns the window caption button for the given FrameButton, if it should On 2014/04/24 21:40:13, flackr wrote: > Since FrameButton is an enum essentially specifying the type of the button, how > about saying the given FrameButton type for clarity. Done.
Hi Sadrul, Could you provide OWNER review for my changes to CustomFrameView?
https://codereview.chromium.org/240163006/diff/60001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/60001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:90: maximum_title_bar_x_(INT_MAX) { Rather than initializing this to INT_MAX, since that value should never be used, how about initializing to 0 or an invalid value like -1 and then you could DCHECK that it is not still -1 when laying out the title bar?
https://codereview.chromium.org/240163006/diff/60001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/60001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:90: maximum_title_bar_x_(INT_MAX) { On 2014/04/25 16:04:18, flackr wrote: > Rather than initializing this to INT_MAX, since that value should never be used, > how about initializing to 0 or an invalid value like -1 and then you could > DCHECK that it is not still -1 when laying out the title bar? Done.
https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:22: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) Move to end after always included files. https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:104: nit: Probably don't need blank lines between all of these button initializations. https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:124: ui->AddWindowButtonOrderObserver(this); Should probably call RemoveWindowButtonOrderObserver, also maybe move this add observer to the constructor so that they're guaranteed to always be called. https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:546: DCHECK(maximum_title_bar_x_ != -1); How about DCHECK_GE(maximum_title_bar_x_, 0); https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:611: button->SetVisible(false); Why change the button visibility when returning NULL? Shouldn't only buttons which are returned be in the frame? https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:180: std::vector<views::FrameButton> trailing_buttons_; It's unfortunate to have a copy of these button orders for every CustomFrameView when as far as I can tell the button order is global, and on all but one platform always the same. I can see this is what we currently do in OpaqueBrowserFrameViewLinux, but perhaps it would be worth generalizing both of these to use a single static button order. This might be a lot of refactoring though and I'm fine with deferring it.
+erg@ https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:267: //// CustomFrameView, ButtonListener implementation: Two // https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:41: public WindowButtonOrderObserver, Instead of making the CustomFrameView a WindowButtonOrderObserver, can there be a WindowButtonOrderObserver implementation that the CustomFrameView conditionally owns on linux-aura?
https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:22: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) On 2014/04/29 15:58:28, flackr wrote: > Move to end after always included files. Done. https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:104: On 2014/04/29 15:58:28, flackr wrote: > nit: Probably don't need blank lines between all of these button > initializations. Done. https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:124: ui->AddWindowButtonOrderObserver(this); On 2014/04/29 15:58:28, flackr wrote: > Should probably call RemoveWindowButtonOrderObserver, also maybe move this add > observer to the constructor so that they're guaranteed to always be called. Done. https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:546: DCHECK(maximum_title_bar_x_ != -1); On 2014/04/29 15:58:28, flackr wrote: > How about DCHECK_GE(maximum_title_bar_x_, 0); Done. https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.cc:611: button->SetVisible(false); Added a comment. Basically we want this button not visible. So we don't want it toggled to visible, or to be laid out. Adapted from the original static layout On 2014/04/29 15:58:28, flackr wrote: > Why change the button visibility when returning NULL? Shouldn't only buttons > which are returned be in the frame? https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/80001/ui/views/window/custom_f... ui/views/window/custom_frame_view.h:180: std::vector<views::FrameButton> trailing_buttons_; Added a TODO On 2014/04/29 15:58:28, flackr wrote: > It's unfortunate to have a copy of these button orders for every CustomFrameView > when as far as I can tell the button order is global, and on all but one > platform always the same. I can see this is what we currently do in > OpaqueBrowserFrameViewLinux, but perhaps it would be worth generalizing both of > these to use a single static button order. This might be a lot of refactoring > though and I'm fine with deferring it.
I have refactored the WindowButtonOrderObserver out of CustomFrameView. I now have no linux specific changes to CustomFrameView. I also followed flackr@ suggestion of making the button ordering a static location.
https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:467: void CustomFrameView::LayoutButton(ImageButton* button, int x, int y) { This doesn't seem to use any members from the class, perhaps make this an anonymous function. https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:472: button->SetBounds(x, y, button_size.width(), button_size.height()); Wait, don't we need to still extend the button width if it's the right/left most button and the window is maximized? (As per comment on line 485). https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:511: minimum_title_bar_x_ = next_button_x; min(width, next_button_x)? https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:525: maximum_title_bar_x_ = std::max(0, next_button_x); Maybe this should be min(minimum_title_bar_x_, next_button_x) https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.h:8: #include <vector> No longer used. https://codereview.chromium.org/240163006/diff/140001/ui/views/window/window_... File ui/views/window/window_button_order_provider.cc (right): https://codereview.chromium.org/240163006/diff/140001/ui/views/window/window_... ui/views/window/window_button_order_provider.cc:17: class WindowButtonOrderObserverDelegate : public WindowButtonOrderProvider, I think this implementation of WindowButtonOrderProvider should be in a linux specific file/folder. In fact, there's a standard pattern for classes which have a common interface but platform specific implementations where we have a single header file and multiple cc files by platform. See ShowCertificateViewer for example: https://code.google.com/p/chromium/codesearch#search/&q=void%5C%20showcertifi... Then we just compile the implementation that matches the platform using gyp rules. Can't help but wonder if the LinuxUI instance should be a WindowButtonOrderProvider rather than observe it and store in a separate class. https://codereview.chromium.org/240163006/diff/140001/ui/views/window/window_... ui/views/window/window_button_order_provider.cc:104: nit: no extra newline at end of file.
https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:467: void CustomFrameView::LayoutButton(ImageButton* button, int x, int y) { On 2014/05/01 15:43:36, flackr wrote: > This doesn't seem to use any members from the class, perhaps make this an > anonymous function. Done. https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:472: button->SetBounds(x, y, button_size.width(), button_size.height()); On 2014/05/01 15:43:36, flackr wrote: > Wait, don't we need to still extend the button width if it's the right/left most > button and the window is maximized? (As per comment on line 485). That is accomplished by offsetting the next_button_x which is provided to this method. This method simply places the button. https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:511: minimum_title_bar_x_ = next_button_x; On 2014/05/01 15:43:36, flackr wrote: > min(width, next_button_x)? Done. https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:525: maximum_title_bar_x_ = std::max(0, next_button_x); On 2014/05/01 15:43:36, flackr wrote: > Maybe this should be min(minimum_title_bar_x_, next_button_x) Done. https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.h:8: #include <vector> On 2014/05/01 15:43:36, flackr wrote: > No longer used. Done. https://codereview.chromium.org/240163006/diff/140001/ui/views/window/window_... File ui/views/window/window_button_order_provider.cc (right): https://codereview.chromium.org/240163006/diff/140001/ui/views/window/window_... ui/views/window/window_button_order_provider.cc:17: class WindowButtonOrderObserverDelegate : public WindowButtonOrderProvider, On 2014/05/01 15:43:36, flackr wrote: > I think this implementation of WindowButtonOrderProvider should be in a linux > specific file/folder. > > In fact, there's a standard pattern for classes which have a common interface > but platform specific implementations where we have a single header file and > multiple cc files by platform. See ShowCertificateViewer for example: > https://code.google.com/p/chromium/codesearch#search/&q=void%5C%20showcertifi... > Then we just compile the implementation that matches the platform using gyp > rules. > > Can't help but wonder if the LinuxUI instance should be a > WindowButtonOrderProvider rather than observe it and store in a separate class. Done.
https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:472: button->SetBounds(x, y, button_size.width(), button_size.height()); On 2014/05/02 17:53:54, jonross wrote: > On 2014/05/01 15:43:36, flackr wrote: > > Wait, don't we need to still extend the button width if it's the right/left > most > > button and the window is maximized? (As per comment on line 485). > > That is accomplished by offsetting the next_button_x which is provided to this > method. This method simply places the button. But the old code actually gives the button a larger width when calling SetBounds, does not not make the button slightly larger so that clicking in the right corner of the screen when a window is maximized still hits the corner button even though it's not normally in the top-right corner? https://codereview.chromium.org/240163006/diff/160001/ui/views/window/window_... File ui/views/window/window_button_order_provider.h (right): https://codereview.chromium.org/240163006/diff/160001/ui/views/window/window_... ui/views/window/window_button_order_provider.h:11: #include "base/memory/scoped_ptr.h" unused.
https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/140001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:472: button->SetBounds(x, y, button_size.width(), button_size.height()); Sorry, lost that part during the refactor. I've re-added it. On 2014/05/06 02:35:01, flackr wrote: > On 2014/05/02 17:53:54, jonross wrote: > > On 2014/05/01 15:43:36, flackr wrote: > > > Wait, don't we need to still extend the button width if it's the right/left > > most > > > button and the window is maximized? (As per comment on line 485). > > > > That is accomplished by offsetting the next_button_x which is provided to this > > method. This method simply places the button. > > But the old code actually gives the button a larger width when calling > SetBounds, does not not make the button slightly larger so that clicking in the > right corner of the screen when a window is maximized still hits the corner > button even though it's not normally in the top-right corner? https://codereview.chromium.org/240163006/diff/160001/ui/views/window/window_... File ui/views/window/window_button_order_provider.h (right): https://codereview.chromium.org/240163006/diff/160001/ui/views/window/window_... ui/views/window/window_button_order_provider.h:11: #include "base/memory/scoped_ptr.h" On 2014/05/06 02:35:01, flackr wrote: > unused. Done.
https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:73: void LayoutButton(ImageButton* button, int x, int y, int extra_width) { It'd probably be better to pass in the target size rect than have to separately account for the extra_width twice (line 518 and line 527) and ask for the preferred size twice (line 524 and line 77). https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:512: (it == leading_buttons.begin())? extra_width : 0); nit: space before ?, no brackets around condition. https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:527: (it == trailing_buttons.rend())? extra_width : 0); ditto https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... File ui/views/window/custom_frame_view_unittest.cc (right): https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... ui/views/window/custom_frame_view_unittest.cc:225: Perhaps it's worth a test to verify the extra width, i.e. button is slightly larger and aligned to the edge of the screen on a maximized window since this is easy to overlook.
https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:73: void LayoutButton(ImageButton* button, int x, int y, int extra_width) { On 2014/05/07 00:37:58, flackr wrote: > It'd probably be better to pass in the target size rect than have to separately > account for the extra_width twice (line 518 and line 527) and ask for the > preferred size twice (line 524 and line 77). Done. https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:512: (it == leading_buttons.begin())? extra_width : 0); On 2014/05/07 00:37:58, flackr wrote: > nit: space before ?, no brackets around condition. Done. https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:527: (it == trailing_buttons.rend())? extra_width : 0); On 2014/05/07 00:37:58, flackr wrote: > ditto Done. https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... File ui/views/window/custom_frame_view_unittest.cc (right): https://codereview.chromium.org/240163006/diff/180001/ui/views/window/custom_... ui/views/window/custom_frame_view_unittest.cc:225: On 2014/05/07 00:37:58, flackr wrote: > Perhaps it's worth a test to verify the extra width, i.e. button is slightly > larger and aligned to the edge of the screen on a maximized window since this is > easy to overlook. Done.
https://codereview.chromium.org/240163006/diff/200001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/200001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:73: void LayoutButton(ImageButton* button, gfx::Rect& bounds) { nit: const gfx::Rect& bounds https://codereview.chromium.org/240163006/diff/200001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:512: (it == leading_buttons.begin() ? extra_width : 0), This should be doable with just the target_bounds rect to avoid also having the button_size class which will be incorrect for the first button. gfx::Rect target_bounds(button->GetPreferredSize()); if (it == leading_buttons.begin()) target_bounds.set_width(extra_width); target_bounds->set* Or, in this case where the location is known at construction time: gfx::Rect target_bounds(gfx::Point(next_button_x, caption_y), button->GetPreferredSize()); ... https://codereview.chromium.org/240163006/diff/200001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:520: next_button_x = width() - FrameBorderThickness() - extra_width; This "- extra_width" should be unnecessary since you can use the button's target_bounds.width() to set the position.
https://codereview.chromium.org/240163006/diff/200001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/200001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:73: void LayoutButton(ImageButton* button, gfx::Rect& bounds) { On 2014/05/07 21:51:05, flackr wrote: > nit: const gfx::Rect& bounds Done. https://codereview.chromium.org/240163006/diff/200001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:512: (it == leading_buttons.begin() ? extra_width : 0), On 2014/05/07 21:51:05, flackr wrote: > This should be doable with just the target_bounds rect to avoid also having the > button_size class which will be incorrect for the first button. > > gfx::Rect target_bounds(button->GetPreferredSize()); > if (it == leading_buttons.begin()) > target_bounds.set_width(extra_width); > target_bounds->set* > > Or, in this case where the location is known at construction time: > > gfx::Rect target_bounds(gfx::Point(next_button_x, caption_y), > button->GetPreferredSize()); > ... Done. https://codereview.chromium.org/240163006/diff/200001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:520: next_button_x = width() - FrameBorderThickness() - extra_width; On 2014/05/07 21:51:05, flackr wrote: > This "- extra_width" should be unnecessary since you can use the button's > target_bounds.width() to set the position. Done.
LGTM with one issue. https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:519: next_button_x = width() - FrameBorderThickness() - extra_width; This shouldn't be removing extra width since it's already accounted for below.
lgtm https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... ui/views/window/custom_frame_view.h:41: // NonClientFrameView: Please don't remove the "Overridden from"; the majority of our code has this and it should be kept for consistency.
https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... ui/views/window/custom_frame_view.h:41: // NonClientFrameView: On 2014/05/08 19:23:40, Elliot Glaysher wrote: > Please don't remove the "Overridden from"; the majority of our code has this and > it should be kept for consistency. I had been told that "Overridden from" was the older style. And had been asked on previous reviews to update any instances in files I touched. I can revert those changes here if desired.
https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... File ui/views/window/custom_frame_view.h (right): https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... ui/views/window/custom_frame_view.h:41: // NonClientFrameView: On 2014/05/08 19:29:56, jonross wrote: > On 2014/05/08 19:23:40, Elliot Glaysher wrote: > > Please don't remove the "Overridden from"; the majority of our code has this > and > > it should be kept for consistency. > > I had been told that "Overridden from" was the older style. And had been asked > on previous reviews to update any instances in files I touched. > > I can revert those changes here if desired. FWIW I was also told that this was outdated in some previous reviews.
On 2014/05/08 19:32:48, varkha wrote: > https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... > File ui/views/window/custom_frame_view.h (right): > > https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... > ui/views/window/custom_frame_view.h:41: // NonClientFrameView: > On 2014/05/08 19:29:56, jonross wrote: > > On 2014/05/08 19:23:40, Elliot Glaysher wrote: > > > Please don't remove the "Overridden from"; the majority of our code has this > > and > > > it should be kept for consistency. > > > > I had been told that "Overridden from" was the older style. And had been asked > > on previous reviews to update any instances in files I touched. > > > > I can revert those changes here if desired. > > FWIW I was also told that this was outdated in some previous reviews. A quick tally (correct me if my regex's are wrong): git grep "// [A-Za-z:]*:$"|wc -l 3566 git grep "// Overridden from "|wc -l 1644 Not sure how we get consensus on things like these but we should do that and add to chromium style guide so there's no confusion :-).
https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:518: // Trailing buttions are laid out in a RTL fashion buttons Do you need to change the layout for RTL environments?
https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:518: // Trailing buttions are laid out in a RTL fashion On 2014/05/09 13:53:09, sadrul wrote: > buttons > > Do you need to change the layout for RTL environments? One of the outer containers already reverses the entire layout. So no RTL locale logic is needed here. https://codereview.chromium.org/240163006/diff/220001/ui/views/window/custom_... ui/views/window/custom_frame_view.cc:519: next_button_x = width() - FrameBorderThickness() - extra_width; On 2014/05/08 19:04:54, flackr wrote: > This shouldn't be removing extra width since it's already accounted for below. Done.
lgtm
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jonross@chromium.org/240163006/260001
Message was sent while issue was closed.
Change committed as 269597
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/276173002/ by mdempsky@chromium.org. The reason for reverting is: Suspected of being responsible for ash_unittests failing on Linux ChromiumOS Tests(1): http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2... . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
