Desktop Capture Picker New UI: Non Mac Structure Change

Desktop Capture Picker New UI: Non Mac Structure Change

This CL uses separate list view to handle |screen_list|, |window_list|
and |tab_list|, and adds buttons to switch between those list views.

BUG=602478

[qiangchen, 2016-04-20 23:22:16]

Description was changed from

==========
Desktop Capture Picker New UI: Non Mac Structure Change

BUG=602478
==========

to

==========
Desktop Capture Picker New UI: Non Mac Structure Change

This CL uses separate list view to handle |screen_list|, |window_list|
and |tab_list|, and adds buttons to switch between those list views.

BUG=602478
==========

[qiangchen, 2016-04-21 22:08:27]

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

[qiangchen, 2016-04-21 22:08:27]

This CL changes the "topology" for the new UI of picker window.

It handles three media_lists in three scroll views. 

The appearance change of the source_views worths another CL.

[qiangchen, 2016-04-21 22:08:36]

Patchset #1 (id:1) has been deleted

[qiangchen, 2016-04-21 22:08:42]

Patchset #1 (id:20001) has been deleted

[msw, 2016-04-22 22:19:56]

I didn't do a more detailed review, I think this UI should use
views::TabbedPane; you're essentially rebuilding the same thing with buttons. A
one-off impl will feel disjoint from other tabbed pane UIs, and it won't benefit
from common UI control testing/polish/fixes/updates.

https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13549: +      <message
name="IDS_DESKTOP_MEDIA_PICKER_SOURCE_TYPE_SCREEN" desc="Text for the button on
the window picker dialog, clicking which takes one to full screen sharing">
nit: say "clicking the button", not "clicking which" here and below.

https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13550: +        YOUR ENTIRE SCREEN
Why are these in ALL CAPS? Should our UI to be yelling at the user?

https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13556: +        CHROME TAB
Shouldn't we insert the actual product string here? (chromium / chrome)

https://codereview.chromium.org/1909663004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_media_picker_views.h (right):

https://codereview.chromium.org/1909663004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.h:176: int current_index_;
nit: current index of what? Maybe |current_source_index_|?

[qiangchen, 2016-04-25 17:27:00]

The strings are from UX people.
I replied in the comment.

For other structural fixes you suggested, I'll drop this CL, and submit another
CL resolving them, because niklase@ thinks it is better to hide new UI behind a
flag, which is a big change too.

https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13550: +        YOUR ENTIRE SCREEN
On 2016/04/22 22:19:56, msw wrote:
> Why are these in ALL CAPS? Should our UI to be yelling at the user?

I just follow the design from UX people.

https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13556: +        CHROME TAB
On 2016/04/22 22:19:56, msw wrote:
> Shouldn't we insert the actual product string here? (chromium / chrome)

Hmm, how about showing "Chrome tab" for official build and "Chromium Tab" for
non-official build?
I think one reason that UX people includes "Chrome" here is to make this tab
button similar size with the other two. 
Otherwise, two big buttons with one small button looks bad.

[msw, 2016-04-25 17:40:21]

https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13550: +        YOUR ENTIRE SCREEN
On 2016/04/25 17:27:00, qiangchenC wrote:
> On 2016/04/22 22:19:56, msw wrote:
> > Why are these in ALL CAPS? Should our UI to be yelling at the user?
> 
> I just follow the design from UX people.

Push back on this, it's unusual. I posted on the bug and design doc.

https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13556: +        CHROME TAB
On 2016/04/25 17:27:00, qiangchenC wrote:
> On 2016/04/22 22:19:56, msw wrote:
> > Shouldn't we insert the actual product string here? (chromium / chrome)
> 
> Hmm, how about showing "Chrome tab" for official build and "Chromium Tab" for
> non-official build?
> I think one reason that UX people includes "Chrome" here is to make this tab
> button similar size with the other two. 
> Otherwise, two big buttons with one small button looks bad.

Yes, use ids_short_product_name or similar; Chromium shouldn't say "Chrome".

[qiangchen, 2016-04-28 18:08:04]

Hi, Mike:

    When I tried to implement the UI change using TabbedPane, I found one
difficulty: It restricts my freedom of controlling the appearance of the pane
tab.

    For example, I did not see a method that I can set the font and layout of
the text in the pane tab.

    Do you have an idea?

    Best

Qiang

On 2016/04/25 17:40:21, msw wrote:
>
https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:13550: +        YOUR ENTIRE SCREEN
> On 2016/04/25 17:27:00, qiangchenC wrote:
> > On 2016/04/22 22:19:56, msw wrote:
> > > Why are these in ALL CAPS? Should our UI to be yelling at the user?
> > 
> > I just follow the design from UX people.
> 
> Push back on this, it's unusual. I posted on the bug and design doc.
> 
>
https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:13556: +        CHROME TAB
> On 2016/04/25 17:27:00, qiangchenC wrote:
> > On 2016/04/22 22:19:56, msw wrote:
> > > Shouldn't we insert the actual product string here? (chromium / chrome)
> > 
> > Hmm, how about showing "Chrome tab" for official build and "Chromium Tab"
for
> > non-official build?
> > I think one reason that UX people includes "Chrome" here is to make this tab
> > button similar size with the other two. 
> > Otherwise, two big buttons with one small button looks bad.
> 
> Yes, use ids_short_product_name or similar; Chromium shouldn't say "Chrome".

[msw, 2016-04-28 18:46:10]

On 2016/04/28 18:08:04, qiangchenC wrote:
> Hi, Mike:
> 
>     When I tried to implement the UI change using TabbedPane, I found one
> difficulty: It restricts my freedom of controlling the appearance of the pane
> tab.
> 
>     For example, I did not see a method that I can set the font and layout of
> the text in the pane tab.
> 
>     Do you have an idea?
> 
>     Best
> 
> Qiang
> 
> On 2016/04/25 17:40:21, msw wrote:
> >
>
https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
> > File chrome/app/generated_resources.grd (right):
> > 
> >
>
https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
> > chrome/app/generated_resources.grd:13550: +        YOUR ENTIRE SCREEN
> > On 2016/04/25 17:27:00, qiangchenC wrote:
> > > On 2016/04/22 22:19:56, msw wrote:
> > > > Why are these in ALL CAPS? Should our UI to be yelling at the user?
> > > 
> > > I just follow the design from UX people.
> > 
> > Push back on this, it's unusual. I posted on the bug and design doc.
> > 
> >
>
https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_re...
> > chrome/app/generated_resources.grd:13556: +        CHROME TAB
> > On 2016/04/25 17:27:00, qiangchenC wrote:
> > > On 2016/04/22 22:19:56, msw wrote:
> > > > Shouldn't we insert the actual product string here? (chromium / chrome)
> > > 
> > > Hmm, how about showing "Chrome tab" for official build and "Chromium Tab"
> for
> > > non-official build?
> > > I think one reason that UX people includes "Chrome" here is to make this
tab
> > > button similar size with the other two. 
> > > Otherwise, two big buttons with one small button looks bad.
> > 
> > Yes, use ids_short_product_name or similar; Chromium shouldn't say "Chrome".

Use, and optionally update, the standard tabbed pane appearance. We do our best
to avoid one-off appearance changes of standard controls for specific UI
surfaces, favoring consistency across Chrome over disjoint designs. If all
tabbed panes should use a new font and layout of the text in the tabs, then go
ahead and change that (there aren't many users anyway); otherwise, stick with
the standard design. If it's *really* necessary, we can add some support for
appearance customizations, but it shouldn't be necessary. Post screenshots on
the bug as needed to discuss deviations between the mocks and standard control
appearances. Keep in mind that the standard appearance might be changing soon
for MD anyway.

[qiangchen, 2016-04-29 20:55:03]

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