[WIN]Save work area of window and adjust bounds to ensure it fit on screen before show.

[WIN]Save work area of window and adjust bounds to ensure it fit on screen before show.

BUG=145752, 225734
TEST=unit_tests --gtest_filter=ShellWindowGeometryCacheTest.*
TEST=On Windows platform, Please refer to the description of crbug.com/225734.

[zhchbin, 2013-06-18 08:09:30]

Hi scheib, this is my proposal patch for the issue on Windows, please review. If
this works fine, I will take a look on Linux Platform. Thanks.

[scheib, 2013-06-18 21:52:06]

Thank you for this work, looking good, though some work still to be done, see
below.

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/cocoa/exten...
File chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/cocoa/exten...
chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.mm:718:
NOTIMPLEMENTED();
I'd like to be able to land your patch (giving you credit) and then follow up in
later patches with implementations for other platforms. Please change
NOTIMPLEMENTED to TODO comments and make the cross platform code run without
error when GetWindowWorkArea is not implemented.

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:199: // App window has cached work
area, make sure it fits on screen in case of the
Why not place this above, before new_params.bounds = bounds is called. Then the
NativeAppWindow::Create will take the adjusted initial conditions.

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:638: void
ShellWindow::AdjustBoundsToBeVisibleOnMonitorContaining(
Monitor is inconsistent with naming elsewhere. 'Work Area' is used in most
places -- but isn't as familiar in the context of a window. Please use 'Screen
Bounds' or 'Screen Size' or 'Screen Area' everywhere.

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:638: void
ShellWindow::AdjustBoundsToBeVisibleOnMonitorContaining(
Please add several tests for this function, for shrinking, enlarging, and screen
sizes that do both, e.g. get wider and shorter at the same time --- combined
with window positions partially and completely off the screen on multiple edges.

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:656:
bounds->set_width(std::min(bounds->width(), work_area.width()));
We can not resize to a smaller window than minimum_size. Yes, the developer may
set a min size that is larger than a screen and force a user to have a window
that is too big. Then the logic below will move window to top left corner of
screen.
We should doc that too:
https://code.google.com/p/chromium/issues/detail?id=251439

[zhchbin, 2013-06-19 05:43:32]

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:199: // App window has cached work
area, make sure it fits on screen in case of the
On 2013/06/18 21:52:06, scheib wrote:
> Why not place this above, before new_params.bounds = bounds is called. Then
the
> NativeAppWindow::Create will take the adjusted initial conditions.

Because in the AdjustBoundsToBeVisibleOnMonitorContaining, it need to get the
current screen bounds for the native app window. So native app window should be
created first.

[scheib, 2013-06-20 00:09:44]

Thank you. FYI general chromium review style is to reply to all comments, even
if you take the reviewer comment and implement it as directed. You will see a
'Done' option for comments, which keeps these replies succinct.

[scheib, 2013-06-20 00:09:58]

https://codereview.chromium.org/17378003/diff/7001/apps/shell_window_geometry...
File apps/shell_window_geometry_cache.h (right):

https://codereview.chromium.org/17378003/diff/7001/apps/shell_window_geometry...
apps/shell_window_geometry_cache.h:101: WindowData() :
window_state(ui::SHOW_STATE_DEFAULT) {}
I ran try jobs, and a style issue has been tripped:

In file included from ../../apps/shell_window_geometry_cache.cc:5:
../../apps/shell_window_geometry_cache.h:101:5: error: [chromium-style] Complex
constructor has an inlined body.
    WindowData() : window_state(ui::SHOW_STATE_DEFAULT) {}

Which is explained on
http://www.chromium.org/developers/coding-style/chromium-style-checker-errors

Because the object is large enough, it requires you to move the constructor to
the .cc file.

https://codereview.chromium.org/17378003/diff/7001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/17378003/diff/7001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:199: // App window has cached
screen bounds, make sure it fits on screen in case of
> > Why not place this above, before new_params.bounds = bounds is called. Then
> the
> > NativeAppWindow::Create will take the adjusted initial conditions.
> 
> Because in the AdjustBoundsToBeVisibleOnMonitorContaining, it need to get the
> current screen bounds for the native app window. So native app window should
be
> created first.

Please add that explanation to the comment here.

https://codereview.chromium.org/17378003/diff/7001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:643: void
ShellWindow::AdjustBoundsToBeVisibleOnScreen(
Please add tests for AdjustBoundsToBeVisibleOnScreen, e.g. testing scenarios
with cached bounds overlapping or completely off the new screen bounds in both
horizontal and vertical locations. You can refactor the member to be static,
allowing simple 'unit test' style calls for it without requiring much setup.

[zhchbin, 2013-06-20 06:35:50]

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/cocoa/exten...
File chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/cocoa/exten...
chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.mm:718:
NOTIMPLEMENTED();
On 2013/06/18 21:52:06, scheib wrote:
> I'd like to be able to land your patch (giving you credit) and then follow up
in
> later patches with implementations for other platforms. Please change
> NOTIMPLEMENTED to TODO comments and make the cross platform code run without
> error when GetWindowWorkArea is not implemented.

Done.

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:638: void
ShellWindow::AdjustBoundsToBeVisibleOnMonitorContaining(
On 2013/06/18 21:52:06, scheib wrote:
> Monitor is inconsistent with naming elsewhere. 'Work Area' is used in most
> places -- but isn't as familiar in the context of a window. Please use 'Screen
> Bounds' or 'Screen Size' or 'Screen Area' everywhere.

Done.

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:638: void
ShellWindow::AdjustBoundsToBeVisibleOnMonitorContaining(
On 2013/06/18 21:52:06, scheib wrote:
> Monitor is inconsistent with naming elsewhere. 'Work Area' is used in most
> places -- but isn't as familiar in the context of a window. Please use 'Screen
> Bounds' or 'Screen Size' or 'Screen Area' everywhere.

Done.

https://codereview.chromium.org/17378003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:656:
bounds->set_width(std::min(bounds->width(), work_area.width()));
On 2013/06/18 21:52:06, scheib wrote:
> We can not resize to a smaller window than minimum_size. Yes, the developer
may
> set a min size that is larger than a screen and force a user to have a window
> that is too big. Then the logic below will move window to top left corner of
> screen.
> We should doc that too:
> https://code.google.com/p/chromium/issues/detail?id=251439

Done.

[zhchbin, 2013-06-20 06:50:52]

While writing the test cases, I have an idea that we can just reposition the
window as its first run instead of doing the calculation, then reposition. I
think it will be better than current strategy which I'm working.

By the way, the patch just handle the case where the window is entirely
offscreen and thus not reachable as:
https://code.google.com/p/chromium/issues/detail?id=145752#c14

https://codereview.chromium.org/17378003/diff/7001/apps/shell_window_geometry...
File apps/shell_window_geometry_cache.h (right):

https://codereview.chromium.org/17378003/diff/7001/apps/shell_window_geometry...
apps/shell_window_geometry_cache.h:101: WindowData() :
window_state(ui::SHOW_STATE_DEFAULT) {}
On 2013/06/20 00:09:59, scheib wrote:
> I ran try jobs, and a style issue has been tripped:
> 
> In file included from ../../apps/shell_window_geometry_cache.cc:5:
> ../../apps/shell_window_geometry_cache.h:101:5: error: [chromium-style]
Complex
> constructor has an inlined body.
>     WindowData() : window_state(ui::SHOW_STATE_DEFAULT) {}
> 
> Which is explained on
> http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
> 
> Because the object is large enough, it requires you to move the constructor to
> the .cc file.

Done.

https://codereview.chromium.org/17378003/diff/7001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/17378003/diff/7001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:199: // App window has cached
screen bounds, make sure it fits on screen in case of
On 2013/06/20 00:09:59, scheib wrote:
> > > Why not place this above, before new_params.bounds = bounds is called.
Then
> > the
> > > NativeAppWindow::Create will take the adjusted initial conditions.
> > 
> > Because in the AdjustBoundsToBeVisibleOnMonitorContaining, it need to get
the
> > current screen bounds for the native app window. So native app window should
> be
> > created first.
> 
> Please add that explanation to the comment here.

Done.

https://codereview.chromium.org/17378003/diff/7001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:643: void
ShellWindow::AdjustBoundsToBeVisibleOnScreen(
On 2013/06/20 00:09:59, scheib wrote:
> Please add tests for AdjustBoundsToBeVisibleOnScreen, e.g. testing scenarios
> with cached bounds overlapping or completely off the new screen bounds in both
> horizontal and vertical locations. You can refactor the member to be static,
> allowing simple 'unit test' style calls for it without requiring much setup.

I had integrated it into the platform_app_browsertest. And I think private
helper method will be better than the static. Is it OK?

[oshima, 2013-06-20 19:46:32]

https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):

https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/native_app_window_views.cc:177:
window_->SetBounds(adjusted_bounds);
The reason why it's currently broken on chromeos is that
this code sets bounds AFTER Widget::Init. ChromeOS already have
a logic to fit to the screen when created. I also think chrome must
have the same logic as we don't want to restore chrome offscreen.
Can't we just set the bounds in init_params, and leave it to underlying logic?

[zhchbin, 2013-06-21 06:36:31]

https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):

https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/native_app_window_views.cc:177:
window_->SetBounds(adjusted_bounds);
On 2013/06/20 19:46:32, oshima wrote:
> The reason why it's currently broken on chromeos is that
> this code sets bounds AFTER Widget::Init. ChromeOS already have
> a logic to fit to the screen when created. I also think chrome must
> have the same logic as we don't want to restore chrome offscreen.
> Can't we just set the bounds in init_params, and leave it to underlying logic?

My patch didn't change these lines. I think the right code position that broke
chromeos is native_app_window_->SetBounds(new_bounds); in the shell_window.cc
line 210(My patch). However, now I'm just working on windows for the problem,
and I can't reproduce this problem on Linux(GTK+) and MAC platform(With the
steps on the issue 225734). I will update this patch to windows only. Do you and
@scheib agree? Hope this code review wouldn't waste your time.

[oshima, 2013-06-21 16:13:30]

https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):

https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/native_app_window_views.cc:177:
window_->SetBounds(adjusted_bounds);
On 2013/06/21 06:36:31, zhchbin wrote:
> On 2013/06/20 19:46:32, oshima wrote:
> > The reason why it's currently broken on chromeos is that
> > this code sets bounds AFTER Widget::Init. ChromeOS already have
> > a logic to fit to the screen when created. I also think chrome must
> > have the same logic as we don't want to restore chrome offscreen.
> > Can't we just set the bounds in init_params, and leave it to underlying
logic?
> 
> My patch didn't change these lines. I think the right code position that broke
> chromeos is native_app_window_->SetBounds(new_bounds); in the shell_window.cc
> line 210(My patch).

Sorry I lied. I mixed up with something else, so please ignore my comments
above.
I'll check existing code again and will comment a bit later.

> However, now I'm just working on windows for the problem,
> and I can't reproduce this problem on Linux(GTK+) and MAC platform(With the
> steps on the issue 225734). I will update this patch to windows only. Do you
and
> @scheib agree? Hope this code review wouldn't waste your time.

This doesn't work on cros as well. Does new patch fixes this on cros as well?

[zhchbin, 2013-06-21 16:20:50]

On 2013/06/21 16:13:30, oshima wrote:
>
https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
> File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):
> 
>
https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
> chrome/browser/ui/views/extensions/native_app_window_views.cc:177:
> window_->SetBounds(adjusted_bounds);
> On 2013/06/21 06:36:31, zhchbin wrote:
> > On 2013/06/20 19:46:32, oshima wrote:
> > > The reason why it's currently broken on chromeos is that
> > > this code sets bounds AFTER Widget::Init. ChromeOS already have
> > > a logic to fit to the screen when created. I also think chrome must
> > > have the same logic as we don't want to restore chrome offscreen.
> > > Can't we just set the bounds in init_params, and leave it to underlying
> logic?
> > 
> > My patch didn't change these lines. I think the right code position that
broke
> > chromeos is native_app_window_->SetBounds(new_bounds); in the
shell_window.cc
> > line 210(My patch).
> 
> Sorry I lied. I mixed up with something else, so please ignore my comments
> above.
> I'll check existing code again and will comment a bit later.
> 
> > However, now I'm just working on windows for the problem,
> > and I can't reproduce this problem on Linux(GTK+) and MAC platform(With the
> > steps on the issue 225734). I will update this patch to windows only. Do you
> and
> > @scheib agree? Hope this code review wouldn't waste your time.
> 
> This doesn't work on cros as well. Does new patch fixes this on cros as well?

Currently I have no knowledge about cros, and didn't setup the environment yet,
so I can't make sure. Sorry for it.

[oshima, 2013-06-21 21:47:08]

On 2013/06/21 16:20:50, zhchbin wrote:
> On 2013/06/21 16:13:30, oshima wrote:
> >
>
https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
> > File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):
> > 
> >
>
https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
> > chrome/browser/ui/views/extensions/native_app_window_views.cc:177:
> > window_->SetBounds(adjusted_bounds);
> > On 2013/06/21 06:36:31, zhchbin wrote:
> > > On 2013/06/20 19:46:32, oshima wrote:
> > > > The reason why it's currently broken on chromeos is that
> > > > this code sets bounds AFTER Widget::Init. ChromeOS already have
> > > > a logic to fit to the screen when created. I also think chrome must
> > > > have the same logic as we don't want to restore chrome offscreen.
> > > > Can't we just set the bounds in init_params, and leave it to underlying
> > logic?
> > > 
> > > My patch didn't change these lines. I think the right code position that
> broke
> > > chromeos is native_app_window_->SetBounds(new_bounds); in the
> shell_window.cc
> > > line 210(My patch).
> > 
> > Sorry I lied. I mixed up with something else, so please ignore my comments
> > above.
> > I'll check existing code again and will comment a bit later.
> > 
> > > However, now I'm just working on windows for the problem,
> > > and I can't reproduce this problem on Linux(GTK+) and MAC platform(With
the
> > > steps on the issue 225734). I will update this patch to windows only. Do
you
> > and
> > > @scheib agree? Hope this code review wouldn't waste your time.
> > 
> > This doesn't work on cros as well. Does new patch fixes this on cros as
well?
> 
> Currently I have no knowledge about cros, and didn't setup the environment
yet,
> so I can't make sure. Sorry for it.

I tested the patch and didn't fix the problem.

[oshima, 2013-06-21 23:30:08]

On 2013/06/21 21:47:08, oshima wrote:
> On 2013/06/21 16:20:50, zhchbin wrote:
> > On 2013/06/21 16:13:30, oshima wrote:
> > >
> >
>
https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
> > > File chrome/browser/ui/views/extensions/native_app_window_views.cc
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/17378003/diff/7003/chrome/browser/ui/views/ex...
> > > chrome/browser/ui/views/extensions/native_app_window_views.cc:177:
> > > window_->SetBounds(adjusted_bounds);
> > > On 2013/06/21 06:36:31, zhchbin wrote:
> > > > On 2013/06/20 19:46:32, oshima wrote:
> > > > > The reason why it's currently broken on chromeos is that
> > > > > this code sets bounds AFTER Widget::Init. ChromeOS already have
> > > > > a logic to fit to the screen when created. I also think chrome must
> > > > > have the same logic as we don't want to restore chrome offscreen.
> > > > > Can't we just set the bounds in init_params, and leave it to
underlying
> > > logic?
> > > > 
> > > > My patch didn't change these lines. I think the right code position that
> > broke
> > > > chromeos is native_app_window_->SetBounds(new_bounds); in the
> > shell_window.cc
> > > > line 210(My patch).
> > > 
> > > Sorry I lied. I mixed up with something else, so please ignore my comments
> > > above.
> > > I'll check existing code again and will comment a bit later.
> > > 
> > > > However, now I'm just working on windows for the problem,
> > > > and I can't reproduce this problem on Linux(GTK+) and MAC platform(With
> the
> > > > steps on the issue 225734). I will update this patch to windows only. Do
> you
> > > and
> > > > @scheib agree? Hope this code review wouldn't waste your time.
> > > 
> > > This doesn't work on cros as well. Does new patch fixes this on cros as
> well?
> > 
> > Currently I have no knowledge about cros, and didn't setup the environment
> yet,
> > so I can't make sure. Sorry for it.
> 
> I tested the patch and didn't fix the problem.

Sorry I missed your comment "I will update this patch to windows only.". I
removed ifdef and
confirmed that this does fix the problem.

[scheib, 2013-06-21 23:47:06]

Thanks for the updates, zhchbin.

lgtm -- I will iterate further after this lands for other platforms.

[zhchbin, 2013-06-24 00:30:27]

Landed via https://chromiumcodereview.appspot.com/17564005.