Minimum size for aura window

Minimum size for aura window
Update restore bounds when SetBounds is called in Fullscreen/Maximized mode.

EventGenerator to generate fake events for various mouse operatings.

BUG=none
TEST=added test case for SetBounds in fullscreen/maximized mode. Added ToplevelWindow test that tests various toplevel operations.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105882

[oshima, 2011-10-14 15:17:05]

This will fix a crash when you resize window to negative size. I'm not 100%
certain about change in widget.cc. Please let me know if that's okay or should
be reverted.

I'll add more tests if the direction is ok.

[Ben Goodger (Google), 2011-10-14 15:40:04]

http://codereview.chromium.org/8273040/diff/1018/ui/aura/toplevel_window_even...
File ui/aura/toplevel_window_event_filter.cc (right):

http://codereview.chromium.org/8273040/diff/1018/ui/aura/toplevel_window_even...
ui/aura/toplevel_window_event_filter.cc:241: y_multiplier * (first_y - second_y)
: 0);
insert a newline after this line

http://codereview.chromium.org/8273040/diff/1018/ui/aura/toplevel_window_even...
ui/aura/toplevel_window_event_filter.cc:243: // Window should have minimum
window size.
Move this comment above the previous line. Reword to:

// Enforce minimum window size.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.cc
File ui/aura/window.cc (right):

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.cc#newcode375
ui/aura/window.cc:375: // TODO: funnel this through the Desktop.
You can remove this comment now.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.h
File ui/aura/window.h (right):

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.h#newcode114
ui/aura/window.h:114: // Sets the minimum size of the window that a user can
resize to.
can resize it to.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.h#newcode115
ui/aura/window.h:115: // A smaller size can still be set using SetBounds.
SetBounds().

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.h#newcode116
ui/aura/window.h:116: void SetMinimumSize(const gfx::Size& minimum_size);
Since this is a pure setter, you can call it set_minimum_size() and inline it
here.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:725: // Fullscreen twice
This comment just says what the code does, not what assumption it's testing
against. How about:

// Calling Fullscreen() twice should have no additional effect.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:733: // SetBounds in fullscreen mode
Same here.

// Calling SetBounds() while in full screen mode should only update the restored
bounds not change the bounds of the window.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:782: // SetBounds in maximized mode
Same as above.

http://codereview.chromium.org/8273040/diff/1018/views/widget/widget.h
File views/widget/widget.h (right):

http://codereview.chromium.org/8273040/diff/1018/views/widget/widget.h#newcod...
views/widget/widget.h:712: // The smallest size the window can be resized
// The smallest size the user can resize the window.

[oshima, 2011-10-15 02:10:49]

Uploaded new path. PTAL.

I added EventGenerator to generate fake events and used in the test. Added
ToplevelWindowTest. I noticed that there is
resize tests in toplevel_window_event_filter_unittest only after I wrote several
resize tests in widow_unittests.cc.
(my bad)

There are slight differences between them, so I'd like to keep it if it's okay
with you, or merge them. Please let me know.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.cc
File ui/aura/window.cc (right):

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.cc#newcode375
ui/aura/window.cc:375: // TODO: funnel this through the Desktop.
On 2011/10/14 15:40:05, Ben Goodger (Google) wrote:
> You can remove this comment now.

Done.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.h
File ui/aura/window.h (right):

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.h#newcode114
ui/aura/window.h:114: // Sets the minimum size of the window that a user can
resize to.
On 2011/10/14 15:40:05, Ben Goodger (Google) wrote:
> can resize it to.

Done.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.h#newcode115
ui/aura/window.h:115: // A smaller size can still be set using SetBounds.
On 2011/10/14 15:40:05, Ben Goodger (Google) wrote:
> SetBounds().

Done.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window.h#newcode116
ui/aura/window.h:116: void SetMinimumSize(const gfx::Size& minimum_size);
On 2011/10/14 15:40:05, Ben Goodger (Google) wrote:
> Since this is a pure setter, you can call it set_minimum_size() and inline it
> here.

Done.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:725: // Fullscreen twice
On 2011/10/14 15:40:05, Ben Goodger (Google) wrote:
> This comment just says what the code does, not what assumption it's testing
> against. How about:
> 
> // Calling Fullscreen() twice should have no additional effect.

Done.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:733: // SetBounds in fullscreen mode
On 2011/10/14 15:40:05, Ben Goodger (Google) wrote:
> Same here.
> 
> // Calling SetBounds() while in full screen mode should only update the
restored
> bounds not change the bounds of the window.

Done.

http://codereview.chromium.org/8273040/diff/1018/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:782: // SetBounds in maximized mode
On 2011/10/14 15:40:05, Ben Goodger (Google) wrote:
> Same as above.

Done.

http://codereview.chromium.org/8273040/diff/1018/views/widget/widget.h
File views/widget/widget.h (right):

http://codereview.chromium.org/8273040/diff/1018/views/widget/widget.h#newcod...
views/widget/widget.h:712: // The smallest size the window can be resized
On 2011/10/14 15:40:05, Ben Goodger (Google) wrote:
> // The smallest size the user can resize the window.

Done.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/toplevel_window_eve...
File ui/aura/toplevel_window_event_filter.cc (right):

http://codereview.chromium.org/8273040/diff/14001/ui/aura/toplevel_window_eve...
ui/aura/toplevel_window_event_filter.cc:132: case ui::ET_MOUSE_PRESSED:
This change was necessary to handle this scenario:
mouse drag to a, release then start dragging without moving mouse.

[Ben Goodger (Google), 2011-10-15 05:36:30]

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generator.h
File ui/aura/test/event_generator.h (right):

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:18: // various mouse operations.
EventGenerator is a tool that generates and dispatches events.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:19: // TODO(oshima): Support key event
events.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:23: EventGenerator(const gfx::Point&
initial_location);
explicit

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:32: // Generates events to click left button.
click (press, release)

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:35: // Generates events to move mouse to given
|point|.
to the

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:42: void MoveMouseBy(int x, int y) {
int dx, int dy

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:58: // Dispatch the |event| to desktop.
Dispatch |event| to the Desktop.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/toplevel_window_eve...
File ui/aura/toplevel_window_event_filter.cc (right):

http://codereview.chromium.org/8273040/diff/14001/ui/aura/toplevel_window_eve...
ui/aura/toplevel_window_event_filter.cc:132: case ui::ET_MOUSE_PRESSED:
We need to capture the window_component_ on every mouse move - it's used by
other code (like the cursor logic) that assumes that it is kept in sync with
mouse moves even if there are no mouse presses.

If you need to do this, do it in ET_MOUSE_PRESSED also.

Perhaps move it into a private function, UpdateWindowComponentForEvent(event);

http://codereview.chromium.org/8273040/diff/14001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8273040/diff/14001/ui/aura/window_unittest.cc#...
ui/aura/window_unittest.cc:879: class ComponentWindowDelegate : public
TestWindowDelegate {
This class and the test class above looks very similar to code in
toplevel_event_filter_unittest.cc. Since you're effectively testing its
functionality, you should move some of these tests into that file instead, and
you won't need this.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/window_unittest.cc#...
ui/aura/window_unittest.cc:932: TEST_F(ToplevelWindowTest, WindowResize) {
We already have tests for this in toplevel_event_filter_unittest, so i think you
can just skip this entire function.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/window_unittest.cc#...
ui/aura/window_unittest.cc:992: w1->set_minimum_size(gfx::Size(30, 30));
...except for this bit, which you can add a new test for in the other file.

[oshima, 2011-10-15 17:47:05]

addressed comments, deleted&merged tests.
PTAL

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generator.h
File ui/aura/test/event_generator.h (right):

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:18: // various mouse operations.
On 2011/10/15 05:36:30, Ben Goodger (Google) wrote:
> EventGenerator is a tool that generates and dispatches events.

Done.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:19: // TODO(oshima): Support key event
On 2011/10/15 05:36:30, Ben Goodger (Google) wrote:
> events.

Done.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:32: // Generates events to click left button.
On 2011/10/15 05:36:30, Ben Goodger (Google) wrote:
> click (press, release) 

Done.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:35: // Generates events to move mouse to given
|point|.
On 2011/10/15 05:36:30, Ben Goodger (Google) wrote:
> to the

Done.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/test/event_generato...
ui/aura/test/event_generator.h:42: void MoveMouseBy(int x, int y) {
On 2011/10/15 05:36:30, Ben Goodger (Google) wrote:
> int dx, int dy

Done.

http://codereview.chromium.org/8273040/diff/14001/ui/aura/toplevel_window_eve...
File ui/aura/toplevel_window_event_filter.cc (right):

http://codereview.chromium.org/8273040/diff/14001/ui/aura/toplevel_window_eve...
ui/aura/toplevel_window_event_filter.cc:132: case ui::ET_MOUSE_PRESSED:
On 2011/10/15 05:36:30, Ben Goodger (Google) wrote:
> We need to capture the window_component_ on every mouse move - it's used by
> other code (like the cursor logic) that assumes that it is kept in sync with
> mouse moves even if there are no mouse presses.
> 
> If you need to do this, do it in ET_MOUSE_PRESSED also.
> 
> Perhaps move it into a private function, UpdateWindowComponentForEvent(event);

Done.

[Ben Goodger (Google), 2011-10-16 22:23:33]

LGTM

http://codereview.chromium.org/8273040/diff/4003/ui/aura/test/event_generator.h
File ui/aura/test/event_generator.h (right):

http://codereview.chromium.org/8273040/diff/4003/ui/aura/test/event_generator...
ui/aura/test/event_generator.h:28: // Creates an EventGenerator with the mouse
location centered at |window|.
centered over |window|.

http://codereview.chromium.org/8273040/diff/4003/ui/aura/test/event_generator...
ui/aura/test/event_generator.h:75: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/8273040/diff/4003/ui/aura/toplevel_window_even...
File ui/aura/toplevel_window_event_filter.cc (right):

http://codereview.chromium.org/8273040/diff/4003/ui/aura/toplevel_window_even...
ui/aura/toplevel_window_event_filter.cc:135: case ui::ET_MOUSE_PRESSED:
Add this comment above this line:

We also update the current window component here because for the
mouse-drag-release case, where the mouse is released over a different window but
there is no mouse move event.

http://codereview.chromium.org/8273040/diff/4003/ui/aura/toplevel_window_even...
File ui/aura/toplevel_window_event_filter_unittest.cc (right):

http://codereview.chromium.org/8273040/diff/4003/ui/aura/toplevel_window_even...
ui/aura/toplevel_window_event_filter_unittest.cc:72:
generator.MoveMouseToCenterOf(window);
Can't you replace this with:

EventGenerator generator(window);

[oshima, 2011-10-17 19:30:57]

http://codereview.chromium.org/8273040/diff/4003/ui/aura/test/event_generator.h
File ui/aura/test/event_generator.h (right):

http://codereview.chromium.org/8273040/diff/4003/ui/aura/test/event_generator...
ui/aura/test/event_generator.h:28: // Creates an EventGenerator with the mouse
location centered at |window|.
On 2011/10/16 22:23:33, Ben Goodger (Google) wrote:
> centered over |window|.

Done.

http://codereview.chromium.org/8273040/diff/4003/ui/aura/test/event_generator...
ui/aura/test/event_generator.h:75: };
On 2011/10/16 22:23:33, Ben Goodger (Google) wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/8273040/diff/4003/ui/aura/toplevel_window_even...
File ui/aura/toplevel_window_event_filter.cc (right):

http://codereview.chromium.org/8273040/diff/4003/ui/aura/toplevel_window_even...
ui/aura/toplevel_window_event_filter.cc:135: case ui::ET_MOUSE_PRESSED:
On 2011/10/16 22:23:33, Ben Goodger (Google) wrote:
> Add this comment above this line:
> 
> We also update the current window component here because for the
> mouse-drag-release case, where the mouse is released over a different window
but
> there is no mouse move event.

This happens when mouse is released and then pressed without move. Updated and
added comment.

http://codereview.chromium.org/8273040/diff/4003/ui/aura/toplevel_window_even...
File ui/aura/toplevel_window_event_filter_unittest.cc (right):

http://codereview.chromium.org/8273040/diff/4003/ui/aura/toplevel_window_even...
ui/aura/toplevel_window_event_filter_unittest.cc:72:
generator.MoveMouseToCenterOf(window);
On 2011/10/16 22:23:33, Ben Goodger (Google) wrote:
> Can't you replace this with:
> 
> EventGenerator generator(window);

It's slightly different because it doesn't generate
move event, but test should pass with or without, so it's done.