Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its
size. The problem is that the size of a RenderWidgetHostViewCocoa is
allowed to get out of sync with its superview while waiting for a
WebContents paint to be committed. If an autolayout is triggered while
waiting for that commit, the WebContents thinks it's been resized and
spawns a new paint.

Since r424609, for 10.11+, Chrome stopped replacing the NSThemeFrame
(which AppKit does not support) and instead started using
NSFullSizeContentViewWindowMask. This seems to opt the window into
additional autolayout triggers, coming from CoreAnimation. This can
engage the code to "re-sync" the sizes of the RenderWidgetHostViewCocoa
and WebContentsViewCocoa when it wasn't done previously.

To fix, "re-sync" sizes in an override of -setFrameSize: rather than
-resizeSubviewsWithOldSize:. This ensures a re-sync only occurs when the
size of the WebContentsViewCocoa changes.

BUG=655112, 655665, 264207
TBR=sky@chromium.org

Committed: https://crrev.com/ba268401a975c321f1e15ae3a4776fd3c60e268e
Cr-Commit-Position: refs/heads/master@{#427290}

[tapted, 2016-10-21 02:20:58]

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

[commit-bot: I haz the power, 2016-10-21 02:21:26]

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

[tapted, 2016-10-21 02:31:51]

Description was changed from

==========
Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its size.
Blah Blah

However, RenderWidgetHostViewCocoa may have a size distinct from that
set by RenderWidgetHostViewMac::SetBounds in anticipation of a something
blah foo.



coaAvoid autolayout from CA

BUG=655112, 655665
==========

to

==========
Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its size.
Blah Blah

However, RenderWidgetHostViewCocoa may have a size distinct from that
set by RenderWidgetHostViewMac::SetBounds in anticipation of a something
blah foo.



coaAvoid autolayout from CA

BUG=655112, 655665, 264207
==========

[commit-bot: I haz the power, 2016-10-21 03:19:10]

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

[commit-bot: I haz the power, 2016-10-21 03:19:11]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-10-21 03:30:50]

Description was changed from

==========
Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its size.
Blah Blah

However, RenderWidgetHostViewCocoa may have a size distinct from that
set by RenderWidgetHostViewMac::SetBounds in anticipation of a something
blah foo.



coaAvoid autolayout from CA

BUG=655112, 655665, 264207
==========

to

==========
Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its
size. The problem is that the size of a RenderWidgetHostViewCocoa is
allowed to get out of sync with its superview while waiting for a
WebContents paint to be committed. If an autolayout is triggered while
waiting for that commit, the WebContents thinks it's been resized and
spawns a new paint.

Since r424609, for 10.11+, Chrome stopped replacing the NSThemeFrame
(which AppKit does not support) and instead started using
NSFullSizeContentViewWindowMask. This seems to opt the window into
additional autolayout triggers, coming from CoreAnimation. This can
engage the code to "re-sync" the sizes of the RenderWidgetHostViewCocoa
and WebContentsViewCocoa when it wasn't done previously.

To fix, "re-sync" sizes in an override of -setFrameSize: rather than
-resizeSubviewsWithOldSize:. This ensures a re-sync only occurs when the
size of the WebContentsViewCocoa changes.

BUG=655112, 655665, 264207
==========

[tapted, 2016-10-21 03:30:51]

tapted@chromium.org changed reviewers:
+ avi@chromium.org, erikchen@chromium.org

[tapted, 2016-10-21 03:36:58]

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

[commit-bot: I haz the power, 2016-10-21 03:37:42]

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-10-21 04:21:49]

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

[commit-bot: I haz the power, 2016-10-21 04:21:49]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-10-21 04:24:48]

http://i.giphy.com/oDDDWKDfDMfRu.gif

[Avi (use Gerrit), 2016-10-21 13:04:12]

On 2016/10/21 04:24:48, tapted wrote:
> http://i.giphy.com/oDDDWKDfDMfRu.gif

Oh boy. Good luck. LGTM

[erikchen, 2016-10-21 16:15:21]

Your comments make heavy use of the team "autolayout". Are we actually
participating in AppKit autolayout? I thought that we exclusively used
autoresizeSubviews mask to perform layout. 

Can you describe a situation where setFrameSize: is called but
resizeSubviewsWithOldSize: is not?

[tapted, 2016-10-24 00:03:44]

On 2016/10/21 16:15:21, erikchen wrote:
> Your comments make heavy use of the team "autolayout". Are we actually
> participating in AppKit autolayout? I thought that we exclusively used
> autoresizeSubviews mask to perform layout. 

I think because we never set -[NSView translatesAutoresizingMaskIntoConstraints]
to 'NO' (only ios code seems to do this), then everything is roped into the
constraints-based autolayout regardless. (But I'd probably say that
autoresizeSubviews is just another facet of "autolayout" anyway, as opposed to
"manual" layout where we set frames with a bunch of NSMaxX([subview[n-1] frame]+
padding) ourselves).

> Can you describe a situation where setFrameSize: is called but
> resizeSubviewsWithOldSize: is not?

Is the salient part of the question whether -[WebContentsViewCocoa
setFrameSize:] is called and [[WebContentsViewCocoa superview]
resizeSubviewsWithOldSize:] is not?

Switching tabs does this [1]. If that's done with the mouse, [NSAnimationContext
endGrouping] triggers a layout (and a call to resizeSubviewsWithOldSize) in the
*same* runloop (via a ~ScopedCocoaDisableScreenUpdates()). However, if the tab
switch if it's done with a keyboard, the relayout happens in a *separate*
runloop iteration in the same way as this bug is being triggered [2] --
something internal to AppKit is called via
CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*).

If the question is actually whether -[RenderWidgetHostViewCocoa setFrameSize:]
is called and [WebContentsViewCocoa resizeSubviewsWithOldSize:] is not, then the
answer is also yes: RenderWidgetHostViewMac::SetBounds(const gfx::Rect& rect)
does this, and that's what (intentionally) causes the size of the
RenderWidgetHostViewCocoa and the WebContentsViewCocoa to get out of sync, in
anticipation of a paint from the renderer.

And what makes this fix viable is the opposite -- there are situations where
[WebContentsViewCocoa resizeSubviewsWithOldSize:] is called and
[WebContentsViewCocoa setFrameSize:] is not. There are also coming internally
from AppKit via that CA::Transaction::observer_callback per
http://crbug.com/655112#c13 and are what's putting the sizes of the views back
in sync at a time we don't want them to be.


[1] Switching tabs:
0   libbase.dylib                       0x000000011641a5fe
_ZN4base5debug10StackTraceC2Ev + 30
1   libbase.dylib                       0x000000011641a665
_ZN4base5debug10StackTraceC1Ev + 21
2   libcontent.dylib                    0x000000011de589cd
-[WebContentsViewCocoa setFrameSize:] + 77
3   AppKit                              0x00007fff9525e90d -[NSView setFrame:] +
476
4   libchrome_dll.dylib                 0x0000000109b23bee
-[TabContentsController ensureContentsVisibleInSuperview:] + 654
5   libchrome_dll.dylib                 0x0000000109b3122d -[TabStripController
swapInTabAtIndex:] + 749
6   libchrome_dll.dylib                 0x0000000109b37ebb -[TabStripController
activateTabWithContents:previousContents:atIndex:reason:] + 475
7   libchrome_dll.dylib                 0x0000000109b46254
_ZN27TabStripModelObserverBridge16ActiveTabChangedEPN7content11WebContentsES2_ii
+ 116


[2] Rogue layout trigger from CA::Transaction::observer_callback
2   libcontent.dylib                    0x000000011de589cd
-[WebContentsViewCocoa setFrameSize:] + 77
3   AppKit                              0x00007fff9525e90d -[NSView setFrame:] +
476
4   libchrome_dll.dylib                 0x0000000109b22ff1
-[TabContentsContainerView resizeSubviewsWithOldSize:] + 481
5   AppKit                              0x00007fff9526d286 NSViewLevelLayout +
165
6   AppKit                              0x00007fff9526d1cb -[NSView layout] + 14
7   AppKit                              0x00007fff952d43e7 -[NSView _doLayout] +
53
8   AppKit                              0x00007fff952d409f -[NSView
_layoutSubtreeWithOldSize:] + 324
9   AppKit                              0x00007fff952d42fb -[NSView
_layoutSubtreeWithOldSize:] + 928
10  AppKit                              0x00007fff952d42fb -[NSView
_layoutSubtreeWithOldSize:] + 928
11  AppKit                              0x00007fff952d42fb -[NSView
_layoutSubtreeWithOldSize:] + 928
12  AppKit                              0x00007fff952d42fb -[NSView
_layoutSubtreeWithOldSize:] + 928
13  AppKit                              0x00007fff952d42fb -[NSView
_layoutSubtreeWithOldSize:] + 928
14  AppKit                              0x00007fff952d42fb -[NSView
_layoutSubtreeWithOldSize:] + 928
15  AppKit                              0x00007fff952d42fb -[NSView
_layoutSubtreeWithOldSize:] + 928
16  AppKit                              0x00007fff952d35d8 -[NSView
layoutSubtreeIfNeeded] + 950
17  AppKit                              0x00007fff952f2f21
-[NSWindow(NSConstraintBasedLayout) _layoutViewTree] + 82
18  AppKit                              0x00007fff953654ff
-[NSWindow(NSConstraintBasedLayout) layoutIfNeeded] + 244
19  AppKit                              0x00007fff959fc155
___NSWindowGetDisplayCycleObserver_block_invoke6358 + 218
20  AppKit                              0x00007fff953774f8 __37+[NSDisplayCycle
currentDisplayCycle]_block_invoke + 719
21  QuartzCore                          0x00007fff94e22f71
_ZN2CA11Transaction19run_commit_handlersE18CATransactionPhase + 85
22  QuartzCore                          0x00007fff94e2242c
_ZN2CA7Context18commit_transactionEPNS_11TransactionE + 160
23  QuartzCore                          0x00007fff94e220ec
_ZN2CA11Transaction6commitEv + 508
24  QuartzCore                          0x00007fff94e2d977
_ZN2CA11Transaction17observer_callbackEP19__CFRunLoopObservermPv + 71

[erikchen, 2016-10-24 17:21:29]

The fact that we're not setting translatesAutoresizingMaskIntoConstraints to NO
is pretty terrifying. The behavior of autolayout is quite opaque. 

Thanks for looking into this. lgtm. At some point in the future, we may want to
revisit and switch completely to manual layout.

[tapted, 2016-10-25 06:01:38]

Description was changed from

==========
Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its
size. The problem is that the size of a RenderWidgetHostViewCocoa is
allowed to get out of sync with its superview while waiting for a
WebContents paint to be committed. If an autolayout is triggered while
waiting for that commit, the WebContents thinks it's been resized and
spawns a new paint.

Since r424609, for 10.11+, Chrome stopped replacing the NSThemeFrame
(which AppKit does not support) and instead started using
NSFullSizeContentViewWindowMask. This seems to opt the window into
additional autolayout triggers, coming from CoreAnimation. This can
engage the code to "re-sync" the sizes of the RenderWidgetHostViewCocoa
and WebContentsViewCocoa when it wasn't done previously.

To fix, "re-sync" sizes in an override of -setFrameSize: rather than
-resizeSubviewsWithOldSize:. This ensures a re-sync only occurs when the
size of the WebContentsViewCocoa changes.

BUG=655112, 655665, 264207
==========

to

==========
Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its
size. The problem is that the size of a RenderWidgetHostViewCocoa is
allowed to get out of sync with its superview while waiting for a
WebContents paint to be committed. If an autolayout is triggered while
waiting for that commit, the WebContents thinks it's been resized and
spawns a new paint.

Since r424609, for 10.11+, Chrome stopped replacing the NSThemeFrame
(which AppKit does not support) and instead started using
NSFullSizeContentViewWindowMask. This seems to opt the window into
additional autolayout triggers, coming from CoreAnimation. This can
engage the code to "re-sync" the sizes of the RenderWidgetHostViewCocoa
and WebContentsViewCocoa when it wasn't done previously.

To fix, "re-sync" sizes in an override of -setFrameSize: rather than
-resizeSubviewsWithOldSize:. This ensures a re-sync only occurs when the
size of the WebContentsViewCocoa changes.

BUG=655112, 655665, 264207
TBR=sky@chromium.org
==========

[tapted, 2016-10-25 06:01:39]

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

[tapted, 2016-10-25 06:03:03]

+sky TBR for (chrome/browser/ui - just re-enabling
BrowserTest.GetSizeForNewRenderView on Mac).

Thanks all!

[tapted, 2016-10-25 06:03:09]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-10-25 06:03:17]

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-10-25 06:41:47]

Description was changed from

==========
Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its
size. The problem is that the size of a RenderWidgetHostViewCocoa is
allowed to get out of sync with its superview while waiting for a
WebContents paint to be committed. If an autolayout is triggered while
waiting for that commit, the WebContents thinks it's been resized and
spawns a new paint.

Since r424609, for 10.11+, Chrome stopped replacing the NSThemeFrame
(which AppKit does not support) and instead started using
NSFullSizeContentViewWindowMask. This seems to opt the window into
additional autolayout triggers, coming from CoreAnimation. This can
engage the code to "re-sync" the sizes of the RenderWidgetHostViewCocoa
and WebContentsViewCocoa when it wasn't done previously.

To fix, "re-sync" sizes in an override of -setFrameSize: rather than
-resizeSubviewsWithOldSize:. This ensures a re-sync only occurs when the
size of the WebContentsViewCocoa changes.

BUG=655112, 655665, 264207
TBR=sky@chromium.org
==========

to

==========
Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its
size. The problem is that the size of a RenderWidgetHostViewCocoa is
allowed to get out of sync with its superview while waiting for a
WebContents paint to be committed. If an autolayout is triggered while
waiting for that commit, the WebContents thinks it's been resized and
spawns a new paint.

Since r424609, for 10.11+, Chrome stopped replacing the NSThemeFrame
(which AppKit does not support) and instead started using
NSFullSizeContentViewWindowMask. This seems to opt the window into
additional autolayout triggers, coming from CoreAnimation. This can
engage the code to "re-sync" the sizes of the RenderWidgetHostViewCocoa
and WebContentsViewCocoa when it wasn't done previously.

To fix, "re-sync" sizes in an override of -setFrameSize: rather than
-resizeSubviewsWithOldSize:. This ensures a re-sync only occurs when the
size of the WebContentsViewCocoa changes.

BUG=655112, 655665, 264207
TBR=sky@chromium.org
==========

[commit-bot: I haz the power, 2016-10-25 06:41:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-25 06:44:40]

Description was changed from

==========
Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its
size. The problem is that the size of a RenderWidgetHostViewCocoa is
allowed to get out of sync with its superview while waiting for a
WebContents paint to be committed. If an autolayout is triggered while
waiting for that commit, the WebContents thinks it's been resized and
spawns a new paint.

Since r424609, for 10.11+, Chrome stopped replacing the NSThemeFrame
(which AppKit does not support) and instead started using
NSFullSizeContentViewWindowMask. This seems to opt the window into
additional autolayout triggers, coming from CoreAnimation. This can
engage the code to "re-sync" the sizes of the RenderWidgetHostViewCocoa
and WebContentsViewCocoa when it wasn't done previously.

To fix, "re-sync" sizes in an override of -setFrameSize: rather than
-resizeSubviewsWithOldSize:. This ensures a re-sync only occurs when the
size of the WebContentsViewCocoa changes.

BUG=655112, 655665, 264207
TBR=sky@chromium.org
==========

to

==========
Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout.

WebContentsViewCocoa has a hook to resize all subviews to match its
size. The problem is that the size of a RenderWidgetHostViewCocoa is
allowed to get out of sync with its superview while waiting for a
WebContents paint to be committed. If an autolayout is triggered while
waiting for that commit, the WebContents thinks it's been resized and
spawns a new paint.

Since r424609, for 10.11+, Chrome stopped replacing the NSThemeFrame
(which AppKit does not support) and instead started using
NSFullSizeContentViewWindowMask. This seems to opt the window into
additional autolayout triggers, coming from CoreAnimation. This can
engage the code to "re-sync" the sizes of the RenderWidgetHostViewCocoa
and WebContentsViewCocoa when it wasn't done previously.

To fix, "re-sync" sizes in an override of -setFrameSize: rather than
-resizeSubviewsWithOldSize:. This ensures a re-sync only occurs when the
size of the WebContentsViewCocoa changes.

BUG=655112, 655665, 264207
TBR=sky@chromium.org

Committed: https://crrev.com/ba268401a975c321f1e15ae3a4776fd3c60e268e
Cr-Commit-Position: refs/heads/master@{#427290}
==========

[commit-bot: I haz the power, 2016-10-25 06:44:43]

Patchset 2 (id:??) landed as
https://crrev.com/ba268401a975c321f1e15ae3a4776fd3c60e268e
Cr-Commit-Position: refs/heads/master@{#427290}