Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1990053002:
Desktop Capture Picker New UI: Non Mac Appearance Change (Closed)
|
Created:
4 years, 7 months ago by qiangchen Modified:
4 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, tfarina, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDesktop Capture Picker New UI: Non Mac Appearance Change
In this CL, we changed the detailed appearance of
DesktopMediaSourceView.
Technically, we made a struct and plug in as parameter to
implement different styles.
TBR=sergeyu@chromium.org
BUG=602478
Committed: https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef
Cr-Commit-Position: refs/heads/master@{#396322}
Patch Set 1 #
Total comments: 73
Patch Set 2 : Comments Resolved #
Total comments: 13
Patch Set 3 : Comments Resolved #
Total comments: 26
Patch Set 4 : Nit Fixes #
Total comments: 4
Patch Set 5 : Nit fixes #Messages
Total messages: 29 (12 generated)
Description was changed from ========== Desktop Capture Picker New UI: Non Mac Appearance Change style Dynamic Style Split files Appearance Style BUG= ========== to ========== Desktop Capture Picker New UI: Non Mac Appearance Change In this CL, we changed the detailed appearance of DesktopMediaSourceView. Technically, we made a struct and plug in as parameter to implement different styles. BUG=602478 ==========
qiangchen@chromium.org changed reviewers: + msw@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Can you take a look? The style change of the source view.
msw@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... File chrome/browser/media/tab_desktop_media_list.cc (left): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... chrome/browser/media/tab_desktop_media_list.cc:127: IDS_DESKTOP_MEDIA_PICKER_CHROME_TAB_TITLE, contents->GetTitle()); Remove IDS_DESKTOP_MEDIA_PICKER_CHROME_TAB_TITLE if it's no longer used. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... File chrome/browser/media/tab_desktop_media_list.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... chrome/browser/media/tab_desktop_media_list.cc:41: canvas.clear(bg_color); Just inline canvas.clear(SK_ColorTRANSPARENT), remove bg_color, also remove the comment and blank line above. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... chrome/browser/media/tab_desktop_media_list.cc:47: gfx::Size target_size(result.width(), result.height()); optional nit: remove |target_size|, inline |size| below, and remove the curly braces. (the |target_size| from |result|'s size should be equal to |size|) https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... chrome/browser/media/tab_desktop_media_list.cc:112: const base::string16 title = contents->GetTitle(); optional nit: inline contents->GetTitle() below, remove |title| and comment. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:35: gfx::Size(single_style_.image_position.width() - Shouldn't we use |generic_style_| if |media_list_| has multiple items? https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:70: const int child_num = child_count(); nit: just inline child_count() below and remove child_num. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:71: const DesktopMediaSourceViewStyle* style = optional nit: add a const ref convenience accessor for this: const DesktopMediaSourceViewStyle& DesktopMediaListView::style() { return child_count() > 1 ? generic_style_ : single_style_; } https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:92: View* source_view = child_at(i); optional nit: inline this and remove |source_view| https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:152: child_count() ? &generic_style_ : &single_style_; Shouldn't this be child_count() > 1? Use the accessor. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:161: if ((child_count() - 1) % generic_style_.columns == 0) optional nit: if (child_count() % generic_style_.columns == 1) https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:164: // We need to switch other item's style nit: "Apply multi-item styling when the second source is added." https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:168: 2 * generic_style_.selection_border_thickness, Use |style| instead of |generic_style_| throughout. It'd be even nicer if you could remove the style struct's |selection_border_thickness| member altogether and use the source_view->border().GetInsets() as needed. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:171: for (int i = 0; i < child_count(); i++) Since there are only two items, and the newly added one was just constructed with the correct style, just do the following (like in OnSourceRemoved): static_cast<DesktopMediaSourceView*>(child_at(0))->SetStyle(generic_style_); https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:180: PreferredSizeChanged(); This is generally only needed when we add a row, but I guess the single->multi style for screens also needs this (unless we use the same style for single/multi item, as I suggest). https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:213: // Switch to single style. nit: Apply single-item styling when the second source is removed. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:22: DesktopMediaSourceViewStyle single_style, The only single-style is for the screen picker, and the styles aren't terribly different (title vs. no-title, and minor size difference). I'd argue that the nuanced appearance for a single-screen doesn't warrant this code complexity, and we should only support a generic style, and *maybe* find some way to support dropping the title in the single-screen case. WDYT? https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:78: const DesktopMediaSourceViewStyle kSingleScreenStyle( Hard-coding all these values is a fairly strange approach, though I suppose the mocks giving four different appearances for somewhat similar lists of items is also quite odd. I wonder what this would look like if we had less shared code for the lists and items, but perhaps that's not a good approach either... +Scott for high-level impressions here. Mocks at: https://docs.google.com/presentation/d/1xIgk_xNpa-4yBSR4DIcoDLfBFigu7SJwFWOdI... https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:81: gfx::Rect(20, 220, 320, 40), // label_position Does this work well enough for internationalization (tall fonts for other languages, etc?) https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:107: screen_scroll_view->set_hide_horizontal_scrollbar(true); Does this mean that we're overflowing the available horizontal space? Shouldn't we be able to avoid this by making sure the expected items per row fit in the available width? (ditto in the two cases below) https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:21: nit: remove https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:75: nit: remove https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:79: void DesktopMediaSourceView::SetStyle(DesktopMediaSourceViewStyle style) { nit: make this a simple accessor, defined in the header: void set_style(const DesktopMediaSourceViewStyle& style) { style_ = style; } https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:116: label_->SetFontList(gfx::FontList(font)); nit: label_->SetFontList(label_->font_list().Derive(0, gfx::Font::BOLD)); https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:122: label_->SetFontList(gfx::FontList(font)); nit: label_->SetFontList(label_->font_list().Derive(0, gfx::Font::NORMAL)); https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:129: if (hovered == hovered_) nit: tracking hovered_ to early return here probably isn't necessary. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:136: const SkColor bg_color = 0x44000000; Use kColorId_HoverMenuItemBackgroundColor https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:21: DesktopMediaSourceViewStyle(); Can we remove the default constructor? https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:22: DesktopMediaSourceViewStyle(const DesktopMediaSourceViewStyle& style); Can we use the default copy ctor? "... = default;" https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:36: gfx::Rect label_position; nit: |label_rect| for position *and size* data passed via rect. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:39: gfx::Rect image_position; nit: |image_rect| for position *and size* data passed via rect. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:40: int selection_border_thickness; |selection_border_thickness| is 2 for everything except tabs, where it's 1. I suggest reducing this code complexity by using a const, at the cost of not matching mocks pixel-for-pixel in this case. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:42: int paint_inset_horizontal; Your horizontal and vertical values always match, and this value is actually used to inset the source view's focus rectangle, as far as I can tell... Change these two into values into a single int named |focus_rectangle_inset| or similar. Does focusing a source view automatically cause it to be selected within the list? If so, this could be called |selection_border_inset| or similar, and we might want to consolidate the way we paint the selection border in DesktopMediaSourceView::SetSelected and the focus rect in DesktopMediaSourceView::OnPaint... https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:55: // Update style. nit: Used to update the style when the number of available items changes. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:56: void SetStyle(DesktopMediaSourceViewStyle style); I hope we won't need this function! (if single==generic)
Fixed and replied. Thanks. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... File chrome/browser/media/tab_desktop_media_list.cc (left): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... chrome/browser/media/tab_desktop_media_list.cc:127: IDS_DESKTOP_MEDIA_PICKER_CHROME_TAB_TITLE, contents->GetTitle()); On 2016/05/20 23:26:35, msw wrote: > Remove IDS_DESKTOP_MEDIA_PICKER_CHROME_TAB_TITLE if it's no longer us You are right. I thought Mac side still used it, but nope, this code is platform independent. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... File chrome/browser/media/tab_desktop_media_list.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... chrome/browser/media/tab_desktop_media_list.cc:41: canvas.clear(bg_color); On 2016/05/20 23:26:36, msw wrote: > Just inline canvas.clear(SK_ColorTRANSPARENT), remove bg_color, also remove the > comment and blank line above. Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... chrome/browser/media/tab_desktop_media_list.cc:47: gfx::Size target_size(result.width(), result.height()); On 2016/05/20 23:26:36, msw wrote: > optional nit: remove |target_size|, inline |size| below, and remove the curly > braces. (the |target_size| from |result|'s size should be equal to |size|) Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/media/ta... chrome/browser/media/tab_desktop_media_list.cc:112: const base::string16 title = contents->GetTitle(); On 2016/05/20 23:26:36, msw wrote: > optional nit: inline contents->GetTitle() below, remove |title| and comment. Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:35: gfx::Size(single_style_.image_position.width() - On 2016/05/20 23:26:36, msw wrote: > Shouldn't we use |generic_style_| if |media_list_| has multiple items? In the constructor, we have no idea how many items "will be" in the media_list. They are later inserted though calling OnSourceAdded, etc. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:70: const int child_num = child_count(); On 2016/05/20 23:26:36, msw wrote: > nit: just inline child_count() below and remove child_num. N/A https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:71: const DesktopMediaSourceViewStyle* style = On 2016/05/20 23:26:36, msw wrote: > optional nit: add a const ref convenience accessor for this: const > DesktopMediaSourceViewStyle& DesktopMediaListView::style() { return > child_count() > 1 ? generic_style_ : single_style_; } Done. I added a pointer field active_style_ and SwitchStyle function to set its value, rather than the function you suggested. This way I can avoid calling child_count() repeatedly, and no worry when calling in OnSourceRemoved and OnSourceAdded, as in these two functions we should distinguish old child_count with new child_count. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:92: View* source_view = child_at(i); On 2016/05/20 23:26:36, msw wrote: > optional nit: inline this and remove |source_view| Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:152: child_count() ? &generic_style_ : &single_style_; On 2016/05/20 23:26:36, msw wrote: > Shouldn't this be child_count() > 1? Use the accessor. This clause is executed before the source really added to UI, so child_count()==0 will result in child_count()==1 afterward. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:161: if ((child_count() - 1) % generic_style_.columns == 0) On 2016/05/20 23:26:36, msw wrote: > optional nit: if (child_count() % generic_style_.columns == 1) Nope. When style_.columns==1 that way will not work. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:164: // We need to switch other item's style On 2016/05/20 23:26:36, msw wrote: > nit: "Apply multi-item styling when the second source is added." N/A now https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:168: 2 * generic_style_.selection_border_thickness, On 2016/05/20 23:26:36, msw wrote: > Use |style| instead of |generic_style_| throughout. It'd be even nicer if you > could remove the style struct's |selection_border_thickness| member altogether > and use the source_view->border().GetInsets() as needed. N/A For using GetInsets: we can only set one size for the whole list, however the border.GetInsets will only work for the selected source view. So it is easier to just read the struct's parameter P.S. (The SetThumbnailSize actually sets the capture size, it is used to optimize performance, e.g. no need to capture the full HD picture for just a thumbnail preview.) https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:171: for (int i = 0; i < child_count(); i++) On 2016/05/20 23:26:36, msw wrote: > Since there are only two items, and the newly added one was just constructed > with the correct style, just do the following (like in OnSourceRemoved): > static_cast<DesktopMediaSourceView*>(child_at(0))->SetStyle(generic_style_); N/A now. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:180: PreferredSizeChanged(); On 2016/05/20 23:26:36, msw wrote: > This is generally only needed when we add a row, but I guess the single->multi > style for screens also needs this (unless we use the same style for single/multi > item, as I suggest). It is necessary regardless of the row change. We need this to trigger Layout(), so that the newly added source view is visible. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:213: // Switch to single style. On 2016/05/20 23:26:36, msw wrote: > nit: Apply single-item styling when the second source is removed. Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:22: DesktopMediaSourceViewStyle single_style, On 2016/05/20 23:26:36, msw wrote: > The only single-style is for the screen picker, and the styles aren't terribly > different (title vs. no-title, and minor size difference). I'd argue that the > nuanced appearance for a single-screen doesn't warrant this code complexity, and > we should only support a generic style, and *maybe* find some way to support > dropping the title in the single-screen case. WDYT? Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:78: const DesktopMediaSourceViewStyle kSingleScreenStyle( On 2016/05/20 23:26:36, msw wrote: > Hard-coding all these values is a fairly strange approach, though I suppose the > mocks giving four different appearances for somewhat similar lists of items is > also quite odd. I wonder what this would look like if we had less shared code > for the lists and items, but perhaps that's not a good approach either... +Scott > for high-level impressions here. Mocks at: > https://docs.google.com/presentation/d/1xIgk_xNpa-4yBSR4DIcoDLfBFigu7SJwFWOdI... My understanding of the different styles is: For tab, it is infeasible to get a clear preview, and we only have an icon, so we use the "list" style. For window, we do not change anything. For screen, in general, the user will only have very few number of screens, and thus displaying it/them in larger size looks better. However different those appearance looks, the underlying UI event handling mechanism is identical, and thus I think it is a good approach to for different styles to share code. Introducing the style struct parameter actually makes the UI easier to adjust. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:81: gfx::Rect(20, 220, 320, 40), // label_position On 2016/05/20 23:26:37, msw wrote: > Does this work well enough for internationalization (tall fonts for other > languages, etc?) It should work, as I use the same height, and not shrink the width. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:107: screen_scroll_view->set_hide_horizontal_scrollbar(true); On 2016/05/20 23:26:36, msw wrote: > Does this mean that we're overflowing the available horizontal space? Shouldn't > we be able to avoid this by making sure the expected items per row fit in the > available width? (ditto in the two cases below) Actually the sizes are guaranteed not to overflow. But without this clause, we are going to see the horizontal scrollbar. Found the reason, it seems DesktopMediaListView::GetPreferredSize()'s width does not affect scroll_view's inner content size. Then we need to call SetSize in DesktopMediaListView::Layout(). We still need to hide horizontal bar for tab list, as we want the hover shade to be the full row. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h:21: On 2016/05/20 23:26:37, msw wrote: > nit: remove Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:75: On 2016/05/20 23:26:37, msw wrote: > nit: remove Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:79: void DesktopMediaSourceView::SetStyle(DesktopMediaSourceViewStyle style) { On 2016/05/20 23:26:37, msw wrote: > nit: make this a simple accessor, defined in the header: > void set_style(const DesktopMediaSourceViewStyle& style) { style_ = style; } Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:116: label_->SetFontList(gfx::FontList(font)); On 2016/05/20 23:26:37, msw wrote: > nit: label_->SetFontList(label_->font_list().Derive(0, gfx::Font::BOLD)); Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:122: label_->SetFontList(gfx::FontList(font)); On 2016/05/20 23:26:37, msw wrote: > nit: label_->SetFontList(label_->font_list().Derive(0, gfx::Font::NORMAL)); Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:129: if (hovered == hovered_) On 2016/05/20 23:26:37, msw wrote: > nit: tracking hovered_ to early return here probably isn't necessary. Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:136: const SkColor bg_color = 0x44000000; On 2016/05/20 23:26:37, msw wrote: > Use kColorId_HoverMenuItemBackgroundColor Done. But I found HoverMenuButtonColor looks better. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:21: DesktopMediaSourceViewStyle(); On 2016/05/20 23:26:37, msw wrote: > Can we remove the default constructor? Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:22: DesktopMediaSourceViewStyle(const DesktopMediaSourceViewStyle& style); On 2016/05/20 23:26:37, msw wrote: > Can we use the default copy ctor? "... = default;" Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:36: gfx::Rect label_position; On 2016/05/20 23:26:37, msw wrote: > nit: |label_rect| for position *and size* data passed via rect. Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:39: gfx::Rect image_position; On 2016/05/20 23:26:37, msw wrote: > nit: |image_rect| for position *and size* data passed via rect. Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:40: int selection_border_thickness; On 2016/05/20 23:26:37, msw wrote: > |selection_border_thickness| is 2 for everything except tabs, where it's 1. I > suggest reducing this code complexity by using a const, at the cost of not > matching mocks pixel-for-pixel in this case. I think it is better to make it a variable. Now I've changed the thickness for single screen to 4. As a too-thick border on a small image would look ugly, and a too-thin border on a large image would look less noticable. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:42: int paint_inset_horizontal; On 2016/05/20 23:26:37, msw wrote: > Your horizontal and vertical values always match, and this value is actually > used to inset the source view's focus rectangle, as far as I can tell... Change > these two into values into a single int named |focus_rectangle_inset| or > similar. Does focusing a source view automatically cause it to be selected > within the list? If so, this could be called |selection_border_inset| or > similar, and we might want to consolidate the way we paint the selection border > in DesktopMediaSourceView::SetSelected and the focus rect in > DesktopMediaSourceView::OnPaint... Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:55: // Update style. On 2016/05/20 23:26:37, msw wrote: > nit: Used to update the style when the number of available items changes. Done. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:56: void SetStyle(DesktopMediaSourceViewStyle style); On 2016/05/20 23:26:37, msw wrote: > I hope we won't need this function! (if single==generic) N/A as that is controlled on the list level. Now the current code is that if single==generic, this function will not be called.
https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:22: DesktopMediaSourceViewStyle single_style, On 2016/05/24 00:03:37, qiangchenC wrote: > On 2016/05/20 23:26:36, msw wrote: > > The only single-style is for the screen picker, and the styles aren't terribly > > different (title vs. no-title, and minor size difference). I'd argue that the > > nuanced appearance for a single-screen doesn't warrant this code complexity, > and > > we should only support a generic style, and *maybe* find some way to support > > dropping the title in the single-screen case. WDYT? > > Done. I meant that we should remove |single_style| altogether. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:107: screen_scroll_view->set_hide_horizontal_scrollbar(true); On 2016/05/24 00:03:37, qiangchenC wrote: > On 2016/05/20 23:26:36, msw wrote: > > Does this mean that we're overflowing the available horizontal space? > Shouldn't > > we be able to avoid this by making sure the expected items per row fit in the > > available width? (ditto in the two cases below) > > Actually the sizes are guaranteed not to overflow. But without this clause, we > are going to see the horizontal scrollbar. > > Found the reason, it seems DesktopMediaListView::GetPreferredSize()'s width does > not affect scroll_view's inner content size. > > Then we need to call SetSize in DesktopMediaListView::Layout(). > > We still need to hide horizontal bar for tab list, as we want the hover shade to > be the full row. I mentioned that calling SetSize in Layout seems wrong in an earlier review: https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view... Perhaps it's necessary, as you suggest, because of a bug in ScrollView? What size does the view get if you skip calling SetSize in Layout? I'd rather have this call |set_hide_horizontal_scrollbar| here than have Layout call SetSize. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:136: const SkColor bg_color = 0x44000000; On 2016/05/24 00:03:37, qiangchenC wrote: > On 2016/05/20 23:26:37, msw wrote: > > Use kColorId_HoverMenuItemBackgroundColor > > Done. > > But I found HoverMenuButtonColor looks better. Please post pictures of each, it seems odd to mix menu and button styles here. https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:163: if (use_single_style_ && child_count() == 1) { nit: remove curly braces (hopefully this can just go away) https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h (right): https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:61: void SwitchStyle(DesktopMediaSourceViewStyle* style); nit: SetStyle (but I hope we can just have one style...) https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:63: static DesktopMediaSourceViewStyle kSingleScreenStyle; Remove this, it's unused. https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:70: bool use_single_style_; This is not what I meant for using a single style... I meant that the list shouldn't support a separate style struct for when there is only one item in the list. It should have a consistent style for a single item and multiple items. Remove |single_style_|, rename |generic_style_| to |sytle_|, remove |use_single_style_|, remove |active_style_|, and remove the code that supports distinct styles for a single item versus multiple items. If we really need distinct styles; remove |use_single_style_| and replace it with |active_style_ == &single_style_|... https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:126: list_views_.push_back(new DesktopMediaListView(this, std::move(window_list), nit: format like lines 152/153? https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:160: // Thus we make the DesktopMediaSourceView to horizontally overflow. Can you avoid overflow by just setting the width of the contents to exactly the available width inside the scroll view? Shouldn't that show the hover color for the full width without overflow? https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:20: const DesktopMediaSourceViewStyle& style) = default; nit: I think you can do this in the header instead.
Fixed most. Let's first pend whether apply different style for single and multiple screens. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:107: screen_scroll_view->set_hide_horizontal_scrollbar(true); On 2016/05/24 18:54:25, msw wrote: > On 2016/05/24 00:03:37, qiangchenC wrote: > > On 2016/05/20 23:26:36, msw wrote: > > > Does this mean that we're overflowing the available horizontal space? > > Shouldn't > > > we be able to avoid this by making sure the expected items per row fit in > the > > > available width? (ditto in the two cases below) > > > > Actually the sizes are guaranteed not to overflow. But without this clause, we > > are going to see the horizontal scrollbar. > > > > Found the reason, it seems DesktopMediaListView::GetPreferredSize()'s width > does > > not affect scroll_view's inner content size. > > > > Then we need to call SetSize in DesktopMediaListView::Layout(). > > > > We still need to hide horizontal bar for tab list, as we want the hover shade > to > > be the full row. > > I mentioned that calling SetSize in Layout seems wrong in an earlier review: > https://codereview.chromium.org/1932413002/diff/120001/chrome/browser/ui/view... > Perhaps it's necessary, as you suggest, because of a bug in ScrollView? What > size does the view get if you skip calling SetSize in Layout? I'd rather have > this call |set_hide_horizontal_scrollbar| here than have Layout call SetSize. Without set_hide_horizontal_scrollbar here and SetSize() in Layout(), we will see the horizontal bar, although the movement range is very small. It is totally unnecessary to have it, as we designed all the sizes and guaranteed no overflow. I guess it might be a bug in ScrollView. As in my investigation, the height of Preferred Size will determine whether we have the vertical bar or not, but the width of Preferred Size does nothing. https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:136: const SkColor bg_color = 0x44000000; On 2016/05/24 18:54:25, msw wrote: > On 2016/05/24 00:03:37, qiangchenC wrote: > > On 2016/05/20 23:26:37, msw wrote: > > > Use kColorId_HoverMenuItemBackgroundColor > > > > Done. > > > > But I found HoverMenuButtonColor looks better. > > Please post pictures of each, it seems odd to mix menu and button styles here. I'll set color for the scroll_view's background to menubackground, which will make things consistent. https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (right): https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:163: if (use_single_style_ && child_count() == 1) { On 2016/05/24 18:54:25, msw wrote: > nit: remove curly braces (hopefully this can just go away) Done. https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h (right): https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:61: void SwitchStyle(DesktopMediaSourceViewStyle* style); On 2016/05/24 18:54:25, msw wrote: > nit: SetStyle (but I hope we can just have one style...) Done. https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:63: static DesktopMediaSourceViewStyle kSingleScreenStyle; On 2016/05/24 18:54:25, msw wrote: > Remove this, it's unused. Done. https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:126: list_views_.push_back(new DesktopMediaListView(this, std::move(window_list), On 2016/05/24 18:54:25, msw wrote: > nit: format like lines 152/153? Done. https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:160: // Thus we make the DesktopMediaSourceView to horizontally overflow. On 2016/05/24 18:54:25, msw wrote: > Can you avoid overflow by just setting the width of the contents to exactly the > available width inside the scroll view? Shouldn't that show the hover color for > the full width without overflow? Dilemma: The width will depend on whether we have the vertical scroll bar. N/A now if we stick to using set_horizontal_scrollbar() https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:20: const DesktopMediaSourceViewStyle& style) = default; On 2016/05/24 18:54:25, msw wrote: > nit: I think you can do this in the header instead. No. It incurs compile warning, as this struct contains non-elemental fields.
Okay, we can support single & generic styling for screens. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (left): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:29: gfx::Size(DesktopMediaPickerDialogView::kThumbnailWidth, Remove all the now unused constants in desktop_media_picker_views.h https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h (right): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:20: DesktopMediaListView(DesktopMediaPickerDialogView* parent, nit: remove this ctor, passing the same style for single and generic is okay. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:68: bool use_single_style_; Remove this, just apply the single/generic style regardless... Or maybe skip the SetStyle call if |single_style_ == generic_style_|? https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:82: gfx::Rect(0, 0, 0, 0), // label_rect nit: use default rect ctor https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:107: nit: remove blank line https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:108: const SkColor bg_color = GetNativeTheme()->GetSystemColor( nit define a single shared |bg_color| above these code blocks. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:139: nit: remove blank line https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:172: nit: remove blank line https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:88: ui::NativeTheme::kColorId_ButtonHighlightColor); Use consistent colors (menu item vs button). Use kColorId_FocusedMenuItemBackgroundColor (like in Layout) or similar? https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:122: if (selected_) { Remove this block; SetSelected already applies an |image_view_| border. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:128: nit: remove blank line https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h (right): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:97: bool hovered_; Remove |hovered_| now that it's unused.
fix and replied https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc (left): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc:29: gfx::Size(DesktopMediaPickerDialogView::kThumbnailWidth, On 2016/05/25 17:22:12, msw wrote: > Remove all the now unused constants in desktop_media_picker_views.h Done. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h (right): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:20: DesktopMediaListView(DesktopMediaPickerDialogView* parent, On 2016/05/25 17:22:13, msw wrote: > nit: remove this ctor, passing the same style for single and generic is okay. Done. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h:68: bool use_single_style_; On 2016/05/25 17:22:12, msw wrote: > Remove this, just apply the single/generic style regardless... > Or maybe skip the SetStyle call if |single_style_ == generic_style_|? Done. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:82: gfx::Rect(0, 0, 0, 0), // label_rect On 2016/05/25 17:22:13, msw wrote: > nit: use default rect ctor Done. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:107: On 2016/05/25 17:22:13, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:108: const SkColor bg_color = GetNativeTheme()->GetSystemColor( On 2016/05/25 17:22:13, msw wrote: > nit define a single shared |bg_color| above these code blocks. Done. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:139: On 2016/05/25 17:22:13, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc:172: On 2016/05/25 17:22:13, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:88: ui::NativeTheme::kColorId_ButtonHighlightColor); On 2016/05/25 17:22:13, msw wrote: > Use consistent colors (menu item vs button). Use > kColorId_FocusedMenuItemBackgroundColor (like in Layout) or similar? Done. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:122: if (selected_) { On 2016/05/25 17:22:13, msw wrote: > Remove this block; SetSelected already applies an |image_view_| border. We need this. When the single/multiple screen style changes, SetSelected() will not be called, and the thickness of the border will follow old style. Or do you think it is better to move this part of code in SetStyle()? https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:128: On 2016/05/25 17:22:13, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h (right): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h:97: bool hovered_; On 2016/05/25 17:22:13, msw wrote: > Remove |hovered_| now that it's unused. Done.
mostly lg, just a couple comments. https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:122: if (selected_) { On 2016/05/25 20:27:30, qiangchenC wrote: > On 2016/05/25 17:22:13, msw wrote: > > Remove this block; SetSelected already applies an |image_view_| border. > > We need this. When the single/multiple screen style changes, SetSelected() will > not be called, and the thickness of the border will follow old style. > > Or do you think it is better to move this part of code in SetStyle()? Yeah, change DesktopMediaSourceView::set_style into a full-fledged function DesktopMediaSourceView::SetStyle. Make SetStyle do everything that is currently in Layout(), and remove the Layout override. https://codereview.chromium.org/1990053002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:134: return NULL; nit: nullptr here and elsewhere https://codereview.chromium.org/1990053002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:153: if (HasFocus()) { I'd be nice to avoid overriding paint and change the border in OnFocus/OnBlur, using some new border that also paints a focus rect, perhaps leveraging DashedFocusPainter, but that's not easy/straightforward right now. No action required, just a thought...
fixed https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:122: if (selected_) { On 2016/05/26 20:50:33, msw wrote: > On 2016/05/25 20:27:30, qiangchenC wrote: > > On 2016/05/25 17:22:13, msw wrote: > > > Remove this block; SetSelected already applies an |image_view_| border. > > > > We need this. When the single/multiple screen style changes, SetSelected() > will > > not be called, and the thickness of the border will follow old style. > > > > Or do you think it is better to move this part of code in SetStyle()? > > Yeah, change DesktopMediaSourceView::set_style into a full-fledged function > DesktopMediaSourceView::SetStyle. Make SetStyle do everything that is currently > in Layout(), and remove the Layout override. Done. https://codereview.chromium.org/1990053002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc (right): https://codereview.chromium.org/1990053002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:134: return NULL; On 2016/05/26 20:50:33, msw wrote: > nit: nullptr here and elsewhere Done. https://codereview.chromium.org/1990053002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc:153: if (HasFocus()) { On 2016/05/26 20:50:33, msw wrote: > I'd be nice to avoid overriding paint and change the border in OnFocus/OnBlur, > using some new border that also paints a focus rect, perhaps leveraging > DashedFocusPainter, but that's not easy/straightforward right now. No action > required, just a thought... Acknowledged.
lgtm; thanks for bearing with my heavy feedback.
The CQ bit was checked by qiangchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1990053002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1990053002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Desktop Capture Picker New UI: Non Mac Appearance Change In this CL, we changed the detailed appearance of DesktopMediaSourceView. Technically, we made a struct and plug in as parameter to implement different styles. BUG=602478 ========== to ========== Desktop Capture Picker New UI: Non Mac Appearance Change In this CL, we changed the detailed appearance of DesktopMediaSourceView. Technically, we made a struct and plug in as parameter to implement different styles. TBR=sergeyu@chromium.org BUG=602478 ==========
qiangchen@chromium.org changed reviewers: + sergeyu@chromium.org
The CQ bit was checked by qiangchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1990053002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1990053002/120001
Message was sent while issue was closed.
Description was changed from ========== Desktop Capture Picker New UI: Non Mac Appearance Change In this CL, we changed the detailed appearance of DesktopMediaSourceView. Technically, we made a struct and plug in as parameter to implement different styles. TBR=sergeyu@chromium.org BUG=602478 ========== to ========== Desktop Capture Picker New UI: Non Mac Appearance Change In this CL, we changed the detailed appearance of DesktopMediaSourceView. Technically, we made a struct and plug in as parameter to implement different styles. TBR=sergeyu@chromium.org BUG=602478 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Desktop Capture Picker New UI: Non Mac Appearance Change In this CL, we changed the detailed appearance of DesktopMediaSourceView. Technically, we made a struct and plug in as parameter to implement different styles. TBR=sergeyu@chromium.org BUG=602478 ========== to ========== Desktop Capture Picker New UI: Non Mac Appearance Change In this CL, we changed the detailed appearance of DesktopMediaSourceView. Technically, we made a struct and plug in as parameter to implement different styles. TBR=sergeyu@chromium.org BUG=602478 Committed: https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef Cr-Commit-Position: refs/heads/master@{#396322} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef Cr-Commit-Position: refs/heads/master@{#396322}
Message was sent while issue was closed.
On 2016/05/26 23:50:06, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as > https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef > Cr-Commit-Position: refs/heads/master@{#396322} This doesn't seem as urgent or trivial as most valid TBR'ed changes.
Message was sent while issue was closed.
On 2016/05/27 00:11:46, msw wrote: > On 2016/05/26 23:50:06, commit-bot: I haz the power wrote: > > Patchset 5 (id:??) landed as > > https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef > > Cr-Commit-Position: refs/heads/master@{#396322} > > This doesn't seem as urgent or trivial as most valid TBR'ed changes. Sorry, but my thinking is that this is not the last CL for the UI change. My next CL changing Mac UI will depend on some changes of this CL. If sergeyu@ has some comments on tab_media_list, I can fix them in the Mac CL, as this is the common code path for both Mac and NonMac UI. |
