Fixed incorrect frame insets for app windows on ChromeOS

Fixed incorrect frame insets for app windows on ChromeOS

HeaderPainter::GetWindowBoundsForClientBounds() shifts the Y
coordinate of the window bounds to ensure it is not negative.
This causes NativeAppWindowViews::GetFrameInsets() to compute
the wrong insets.

Therefore we should leave the Y coordinate of the window bounds
negative in HeaderPainter::GetWindowBoundsForClientBounds().

BUG=349220
TEST=browser_tests (AppWindowBrowserTest.*)

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

[tmdiep, 2014-03-05 01:18:31]

pkotwicz: for ash
benwells: for apps

Just some background on this patch for pkotwicz - a new app window bounds API
has been proposed, where app developers can work with inner bounds (client) or
outer bounds (window). Please see crbug.com/315471.

During the implementation of this new API, it was found that the existing
NativeAppWindow::GetFrameInsets() function was returning the wrong insets on a
number of platforms (the padding for the title bar ended up at the bottom,
rather than top), leading to inconsistencies and bugs in relation to getting and
setting bounds in the current API.

It is important to get the frame insets correct as it is used to transform
between inner and outer bounds in the new API.

[pkotwicz, 2014-03-05 01:43:57]

Thank you very much for the context.

LGTM!

[pkotwicz, 2014-03-05 01:53:59]

Actually, can you please make sure that your CL does not have any negative
interactions with BrowserView::GetSavedWindowPlacement()

[pkotwicz, 2014-03-05 03:54:11]

Still LGTM because BrowserNonClientFrameViewAsh's client bounds are the same as
its non client bounds.

However, you will need to deal with BrowserView::GetSavedWindowPlacement() when
you change OpaqueBrowserFrameViewLayout::GetWindowBoundsForClientBounds()

[tmdiep, 2014-03-05 05:28:09]

On 2014/03/05 03:54:11, pkotwicz wrote:
> Still LGTM because BrowserNonClientFrameViewAsh's client bounds are the same
as
> its non client bounds.
> 
> However, you will need to deal with BrowserView::GetSavedWindowPlacement()
when
> you change OpaqueBrowserFrameViewLayout::GetWindowBoundsForClientBounds()

I think it should be ok. Are you referring to these specific lines of code?

    gfx::Rect window_rect = frame_->non_client_view()->
        GetWindowBoundsForClientBounds(*bounds);
    window_rect.set_origin(bounds->origin());

We are effectively only using the width & height returned by
GetWindowBoundsForClientBounds() because in the next line, the original origin
from bounds is then copied over.

[benwells, 2014-03-05 06:32:23]

https://codereview.chromium.org/180803013/diff/30001/apps/app_window_browsert...
File apps/app_window_browsertest.cc (right):

https://codereview.chromium.org/180803013/diff/30001/apps/app_window_browsert...
apps/app_window_browsertest.cc:36: #if !defined(OS_LINUX) || defined(USE_AURA)
Are these just trying to disable on GTK? If so, defined(TOOLKIT_GTK) might be
mroe obvious.

[tmdiep, 2014-03-05 07:08:05]

https://codereview.chromium.org/180803013/diff/30001/apps/app_window_browsert...
File apps/app_window_browsertest.cc (right):

https://codereview.chromium.org/180803013/diff/30001/apps/app_window_browsert...
apps/app_window_browsertest.cc:36: #if !defined(OS_LINUX) || defined(USE_AURA)
On 2014/03/05 06:32:24, benwells wrote:
> Are these just trying to disable on GTK? If so, defined(TOOLKIT_GTK) might be
> mroe obvious.

Yes. Changed now.

[benwells, 2014-03-05 07:11:54]

lgtm

[tmdiep, 2014-03-07 01:53:44]

On 2014/03/05 05:28:09, tmdiep wrote:
> On 2014/03/05 03:54:11, pkotwicz wrote:
> > Still LGTM because BrowserNonClientFrameViewAsh's client bounds are the same
> as
> > its non client bounds.
> > 
> > However, you will need to deal with BrowserView::GetSavedWindowPlacement()
> when
> > you change OpaqueBrowserFrameViewLayout::GetWindowBoundsForClientBounds()
> 
> I think it should be ok. Are you referring to these specific lines of code?
> 
>     gfx::Rect window_rect = frame_->non_client_view()->
>         GetWindowBoundsForClientBounds(*bounds);
>     window_rect.set_origin(bounds->origin());
> 
> We are effectively only using the width & height returned by
> GetWindowBoundsForClientBounds() because in the next line, the original origin
> from bounds is then copied over.

pkotwicz: Just wondering whether you were satisfied with this reasoning (in case
you missed the email)? Thanks.

[pkotwicz, 2014-03-08 00:19:33]

Yes, I am satisfied with the reasoning :)

[tmdiep, 2014-03-09 22:27:22]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-03-09 22:27:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/180803013/50001

[commit-bot: I haz the power, 2014-03-10 06:18:51]

Change committed as 255878