Remove layout during paint in Tab

Remove layout during paint in Tab

I am trying to separate out changing compositor layer sizes in the
middle of paint.  Blink no longer does this at all, and this appears to
be one (or the only) remaining place that continues to do this.  In
order to support this, the compositor has to have some confusing logic
to keep around several values and pick the real one that I'd like to
remove.  (See: strict property checking in cc)

It appears that icons and media indicators will get a layout the next
time Tab::SetData is called, so does not need to be re-checked during
paint.  The close button is not the same (which may be hidden in cases
where only the active tab shows it) and so needs to be checked in
ActiveStateChanged.

R=sky@chromium.org,miu@chromium.org

Committed: https://crrev.com/7624ce8b98f34253f57889ed41a30a6bbf2a5bb5
Cr-Commit-Position: refs/heads/master@{#369551}

[miu, 2016-01-08 00:25:39]

Mostly looks good.  However, one important consideration:

https://codereview.chromium.org/1566313002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1566313002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab.cc:549: if (ShouldShowCloseBox() !=
showing_close_button_)
I think we have to unconditionally call Layout() here.  The reason is that the
tab's active state changes the set of which buttons, favicon, title, etc. are
visible.

For reference, see the three methods starting here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Even if what you have here always results in the correct behavior, it implies
knowledge internal to c/b/ui/tabs/tab_utils.cc.

[enne (OOO), 2016-01-08 01:46:25]

https://codereview.chromium.org/1566313002/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1566313002/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab.cc:549: if (ShouldShowCloseBox() !=
showing_close_button_)
On 2016/01/08 at 00:25:39, miu wrote:
> I think we have to unconditionally call Layout() here.  The reason is that the
tab's active state changes the set of which buttons, favicon, title, etc. are
visible.
> 
> For reference, see the three methods starting here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> Even if what you have here always results in the correct behavior, it implies
knowledge internal to c/b/ui/tabs/tab_utils.cc.

Thanks! Updated the patch.

[miu, 2016-01-08 07:13:30]

lgtm

[sky, 2016-01-08 16:40:32]

In general how are you going to ensure no one attempts to change the layer size
during paint? It's often very difficult to determine if that does or doesn't
happen?

Lastly, is this the cause of a crash I got yesterday:
https://crash.corp.google.com/browse?stbtiq=fd2091b27b557cf2%20 ?

https://codereview.chromium.org/1566313002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1566313002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:548: Layout();
Can we call layout only if we actually need to?

[enne (OOO), 2016-01-08 18:22:28]

On 2016/01/08 at 16:40:32, sky wrote:
> In general how are you going to ensure no one attempts to change the layer
size during paint? It's often very difficult to determine if that does or
doesn't happen?
>
> Lastly, is this the cause of a crash I got yesterday:
https://crash.corp.google.com/browse?stbtiq=fd2091b27b557cf2%20 ?

Yes, it is.

Long-term, I was going to leave DCHECKs instead of that CHECK that you hit.

https://codereview.chromium.org/1566313002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/1566313002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:548: Layout();
On 2016/01/08 at 16:40:32, sky wrote:
> Can we call layout only if we actually need to?

Can you and miu sort out what you want to do here? Patch 1 had a conditional
here, but miu didn't want it.

[sky, 2016-01-08 18:24:51]

Ok, LGTM

[sky, 2016-01-08 18:25:09]

By which I mean I missed miu's reasoning, which looks right.

[enne (OOO), 2016-01-08 18:25:44]

Thanks.  :)

[enne (OOO), 2016-01-08 18:25:48]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-01-08 18:25:58]

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

[commit-bot: I haz the power, 2016-01-08 18:52:43]

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

[commit-bot: I haz the power, 2016-01-08 18:52:44]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2016-01-12 21:16:06]

Oops, my apologies for not fixing these tests first.  I thought they'd be in
views_unittests, but they're instead just in normal unit_tests.  PTAL

I updated the tab strip to call Layout or ActiveStateChanged in a few more
places.  This keeps the button updated correctly.

The only question I have is that the TabCloseButtonVisibilityWhenStacked seems
to be doing the wrong thing.  The comment says it is trying to close the active
tab (which is #2), but closes #1 instead.  However, because close appears to
have an animation, I can't check that the close button is gone.  If I update the
test on master to close tab #2 instead, #2 continues to be active and show the
close button even after closing it and painting.  Can you help me understand
what should be tested here?  In my patch above, I have just left this last case,
as it appears in practice closing a tab will cause a layout eventually, even if
I don't know what to do in this test.

[sky, 2016-01-12 22:29:11]

As to the test failure. What specific part of the test are you referring to
(what line number)?

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:733: tab_at(i)->Layout();
Can you elaborate as to why this is necessary (and add a comment too). In theory
if we do something interesting here, then SwapLayoutIfNecessary will trigger an
animation that will cause each tabs bounds to change and trigger a layout.

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1149: int old_active =
controller_->GetActiveIndex();
How come TabStrip::SetSelection doesn't cover this?

[enne (OOO), 2016-01-12 22:36:29]

On 2016/01/12 at 22:29:11, sky wrote:
> As to the test failure. What specific part of the test are you referring to
(what line number)?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:733: tab_at(i)->Layout();
On 2016/01/12 at 22:29:11, sky wrote:
> Can you elaborate as to why this is necessary (and add a comment too). In
theory if we do something interesting here, then SwapLayoutIfNecessary will
trigger an animation that will cause each tabs bounds to change and trigger a
layout.

Changing stacked layout can change whether or not buttons are visible. 
Animations seem somewhat of an implementation detail, but I don't know this
code.  Should the test just call layout, similar to how it was calling paint
before?  I'm not sure that makes the test very valuable, but maybe that's the
best approach.

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1149: int old_active =
controller_->GetActiveIndex();
On 2016/01/12 at 22:29:11, sky wrote:
> How come TabStrip::SetSelection doesn't cover this?

SelectTab and SetSelection are both public functions that do not call each
other.  The test calls SelectTab, and so SetSelection is not relevant here.

[sky, 2016-01-12 22:41:02]

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:733: tab_at(i)->Layout();
On 2016/01/12 22:36:28, enne wrote:
> On 2016/01/12 at 22:29:11, sky wrote:
> > Can you elaborate as to why this is necessary (and add a comment too). In
> theory if we do something interesting here, then SwapLayoutIfNecessary will
> trigger an animation that will cause each tabs bounds to change and trigger a
> layout.
> 
> Changing stacked layout can change whether or not buttons are visible. 
> Animations seem somewhat of an implementation detail, but I don't know this
> code.  Should the test just call layout, similar to how it was calling paint
> before?

Yes, because the animation triggers layout.

> I'm not sure that makes the test very valuable, but maybe that's the
> best approach.

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1149: int old_active =
controller_->GetActiveIndex();
On 2016/01/12 22:36:29, enne wrote:
> On 2016/01/12 at 22:29:11, sky wrote:
> > How come TabStrip::SetSelection doesn't cover this?
> 
> SelectTab and SetSelection are both public functions that do not call each
> other.  The test calls SelectTab, and so SetSelection is not relevant here.

SelectTab calls to the TabStripController, which should result in a call back to
SetSelection. Are you seeing otherwise? If it's from a test, then the test may
not be calling SetSelection as real code does.

[sky, 2016-01-12 22:47:41]

On 2016/01/12 22:41:02, sky wrote:
>
https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
> File chrome/browser/ui/views/tabs/tab_strip.cc (right):
> 
>
https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
> chrome/browser/ui/views/tabs/tab_strip.cc:733: tab_at(i)->Layout();
> On 2016/01/12 22:36:28, enne wrote:
> > On 2016/01/12 at 22:29:11, sky wrote:
> > > Can you elaborate as to why this is necessary (and add a comment too). In
> > theory if we do something interesting here, then SwapLayoutIfNecessary will
> > trigger an animation that will cause each tabs bounds to change and trigger
a
> > layout.
> > 
> > Changing stacked layout can change whether or not buttons are visible. 
> > Animations seem somewhat of an implementation detail, but I don't know this
> > code.  Should the test just call layout, similar to how it was calling paint
> > before?
> 
> Yes, because the animation triggers layout.

I take that back. We aren't always guaranteed that the bounds will change. You
should add a function to Tab that is called in this case, something like
HideCloseButtonForInactiveTabsChanged() that in turn calls Layout().
> 
> > I'm not sure that makes the test very valuable, but maybe that's the
> > best approach.
> 
>
https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
> chrome/browser/ui/views/tabs/tab_strip.cc:1149: int old_active =
> controller_->GetActiveIndex();
> On 2016/01/12 22:36:29, enne wrote:
> > On 2016/01/12 at 22:29:11, sky wrote:
> > > How come TabStrip::SetSelection doesn't cover this?
> > 
> > SelectTab and SetSelection are both public functions that do not call each
> > other.  The test calls SelectTab, and so SetSelection is not relevant here.
> 
> SelectTab calls to the TabStripController, which should result in a call back
to
> SetSelection. Are you seeing otherwise? If it's from a test, then the test may
> not be calling SetSelection as real code does.

[enne (OOO), 2016-01-12 22:54:54]

On 2016/01/12 at 22:47:41, sky wrote:
> On 2016/01/12 22:41:02, sky wrote:
> >
https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/tabs/tab_strip.cc (right):
> > 
> >
https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
> > chrome/browser/ui/views/tabs/tab_strip.cc:733: tab_at(i)->Layout();
> > On 2016/01/12 22:36:28, enne wrote:
> > > On 2016/01/12 at 22:29:11, sky wrote:
> > > > Can you elaborate as to why this is necessary (and add a comment too).
In
> > > theory if we do something interesting here, then SwapLayoutIfNecessary
will
> > > trigger an animation that will cause each tabs bounds to change and
trigger a
> > > layout.
> > > 
> > > Changing stacked layout can change whether or not buttons are visible. 
> > > Animations seem somewhat of an implementation detail, but I don't know
this
> > > code.  Should the test just call layout, similar to how it was calling
paint
> > > before?
> > 
> > Yes, because the animation triggers layout.
> 
> I take that back. We aren't always guaranteed that the bounds will change. You
should add a function to Tab that is called in this case, something like
HideCloseButtonForInactiveTabsChanged() that in turn calls Layout().

Done.

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1149: int old_active =
controller_->GetActiveIndex();
On 2016/01/12 at 22:41:02, sky wrote:
> On 2016/01/12 22:36:29, enne wrote:
> > On 2016/01/12 at 22:29:11, sky wrote:
> > > How come TabStrip::SetSelection doesn't cover this?
> > 
> > SelectTab and SetSelection are both public functions that do not call each
> > other.  The test calls SelectTab, and so SetSelection is not relevant here.
> 
> SelectTab calls to the TabStripController, which should result in a call back
to SetSelection. Are you seeing otherwise? If it's from a test, then the test
may not be calling SetSelection as real code does.

It does not appear to call SetSelection from the test.

[sky, 2016-01-13 00:41:09]

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1149: int old_active =
controller_->GetActiveIndex();
On 2016/01/12 22:54:54, enne wrote:
> On 2016/01/12 at 22:41:02, sky wrote:
> > On 2016/01/12 22:36:29, enne wrote:
> > > On 2016/01/12 at 22:29:11, sky wrote:
> > > > How come TabStrip::SetSelection doesn't cover this?
> > > 
> > > SelectTab and SetSelection are both public functions that do not call each
> > > other.  The test calls SelectTab, and so SetSelection is not relevant
here.
> > 
> > SelectTab calls to the TabStripController, which should result in a call
back
> to SetSelection. Are you seeing otherwise? If it's from a test, then the test
> may not be calling SetSelection as real code does.
> 
> It does not appear to call SetSelection from the test.

Please update the caller to do that then as that is the expectation of
Controller::SelectTab.

[enne (OOO), 2016-01-13 21:44:12]

On 2016/01/13 at 00:41:09, sky wrote:
>
https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
> File chrome/browser/ui/views/tabs/tab_strip.cc (right):
> 
>
https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
> chrome/browser/ui/views/tabs/tab_strip.cc:1149: int old_active =
controller_->GetActiveIndex();
> On 2016/01/12 22:54:54, enne wrote:
> > On 2016/01/12 at 22:41:02, sky wrote:
> > > On 2016/01/12 22:36:29, enne wrote:
> > > > On 2016/01/12 at 22:29:11, sky wrote:
> > > > > How come TabStrip::SetSelection doesn't cover this?
> > > > 
> > > > SelectTab and SetSelection are both public functions that do not call
each
> > > > other.  The test calls SelectTab, and so SetSelection is not relevant
here.
> > > 
> > > SelectTab calls to the TabStripController, which should result in a call
back
> > to SetSelection. Are you seeing otherwise? If it's from a test, then the
test
> > may not be calling SetSelection as real code does.
> > 
> > It does not appear to call SetSelection from the test.
> 
> Please update the caller to do that then as that is the expectation of
Controller::SelectTab.

It looks like the FakeBaseTabStripController doesn't actually have a model and
nothing touches the selection model, so I can't just call SetSelection directly
unless I keep the selection model up to date in the fake controller.

Are you saying I should add all of the TabStripModel logic that
BrowserTabStripController has and move that into FakeBaseTabStripController? I
think I'd also need to make a FakeTabStripModelDelegate in order to make the
TabStripModel?  Alternatively, should I update the FakeBaseTabStripController to
update its ListSelectionModel?  In either case, both of these pieces of code are
producing fake data to call back into SetSelection, so if there's a bug in
TabStripModel, this test will not catch it.

Could you give me a little bit more guidance?  Just double checking before I do
all of this to satisfy this unit test.

[sky, 2016-01-14 04:05:27]

On 2016/01/13 21:44:12, enne wrote:
> On 2016/01/13 at 00:41:09, sky wrote:
> >
>
https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/tabs/tab_strip.cc (right):
> > 
> >
>
https://codereview.chromium.org/1566313002/diff/60001/chrome/browser/ui/views...
> > chrome/browser/ui/views/tabs/tab_strip.cc:1149: int old_active =
> controller_->GetActiveIndex();
> > On 2016/01/12 22:54:54, enne wrote:
> > > On 2016/01/12 at 22:41:02, sky wrote:
> > > > On 2016/01/12 22:36:29, enne wrote:
> > > > > On 2016/01/12 at 22:29:11, sky wrote:
> > > > > > How come TabStrip::SetSelection doesn't cover this?
> > > > > 
> > > > > SelectTab and SetSelection are both public functions that do not call
> each
> > > > > other.  The test calls SelectTab, and so SetSelection is not relevant
> here.
> > > > 
> > > > SelectTab calls to the TabStripController, which should result in a call
> back
> > > to SetSelection. Are you seeing otherwise? If it's from a test, then the
> test
> > > may not be calling SetSelection as real code does.
> > > 
> > > It does not appear to call SetSelection from the test.
> > 
> > Please update the caller to do that then as that is the expectation of
> Controller::SelectTab.
> 
> It looks like the FakeBaseTabStripController doesn't actually have a model and
> nothing touches the selection model, so I can't just call SetSelection
directly
> unless I keep the selection model up to date in the fake controller.
> 
> Are you saying I should add all of the TabStripModel logic that
> BrowserTabStripController has and move that into FakeBaseTabStripController? I
> think I'd also need to make a FakeTabStripModelDelegate in order to make the
> TabStripModel?  Alternatively, should I update the FakeBaseTabStripController
to
> update its ListSelectionModel?  In either case, both of these pieces of code
are
> producing fake data to call back into SetSelection, so if there's a bug in
> TabStripModel, this test will not catch it.
> 
> Could you give me a little bit more guidance?  Just double checking before I
do
> all of this to satisfy this unit test.

I believe this is all you need:

dex 2ea2fb8..5054a37 100644                                                     
                                                                                

--- a/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc            
                                                                                
  
+++ b/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc            
                                                                                
  
@@ -66,9 +66,13 @@ bool FakeBaseTabStripController::IsNewTabPage(int index)
const {
 }                                                                              
                                                                                
  
                                                                                
                                                                                
  
 void FakeBaseTabStripController::SelectTab(int index) {                        
                                                                                
  
-  if (!IsValidIndex(index))                                                    
                                                                                
  
+  if (!IsValidIndex(index) || active_index_ == index)                          
                                                                                
  
     return;                                                                    
                                                                                
  
+  ui::ListSelectionModel old_selection_model;                                  
                                                                                
  
+  old_selection_model.SetSelectedIndex(active_index_);                         
                                                                                
  
   active_index_ = index;                                                       
                                                                                
  
+  selection_model_.SetSelectedIndex(active_index_);                            
                                                                                
  
+  tab_strip_->SetSelection(old_selection_model, selection_model_);             
                                                                                
  
 }                                                                              
                                                                                
  
                                                                                
                                                                                
  
 void FakeBaseTabStripController::ExtendSelectionTo(int index) {      

At least with your latest patch and removing the changes to TabStrip::SelectTab
that got things working. Really FakeBaseTabStripController should keep
selection_model_ up to date. But I suspect it wasn't needed for the test
coverage at the time.

[enne (OOO), 2016-01-14 18:02:18]

Thank you so much.  I didn't mean for you to just hand me a patch.  :C

I've added those changes and removed the SelectTab changes in the latest
patchset.

[sky, 2016-01-14 20:38:24]

LGTM - I wasn't sure how involved the test change was, so I poked at it.

[enne (OOO), 2016-01-14 20:43:20]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-01-14 20:43:20]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1566313002/#ps100001
(title: "With sky's changes to fake controller")

[commit-bot: I haz the power, 2016-01-14 20:44:32]

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

[commit-bot: I haz the power, 2016-01-14 21:42:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-14 22:03:40]

Description was changed from

==========
Remove layout during paint in Tab

I am trying to separate out changing compositor layer sizes in the
middle of paint.  Blink no longer does this at all, and this appears to
be one (or the only) remaining place that continues to do this.  In
order to support this, the compositor has to have some confusing logic
to keep around several values and pick the real one that I'd like to
remove.  (See: strict property checking in cc)

It appears that icons and media indicators will get a layout the next
time Tab::SetData is called, so does not need to be re-checked during
paint.  The close button is not the same (which may be hidden in cases
where only the active tab shows it) and so needs to be checked in
ActiveStateChanged.

R=sky@chromium.org,miu@chromium.org
==========

to

==========
Remove layout during paint in Tab

I am trying to separate out changing compositor layer sizes in the
middle of paint.  Blink no longer does this at all, and this appears to
be one (or the only) remaining place that continues to do this.  In
order to support this, the compositor has to have some confusing logic
to keep around several values and pick the real one that I'd like to
remove.  (See: strict property checking in cc)

It appears that icons and media indicators will get a layout the next
time Tab::SetData is called, so does not need to be re-checked during
paint.  The close button is not the same (which may be hidden in cases
where only the active tab shows it) and so needs to be checked in
ActiveStateChanged.

R=sky@chromium.org,miu@chromium.org

Committed: https://crrev.com/7624ce8b98f34253f57889ed41a30a6bbf2a5bb5
Cr-Commit-Position: refs/heads/master@{#369551}
==========

[commit-bot: I haz the power, 2016-01-14 22:03:41]

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