Bug Fix: Desktop Picker Window Shows Unnecessary Scrollbar

chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 100 matching lines @@
     if (i > 0 && i % active_style_->columns == 0) {
       x = 0;
       y += active_style_->item_size.height();
     }
 
     child_at(i)->SetBounds(x, y, active_style_->item_size.width(),
                            active_style_->item_size.height());
 
     x += active_style_->item_size.width();
   }
+
+  SetSize(gfx::Size(active_style_->columns * active_style_->item_size.width(),

[msw, 2016/09/02 00:35:29]

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:19:52]

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]

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:10:57]

Done.

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

+                    y + active_style_->item_size.height()));
 }
 
 bool DesktopMediaListView::OnKeyPressed(const ui::KeyEvent& event) {
   int position_increment = 0;
   switch (event.key_code()) {
     case ui::VKEY_UP:
       position_increment = -active_style_->columns;
       break;
     case ui::VKEY_DOWN:
       position_increment = active_style_->columns;
@@ skipping 142 matching lines @@
 }
 
 DesktopMediaSourceView* DesktopMediaListView::GetChild(int index) {
   return static_cast<DesktopMediaSourceView*>(child_at(index));
 }
 
 void DesktopMediaListView::GetAccessibleState(ui::AXViewState* state) {
   state->role = ui::AX_ROLE_GROUP;
   state->name = accessible_name_;
 }