Bug Fix: List Too Small When Sharing Tab Only

Bug Fix: List Too Small When Sharing Tab Only

When the javascript only includes "tab" as source, the source list is too small, and thus inconvenient for users to pick.

The reason is that we preset the height too small.

In this CL we preset the height larger to a reasonable value.

BUG=697516
Review-Url: https://codereview.chromium.org/2725833003
Cr-Commit-Position: refs/heads/master@{#454317}
Committed: https://chromium.googlesource.com/chromium/src/+/866596040a23db3ab4152dba1d41f0a574fc2b83

[qiangchen, 2017-03-01 18:21:53]

Description was changed from

==========
Adjust the list size larger for tab capture

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

to

==========
Bug Fix: List Too Small When Sharing Tab Only

When the javascript only includes "tab" as source, the source list is too small,
and thus inconvenient for users to pick.

The reason is that we preset the height too small.

In this CL we preset the height larger to a reasonable value.

BUG=697516
==========

[qiangchen, 2017-03-01 18:21:53]

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

[qiangchen, 2017-03-01 18:23:02]

A simple fix. Can you take a look?

Thanks.

[msw, 2017-03-01 19:00:03]

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

https://codereview.chromium.org/2725833003/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:172:
tab_scroll_view->ClipHeightTo(kTabStyle.item_size.height() * 7,
Can you post some screenshots of before and after? What happens when the
dialog's minimum height (now 7 times the old minimum height) exceeds the
available height of the browser window? Should the minimum be left alone, but
the maximum increased?

[qiangchen, 2017-03-01 20:59:42]

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

https://codereview.chromium.org/2725833003/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:172:
tab_scroll_view->ClipHeightTo(kTabStyle.item_size.height() * 7,
On 2017/03/01 19:00:03, msw wrote:
> Can you post some screenshots of before and after? What happens when the
> dialog's minimum height (now 7 times the old minimum height) exceeds the
> available height of the browser window? Should the minimum be left alone, but
> the maximum increased?

Reduce it to 5. As 5 * tab height is almost 1 * window height.

Attached screenshot on crbug

[msw, 2017-03-01 23:26:48]

lgtm with a nit

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

https://codereview.chromium.org/2725833003/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:172:
tab_scroll_view->ClipHeightTo(kTabStyle.item_size.height() * 7,
On 2017/03/01 20:59:42, qiangchen wrote:
> On 2017/03/01 19:00:03, msw wrote:
> > Can you post some screenshots of before and after? What happens when the
> > dialog's minimum height (now 7 times the old minimum height) exceeds the
> > available height of the browser window? Should the minimum be left alone,
but
> > the maximum increased?
> 
> Reduce it to 5. As 5 * tab height is almost 1 * window height.
> 
> Attached screenshot on crbug

nit: I still think it might be better to use just one tab height for the
minimum. That way the dialog will still shrink the tab selection area to show
the Cancel/Share buttons, even when the parent chrome window is *super* short.
Otherwise, what's the point of showing 5 tabs if you can't click Share? (please
test out super short chrome windows and lmk what you think).

[qiangchen, 2017-03-02 17:45:45]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2017-03-02 17:45:45]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2725833003/#ps40001
(title: "Reduce to 1")

[commit-bot: I haz the power, 2017-03-02 17:46:05]

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, 2017-03-02 18:41:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488476745365510,
"parent_rev": "d41e1901f74d16f33d861ada539296044fec32c1", "commit_rev":
"866596040a23db3ab4152dba1d41f0a574fc2b83"}

[commit-bot: I haz the power, 2017-03-02 18:43:09]

Description was changed from

==========
Bug Fix: List Too Small When Sharing Tab Only

When the javascript only includes "tab" as source, the source list is too small,
and thus inconvenient for users to pick.

The reason is that we preset the height too small.

In this CL we preset the height larger to a reasonable value.

BUG=697516
==========

to

==========
Bug Fix: List Too Small When Sharing Tab Only

When the javascript only includes "tab" as source, the source list is too small,
and thus inconvenient for users to pick.

The reason is that we preset the height too small.

In this CL we preset the height larger to a reasonable value.

BUG=697516

Review-Url: https://codereview.chromium.org/2725833003
Cr-Commit-Position: refs/heads/master@{#454317}
Committed:
https://chromium.googlesource.com/chromium/src/+/866596040a23db3ab4152dba1d41...
==========

[commit-bot: I haz the power, 2017-03-02 18:43:10]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/866596040a23db3ab4152dba1d41...

[msw, 2017-03-02 18:48:02]

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

https://codereview.chromium.org/2725833003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:172:
tab_scroll_view->ClipHeightTo(kTabStyle.item_size.height() * 1,
It doesn't really matter, but multiplying by 1 isn't actually needed.

[qiangchen, 2017-03-02 18:52:46]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2731573002/ by qiangchen@chromium.org.

The reason for reverting is: A coding style issue.

[qiangchen, 2017-03-03 16:39:32]

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

https://codereview.chromium.org/2725833003/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:172:
tab_scroll_view->ClipHeightTo(kTabStyle.item_size.height() * 7,
On 2017/03/01 23:26:48, msw wrote:
> On 2017/03/01 20:59:42, qiangchen wrote:
> > On 2017/03/01 19:00:03, msw wrote:
> > > Can you post some screenshots of before and after? What happens when the
> > > dialog's minimum height (now 7 times the old minimum height) exceeds the
> > > available height of the browser window? Should the minimum be left alone,
> but
> > > the maximum increased?
> > 
> > Reduce it to 5. As 5 * tab height is almost 1 * window height.
> > 
> > Attached screenshot on crbug
> 
> nit: I still think it might be better to use just one tab height for the
> minimum. That way the dialog will still shrink the tab selection area to show
> the Cancel/Share buttons, even when the parent chrome window is *super* short.
> Otherwise, what's the point of showing 5 tabs if you can't click Share?
(please
> test out super short chrome windows and lmk what you think).

Done.