Mac: Do layout before adding the WebContents to the view hierarchy when switching tabs

Mac: Do layout before adding the WebContents to the view hierarchy when switching tabs

Currently you can sometimes see a flash of webcontent at the size it was
when a tab was last active, before it updates to fill the window.

This regressed in r378605, which tied a call to WebContents::WasShown to
the insertion into the view hierarchy, which happens before setting the
WebContents size.

There's currently some tricky code to suppress auto-resizing when
switching to/from a tab which might have an infobar showing. Make this
simpler and fix the regression by establishing a correct size for the
WebContents *before* it gets added to the view hierarchy, rather than
after.

Do this by having TabStripController delegate resposibility for setting
view sizes and updating the view heirarchy to TabContentsController.
This is probably nicer anyway, since TabContentsController already has
most of the logic to pick a target WebContents size - it just needs to
know what the size of its superview _will_ be rather than what it
currently is.

BUG=627255
TEST=On Mac, create 2 tabs, make the window small, switch tabs and make
the window big. Switch back to the first tab. There should be no flash
the old, small web contents before it fills the window.

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

[tapted, 2016-07-12 07:27:14]

Description was changed from

==========
Mac: Layout with the WebContents removed from the view hierarchy when switching
tabs




BUG=627255
==========

to

==========
Mac: Layout with the WebContents removed from the view hierarchy when switching
tabs

Currently you can sometimes see a flash of webcontent at the size it was
when a tab was last active, before it updates to fill the window.

This regressed in r378605, which tied a call to WebContents::WasShown to
the insertion into the view hierarchy, which happens before setting the
WebContents size.

There's currently some tricky code to suppress auto-resizing when
switching to/from a tab which might have an infobar showing. Make this
simpler and fix the regression by establishing a correct size for the
WebContents *before* it gets added to the view hierarchy, rather than
after.

Do this by having TabStripController delegate resposibility for setting
view sizes and updating the view heirarchy to TabContentsController.
This is probably nicer anyway, since TabContentsController already has
most of the logic to pick a target WebContents size - it just needs to
know what the size of its superview _will_ be rather than what it
currently is.

BUG=627255
TEST=(1) On Mac, create 2 tabs, make the window small, switch tabs and
make the window big. Switch back to the first tab. There should be no
flash the old, small web contents before it fills the window.
==========

[tapted, 2016-07-12 07:27:14]

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

[tapted, 2016-07-12 07:27:33]

Description was changed from

==========
Mac: Layout with the WebContents removed from the view hierarchy when switching
tabs

Currently you can sometimes see a flash of webcontent at the size it was
when a tab was last active, before it updates to fill the window.

This regressed in r378605, which tied a call to WebContents::WasShown to
the insertion into the view hierarchy, which happens before setting the
WebContents size.

There's currently some tricky code to suppress auto-resizing when
switching to/from a tab which might have an infobar showing. Make this
simpler and fix the regression by establishing a correct size for the
WebContents *before* it gets added to the view hierarchy, rather than
after.

Do this by having TabStripController delegate resposibility for setting
view sizes and updating the view heirarchy to TabContentsController.
This is probably nicer anyway, since TabContentsController already has
most of the logic to pick a target WebContents size - it just needs to
know what the size of its superview _will_ be rather than what it
currently is.

BUG=627255
TEST=(1) On Mac, create 2 tabs, make the window small, switch tabs and
make the window big. Switch back to the first tab. There should be no
flash the old, small web contents before it fills the window.
==========

to

==========
Mac: Layout with the WebContents removed from the view hierarchy when switching
tabs

Currently you can sometimes see a flash of webcontent at the size it was
when a tab was last active, before it updates to fill the window.

This regressed in r378605, which tied a call to WebContents::WasShown to
the insertion into the view hierarchy, which happens before setting the
WebContents size.

There's currently some tricky code to suppress auto-resizing when
switching to/from a tab which might have an infobar showing. Make this
simpler and fix the regression by establishing a correct size for the
WebContents *before* it gets added to the view hierarchy, rather than
after.

Do this by having TabStripController delegate resposibility for setting
view sizes and updating the view heirarchy to TabContentsController.
This is probably nicer anyway, since TabContentsController already has
most of the logic to pick a target WebContents size - it just needs to
know what the size of its superview _will_ be rather than what it
currently is.

BUG=627255
TEST=On Mac, create 2 tabs, make the window small, switch tabs and make
the window big. Switch back to the first tab. There should be no flash
the old, small web contents before it fills the window.
==========

[tapted, 2016-07-12 07:28:28]

Description was changed from

==========
Mac: Layout with the WebContents removed from the view hierarchy when switching
tabs

Currently you can sometimes see a flash of webcontent at the size it was
when a tab was last active, before it updates to fill the window.

This regressed in r378605, which tied a call to WebContents::WasShown to
the insertion into the view hierarchy, which happens before setting the
WebContents size.

There's currently some tricky code to suppress auto-resizing when
switching to/from a tab which might have an infobar showing. Make this
simpler and fix the regression by establishing a correct size for the
WebContents *before* it gets added to the view hierarchy, rather than
after.

Do this by having TabStripController delegate resposibility for setting
view sizes and updating the view heirarchy to TabContentsController.
This is probably nicer anyway, since TabContentsController already has
most of the logic to pick a target WebContents size - it just needs to
know what the size of its superview _will_ be rather than what it
currently is.

BUG=627255
TEST=On Mac, create 2 tabs, make the window small, switch tabs and make
the window big. Switch back to the first tab. There should be no flash
the old, small web contents before it fills the window.
==========

to

==========
Mac: Do layout before adding the WebContents to the view hierarchy when
switching tabs

Currently you can sometimes see a flash of webcontent at the size it was
when a tab was last active, before it updates to fill the window.

This regressed in r378605, which tied a call to WebContents::WasShown to
the insertion into the view hierarchy, which happens before setting the
WebContents size.

There's currently some tricky code to suppress auto-resizing when
switching to/from a tab which might have an infobar showing. Make this
simpler and fix the regression by establishing a correct size for the
WebContents *before* it gets added to the view hierarchy, rather than
after.

Do this by having TabStripController delegate resposibility for setting
view sizes and updating the view heirarchy to TabContentsController.
This is probably nicer anyway, since TabContentsController already has
most of the logic to pick a target WebContents size - it just needs to
know what the size of its superview _will_ be rather than what it
currently is.

BUG=627255
TEST=On Mac, create 2 tabs, make the window small, switch tabs and make
the window big. Switch back to the first tab. There should be no flash
the old, small web contents before it fills the window.
==========

[tapted, 2016-07-12 07:33:21]

Hi Chris please take a look

[ccameron, 2016-07-12 18:56:58]

lgtm w/suggestions.

LMK if we should plow on with the ScopedRWHViewActionDisabler beast ... it might
prove useful in other contents.

https://codereview.chromium.org/2138213003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm (right):

https://codereview.chromium.org/2138213003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:220: -
(void)addContentsToView:(NSView*)superview {
I'd prefer to avoid having the "nil defaults to something" behavior. Maybe have
this function be

- (void)ensureContentsVisibleInsuperview:(NSView*)superview

And require that callers that would specify nil specify [contentsContainer
superview].

https://codereview.chromium.org/2138213003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:224:
ScopedCAActionDisabler disabler;
Would it be useful to have a "ScopedRWHViewActionDisabler" structure, which
would 
- have a scoped CAAction disabler
- delay any RWHView resizes until its dtor is called
- delay any RWHView show/hide calls until its dtor is called
- would do any required "wait for resized frame from GPU process" in the dtor

Food for thought.

https://codereview.chromium.org/2138213003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:339: [self
addContentsToView:nil];
[self ensureContentsVisibleWithSuperview:[[self view] superview]]

https://codereview.chromium.org/2138213003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:346: [self
addContentsToView:nil];
ditto.

[tapted, 2016-07-13 03:33:18]

https://codereview.chromium.org/2138213003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm (right):

https://codereview.chromium.org/2138213003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:220: -
(void)addContentsToView:(NSView*)superview {
On 2016/07/12 18:56:58, ccameron wrote:
> I'd prefer to avoid having the "nil defaults to something" behavior. Maybe
have
> this function be
> 
> - (void)ensureContentsVisibleInsuperview:(NSView*)superview
> 
> And require that callers that would specify nil specify [contentsContainer
> superview].

hehe - I actually started with this, but it made it harder to write the
`defensive` DCHECK in below. Instead of

  DCHECK(!superview || ![contentsContainer superview]);

I'd need to write something like

  DCHECK(![contentsContainer superview] || superview == [contentsContainer
superview]);

but that's not interesting because if a caller were to "forget" to remove from
the view hierarchy in the first place, it's going to be [contentsContainer
superview] anyway.

So, I've just removed the DCHECK - it's less confusing. If I get inspired I'll
look into a way to write tests for these, but with the pause in setFrameSize
potentially timing out, it's going to require some finesse to ensure they don't
flake.

https://codereview.chromium.org/2138213003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:224:
ScopedCAActionDisabler disabler;
On 2016/07/12 18:56:58, ccameron wrote:
> Would it be useful to have a "ScopedRWHViewActionDisabler" structure, which
> would 
> - have a scoped CAAction disabler
> - delay any RWHView resizes until its dtor is called
> - delay any RWHView show/hide calls until its dtor is called
> - would do any required "wait for resized frame from GPU process" in the dtor
> 
> Food for thought.

I'll keep it at the back of my mind - it does sound useful. I tried thinking of
use-cases it couldn't support, and couldn't think of much. Just some uncertainty
about how it would interact with with AppKit codepaths, like those to/from
setFrameSize

E.g. the call to [contentsNativeView setFrame:] below may pause inside
setFrameSize. We'd want to move that pause to a ScopedRWHViewActionDisabler dtor
in a higher stack (if there is one), but AppKit may have other expectations when
setFrameSize returns, like for generating window shadows or thumbnails (in
reality that specific case probably happens later anyway, but it's hard to know)

https://codereview.chromium.org/2138213003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:339: [self
addContentsToView:nil];
On 2016/07/12 18:56:58, ccameron wrote:
> [self ensureContentsVisibleWithSuperview:[[self view] superview]]

Done.

https://codereview.chromium.org/2138213003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:346: [self
addContentsToView:nil];
On 2016/07/12 18:56:58, ccameron wrote:
> ditto.

Done.

[tapted, 2016-07-13 03:33:35]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-07-13 03:33:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2138213003/#ps80001
(title: "ensureContentsVisibleInSuperview")

[commit-bot: I haz the power, 2016-07-13 03:35:04]

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-07-13 03:45:55]

Description was changed from

==========
Mac: Do layout before adding the WebContents to the view hierarchy when
switching tabs

Currently you can sometimes see a flash of webcontent at the size it was
when a tab was last active, before it updates to fill the window.

This regressed in r378605, which tied a call to WebContents::WasShown to
the insertion into the view hierarchy, which happens before setting the
WebContents size.

There's currently some tricky code to suppress auto-resizing when
switching to/from a tab which might have an infobar showing. Make this
simpler and fix the regression by establishing a correct size for the
WebContents *before* it gets added to the view hierarchy, rather than
after.

Do this by having TabStripController delegate resposibility for setting
view sizes and updating the view heirarchy to TabContentsController.
This is probably nicer anyway, since TabContentsController already has
most of the logic to pick a target WebContents size - it just needs to
know what the size of its superview _will_ be rather than what it
currently is.

BUG=627255
TEST=On Mac, create 2 tabs, make the window small, switch tabs and make
the window big. Switch back to the first tab. There should be no flash
the old, small web contents before it fills the window.
==========

to

==========
Mac: Do layout before adding the WebContents to the view hierarchy when
switching tabs

Currently you can sometimes see a flash of webcontent at the size it was
when a tab was last active, before it updates to fill the window.

This regressed in r378605, which tied a call to WebContents::WasShown to
the insertion into the view hierarchy, which happens before setting the
WebContents size.

There's currently some tricky code to suppress auto-resizing when
switching to/from a tab which might have an infobar showing. Make this
simpler and fix the regression by establishing a correct size for the
WebContents *before* it gets added to the view hierarchy, rather than
after.

Do this by having TabStripController delegate resposibility for setting
view sizes and updating the view heirarchy to TabContentsController.
This is probably nicer anyway, since TabContentsController already has
most of the logic to pick a target WebContents size - it just needs to
know what the size of its superview _will_ be rather than what it
currently is.

BUG=627255
TEST=On Mac, create 2 tabs, make the window small, switch tabs and make
the window big. Switch back to the first tab. There should be no flash
the old, small web contents before it fills the window.
==========

[commit-bot: I haz the power, 2016-07-13 03:45:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-13 03:46:34]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 03:48:13]

Description was changed from

==========
Mac: Do layout before adding the WebContents to the view hierarchy when
switching tabs

Currently you can sometimes see a flash of webcontent at the size it was
when a tab was last active, before it updates to fill the window.

This regressed in r378605, which tied a call to WebContents::WasShown to
the insertion into the view hierarchy, which happens before setting the
WebContents size.

There's currently some tricky code to suppress auto-resizing when
switching to/from a tab which might have an infobar showing. Make this
simpler and fix the regression by establishing a correct size for the
WebContents *before* it gets added to the view hierarchy, rather than
after.

Do this by having TabStripController delegate resposibility for setting
view sizes and updating the view heirarchy to TabContentsController.
This is probably nicer anyway, since TabContentsController already has
most of the logic to pick a target WebContents size - it just needs to
know what the size of its superview _will_ be rather than what it
currently is.

BUG=627255
TEST=On Mac, create 2 tabs, make the window small, switch tabs and make
the window big. Switch back to the first tab. There should be no flash
the old, small web contents before it fills the window.
==========

to

==========
Mac: Do layout before adding the WebContents to the view hierarchy when
switching tabs

Currently you can sometimes see a flash of webcontent at the size it was
when a tab was last active, before it updates to fill the window.

This regressed in r378605, which tied a call to WebContents::WasShown to
the insertion into the view hierarchy, which happens before setting the
WebContents size.

There's currently some tricky code to suppress auto-resizing when
switching to/from a tab which might have an infobar showing. Make this
simpler and fix the regression by establishing a correct size for the
WebContents *before* it gets added to the view hierarchy, rather than
after.

Do this by having TabStripController delegate resposibility for setting
view sizes and updating the view heirarchy to TabContentsController.
This is probably nicer anyway, since TabContentsController already has
most of the logic to pick a target WebContents size - it just needs to
know what the size of its superview _will_ be rather than what it
currently is.

BUG=627255
TEST=On Mac, create 2 tabs, make the window small, switch tabs and make
the window big. Switch back to the first tab. There should be no flash
the old, small web contents before it fills the window.

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

[commit-bot: I haz the power, 2016-07-13 03:48:15]

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