apps: Move chrome's ShellWindowFrameView into apps/

apps: Move chrome's ShellWindowFrameView into apps/

BUG=306535
TEST=compiles, can still resize a v2 app (like "Text")
TBR=sky@chromium.org for moving shell_window_frame_view.h/cc out of its old location in Chrome

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228619

[James Cook, 2013-10-11 23:05:06]

stevenjb, PTAL especially at the ShellWindowFrameView part.

benwells, PTAL especially at what I did with a new directory (apps/ui/views),
GYP changes and DEPS.

https://codereview.chromium.org/27029006/diff/3001/apps/ui/views/shell_window...
File apps/ui/views/shell_window_frame_view.cc (left):

https://codereview.chromium.org/27029006/diff/3001/apps/ui/views/shell_window...
apps/ui/views/shell_window_frame_view.cc:184: if
(!expanded_bounds.Contains(point))
This was subtly wrong - the code should have been doing a bounds check even in
the non-Ash case.  See c/b/ui/views/frame/opaque_browser_frame_view.cc for
example.

https://codereview.chromium.org/27029006/diff/3001/apps/ui/views/shell_window...
File apps/ui/views/shell_window_frame_view.cc (right):

https://codereview.chromium.org/27029006/diff/3001/apps/ui/views/shell_window...
apps/ui/views/shell_window_frame_view.cc:171:
expanded_bounds.Inset(window->hit_test_bounds_override_outer_touch());
These bounds override insets are 0 in the normal (non-Ash) case, so it's safe to
always apply them.

https://codereview.chromium.org/27029006/diff/3001/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/27029006/diff/3001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/apps/native_app_window_views.cc:302: bool
NativeAppWindowViews::ShouldUseChromeStyleFrame() const {
I just moved it to match the header order.

[stevenjb, 2013-10-12 00:16:42]

https://codereview.chromium.org/27029006/diff/3001/apps/ui/views/shell_window...
File apps/ui/views/shell_window_frame_view.cc (right):

https://codereview.chromium.org/27029006/diff/3001/apps/ui/views/shell_window...
apps/ui/views/shell_window_frame_view.cc:8: #include
"chrome/browser/ui/views/apps/native_app_window_views.h"
Is the intention to check this in with the chrome dependency, or to fix that
first? It looks like this could take a apps::NativeAppWindow* instead of a
NativeAppWindowViews* with just a couple of minor additions to the
NativeAppWindowViews interface, which seems worth doing to avoid the dependency,
even if we just end up removing them later when we move NativeAppWindowViews to
ui/. (Although less dependency on ui/ isn't bad anyway).

https://codereview.chromium.org/27029006/diff/3001/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/apps/native_app_window_views.h (right):

https://codereview.chromium.org/27029006/diff/3001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/apps/native_app_window_views.h:53: SkRegion*
draggable_region() { return draggable_region_.get(); }
These two are the only additional public interfaces. HasFrame() and
GetDraggableRegion() seem like reasonable additions to NativeAppWindow, which
already has GetFrameInsets() and UpdateDraggableRegions().

[benwells, 2013-10-14 02:51:39]

Awesome :)

lgtm with stevenjb's suggestion about using the generic NativeAppWindow class.

Do you think the generic NativeAppWindow header file should go in the base ui
folder, now that we have it?

[James Cook, 2013-10-14 20:37:29]

stevenjb, please take another look

The declaration order vs. implementation order is a mess in these files. I'm
planning to clean it up in a later CL that just moves the code around.

[stevenjb, 2013-10-14 22:16:31]

LGTM with const comment

https://codereview.chromium.org/27029006/diff/13001/apps/native_app_window.h
File apps/native_app_window.h (right):

https://codereview.chromium.org/27029006/diff/13001/apps/native_app_window.h#...
apps/native_app_window.h:26: virtual SkRegion* GetDraggableRegion() const = 0;
Generally, if we return a non-const pointer, we don't make the method const. If
that's a problem (sometimes it is, especially with Views), this is fine, maybe
add a comment, otherwise return a const* or make non const.

[James Cook, 2013-10-14 22:25:51]

https://codereview.chromium.org/27029006/diff/13001/apps/native_app_window.h
File apps/native_app_window.h (right):

https://codereview.chromium.org/27029006/diff/13001/apps/native_app_window.h#...
apps/native_app_window.h:26: virtual SkRegion* GetDraggableRegion() const = 0;
On 2013/10/14 22:16:31, stevenjb wrote:
> Generally, if we return a non-const pointer, we don't make the method const.
If
> that's a problem (sometimes it is, especially with Views), this is fine, maybe
> add a comment, otherwise return a const* or make non const.

Oops, typo. Done.

[commit-bot: I haz the power, 2013-10-14 22:28:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/27029006/34001

[commit-bot: I haz the power, 2013-10-15 04:54:33]

Change committed as 228619

[fahadnmnmnmnm, 2014-09-13 12:52:10]

fahadnmnmnmnm@gmail.com changed reviewers:
+ fahadnmnmnmnm@gmail.com