Bug Fix: Desktop Picker Window Shows Unnecessary Scrollbar

Bug Fix: Desktop Picker Window Shows Unnecessary Scrollbar

When the number of source items just exactly fill the container,
we displayed the scroll bar.

In this CL, we fixed the issue, and not displayed scroll bar in
that case.

BUG=636338
Committed: https://crrev.com/6d191b6ae63639d0c55046ef4bc909726d25a30c
Cr-Commit-Position: refs/heads/master@{#416763}

[qiangchen, 2016-09-01 23:42:36]

Description was changed from

==========
Bug Fix: Desktop Picker Window Shows Unnecessary Scrollbar

BUG=
==========

to

==========
Bug Fix: Desktop Picker Window Shows Unnecessary Scrollbar

When the number of source items just exactly fill the container,
we displayed the scroll bar. 

In this CL, we fixed the issue, and not displayed scroll bar in
that case. 

BUG=636338
==========

[qiangchen, 2016-09-01 23:42:36]

qiangchen@chromium.org changed reviewers:
+ msw@chromium.org

[qiangchen, 2016-09-01 23:50:18]

Experimented.

I think we still need to SetSize in the ListView's Layout.

[msw, 2016-09-02 00:35:30]

https://codereview.chromium.org/2304743002/diff/1/chrome/browser/ui/views/des...
File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right):

https://codereview.chromium.org/2304743002/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:122:
SetSize(gfx::Size(active_style_->columns * active_style_->item_size.width(),
Layout() is typically called in response to size changes; it's not common (and
potentially dangerous?) to change the view size during layout, afaik. (ie. this
SetSize() should re-trigger Layout() until your SetSize() argument matches the
current View::size(), which seems potentially dangerous)

It's more likely that the preferred size is slightly incorrect (missing some
padding or border or scrollbar size or something?) or isn't correctly updated
before layout. Here are some questions that should help determine where the
problem lies:

1) What is the difference between the current size() here and the size you're
attempting to set?
2) What is the last value returned from GetPreferredSize? (should match size()
here)
3) When is Layout called relative to AddChildViewAt/PreferredSizeChanged calls
in OnSourceAdded?

[qiangchen, 2016-09-02 16:14:49]

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

[qiangchen, 2016-09-02 16:19:52]

Add sky@ in.

This CL is a superfical fix for crbug/636338.

My suggest thorough fix is stated in comment #7 in the bug.

But that would touch the fundamental view file scroll_view.cc, and it might
affect other UI components.

PTAL.

https://codereview.chromium.org/2304743002/diff/1/chrome/browser/ui/views/des...
File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right):

https://codereview.chromium.org/2304743002/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:122:
SetSize(gfx::Size(active_style_->columns * active_style_->item_size.width(),
On 2016/09/02 00:35:29, msw wrote:
> Layout() is typically called in response to size changes; it's not common (and
> potentially dangerous?) to change the view size during layout, afaik. (ie.
this
> SetSize() should re-trigger Layout() until your SetSize() argument matches the
> current View::size(), which seems potentially dangerous)
> 
> It's more likely that the preferred size is slightly incorrect (missing some
> padding or border or scrollbar size or something?) or isn't correctly updated
> before layout. Here are some questions that should help determine where the
> problem lies:
> 
> 1) What is the difference between the current size() here and the size you're
> attempting to set?
> 2) What is the last value returned from GetPreferredSize? (should match size()
> here)
> 3) When is Layout called relative to AddChildViewAt/PreferredSizeChanged calls
> in OnSourceAdded?

See my comment#5 in the bug for a thorough cause. In short
1. The Preferred Size does not play a role at all from my testing.
2. The size will always be set just by its parent, just before layout() gets
called. 
3. If we do not SetSize in layout then scroll bar will always be visible.

[sky, 2016-09-02 18:38:30]

https://codereview.chromium.org/2304743002/diff/1/chrome/browser/ui/views/des...
File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right):

https://codereview.chromium.org/2304743002/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:122:
SetSize(gfx::Size(active_style_->columns * active_style_->item_size.width(),
On 2016/09/02 00:35:29, msw wrote:
> Layout() is typically called in response to size changes; it's not common (and
> potentially dangerous?) to change the view size during layout, afaik. (ie.
this
> SetSize() should re-trigger Layout() until your SetSize() argument matches the
> current View::size(), which seems potentially dangerous)
> 
> It's more likely that the preferred size is slightly incorrect (missing some
> padding or border or scrollbar size or something?) or isn't correctly updated
> before layout. Here are some questions that should help determine where the
> problem lies:
> 
> 1) What is the difference between the current size() here and the size you're
> attempting to set?
> 2) What is the last value returned from GetPreferredSize? (should match size()
> here)
> 3) When is Layout called relative to AddChildViewAt/PreferredSizeChanged calls
> in OnSourceAdded?

I agree this seems weird, but ScrollView has weird semantics and in general
views contained in a ScrollView set their bounds in layout. We should really
make ScrollView behave better in this regard, but until them I'm fine with this.
One comment though, it's pretty typical to go make sure the contents view of a
scrollview is at least as wide/high as the parent (see TableView::Layout). Are
you sure you don't want that? The reason for that is so you get whatever
background you may have on the contents view filling the bounds.

[qiangchen, 2016-09-02 22:08:52]

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

[commit-bot: I haz the power, 2016-09-02 22:09:12]

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

[qiangchen, 2016-09-02 22:10:57]

Thanks. Fixed, and put a TODO comment in the code.

https://codereview.chromium.org/2304743002/diff/1/chrome/browser/ui/views/des...
File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right):

https://codereview.chromium.org/2304743002/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:122:
SetSize(gfx::Size(active_style_->columns * active_style_->item_size.width(),
On 2016/09/02 18:38:30, sky wrote:
> On 2016/09/02 00:35:29, msw wrote:
> > Layout() is typically called in response to size changes; it's not common
(and
> > potentially dangerous?) to change the view size during layout, afaik. (ie.
> this
> > SetSize() should re-trigger Layout() until your SetSize() argument matches
the
> > current View::size(), which seems potentially dangerous)
> > 
> > It's more likely that the preferred size is slightly incorrect (missing some
> > padding or border or scrollbar size or something?) or isn't correctly
updated
> > before layout. Here are some questions that should help determine where the
> > problem lies:
> > 
> > 1) What is the difference between the current size() here and the size
you're
> > attempting to set?
> > 2) What is the last value returned from GetPreferredSize? (should match
size()
> > here)
> > 3) When is Layout called relative to AddChildViewAt/PreferredSizeChanged
calls
> > in OnSourceAdded?
> 
> I agree this seems weird, but ScrollView has weird semantics and in general
> views contained in a ScrollView set their bounds in layout. We should really
> make ScrollView behave better in this regard, but until them I'm fine with
this.
> One comment though, it's pretty typical to go make sure the contents view of a
> scrollview is at least as wide/high as the parent (see TableView::Layout). Are
> you sure you don't want that? The reason for that is so you get whatever
> background you may have on the contents view filling the bounds.

Done.

As our logic guarantees no horizontal overflow, and thus the width will be just
parent_content_size.width().

[commit-bot: I haz the power, 2016-09-02 22:59:38]

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

[commit-bot: I haz the power, 2016-09-02 22:59:38]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-06 18:08:51]

https://codereview.chromium.org/2304743002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right):

https://codereview.chromium.org/2304743002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:127:
SetSize(gfx::Size(parent_content_size.width(),
It seems like you have an inherent preferred width based on the number of
columns/children? I'm suggesting the with should be a
std::max(parent_content_size.width(), GetPreferredSize().width())
I have GetPreferredSize().width(), but I suspect you could also calculate the
pref width in this function as well.

[qiangchen, 2016-09-06 18:38:10]

Fixed. Thanks.

https://codereview.chromium.org/2304743002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right):

https://codereview.chromium.org/2304743002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:127:
SetSize(gfx::Size(parent_content_size.width(),
On 2016/09/06 18:08:50, sky wrote:
> It seems like you have an inherent preferred width based on the number of
> columns/children? I'm suggesting the with should be a
> std::max(parent_content_size.width(), GetPreferredSize().width())
> I have GetPreferredSize().width(), but I suspect you could also calculate the
> pref width in this function as well.

Done.

[msw, 2016-09-06 19:01:13]

lgtm with nits; thanks for explaining the underlying issue.

https://codereview.chromium.org/2304743002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right):

https://codereview.chromium.org/2304743002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:124: //
container. See crbug/636338.
nit: file a new bug for the TODO to fix the underlying issue and remove this
code, independent of this screen picker manifestation.

https://codereview.chromium.org/2304743002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:128:
SetSize(gfx::Size(
optional nit: use Size::SetToMax():
gfx::Size preferred_size = GetPreferredSize();
preferred_size.SetToMax(parent()->GetContentsBounds().size());
SetSize(preferred_size);

[sky, 2016-09-06 20:33:40]

LGTM

[qiangchen, 2016-09-06 21:48:03]

https://codereview.chromium.org/2304743002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right):

https://codereview.chromium.org/2304743002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:124: //
container. See crbug/636338.
On 2016/09/06 19:01:13, msw wrote:
> nit: file a new bug for the TODO to fix the underlying issue and remove this
> code, independent of this screen picker manifestation.

Done.

https://codereview.chromium.org/2304743002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:128:
SetSize(gfx::Size(
On 2016/09/06 19:01:13, msw wrote:
> optional nit: use Size::SetToMax():
> gfx::Size preferred_size = GetPreferredSize();
> preferred_size.SetToMax(parent()->GetContentsBounds().size());
> SetSize(preferred_size);

Done.

[qiangchen, 2016-09-06 21:48:11]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-09-06 21:48:12]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2304743002/#ps60001
(title: "Comment Fix")

[commit-bot: I haz the power, 2016-09-06 21:48:53]

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-06 23:17:35]

Description was changed from

==========
Bug Fix: Desktop Picker Window Shows Unnecessary Scrollbar

When the number of source items just exactly fill the container,
we displayed the scroll bar. 

In this CL, we fixed the issue, and not displayed scroll bar in
that case. 

BUG=636338
==========

to

==========
Bug Fix: Desktop Picker Window Shows Unnecessary Scrollbar

When the number of source items just exactly fill the container,
we displayed the scroll bar. 

In this CL, we fixed the issue, and not displayed scroll bar in
that case. 

BUG=636338
==========

[commit-bot: I haz the power, 2016-09-06 23:17:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-06 23:20:03]

Description was changed from

==========
Bug Fix: Desktop Picker Window Shows Unnecessary Scrollbar

When the number of source items just exactly fill the container,
we displayed the scroll bar. 

In this CL, we fixed the issue, and not displayed scroll bar in
that case. 

BUG=636338
==========

to

==========
Bug Fix: Desktop Picker Window Shows Unnecessary Scrollbar

When the number of source items just exactly fill the container,
we displayed the scroll bar.

In this CL, we fixed the issue, and not displayed scroll bar in
that case.

BUG=636338

Committed: https://crrev.com/6d191b6ae63639d0c55046ef4bc909726d25a30c
Cr-Commit-Position: refs/heads/master@{#416763}
==========

[commit-bot: I haz the power, 2016-09-06 23:20:05]

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