Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So invoking -[BrowserWindowController
layoutSubviews] isn't necessary to update the bubble

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews). This sequence of events seems to be responsible
for some AppKit crashes on older versions of OSX.

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
TEST=Manually tested fullscreen flows, and ensured there were no
regressions running `interactive_ui_tests
--gtest_filter=FullscreenControllerInteractiveTest.*
--gtest_also_run_disabled_tests` locally.

Committed: https://crrev.com/19b0e2dd976fb5fc1ec028c1cbbc584b4ca7dde2
Cr-Commit-Position: refs/heads/master@{#421974}

[tapted, 2016-09-28 04:02:58]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 04:03:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 04:31:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 04:31:52]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-09-29 00:57:44]

Description was changed from

==========
Mac: Don't call BWC layoutSubviews before updating the fullscreen bubble

ExclusiveAccessController always repositions the bubble according to the
size of the browser window.

Doing layoutSubviews is expensive, unnecessary, and can cause a repositioning
of a bubble that is about to go away, since the bubble type isn't checked
until updateFullscreenExitBubble calls BWC(Private) updateFullscreenExitBubble.

And, actually, ExclusiveAccessController::Layout(..) method ignores its max_y
argument, so purge all the goo around that too.

BUG=650538
==========

to

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So -[BrowserWindowController layoutSubviews]
doesn't need to affect the bubble.

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews).

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
==========

[tapted, 2016-09-29 00:58:35]

Description was changed from

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So -[BrowserWindowController layoutSubviews]
doesn't need to affect the bubble.

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews).

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
==========

to

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So -[BrowserWindowController layoutSubviews]
doesn't need to affect the bubble.

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews). This sequence of events seems to be responsible
for some AppKit crashes on older versions of OSX.

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
==========

[tapted, 2016-09-29 00:59:01]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 00:59:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 01:18:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 01:18:56]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-09-29 01:52:24]

Description was changed from

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So -[BrowserWindowController layoutSubviews]
doesn't need to affect the bubble.

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews). This sequence of events seems to be responsible
for some AppKit crashes on older versions of OSX.

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
==========

to

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So -[BrowserWindowController layoutSubviews]
doesn't need to affect the bubble.

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews). This sequence of events seems to be responsible
for some AppKit crashes on older versions of OSX.

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
TEST=Manually tested fullscreen flows, and ensured there were no
regressions running `interactive_ui_tests
--gtest_filter=FullscreenControllerInteractiveTest.*
--gtest_also_run_disabled_tests` locally.
==========

[tapted, 2016-09-29 01:52:24]

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

[Matt Giuca, 2016-09-29 03:34:19]

Hmmm looks fine, but I don't know any of this code. Should you get a Mac person
to review?

[tapted, 2016-09-29 03:38:35]

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

[tapted, 2016-09-29 03:38:36]

+spqchan - you probably know this BWC stuff the best - please take a look

On 2016/09/29 03:34:19, Matt Giuca wrote:
> Hmmm looks fine, but I don't know any of this code. Should you get a Mac
person
> to review?

[Matt Giuca, 2016-09-29 03:48:46]

Can you make this sentence clearer:

> So -[BrowserWindowController layoutSubviews]
> doesn't need to affect the bubble.

First what is the - sign there? (And in the first line.)

Second what does "doesn't need to affect the bubble" mean? Do you mean "isn't
necessary to update the bubble"?

lgtm otherwise

[spqchan, 2016-09-29 19:13:34]

lgtm

[tapted, 2016-09-29 23:05:48]

Description was changed from

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So -[BrowserWindowController layoutSubviews]
doesn't need to affect the bubble.

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews). This sequence of events seems to be responsible
for some AppKit crashes on older versions of OSX.

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
TEST=Manually tested fullscreen flows, and ensured there were no
regressions running `interactive_ui_tests
--gtest_filter=FullscreenControllerInteractiveTest.*
--gtest_also_run_disabled_tests` locally.
==========

to

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So invoking -[BrowserWindowController
layoutSubviews] isn't necessary to update the bubble

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews). This sequence of events seems to be responsible
for some AppKit crashes on older versions of OSX.

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
TEST=Manually tested fullscreen flows, and ensured there were no
regressions running `interactive_ui_tests
--gtest_filter=FullscreenControllerInteractiveTest.*
--gtest_also_run_disabled_tests` locally.
==========

[tapted, 2016-09-29 23:07:13]

Thanks all!

On 2016/09/29 03:48:46, Matt Giuca (OOO til Oct 4) wrote:
> Can you make this sentence clearer:
> 
> > So -[BrowserWindowController layoutSubviews]
> > doesn't need to affect the bubble.
> 
> First what is the - sign there? (And in the first line.)

That's to say it's a instance method rather than a class method (+) - part of
the crazy ObjC syntax :)

> Second what does "doesn't need to affect the bubble" mean? Do you mean "isn't
> necessary to update the bubble"?

Yup - updated.

[tapted, 2016-09-29 23:07:23]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-09-29 23:08:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 23:14:24]

Description was changed from

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So invoking -[BrowserWindowController
layoutSubviews] isn't necessary to update the bubble

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews). This sequence of events seems to be responsible
for some AppKit crashes on older versions of OSX.

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
TEST=Manually tested fullscreen flows, and ensured there were no
regressions running `interactive_ui_tests
--gtest_filter=FullscreenControllerInteractiveTest.*
--gtest_also_run_disabled_tests` locally.
==========

to

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So invoking -[BrowserWindowController
layoutSubviews] isn't necessary to update the bubble

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews). This sequence of events seems to be responsible
for some AppKit crashes on older versions of OSX.

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
TEST=Manually tested fullscreen flows, and ensured there were no
regressions running `interactive_ui_tests
--gtest_filter=FullscreenControllerInteractiveTest.*
--gtest_also_run_disabled_tests` locally.
==========

[commit-bot: I haz the power, 2016-09-29 23:14:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-29 23:16:31]

Description was changed from

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So invoking -[BrowserWindowController
layoutSubviews] isn't necessary to update the bubble

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews). This sequence of events seems to be responsible
for some AppKit crashes on older versions of OSX.

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
TEST=Manually tested fullscreen flows, and ensured there were no
regressions running `interactive_ui_tests
--gtest_filter=FullscreenControllerInteractiveTest.*
--gtest_also_run_disabled_tests` locally.
==========

to

==========
Mac: Don't call -[BWC layoutSubviews] before updating the fullscreen bubble.

ExclusiveAccessController always repositions the bubble according to the
size of the browser window. So invoking -[BrowserWindowController
layoutSubviews] isn't necessary to update the bubble

Doing layoutSubviews is expensive, unnecessary, and can cause a
repositioning attempt of a bubble that is about to go away, since the
bubble type isn't checked until updateFullscreenExitBubble calls
BWC(Private) updateFullscreenExitBubble (which it does immediately
*after* layoutSubviews). This sequence of events seems to be responsible
for some AppKit crashes on older versions of OSX.

ExclusiveAccessController::Layout() can be removed completely (it
already ignores it's max_y argument, so remove some plumbing for that).

BUG=650538
TEST=Manually tested fullscreen flows, and ensured there were no
regressions running `interactive_ui_tests
--gtest_filter=FullscreenControllerInteractiveTest.*
--gtest_also_run_disabled_tests` locally.

Committed: https://crrev.com/19b0e2dd976fb5fc1ec028c1cbbc584b4ca7dde2
Cr-Commit-Position: refs/heads/master@{#421974}
==========

[commit-bot: I haz the power, 2016-09-29 23:16:32]

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