Fix positioning of Mac permission prompts when fullscreen.

Fix positioning of Mac permission prompts when fullscreen.

When fullscreen on Mac OS X the location bar may or may not be visible.
If it is not visible, permission bubbles should be positioned in the
center of the screen.

BUG=604085
Committed: https://crrev.com/070786c56dafefdaa631a66c4248e7689e91b0fc
Cr-Commit-Position: refs/heads/master@{#390596}

[benwells, 2016-04-22 04:05:05]

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

[tapted, 2016-04-22 05:27:40]

Not sure if you want to deal with popups in this CL too... "They're weirder."

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
File
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browser_test.mm
(right):

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browser_test.mm:38:
while (faker.IsInTransition())
This looks like it would spin on the CPU - I don't think it *actually* will
because the faker doesn't post any delayed tasks like a "real" fullscreen would.
i.e. it's guaranteed to only ever loop once. But that means it should be enough
to do something like

EXPECT_TRUE(faker.IsInTransition());
content::RunAllPendingInMessageLoop();
EXPECT_FALSE(faker.IsInTransition());

more below.

You could also do something like faker.FinishTransition(); -- which does
effectively the above code with DCHECKs, and base::RunLoo().RunUntilIdle(); 

(but if that doesn't work, it should use a WindowedNSNotificationObserver rather
than spinning the CPU)

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h
(right):

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h:52: //
Returns true of the browser has a visible location bar.
nit: of -> if

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
(right):

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:470:
bool has_bar = browser_->SupportsWindowFeature(Browser::FEATURE_LOCATIONBAR);
nit: has_bar -> hasBar since it's between @implementation/@end

(alternatively just `if (!browser_->Supports...)` )

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:487: //
Otherwise this is fullscreen without a toolbar, so there is no visible
what about popups?

E.g. https://permission.site -> Tools -> Add to Applications..., Check "open in
Window".

I think that has a hidden location bar as well (and the popup opens in a weird
spot..)

https://codereview.chromium.org/1905273002/diff/1/ui/base/test/scoped_fake_ns...
File ui/base/test/scoped_fake_nswindow_fullscreen.h (right):

https://codereview.chromium.org/1905273002/diff/1/ui/base/test/scoped_fake_ns...
ui/base/test/scoped_fake_nswindow_fullscreen.h:27: bool IsInTransition();
comment for this?

E.g.

Whether a fullscreen transition has started, but the asynchronous tasks posted
to the MessageLoop that simulate its completion have not yet been run.

(alternatively, replace with CompleteTransition() or similar - see below)

[benwells, 2016-04-28 07:32:44]

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
File
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browser_test.mm
(right):

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browser_test.mm:38:
while (faker.IsInTransition())
On 2016/04/22 05:27:40, tapted wrote:
> This looks like it would spin on the CPU - I don't think it *actually* will
> because the faker doesn't post any delayed tasks like a "real" fullscreen
would.
> i.e. it's guaranteed to only ever loop once. But that means it should be
enough
> to do something like
> 
> EXPECT_TRUE(faker.IsInTransition());
> content::RunAllPendingInMessageLoop();
> EXPECT_FALSE(faker.IsInTransition());
> 
> more below.
> 
> You could also do something like faker.FinishTransition(); -- which does
> effectively the above code with DCHECKs, and base::RunLoo().RunUntilIdle(); 
> 
> (but if that doesn't work, it should use a WindowedNSNotificationObserver
rather
> than spinning the CPU)

Done.

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h
(right):

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h:52: //
Returns true of the browser has a visible location bar.
On 2016/04/22 05:27:40, tapted wrote:
> nit: of -> if

Done.

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
(right):

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:470:
bool has_bar = browser_->SupportsWindowFeature(Browser::FEATURE_LOCATIONBAR);
On 2016/04/22 05:27:40, tapted wrote:
> nit: has_bar -> hasBar since it's between @implementation/@end
> 
> (alternatively just `if (!browser_->Supports...)` )

Done.

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:487: //
Otherwise this is fullscreen without a toolbar, so there is no visible
On 2016/04/22 05:27:40, tapted wrote:
> what about popups?
> 
> E.g. https://permission.site -> Tools -> Add to Applications..., Check "open
in
> Window".
> 
> I think that has a hidden location bar as well (and the popup opens in a weird
> spot..)

That does have a hidden location bar, but those windows aren't released yet.

The other types of popup windows (i.e. regular popups, e.g.
http://www.ingdirect.com.au/ then click 'Login') have visible location bars) so
shouldn't be a problem.

https://codereview.chromium.org/1905273002/diff/1/ui/base/test/scoped_fake_ns...
File ui/base/test/scoped_fake_nswindow_fullscreen.h (right):

https://codereview.chromium.org/1905273002/diff/1/ui/base/test/scoped_fake_ns...
ui/base/test/scoped_fake_nswindow_fullscreen.h:27: bool IsInTransition();
On 2016/04/22 05:27:40, tapted wrote:
> comment for this?
> 
> E.g.
> 
> Whether a fullscreen transition has started, but the asynchronous tasks posted
> to the MessageLoop that simulate its completion have not yet been run.
> 
> (alternatively, replace with CompleteTransition() or similar - see below)

Done.

[benwells, 2016-04-28 07:32:44]

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
File
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browser_test.mm
(right):

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_browser_test.mm:38:
while (faker.IsInTransition())
On 2016/04/22 05:27:40, tapted wrote:
> This looks like it would spin on the CPU - I don't think it *actually* will
> because the faker doesn't post any delayed tasks like a "real" fullscreen
would.
> i.e. it's guaranteed to only ever loop once. But that means it should be
enough
> to do something like
> 
> EXPECT_TRUE(faker.IsInTransition());
> content::RunAllPendingInMessageLoop();
> EXPECT_FALSE(faker.IsInTransition());
> 
> more below.
> 
> You could also do something like faker.FinishTransition(); -- which does
> effectively the above code with DCHECKs, and base::RunLoo().RunUntilIdle(); 
> 
> (but if that doesn't work, it should use a WindowedNSNotificationObserver
rather
> than spinning the CPU)

Done.

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h
(right):

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h:52: //
Returns true of the browser has a visible location bar.
On 2016/04/22 05:27:40, tapted wrote:
> nit: of -> if

Done.

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
(right):

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:470:
bool has_bar = browser_->SupportsWindowFeature(Browser::FEATURE_LOCATIONBAR);
On 2016/04/22 05:27:40, tapted wrote:
> nit: has_bar -> hasBar since it's between @implementation/@end
> 
> (alternatively just `if (!browser_->Supports...)` )

Done.

https://codereview.chromium.org/1905273002/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:487: //
Otherwise this is fullscreen without a toolbar, so there is no visible
On 2016/04/22 05:27:40, tapted wrote:
> what about popups?
> 
> E.g. https://permission.site -> Tools -> Add to Applications..., Check "open
in
> Window".
> 
> I think that has a hidden location bar as well (and the popup opens in a weird
> spot..)

That does have a hidden location bar, but those windows aren't released yet.

The other types of popup windows (i.e. regular popups, e.g.
http://www.ingdirect.com.au/ then click 'Login') have visible location bars) so
shouldn't be a problem.

https://codereview.chromium.org/1905273002/diff/1/ui/base/test/scoped_fake_ns...
File ui/base/test/scoped_fake_nswindow_fullscreen.h (right):

https://codereview.chromium.org/1905273002/diff/1/ui/base/test/scoped_fake_ns...
ui/base/test/scoped_fake_nswindow_fullscreen.h:27: bool IsInTransition();
On 2016/04/22 05:27:40, tapted wrote:
> comment for this?
> 
> E.g.
> 
> Whether a fullscreen transition has started, but the asynchronous tasks posted
> to the MessageLoop that simulate its completion have not yet been run.
> 
> (alternatively, replace with CompleteTransition() or similar - see below)

Done.

[tapted, 2016-04-29 06:44:59]

lgtm

[benwells, 2016-04-29 06:49:56]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2016-04-29 06:49:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1905273002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1905273002/40001

[commit-bot: I haz the power, 2016-04-29 06:54:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-30 17:24:27]

Patchset 3 (id:??) landed as
https://crrev.com/070786c56dafefdaa631a66c4248e7689e91b0fc
Cr-Commit-Position: refs/heads/master@{#390596}

[commit-bot: I haz the power, 2016-04-30 17:59:45]

Description was changed from

==========
Fix positioning of Mac permission prompts when fullscreen.

When fullscreen on Mac OS X the location bar may or may not be visible.
If it is not visible, permission bubbles should be positioned in the
center of the screen.

BUG=604085
==========

to

==========
Fix positioning of Mac permission prompts when fullscreen.

When fullscreen on Mac OS X the location bar may or may not be visible.
If it is not visible, permission bubbles should be positioned in the
center of the screen.

BUG=604085

Committed: https://crrev.com/070786c56dafefdaa631a66c4248e7689e91b0fc
Cr-Commit-Position: refs/heads/master@{#390596}
==========