[MacViews] Update AppWindow when the native window changes fullscreen state.

[MacViews] Update AppWindow when the native window changes fullscreen state.

This is necessary on Mac because the window can be made fullscreen without
calling through AppWindow::OSFullscreen.

BUG=459877
Committed: https://crrev.com/094d214d67451e91a2a2cb0e9ca248e2873be5ec
Cr-Commit-Position: refs/heads/master@{#333884}

[jackhou1, 2015-06-09 01:40:50]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[jackhou1, 2015-06-09 01:40:50]

tapted, PTAL

[tapted, 2015-06-09 03:04:27]

the fullscreen transition is an implementation that I'm trying to hide... i.e.
Widget::IsFullscreen() always reports where it's trying to go, rather than what
just finished, so, any cached state should be updated when the transition starts
otherwise they will be out of sync.

I think there's a way to do this without a new function on NativeWidgetMac.
ChromeNativeAppWindowViews[Mac] is a views::WidgetObserver. Can you observe
OnWidgetBoundsChanged and update ChromeNativeAppWindowViews::is_fullscreen_ if
it's different to widget()->IsFullscreen()?

Note I'm not even sure we need ChromeNativeAppWindowViews::is_fullscreen_ ....
Implementing `ChromeNativeAppWindowViews::IsFullscreenOrPending()` as simply
`widget()->IsFullscreen()` should actually work fine with the implementation
that exists on Mac..

Then also extensions::AppWindow needs a way to inform it of an externally
triggered fullscreen change. I don't think we should call OSFullscreen(), since
that would call Widget::SetFullscreen again (right?) -- we shouldn't be relying
on that being a no-op the second time.

https://codereview.chromium.org/1172483003/diff/1/chrome/browser/ui/views/app...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1172483003/diff/1/chrome/browser/ui/views/app...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:38:
static_cast<extensions::NativeAppWindow*>(native_app_window_);
static_cast -> implicit_cast? (assuming this is just to call some overrides made
private, and it's not straightforward just to make those overrides accessible)

[jackhou1, 2015-06-09 05:20:50]

On 2015/06/09 03:04:27, tapted wrote:
> the fullscreen transition is an implementation that I'm trying to hide... i.e.
> Widget::IsFullscreen() always reports where it's trying to go, rather than
what
> just finished, so, any cached state should be updated when the transition
starts
> otherwise they will be out of sync.
> 
> I think there's a way to do this without a new function on NativeWidgetMac.
> ChromeNativeAppWindowViews[Mac] is a views::WidgetObserver. Can you observe
> OnWidgetBoundsChanged and update ChromeNativeAppWindowViews::is_fullscreen_ if
> it's different to widget()->IsFullscreen()?

Done.

> Note I'm not even sure we need ChromeNativeAppWindowViews::is_fullscreen_ ....
> Implementing `ChromeNativeAppWindowViews::IsFullscreenOrPending()` as simply
> `widget()->IsFullscreen()` should actually work fine with the implementation
> that exists on Mac..

Done but see below.

> Then also extensions::AppWindow needs a way to inform it of an externally
> triggered fullscreen change. I don't think we should call OSFullscreen(),
since
> that would call Widget::SetFullscreen again (right?) -- we shouldn't be
relying
> on that being a no-op the second time.

Done. If we can't rely on calling Widget::SetFullscreen multiple times, then we
probably can't remove CNAWV::is_fullscreen_ either since (Native)AppWindow
distinguishes between different types of fullscreen whereas Widget does not.

[tapted, 2015-06-09 06:04:01]

a few more questions (sorry about the back&forth)

https://codereview.chromium.org/1172483003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.h (right):

https://codereview.chromium.org/1172483003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.h:33: const
gfx::Rect& new_bounds) override;
nit: should come after NativeAppWindow, to match the inheritance order on
native_app_window::NativeAppWindowViews

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:608: fullscreen_types_ |=
FULLSCREEN_TYPE_OS;
why not just `=`? i.e. shouldn't this override any other way of going
fullscreen?

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:611: SetNativeWindowFullscreen();
Is this still needed?

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:614: OnNativeWindowChanged();
What about just rolling the fullscreen check into OnNativeWindowChanged()?

And... actually, does a call into void
NativeAppWindowViews::SaveWindowPlacement(..) already occur for fullscreen
changes?  (maybe we don't need that widgetobserver hook after all)

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:293: void
UpdateOSFullscreenState(bool is_fullscreen);
nit: Maybe OnOSFullscreenStateChanged? or OnBecameFullscreenByOS?

[jackhou1, 2015-06-10 05:06:04]

Yeah, this is much simpler.

The is_fullscreen_ removal is not necessary for this CL, so I've split it out:
https://codereview.chromium.org/1176823002/

https://codereview.chromium.org/1172483003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.h (right):

https://codereview.chromium.org/1172483003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.h:33: const
gfx::Rect& new_bounds) override;
On 2015/06/09 06:04:01, tapted wrote:
> nit: should come after NativeAppWindow, to match the inheritance order on
> native_app_window::NativeAppWindowViews

Removed.

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:608: fullscreen_types_ |=
FULLSCREEN_TYPE_OS;
On 2015/06/09 06:04:01, tapted wrote:
> why not just `=`? i.e. shouldn't this override any other way of going
> fullscreen?

Yes, but the way it works now in patch set 5 means that any type of fullscreen
will also set FULLSCREEN_TYPE_OS, but it should not override the other types.

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:611: SetNativeWindowFullscreen();
On 2015/06/09 06:04:01, tapted wrote:
> Is this still needed?

It's needed to set the always-on-top state. But that should be done when
entering fullscreen too.

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:614: OnNativeWindowChanged();
On 2015/06/09 06:04:01, tapted wrote:
> What about just rolling the fullscreen check into OnNativeWindowChanged()?
> 
> And... actually, does a call into void
> NativeAppWindowViews::SaveWindowPlacement(..) already occur for fullscreen
> changes?  (maybe we don't need that widgetobserver hook after all)

Done.

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/1172483003/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:293: void
UpdateOSFullscreenState(bool is_fullscreen);
On 2015/06/09 06:04:01, tapted wrote:
> nit: Maybe OnOSFullscreenStateChanged? or OnBecameFullscreenByOS?

Removed.

[tapted, 2015-06-10 05:45:25]

cool - looks nice!

cl description needs an update though

and.. there's probably an API test for this disabled somewhere (is there? does
it still run when we're not worry about it being flaky?)

https://codereview.chromium.org/1172483003/diff/80001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/1172483003/diff/80001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:457: #if defined(OS_MACOSX)
before this, I'd just do

  if (!native_app_window_)
    return;

AppWindow::SaveWindowPosition() does this also, so the call below isn't missing
out. Further, we can just turn the ckech in SaveWindowPosition() into a DCHECK
since the method is private and OnNativeWindowChanged() is the only caller.

In fact... I'm sorely tempted to just DCHECK here as well... the other calls to
OnNativeWindowChanged() in AppWindow are in functions that have already assumed
native_app_window is non-null. The other calls all seem to be from the native
app window itself :/. bleh, maybe we turn it into a CHECK() in a follow-up or do
some git archaeology to smack a comment about why we need to check if it's
non-null.

https://codereview.chromium.org/1172483003/diff/80001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:468: if (native_is_fullscreen !=
was_fullscreen && cached_always_on_top_)
I don't think the `native_is_fullscreen != was_fullscreen` adds anything.
UpdateNativeAlwaysOnTop() has it's own checking to ensure SetAlwaysOnTop(FOO) is
only called if it's not currently FOO. (and the check below for windows doesn't
try detect toggles).

Then this can just be

if (native_app_window_->IsFullscreen())
  fullscreen_types_ |= FULLSCREEN_TYPE_OS;
else
  fullscreen_types_ = FULLSCREEN_TYPE_NONE;

if (cached_always_on_top_)
  UpdateNativeAlwaysOnTop();  // Same as in SetNativeWindowFullscreen.



(there's probably some #ifdef simplifications too... but I'm more inclined to
leave the WIN codepaths alone)

[jackhou1, 2015-06-10 07:52:32]

> and.. there's probably an API test for this disabled somewhere (is there? does
> it still run when we're not worry about it being flaky?)

Not really. The interactive test below just checks that a window can be created
fullscreen:
chrome/test/data/extensions/platform_apps/window_api_interactive/test.js

The other tests are for Esc behavior, and are mostly disabled:
chrome/browser/apps/app_window_interactive_uitest.cc

https://codereview.chromium.org/1172483003/diff/80001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/1172483003/diff/80001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:457: #if defined(OS_MACOSX)
On 2015/06/10 05:45:25, tapted wrote:
> before this, I'd just do
> 
>   if (!native_app_window_)
>     return;
> 
> AppWindow::SaveWindowPosition() does this also, so the call below isn't
missing
> out. Further, we can just turn the ckech in SaveWindowPosition() into a DCHECK
> since the method is private and OnNativeWindowChanged() is the only caller.
> 
> In fact... I'm sorely tempted to just DCHECK here as well... the other calls
to
> OnNativeWindowChanged() in AppWindow are in functions that have already
assumed
> native_app_window is non-null. The other calls all seem to be from the native
> app window itself :/. bleh, maybe we turn it into a CHECK() in a follow-up or
do
> some git archaeology to smack a comment about why we need to check if it's
> non-null.

Done.

https://codereview.chromium.org/1172483003/diff/80001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:468: if (native_is_fullscreen !=
was_fullscreen && cached_always_on_top_)
On 2015/06/10 05:45:25, tapted wrote:
> I don't think the `native_is_fullscreen != was_fullscreen` adds anything.
> UpdateNativeAlwaysOnTop() has it's own checking to ensure SetAlwaysOnTop(FOO)
is
> only called if it's not currently FOO. (and the check below for windows
doesn't
> try detect toggles).
> 
> Then this can just be
> 
> if (native_app_window_->IsFullscreen())
>   fullscreen_types_ |= FULLSCREEN_TYPE_OS;
> else
>   fullscreen_types_ = FULLSCREEN_TYPE_NONE;
> 
> if (cached_always_on_top_)
>   UpdateNativeAlwaysOnTop();  // Same as in SetNativeWindowFullscreen.
> 
> 
> 
> (there's probably some #ifdef simplifications too... but I'm more inclined to
> leave the WIN codepaths alone)

Done.

[tapted, 2015-06-10 11:02:58]

lgtm

[jackhou1, 2015-06-11 00:18:32]

jackhou@chromium.org changed reviewers:
+ benwells@chromium.org

[jackhou1, 2015-06-11 00:18:33]

benwells, please review for OWNERS:

extensions/browser/app_window/app_window.cc

[benwells, 2015-06-11 02:05:41]

rs lgtm

[jackhou1, 2015-06-11 02:38:10]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2015-06-11 02:38:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1172483003/100001

[commit-bot: I haz the power, 2015-06-11 03:50:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-11 03:51:57]

Patchset 6 (id:??) landed as
https://crrev.com/094d214d67451e91a2a2cb0e9ca248e2873be5ec
Cr-Commit-Position: refs/heads/master@{#333884}