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
Committed: https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda
Cr-Commit-Position: refs/heads/master@{#392138}

[qiangchen, 2016-04-29 20:54:47]

Description was changed from

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


Topology


Topology

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

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-29 20:54:47]

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

[qiangchen, 2016-04-29 22:21:47]

Patchset #1 (id:1) has been deleted

[qiangchen, 2016-04-29 22:21:55]

Patchset #1 (id:20001) has been deleted

[qiangchen, 2016-05-02 18:19:28]

Patchset #2 (id:60001) has been deleted

[qiangchen, 2016-05-02 18:25:20]

qiangchen@chromium.org changed reviewers:
+ isheriff@chromium.org, rockot@chromium.org

[qiangchen, 2016-05-02 18:25:20]

Hi, all

    Can you take a look at this CL, we are doing the new UI for desktop capture
picker window.

    For rockot@, can you mainly focus on switches.h and switches.cc, as we add a
flag to control new UI or old UI.

    For isheriff@, I add a value in histogrmas.xml for the flag. Quesion: is
there a problem if we do not add the value in histograms.xml? (Actually, we do
not need to track UMA for that flag)

    For msw@, you are the major reviewer for UI change.

Thanks.

[Ken Rockot(use gerrit already), 2016-05-02 19:01:54]

https://codereview.chromium.org/1932413002/diff/80001/extensions/common/switc...
File extensions/common/switches.h (right):

https://codereview.chromium.org/1932413002/diff/80001/extensions/common/switc...
extensions/common/switches.h:17: extern const char kDisableDesktopPickerNewUI[];
Please make this name more specific. There are many pickers in Chrome.
kDisableDesktopCatpurePickerNewUI?

[qiangchen, 2016-05-02 20:06:42]

qiangchen@chromium.org changed reviewers:
+ isherman@chromium.org
- isheriff@chromium.org

[qiangchen, 2016-05-02 20:19:30]

Patchset #3 (id:100001) has been deleted

[qiangchen, 2016-05-02 21:01:38]

Fixed the flag name.

isherman@, can you take a look at the histograms.xml change.
I just add a value for the flag. 
Quesion: If we do not care about the UMA statistics of the flag, can we not add
the entry in histograms.xml?

https://codereview.chromium.org/1932413002/diff/80001/extensions/common/switc...
File extensions/common/switches.h (right):

https://codereview.chromium.org/1932413002/diff/80001/extensions/common/switc...
extensions/common/switches.h:17: extern const char kDisableDesktopPickerNewUI[];
On 2016/05/02 19:01:53, Ken Rockot wrote:
> Please make this name more specific. There are many pickers in Chrome.
> kDisableDesktopCatpurePickerNewUI?

Done.

[Ken Rockot(use gerrit already), 2016-05-02 21:04:04]

//extensions lgtm

[msw, 2016-05-02 23:49:59]

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:13616: +        Your Entire Window
"Your Entire Window" means the screen? Is that right?

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/google_chro...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/google_chro...
chrome/app/google_chrome_strings.grd:1130: Chromium Tab
Shouldn't this be "Chrome Tab"? Please just add a single shared string with a
"<ph name="PRODUCT_NAME">$1<ex>Chrome</ex></ph>" field for IDS_PRODUCT_NAME,
like eg. IDS_SYNC_UPGRADE_CLIENT.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
(right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:38:
#include "ui/views/controls/button/label_button.h"
Remove this.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:39:
#include "ui/views/controls/button/label_button_border.h"
Remove this.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:258: int x
= views::kPanelHorizMargin * 2;
Why add a new margin? How did you come up with this value?

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:274:
SetSize(gfx::Size(kTotalListWidth + views::kPanelHorizMargin * 2, y));
Shouldn't this be *4? I'm not sure it makes sense to call SetSize in Layout()
anyway... The size should be set by the parent view (respecting this view's
preferred size), which then triggers layout.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:339: if
(index == 0 && source.id.type == DesktopMediaID::TYPE_SCREEN) {
nit: remove curlies

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:340:
source_view->OnFocus();
This should probably call RequestFocus instead (ditto below?).

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:422:
scroll_views_.push_back(views::ScrollView::CreateScrollViewWithBorder());
|scroll_views_| isn't accessed beyond this ctor's init; make it function-local.
Or, maybe, avoid the vector altogether, init each one within its pane's block.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:480: //
Focus on the first non-null media_list
nit: add a trailing period.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:481:
SwitchSourceType(0);
Should this call TabSelectedAt instead (to also UpdateDialogButtons?), if so,
consider inlining SwitchSourceType into TabSelectedAt.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:519: for
(auto& list_view : list_views_) {
nit: remove curlies

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:532: //
Set whether the checkbox is visible based on the source type
nit: add a trailing period.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:586: void
DesktopMediaPickerDialogView::Layout() {
Can you use BoxLayout instead of doing manual layout/sizing?

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:587: //
DialogDelegate uses the bubble style frame.
What is the purpose of this comment? All dialogs use a bubble frame.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:591:
rect.Inset(views::kPanelHorizMargin * 2, views::kPanelVertMargin);
Perhaps giving the view an empty border (or overriding GetInsets) would simplify
your code?

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:597: //
Put on Label
These comments don't have any value; remove them (or s/Put on/Add the/, and add
trailing periods)

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:604: int
checkbox_height = 0;
You should make this checkbox the dialog's 'extra' view; then it'll be placed
left of the dialog buttons, and you won't need to lay it out under the tabbed
pane. see DialogDelegate::CreateExtraView().

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:605: if
(audio_share_checkbox_) {
Should this also check for the checkbox visibility?

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:630: if
(button == ui::DIALOG_BUTTON_OK) {
nit: remove curlies

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h
(right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:17: class
LabelButton;
Remove this.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:139: //
Called by clicking pane tab buttons.
Typically subclass function overrides are simply annotated with the name of the
subclass, like below "// views::View overrides.", change this to "//
views::TabbedPaneListener overrides."

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:152: bool
ShouldDefaultButtonBeBlue() const override;
Order just before GetDialogButtonLabel(), to match the subclass decl order.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:166: void
SwitchSourceType(int index);
nit: add a blank line after

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc
(right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:21:
#include "ui/views/controls/button/label_button.h"
Remove this.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:30:
namespace views {
nit: add a blank line below.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:55:
picker_views_->Show(NULL, test_helper_.GetContext(), NULL, app_name,
nit: nullptr here and elsewhere

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:137:
// Default choose the first screen for screen share.
nit: "By default, the first screen should be selected."

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:138:
if (source_type == DesktopMediaID::TYPE_SCREEN) {
replace this block with EXPECT_EQ(source_type == DesktopMediaID::TYPE_SCREEN,
source_view_0->is_selected());

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:212:
EXPECT_EQ(NULL, GetPickerDialogView()->GetMediaSourceViewForTesting(0));
nit: EXPECT_FALSE here and line 222?

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:243:
// Default choose the first screen for screen share.
Lines 243-249 are redundant with line 137-143, remove these.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:280:
// Verifies that the MediaListView get the initial focus.
nit: gets

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:287:
bool expect_value;
nit: init this to false, remove the #else case below.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:311:
// It is also the real workflow when a source is generated in media_list,
nit: s/It is also/This matches/

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:321:
ui::MouseEvent double_click(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
Instead of double clicking here, just do
GetPickerDialogView()->GetDialogClientView()->AcceptWindow();

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_deprecated.cc (right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated.cc:77: namespace
deprecated {
nit: add a blank line below.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_deprecated.h (right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated.h:19: //
TODO(qiangchen): Remove this file after new UI is done.
nit: and IDS_DESKTOP_MEDIA_PICKER_AUDIO_SHARE_TOOLTIP_NONE

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated.h:20: namespace
deprecated {
nit: add a blank line below.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated.h:205: }  //
namespace deprecated
nit: add blank lines above and below.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (left):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Perhaps we should keep this testing around until the deprecated view is deleted.

[Ilya Sherman, 2016-05-03 01:30:26]

histograms.xml lgtm

[qiangchen, 2016-05-05 22:17:55]

Fixed.

Thank you very much for reviewing.

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:13616: +        Your Entire Window
On 2016/05/02 23:49:57, msw wrote:
> "Your Entire Window" means the screen? Is that right?

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/google_chro...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/google_chro...
chrome/app/google_chrome_strings.grd:1130: Chromium Tab
On 2016/05/02 23:49:57, msw wrote:
> Shouldn't this be "Chrome Tab"? Please just add a single shared string with a
> "<ph name="PRODUCT_NAME">$1<ex>Chrome</ex></ph>" field for IDS_PRODUCT_NAME,
> like eg. IDS_SYNC_UPGRADE_CLIENT.

That way the "git cl upload" thinks it is an error and tells me to do like this.
Maybe we changed the regulation at some time? Theoretically this way is more
translator friendly.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
(right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:38:
#include "ui/views/controls/button/label_button.h"
On 2016/05/02 23:49:57, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:39:
#include "ui/views/controls/button/label_button_border.h"
On 2016/05/02 23:49:58, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:258: int x
= views::kPanelHorizMargin * 2;
On 2016/05/02 23:49:58, msw wrote:
> Why add a new margin? How did you come up with this value?

Changed it back.

My original change makes the tabbed_pane's width to expand the full picker
window regardless of the margin, and thus add a margin in the content of
tabbed_pane.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:274:
SetSize(gfx::Size(kTotalListWidth + views::kPanelHorizMargin * 2, y));
On 2016/05/02 23:49:58, msw wrote:
> Shouldn't this be *4? I'm not sure it makes sense to call SetSize in Layout()
> anyway... The size should be set by the parent view (respecting this view's
> preferred size), which then triggers layout.

You are right, no need to SetSize here.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:339: if
(index == 0 && source.id.type == DesktopMediaID::TYPE_SCREEN) {
On 2016/05/02 23:49:57, msw wrote:
> nit: remove curlies

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:340:
source_view->OnFocus();
On 2016/05/02 23:49:57, msw wrote:
> This should probably call RequestFocus instead (ditto below?).

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:422:
scroll_views_.push_back(views::ScrollView::CreateScrollViewWithBorder());
On 2016/05/02 23:49:57, msw wrote:
> |scroll_views_| isn't accessed beyond this ctor's init; make it
function-local.
> Or, maybe, avoid the vector altogether, init each one within its pane's block.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:480: //
Focus on the first non-null media_list
On 2016/05/02 23:49:58, msw wrote:
> nit: add a trailing period.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:481:
SwitchSourceType(0);
On 2016/05/02 23:49:57, msw wrote:
> Should this call TabSelectedAt instead (to also UpdateDialogButtons?), if so,
> consider inlining SwitchSourceType into TabSelectedAt.

No. This is the constructor function, if we call TabSelectedAt, it will call
UpdateDialogButtons, but at this time GetDialogClientView is not ready, and it
will crash.

No need to call UpdateDialogButtons in the ctor, because the initial draw of UI
will anyway draw it, so no need to send such notification separately.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:519: for
(auto& list_view : list_views_) {
On 2016/05/02 23:49:57, msw wrote:
> nit: remove curlies

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:532: //
Set whether the checkbox is visible based on the source type
On 2016/05/02 23:49:58, msw wrote:
> nit: add a trailing period.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:586: void
DesktopMediaPickerDialogView::Layout() {
On 2016/05/02 23:49:58, msw wrote:
> Can you use BoxLayout instead of doing manual layout/sizing?

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:587: //
DialogDelegate uses the bubble style frame.
On 2016/05/02 23:49:58, msw wrote:
> What is the purpose of this comment? All dialogs use a bubble frame.
N/A
This is copied from old code.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:591:
rect.Inset(views::kPanelHorizMargin * 2, views::kPanelVertMargin);
On 2016/05/02 23:49:57, msw wrote:
> Perhaps giving the view an empty border (or overriding GetInsets) would
simplify
> your code?

N/A

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:597: //
Put on Label
On 2016/05/02 23:49:58, msw wrote:
> These comments don't have any value; remove them (or s/Put on/Add the/, and
add
> trailing periods)

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:604: int
checkbox_height = 0;
On 2016/05/02 23:49:57, msw wrote:
> You should make this checkbox the dialog's 'extra' view; then it'll be placed
> left of the dialog buttons, and you won't need to lay it out under the tabbed
> pane. see DialogDelegate::CreateExtraView().

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:605: if
(audio_share_checkbox_) {
On 2016/05/02 23:49:58, msw wrote:
> Should this also check for the checkbox visibility?

N/A after accepting previous comment.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:630: if
(button == ui::DIALOG_BUTTON_OK) {
On 2016/05/02 23:49:57, msw wrote:
> nit: remove curlies

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h
(right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:17: class
LabelButton;
On 2016/05/02 23:49:58, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:139: //
Called by clicking pane tab buttons.
On 2016/05/02 23:49:58, msw wrote:
> Typically subclass function overrides are simply annotated with the name of
the
> subclass, like below "// views::View overrides.", change this to "//
> views::TabbedPaneListener overrides."

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:152: bool
ShouldDefaultButtonBeBlue() const override;
On 2016/05/02 23:49:58, msw wrote:
> Order just before GetDialogButtonLabel(), to match the subclass decl order.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:166: void
SwitchSourceType(int index);
On 2016/05/02 23:49:58, msw wrote:
> nit: add a blank line after

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc
(right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:21:
#include "ui/views/controls/button/label_button.h"
On 2016/05/02 23:49:58, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:30:
namespace views {
On 2016/05/02 23:49:59, msw wrote:
> nit: add a blank line below.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:55:
picker_views_->Show(NULL, test_helper_.GetContext(), NULL, app_name,
On 2016/05/02 23:49:58, msw wrote:
> nit: nullptr here and elsewhere

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:137:
// Default choose the first screen for screen share.
On 2016/05/02 23:49:59, msw wrote:
> nit: "By default, the first screen should be selected."

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:138:
if (source_type == DesktopMediaID::TYPE_SCREEN) {
On 2016/05/02 23:49:58, msw wrote:
> replace this block with EXPECT_EQ(source_type == DesktopMediaID::TYPE_SCREEN,
> source_view_0->is_selected());

Done.

Good idea.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:212:
EXPECT_EQ(NULL, GetPickerDialogView()->GetMediaSourceViewForTesting(0));
On 2016/05/02 23:49:59, msw wrote:
> nit: EXPECT_FALSE here and line 222?

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:243:
// Default choose the first screen for screen share.
On 2016/05/02 23:49:58, msw wrote:
> Lines 243-249 are redundant with line 137-143, remove these.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:280:
// Verifies that the MediaListView get the initial focus.
On 2016/05/02 23:49:58, msw wrote:
> nit: gets

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:287:
bool expect_value;
On 2016/05/02 23:49:58, msw wrote:
> nit: init this to false, remove the #else case below.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:311:
// It is also the real workflow when a source is generated in media_list,
On 2016/05/02 23:49:59, msw wrote:
> nit: s/It is also/This matches/

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:321:
ui::MouseEvent double_click(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
On 2016/05/02 23:49:58, msw wrote:
> Instead of double clicking here, just do
> GetPickerDialogView()->GetDialogClientView()->AcceptWindow();

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_deprecated.cc (right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated.cc:77: namespace
deprecated {
On 2016/05/02 23:49:59, msw wrote:
> nit: add a blank line below.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_deprecated.h (right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated.h:19: //
TODO(qiangchen): Remove this file after new UI is done.
On 2016/05/02 23:49:59, msw wrote:
> nit: and IDS_DESKTOP_MEDIA_PICKER_AUDIO_SHARE_TOOLTIP_NONE

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated.h:20: namespace
deprecated {
On 2016/05/02 23:49:59, msw wrote:
> nit: add a blank line below.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated.h:205: }  //
namespace deprecated
On 2016/05/02 23:49:59, msw wrote:
> nit: add blank lines above and below.

Done.

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (left):

https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2016/05/02 23:49:57, msw wrote:
> Perhaps we should keep this testing around until the deprecated view is
deleted.

Done.

[msw, 2016-05-05 22:59:49]

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/google_chro...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/google_chro...
chrome/app/google_chrome_strings.grd:1130: Chromium Tab
On 2016/05/05 22:17:53, qiangchenC wrote:
> On 2016/05/02 23:49:57, msw wrote:
> > Shouldn't this be "Chrome Tab"? Please just add a single shared string with
a
> > "<ph name="PRODUCT_NAME">$1<ex>Chrome</ex></ph>" field for IDS_PRODUCT_NAME,
> > like eg. IDS_SYNC_UPGRADE_CLIENT.
> 
> That way the "git cl upload" thinks it is an error and tells me to do like
this.
> Maybe we changed the regulation at some time? Theoretically this way is more
> translator friendly.

That doesn't make sense to me. What is the git cl upload error?

https://codereview.chromium.org/1932413002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
(right):

https://codereview.chromium.org/1932413002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:482:
AddChildView(pane_);
nit: maybe DCHECK that the pane has at least one tab?

https://codereview.chromium.org/1932413002/diff/140001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc
(right):

https://codereview.chromium.org/1932413002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:137:
// By default, the first screen is selected, but not for other sharing type.
nit: types

[qiangchen, 2016-05-05 23:36:27]

fixed and replied

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/google_chro...
File chrome/app/google_chrome_strings.grd (right):

https://codereview.chromium.org/1932413002/diff/120001/chrome/app/google_chro...
chrome/app/google_chrome_strings.grd:1130: Chromium Tab
On 2016/05/05 22:59:49, msw wrote:
> On 2016/05/05 22:17:53, qiangchenC wrote:
> > On 2016/05/02 23:49:57, msw wrote:
> > > Shouldn't this be "Chrome Tab"? Please just add a single shared string
with
> a
> > > "<ph name="PRODUCT_NAME">$1<ex>Chrome</ex></ph>" field for
IDS_PRODUCT_NAME,
> > > like eg. IDS_SYNC_UPGRADE_CLIENT.
> > 
> > That way the "git cl upload" thinks it is an error and tells me to do like
> this.
> > Maybe we changed the regulation at some time? Theoretically this way is more
> > translator friendly.
> 
> That doesn't make sense to me. What is the git cl upload error?

Don't use PRODUCT_NAME placeholders in string resources. Instead, add separate
strings to google_chrome_strings.grd and chromium_strings.grd. See
http://goo.gl/6614MQ for more information.Problems with this check? Contact
dubroy@chromium.org.

The reason is for translation.

https://codereview.chromium.org/1932413002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
(right):

https://codereview.chromium.org/1932413002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:482:
AddChildView(pane_);
On 2016/05/05 22:59:49, msw wrote:
> nit: maybe DCHECK that the pane has at least one tab?

Done.

https://codereview.chromium.org/1932413002/diff/140001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc
(right):

https://codereview.chromium.org/1932413002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:137:
// By default, the first screen is selected, but not for other sharing type.
On 2016/05/05 22:59:49, msw wrote:
> nit: types

Done.

[msw, 2016-05-05 23:56:10]

lgtm with nits

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
(right):

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:412:
app_name_(app_name),
nit: |app_name_| is no longer used, remove it and the ctor parameter.

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:482:
DCHECK(source_types_.size() > 0);
nit: DCHECK_GT

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc
(right):

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:12:
#include "base/strings/utf_string_conversions.h"
nit: remove with |app_name|

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:52:
base::string16 app_name = base::ASCIIToUTF16("foo");
nit: remove with ctor param

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_deprecated_unittest.cc
(right):

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated_unittest.cc:11:
#include "base/strings/utf_string_conversions.h"
nit: remove with |app_name|

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated_unittest.cc:39:
base::string16 app_name = base::ASCIIToUTF16("foo");
nit: remove with ctor param

https://codereview.chromium.org/1932413002/diff/160001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1932413002/diff/160001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:684:
'browser/ui/views/desktop_capture/desktop_media_picker_views.cc',
nit/q: why create a desktop_capture subdir? Seems unnecessary.

[qiangchen, 2016-05-06 18:38:36]

app_name is still in use in the label description, but app_name_ can be remove.

To make a folder is that I anticipate the expansion of the classes in
desktop_media_picker_views.cc in the following CL and have the plan to split
them into separate files.

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc
(right):

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:412:
app_name_(app_name),
On 2016/05/05 23:56:10, msw wrote:
> nit: |app_name_| is no longer used, remove it and the ctor parameter.

app_name_ is removed, but the ctor parameter is used.

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:482:
DCHECK(source_types_.size() > 0);
On 2016/05/05 23:56:10, msw wrote:
> nit: DCHECK_GT

Done.

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc
(right):

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:12:
#include "base/strings/utf_string_conversions.h"
On 2016/05/05 23:56:10, msw wrote:
> nit: remove with |app_name|

Nope. the parameter is still in use.

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc:52:
base::string16 app_name = base::ASCIIToUTF16("foo");
On 2016/05/05 23:56:10, msw wrote:
> nit: remove with ctor param

Ditto.

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_deprecated_unittest.cc
(right):

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated_unittest.cc:11:
#include "base/strings/utf_string_conversions.h"
On 2016/05/05 23:56:10, msw wrote:
> nit: remove with |app_name|

Ditto

https://codereview.chromium.org/1932413002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_deprecated_unittest.cc:39:
base::string16 app_name = base::ASCIIToUTF16("foo");
On 2016/05/05 23:56:10, msw wrote:
> nit: remove with ctor param

Ditto.

https://codereview.chromium.org/1932413002/diff/160001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1932413002/diff/160001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:684:
'browser/ui/views/desktop_capture/desktop_media_picker_views.cc',
On 2016/05/05 23:56:10, msw wrote:
> nit/q: why create a desktop_capture subdir? Seems unnecessary.

In next CL, we are going to apply different appearance styles for the
source_view of different types. Thus the code size of SourceView and ListView
will be much larger. And we are going to split those classes into new files.

[qiangchen, 2016-05-06 18:38:39]

app_name is still in use in the label description, but app_name_ can be remove.

To make a folder is that I anticipate the expansion of the classes in
desktop_media_picker_views.cc in the following CL and have the plan to split
them into separate files.

[qiangchen, 2016-05-06 19:18:39]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-05-06 19:18:39]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
isherman@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1932413002/#ps200001
(title: "Nit")

[commit-bot: I haz the power, 2016-05-06 19:18:54]

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

[commit-bot: I haz the power, 2016-05-06 20:29:54]

Description was changed from

==========
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
==========

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
==========

[commit-bot: I haz the power, 2016-05-06 20:29:55]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2016-05-06 20:32:19]

Description was changed from

==========
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
==========

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

Committed: https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda
Cr-Commit-Position: refs/heads/master@{#392138}
==========

[commit-bot: I haz the power, 2016-05-06 20:32:21]

Patchset 7 (id:??) landed as
https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda
Cr-Commit-Position: refs/heads/master@{#392138}

[Nico, 2016-05-07 14:11:06]

This broke DesktopMediaPickerViewsDeprecatedTest on various main waterfall debug
bots (e.g.
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...)

[4208:2052:0506/162201:4424718:FATAL:l10n_util.cc(712)] Check failed:
std::string::npos != pos (4294967295 vs. 4294967295) Didn't find a $1
placeholder in Share your screen
Backtrace:
	base::CancellationFlag::Set [0x0F7520F1+508917]
	base::CancellationFlag::Set [0x0F7AC6AB+879023]
	ui::AreTouchEventsEnabled [0x1424C6B2+469462]
	ui::AreTouchEventsEnabled [0x1424C0F4+467992]
	deprecated::DesktopMediaPickerDialogView::GetWindowTitle [0x06A160F9+41]
	views::TouchSelectionControllerImpl::GetSelectionHandle2Bounds
[0x17A7D01D+1794589]


Please revert. (Reverting through rietveld seems to not work for this change.)

[Nico, 2016-05-07 14:25:47]

Manually created a revert here: https://codereview.chromium.org/1957063002/