Desktop Capture Picker Window New UI |
|||||||||||||||||||
Issue descriptionAfter the development of Tab Capture and Audio Capture feature for Desktop Sharing. The old UI of picker window looks confusing for the end user. Thus we need a new UI design for the picker window. The design doc: https://docs.google.com/presentation/d/1xIgk_xNpa-4yBSR4DIcoDLfBFigu7SJwFWOdIzSC8r4/
,
Apr 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0e89b88cb035447d45781c15c85f140ae8b47518 commit 0e89b88cb035447d45781c15c85f140ae8b47518 Author: qiangchen <qiangchen@chromium.org> Date: Fri Apr 15 21:40:38 2016 Desktop Capture Picker New UI: Preliminary Refactor This CL essentially changes the signature of DesktopMediaPicker::Show to take three media lists. The reason doing this is for new UI of the picker window. BUG= 602478 Review URL: https://codereview.chromium.org/1880693002 Cr-Commit-Position: refs/heads/master@{#387706} [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/extensions/api/desktop_capture/desktop_capture_apitest.cc [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/extensions/api/desktop_capture/desktop_capture_base.h [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/media/desktop_media_picker.h [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.h [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.mm [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/ui/views/desktop_media_picker_views.cc [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/ui/views/desktop_media_picker_views.h [modify] https://crrev.com/0e89b88cb035447d45781c15c85f140ae8b47518/chrome/browser/ui/views/desktop_media_picker_views_unittest.cc
,
Apr 25 2016
,
Apr 25 2016
Christopher/Alex, why are the titles in ALL-CAPS here? (see design doc from comment #1) This doesn't seem like a pattern we use anywhere else in Chrome UI... https://codereview.chromium.org/1909663004/diff/40001/chrome/app/generated_resources.grd#newcode13550
,
Apr 26 2016
Maybe it's part of the move to MD? bettes@ will better know the status there / whether this is deliberate.
,
Apr 26 2016
This is the way that tabs are represented in MD. If this is too early for that change in the Chrome UI, feel free to change them to sentence case.
,
Apr 26 2016
Re#5: What is MD?
,
Apr 26 2016
Material Design, https://www.google.com/design/spec/material-design/introduction.html
,
Apr 28 2016
Question: whether we should use views::TabbedPane to implement the switching between media lists. One advantage using this structure is that our code can be simpler, and the pane appearance is consistent with other places of chrome, but the down side is that we have little control on the appearance of the pane itself. Look at UsingPane.png for example. We can hardly change the appearance of the pane tab buttons. The other way is adding buttons, and handling the switching event by ourselves. In this case we can make the appearance as close to MD as possible. Look at NotUsingPane.png for example.
,
Apr 28 2016
Note that I *highly* recommend using the standard tabbed pane UI control. Building a similar one-off solution not only causes appearance inconsistencies in our UI, but it allows divergence in the behavior, accessibility, and testing of these components and it adds unnecessary maintenance costs going forward. Instead, the standard tabbed pane UI should be updated to support the MD appearance behind a flag or experiment switch, like the rest of MD.
,
Apr 28 2016
You should be able to adjust the dialog layout to give the switcher the same dimensions in either case, and that should hopefully eliminate the scrollbars in your tabbed pane example.
,
Apr 28 2016
Re #11: yes, we can control the layout of pane in the dialog, and we can control the content in the pane. The only thing we cannot config is the appearance of the pane tab button.
,
Apr 28 2016
,
May 4 2016
,
May 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/59c1931d98600ca11a4a11f0ee7169bdc6325bda commit 59c1931d98600ca11a4a11f0ee7169bdc6325bda Author: qiangchen <qiangchen@chromium.org> Date: Fri May 06 20:29:40 2016 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 Review-Url: https://codereview.chromium.org/1932413002 Cr-Commit-Position: refs/heads/master@{#392138} [modify] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/app/chromium_strings.grd [modify] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/app/generated_resources.grd [modify] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/app/google_chrome_strings.grd [modify] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/browser/about_flags.cc [copy] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc [copy] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h [add] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc [rename] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/browser/ui/views/desktop_media_picker_views_deprecated.cc [rename] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/browser/ui/views/desktop_media_picker_views_deprecated.h [rename] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/browser/ui/views/desktop_media_picker_views_deprecated_unittest.cc [modify] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/chrome_browser_ui.gypi [modify] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/chrome/chrome_tests_unit.gypi [modify] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/extensions/common/switches.cc [modify] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/extensions/common/switches.h [modify] https://crrev.com/59c1931d98600ca11a4a11f0ee7169bdc6325bda/tools/metrics/histograms/histograms.xml
,
May 6 2016
For Non-mac platform. You can now experience the new desktop capture picker UI by turn on the flag "Disable Desktop Capture Picker Window Old UI" on the setting page "chrome://flags". It is not completely finished as the design, but we've already separate the sources into different pane according to their type.
,
May 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c0f67d648495f568b4c034e9e5c0fe63d688fc4b commit c0f67d648495f568b4c034e9e5c0fe63d688fc4b Author: Nico Weber <thakis@chromium.org> Date: Sat May 07 14:24:05 2016 Revert "Desktop Capture Picker New UI: Non Mac Structure Change" (https://codereview.chromium.org/1932413002/) 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] BUG= 602478 TBR=qiangchen@chromium.org Review URL: https://codereview.chromium.org/1957063002 . Cr-Commit-Position: refs/heads/master@{#392261} [modify] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/chrome/app/chromium_strings.grd [modify] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/chrome/app/generated_resources.grd [modify] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/chrome/app/google_chrome_strings.grd [modify] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/chrome/browser/about_flags.cc [delete] https://crrev.com/f520475a947e34407e89844d5889967e66928165/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc [delete] https://crrev.com/f520475a947e34407e89844d5889967e66928165/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h [delete] https://crrev.com/f520475a947e34407e89844d5889967e66928165/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc [rename] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/chrome/browser/ui/views/desktop_media_picker_views.cc [rename] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/chrome/browser/ui/views/desktop_media_picker_views.h [rename] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/chrome/browser/ui/views/desktop_media_picker_views_unittest.cc [modify] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/chrome/chrome_browser_ui.gypi [modify] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/chrome/chrome_tests_unit.gypi [modify] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/extensions/common/switches.cc [modify] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/extensions/common/switches.h [modify] https://crrev.com/c0f67d648495f568b4c034e9e5c0fe63d688fc4b/tools/metrics/histograms/histograms.xml
,
May 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ec8a48393ac2f10940f3b6e63d1319222e660cff commit ec8a48393ac2f10940f3b6e63d1319222e660cff Author: qiangchen <qiangchen@chromium.org> Date: Mon May 09 20:11:27 2016 Desktop Capture Picker New UI: Non Mac Structure Change A re-commit of 1932413002. The reason of unittest crash is that we removed a placeholder in the string resource for window title, but we still used GetStringFUTF16 in the code of old UI. In this CL, we add a deprecated string resource with placeholder for old picker UI to use. BUG= 602478 , 610016 TBR=isherman@chromium.org,rockot@chromium.org Review-Url: https://codereview.chromium.org/1962073002 Cr-Commit-Position: refs/heads/master@{#392406} [modify] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/app/chromium_strings.grd [modify] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/app/generated_resources.grd [modify] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/app/google_chrome_strings.grd [modify] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/browser/about_flags.cc [copy] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc [copy] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h [add] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc [rename] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/browser/ui/views/desktop_media_picker_views_deprecated.cc [rename] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/browser/ui/views/desktop_media_picker_views_deprecated.h [rename] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/browser/ui/views/desktop_media_picker_views_deprecated_unittest.cc [modify] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/chrome_browser_ui.gypi [modify] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/chrome/chrome_tests_unit.gypi [modify] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/extensions/common/switches.cc [modify] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/extensions/common/switches.h [modify] https://crrev.com/ec8a48393ac2f10940f3b6e63d1319222e660cff/tools/metrics/histograms/histograms.xml
,
May 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623 commit 31c2d5a8cbeced5cb648ed5fea9a9e92efb13623 Author: Stanislav Chiknavaryan <stanisc@chromium.org> Date: Mon May 09 22:34:53 2016 Revert "Desktop Capture Picker New UI: Non Mac Structure Change" This reverts commit ec8a48393ac2f10940f3b6e63d1319222e660cff. Reason for the revert: Breaks a number of DesktopMediaPickerControllerTest tests on Builder: Mac10.9 Tests (dbg) (stats). https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29 TBR=isherman@chromium.org,rockot@chromium.org,qiangchenC@chromium.org BUG= 602478 , 610016 Review URL: https://codereview.chromium.org/1958293002 . Cr-Commit-Position: refs/heads/master@{#392451} [modify] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/chrome/app/chromium_strings.grd [modify] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/chrome/app/generated_resources.grd [modify] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/chrome/app/google_chrome_strings.grd [modify] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/chrome/browser/about_flags.cc [delete] https://crrev.com/58b07f5147cb47dc85302bd3a112efb627c972ff/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc [delete] https://crrev.com/58b07f5147cb47dc85302bd3a112efb627c972ff/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h [delete] https://crrev.com/58b07f5147cb47dc85302bd3a112efb627c972ff/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc [rename] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/chrome/browser/ui/views/desktop_media_picker_views.cc [rename] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/chrome/browser/ui/views/desktop_media_picker_views.h [rename] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/chrome/browser/ui/views/desktop_media_picker_views_unittest.cc [modify] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/chrome/chrome_browser_ui.gypi [modify] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/chrome/chrome_tests_unit.gypi [modify] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/extensions/common/switches.cc [modify] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/extensions/common/switches.h [modify] https://crrev.com/31c2d5a8cbeced5cb648ed5fea9a9e92efb13623/tools/metrics/histograms/histograms.xml
,
May 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5ced272025f70119b5747514bdb112591f36557e commit 5ced272025f70119b5747514bdb112591f36557e Author: qiangchen <qiangchen@chromium.org> Date: Tue May 10 23:31:20 2016 Desktop Capture Picker New UI: Non Mac Structure Change A re-commit of 1932413002. The reason of unittest crash is that we removed a placeholder in the string resource for window title, but we still used GetStringFUTF16 in the code of old UI. In this CL, we add a deprecated string resource with placeholder for old picker UI to use. BUG= 602478 , 610016 TBR=isherman@chromium.org,rockot@chromium.org Review-Url: https://codereview.chromium.org/1966653002 Cr-Commit-Position: refs/heads/master@{#392770} [modify] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/app/chromium_strings.grd [modify] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/app/generated_resources.grd [modify] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/app/google_chrome_strings.grd [modify] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/browser/about_flags.cc [modify] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm [copy] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc [copy] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h [add] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc [rename] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/browser/ui/views/desktop_media_picker_views_deprecated.cc [rename] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/browser/ui/views/desktop_media_picker_views_deprecated.h [rename] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/browser/ui/views/desktop_media_picker_views_deprecated_unittest.cc [modify] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/chrome_browser_ui.gypi [modify] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/chrome/chrome_tests_unit.gypi [modify] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/extensions/common/switches.cc [modify] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/extensions/common/switches.h [modify] https://crrev.com/5ced272025f70119b5747514bdb112591f36557e/tools/metrics/histograms/histograms.xml
,
May 11 2016
,
May 13 2016
Hi all, I was able to load the new UI up on a Windows test machine we have here. It is still very early. At what point do you want UI bugs filed?
,
May 13 2016
Yes, currently, the above CL just changes the topology of the picker. The detailed appearance is still old style. So the bug is still open.
,
May 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1152870b3290f647999811619b92b2f37c81af92 commit 1152870b3290f647999811619b92b2f37c81af92 Author: qiangchen <qiangchen@chromium.org> Date: Tue May 17 20:14:49 2016 Desktop Capture Picker New UI: Split File This CL is the preliminary change for detailed appearance change of desktop capture picker UI. We separate DesktopMediaListView and DesktopMediaSourceView into their own files, as we anticipated that after appearance change, the code size of those two classes will be too large to squeeze in DesktopMediaPickerDialogView's file. BUG= 602478 Review-Url: https://codereview.chromium.org/1978633002 Cr-Commit-Position: refs/heads/master@{#394204} [add] https://crrev.com/1152870b3290f647999811619b92b2f37c81af92/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc [add] https://crrev.com/1152870b3290f647999811619b92b2f37c81af92/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h [modify] https://crrev.com/1152870b3290f647999811619b92b2f37c81af92/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc [modify] https://crrev.com/1152870b3290f647999811619b92b2f37c81af92/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h [modify] https://crrev.com/1152870b3290f647999811619b92b2f37c81af92/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_unittest.cc [add] https://crrev.com/1152870b3290f647999811619b92b2f37c81af92/chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc [add] https://crrev.com/1152870b3290f647999811619b92b2f37c81af92/chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h [modify] https://crrev.com/1152870b3290f647999811619b92b2f37c81af92/chrome/chrome_browser_ui.gypi
,
May 24 2016
Question for designers: Do we really need separate styling for a single screen versus multiple screens? It'd simplify the code a bit to use the same item sizing/layout/titling regardless of the number of screens available to share.
,
May 24 2016
Follow up question: If we can only support a style for both single screen and multi screen case. Should we use the big view (page 4) or small view (page 5)?
,
May 25 2016
The reason that I designed the 2 different layouts is that most people have only one screen, and it seemed like if we used the multiscreen design for the single screen (the more common case), there was a lot of wasted space. I went ahead and added a new slide to the slide deck to illustrate what this would look like if we used the multi-screen template for single screens (slide 8). I think it looks half-done. See here: https://docs.google.com/presentation/d/1xIgk_xNpa-4yBSR4DIcoDLfBFigu7SJwFWOdIzSC8r4/edit#slide=id.g133571cd28_1_1 I don't think we can use the single screen layout for multiscreen either. Obviously, if this is going to add a significant amount of overhead or potential problems, we should talk more, but I think that this design is both visually clearer and aids in understanding for the majority of users. I think it is a pretty good win.
,
May 25 2016
The major overhead comes from that we need to adjust the style on the fly because: 1. In initialization, we do not know how many screens the user has. 2. During previewing, it is possible that the number of screens changes. For example, the user plugs in or remove a monitor. Then we need to adjust the style after detecting that.
,
May 25 2016
Okay, I suppose the added complexity might be worthwhile.
,
May 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d86a0fe62259a05cd15b01165171fcb0baad85ef commit d86a0fe62259a05cd15b01165171fcb0baad85ef Author: qiangchen <qiangchen@chromium.org> Date: Thu May 26 23:47:42 2016 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 Review-Url: https://codereview.chromium.org/1990053002 Cr-Commit-Position: refs/heads/master@{#396322} [modify] https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef/chrome/app/generated_resources.grd [modify] https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef/chrome/browser/media/tab_desktop_media_list.cc [modify] https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef/chrome/browser/media/tab_desktop_media_list_unittest.cc [modify] https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc [modify] https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h [modify] https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.cc [modify] https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h [modify] https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef/chrome/browser/ui/views/desktop_capture/desktop_media_source_view.cc [modify] https://crrev.com/d86a0fe62259a05cd15b01165171fcb0baad85ef/chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h
,
May 31 2016
Non Mac UI is done. Can you take a look pkchrisjohnson@? Major visual differences from design are 1. The appearance of tab buttons, namely "Screen", "Window", "Tab" We finally decide to stick to using existing tab structure, rather than implementing the modern UI using buttons, which will make UI consistent with other part of chrome. And thus we made some other minor changes to let the picker window stay in a traditional looking. 2. We did not group tabs by their parent chrome window. For reasons: 2.a There is no name for chrome window. 2.b If we simply name them as "Window 1", "Window 2", then it could be confusing somehow, when the user drags a tab from a window and merge with another window. 2.c We ordered the tab by their most recent active instant, which is incompatible with window group.
,
Jun 9 2016
,
Jun 29 2016
Yep, but I think it is better to 1. Turn on the flag "Desktop Share with tab source" 2. Use website "https://test.webrtc.org/manual/peer2peer/" So that you can see the "Chromium Tab" source.
,
Jun 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f8a7f3f0519e401ddadc437dfce377be5d43ed46 commit f8a7f3f0519e401ddadc437dfce377be5d43ed46 Author: qiangchen <qiangchen@chromium.org> Date: Thu Jun 30 18:08:55 2016 Desktop Capture Picker Window New UI For Mac This CL develops the new Mac UI for Desktop Capture Picker window. The main changes are 1. Separate the items of different source types into different browser view. 2. Use table view rather than image view for tab capture, because we do not have HD preview for tab. BUG= 602478 , 618796 Review-Url: https://codereview.chromium.org/2072003002 Cr-Commit-Position: refs/heads/master@{#403223} [modify] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h [modify] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.mm [modify] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.h [modify] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.mm [modify] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h [modify] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm [add] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_deprecated.h [add] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_deprecated.mm [add] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_deprecated_unittest.mm [modify] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm [modify] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/chrome_browser_ui.gypi [modify] https://crrev.com/f8a7f3f0519e401ddadc437dfce377be5d43ed46/chrome/chrome_tests_unit.gypi
,
Jun 30 2016
Mark it as fixed as the major work is done. Suggestions about new picker UI are welcome.
,
Jun 30 2016
Is there a bug open for deleting the old code? If not, please file one, thanks.
,
Jul 7 2016
The Mac UI hasn't landed yet, right? Can you please provide some more instructions what you mean with 2. Use website "https://test.webrtc.org/manual/peer2peer/" ? When testing on Linux, I got attached picker using beta (52.0.2743.60)
,
Jul 7 2016
I got confused by the enable/disable text in chrome://flags. I now see the new picker UI in canary and I also understand what you mean with "2. Use website "https://test.webrtc.org/manual/peer2peer/" ? ". The original design proposal by pkchrisjohnson@ included a 3. tab labelled as "Chrome tab", when tabs are available (see #9). In the attached screenshot, the tab shows up as an application window. I think this is confusing to the user. From a design perspective, I think it would be great to have all screen thumbnails with the same height in OS X as shown in the Linux screen shot.
,
Jul 7 2016
Looks as if we got the old picker in #38. Tried again on Linux and got the attached result. All non-Chrome windows were black thumbnails https://test.webrtc.org/manual/peer2peer/ did not show up in the window list, nor was an additional tab in the picker created. Re. thumbnail size in picker, all seem to have the same width, correct?
,
Jul 8 2016
Re #38, #40: For M52, we do not have Mac New UI. Mac New UI is available in M53 behind a flag, or M54 by default. Re #39: Another flag controls whether we enable tab sharing in the picker or not. Search "Desktop Share with tab source" in about:flags to enable it. Again, similar to above, the functionality is behind a flag in M52, and on by default in M54.
,
Jul 8 2016
Re #40: Yes, the widths are the same. It is a known low level bug about linux that if the window is partially out of boundary, then the window capture will fail.
,
Jul 8 2016
OK, I am using 54.0.2791.0 canary (64-bit) on Mac OS my flags are: Disable Audio For Desktop Share - Enabled Disable Desktop Capture Picker Window Old UI - Disable (I guess this means enabled) I have http://test.webrtc.org/manual/peer2peer/ in a tab open The picker UI does not show a "Chrome tabs" tab
,
Jul 8 2016
It is this flag "Desktop Share with tab source" that matters whether "Chromium Tab" shows or not.
,
Jul 8 2016
yes, of course ;-) copy/paste error on my side. Desktop Share with tab source was enabled in above screenshot. I was neither on Linux nor on Mac able to get the "Chrome tabs" tab shown in the picker, when having http://test.webrtc.org/manual/peer2peer/ open. Am I doing something wrong? On Linux, the tab with http://test.webrtc.org/manual/peer2peer/ was shown in the "Application Window" tab as a thumbnail (#39).
,
Jul 8 2016
Ah, another thing I forget to mention. You may need to re-download the most recent extension for "http://test.webrtc.org/manual/peer2peer/". As I updated some code there (namely add "tab" as a capture source.) For convenience, I attach the extension here.
,
Jul 11 2016
One more issue I noticed: The font of the active tab is not becoming black but grey when the picker window is not in focus. How to repro: 1) start a screen share 2) when picker window is open, click on desktop background 3) active tab font color is changing from white to grey expected result: font color should change to black See attached screenshots of a system settings dialogue (tab "General") and compare the font colors to the screen shot in #47 (tab "Application Window")
,
Jul 11 2016
Re #47: Please make sure the new extension overwrites the old extension. If you install the new extension in a new place, there is a chance that the website still gets data from old extension, and thus "tab" is not there.
,
Jul 12 2016
As this feature in status fixed, has anyone tested this besides me? I am struggling now since 4 days to get this fully working. I also see other features related to this one being enabled by default and I wonder if we are acting here to quick. Changing status back to unconfirmed, we should get WebRTC testing involved for all platforms and hangouts, if it hasn't happened until now. Adding tommi@ and jansson@
,
Jul 12 2016
I'm seeing the same as in comment #47. This is with or without the extension. Can you elaborate on what role the extension plays?
,
Jul 12 2016
This works for me without problems. If you want to see tabs as an option you need to call chooseDesktopMedia with 'tab' as constraint. chooseDesktopMedia is an extension API so it needs to be called from an extension. You can do this by installing and launching the extension Qiang provided. You can't test this with Hangouts since that extension is bundled with Chrome and isn't modified to enable tab sharing.
,
Jul 12 2016
If you have the chromium code base, then you can 1. Click "Developer mode" checkbox; 2. Load unpacked extension src/chrome/common/extensions/docs/examples/api/desktopCapture 3. Launch this example extension This extension must be updated, and with "tab" included in the source.
,
Jul 13 2016
qiangchen@ Could you update the extension so that this is testable out of the box? Just to confirm, all that is needed is to add the string 'tab' to the array that's passed in to chooseDesktopMedia (https://cs.chromium.org/chromium/src/chrome/common/extensions/docs/examples/api/desktopCapture/background.js?l=16)?
,
Jul 13 2016
Thanks. I got it working now. Looks very good!
,
Jul 14 2016
Tested the new desktop capture picker UI in Win, Mac, Linux in M53, and it looks good. Don't see any issues so far
,
Jul 14 2016
See the attachments for the developed version of the new UI. Major difference: 1. "Screen", "Window", "Tab" button appearance. As we decided to stick using the existing TabbedPane framework, which does not give us the freedom adjusting the title buttons' appearance. Then with this old style, we cannot set the width of the TabbedPane to be the same with as its parent window as in the design, otherwise it looks ugly. 2. For window list, we did not have the icon. Theoretically, it can be done. However, the current webrtc::WindowCapturer does not provide with us such resource. In order to do that, we need to investigate on each platform what API to call to retrieve the raw icon data, and convert to the format chrome is using, then pass it into DesktopMediaList, then pass it into the picker window to render. The project size for this minor feature is probably larger than other parts of this UI change. On the other hand, this UI change blocks our release of tab and audio capture, and thus I inclined to put this as a future enhancement, and let major functionality go first. 3. Audio share checkbox is still a standard checkbox, not using audio icon. The same reason as 1.
,
Jul 14 2016
msw@: I hear from pkchrisjohnson@google.com that Chrome is doing a UI migration from the traditional appearance to modern appearance. Do you have an idea of the timeline? I am asking this because I think in order to make the picker window closer to the one in the design, I still need to do modification after the migration.
,
Jul 14 2016
Chrome is already undergoing material design changes, and numerous UI surfaces have already been updated (some enabled by flags), but I don't know the timeline. It seems reasonable to update the views tabbed pane control for material design. I don't know if there are already plans for that; ainslie@ or pkchrisjohnson@ might know or know who to ask.
,
Jul 15 2016
@Qiang, could you please post the latest screenshots of the Windows UI on here also. I know it is similar to Linux, but for completeness, it would be useful to see.
,
Jul 15 2016
I'll upload it asap, but it takes my windows laptop a long time to compile chromium. The only difference is that on Screen share, it has "audio share" checkbox. Other than that, everything should be the same.
,
Jul 18 2016
Windows snapshot. Actually, we did not hardcode any button background color, or item background color. They are determined by the system theme.
,
Jul 18 2016
Why does the primary call to action move between Windows and Linux? @Alex, are we deviating from the standard Material Design pattern of having the primary call to action on the outside?
,
Jul 18 2016
This is also controlled by OS' standard. For example, on linux, we have "OK" to the right of "Cancel". But on windows, we have "OK" to the left of "Cancel". We do this arrangement to make our UI consistent with OS standard. Besides, this setting is out of my development control, it is in the UI foundation development.
,
Aug 31 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2b4bb81ea09b9e553a1721ee04da67b5b61dd62c commit 2b4bb81ea09b9e553a1721ee04da67b5b61dd62c Author: qiangchen <qiangchen@chromium.org> Date: Wed Aug 31 20:47:10 2016 Desktop Capture Picker Window String Change In the major CL, I did not notice the designed actually changed the statement a little, namely "would like" was changed to "wants" This CL fix this. BUG= 602478 , 642778 Review-Url: https://codereview.chromium.org/2296063002 Cr-Commit-Position: refs/heads/master@{#415746} [modify] https://crrev.com/2b4bb81ea09b9e553a1721ee04da67b5b61dd62c/chrome/app/generated_resources.grd
,
Sep 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cbcad42ac5afb2d0ce5da8888a3daf3a95d2f1a8 commit cbcad42ac5afb2d0ce5da8888a3daf3a95d2f1a8 Author: qiangchen <qiangchen@chromium.org> Date: Thu Sep 01 21:40:55 2016 Desktop Capture Picker Window String Change[M54] In the major CL, I did not notice the designed actually changed the statement a little, namely "would like" was changed to "wants" This CL fix this. BUG= 602478 , 642778 TBR=msw@chromium.org NOPRESUBMIT=true NOTRY=true Review-Url: https://codereview.chromium.org/2296063002 Cr-Commit-Position: refs/heads/master@{#415746} (cherry picked from commit 2b4bb81ea09b9e553a1721ee04da67b5b61dd62c) Review-Url: https://codereview.chromium.org/2300143003 Cr-Commit-Position: refs/branch-heads/2840@{#111} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/cbcad42ac5afb2d0ce5da8888a3daf3a95d2f1a8/chrome/app/generated_resources.grd
,
Sep 6 2016
qiangchen@ - Is there any manual test steps available to verify this issue ? If yes, then please provide the steps to verify it from chrome-Te team.
,
Sep 6 2016
Yes. 1. Launch desktop share picker dialog. A best way is use the "screen share" functionality on https://test.webrtc.org/manual/peer2peer/ 2. Play around the UI making sure the followings: 2.a Screen share can be done with/without audio 2.b Window share can be done. 2.c Tab Share can be done with/without audio 3. Any suggestions on usability improvement are welcome.
,
Sep 7 2016
Verified the issue on windows 10, Ubuntu 14.04 and Mac 10.11.6 using chrome dev version #54.0.2840.14 as per the comment #68 Observed that the fix is working as expected. Attaching screenshots for reference Thanks,
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cbcad42ac5afb2d0ce5da8888a3daf3a95d2f1a8 commit cbcad42ac5afb2d0ce5da8888a3daf3a95d2f1a8 Author: qiangchen <qiangchen@chromium.org> Date: Thu Sep 01 21:40:55 2016 Desktop Capture Picker Window String Change[M54] In the major CL, I did not notice the designed actually changed the statement a little, namely "would like" was changed to "wants" This CL fix this. BUG= 602478 , 642778 TBR=msw@chromium.org NOPRESUBMIT=true NOTRY=true Review-Url: https://codereview.chromium.org/2296063002 Cr-Commit-Position: refs/heads/master@{#415746} (cherry picked from commit 2b4bb81ea09b9e553a1721ee04da67b5b61dd62c) Review-Url: https://codereview.chromium.org/2300143003 Cr-Commit-Position: refs/branch-heads/2840@{#111} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/cbcad42ac5afb2d0ce5da8888a3daf3a95d2f1a8/chrome/app/generated_resources.grd |
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by qiangchen@chromium.org
, Apr 11 2016