Display Window Icon In Picker UI

chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc

Index: chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc
diff --git a/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc b/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc
index ef1b7644b09028168643bc7f842db7ecd5d84138..910d52651949560aa5ff2a205a990caa7fd907cd 100644
--- a/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc
+++ b/chrome/browser/ui/views/desktop_capture/desktop_media_list_view.cc
@@ -7,11 +7,16 @@
 #include "base/command_line.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/media/desktop_media_list.h"
+#include "chrome/browser/media/window_icon_util.h"
 #include "chrome/browser/ui/views/desktop_capture/desktop_media_picker_views.h"
 #include "chrome/browser/ui/views/desktop_capture/desktop_media_source_view.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/browser_thread.h"
+#include "grit/theme_resources.h"
 #include "ui/accessibility/ax_view_state.h"
+#include "ui/aura/client/aura_constants.h"
+#include "ui/aura/window.h"
+#include "ui/base/resource/resource_bundle.h"
 #include "ui/views/focus/focus_manager.h"
 
 using content::DesktopMediaID;
@@ -20,6 +25,29 @@ namespace {
 
 const int kDesktopMediaSourceViewGroupId = 1;
 
+const gfx::ImageSkia GetWindowIconHelper(DesktopMediaID id) {
+#if (defined(OS_LINUX) && !defined(OS_CHROMEOS)) || defined(OS_WIN) || \

[msw, 2016/08/23 21:37:12]

q: would #if !defined(OS_CHROMEOS) suffice? Is this file used on android/ios or
elsewhere that would need to be handled separately? Consider:
#if defined(OS_CHROMEOS)
  // cros code
#else
  // desktop code (if this file is not built on ios/android/etc.)
#endif

Or:
#if defined(OS_CHROMEOS)
  // cros code
#elif defined(OS_LINUX) || defined(OS_WIN) || defined(OS_MACOSX)
  // desktop code (if this file is build on ios/android/etc.)
#else
  // ios/android/etc.'s return gfx::ImageSkia();
#endif

[qiangchen, 2016/08/23 23:24:30]

Moved to a single file as implementation of GetWindowIcon on chromeos, and thus
UI part will look platform independent.

+    defined(OS_MACOSX)
+  return GetWindowIcon(id);
+#elif defined(OS_CHROMEOS)

[msw, 2016/08/23 21:37:12]

Does the code here actually belong in the GetWindowIcon impl for cros?

[qiangchen, 2016/08/23 23:24:30]

N/A now.

+  aura::Window* window = DesktopMediaID::GetAuraWindowById(id);
+  const gfx::ImageSkia* icon_image_ptr = nullptr;
+  if (window)
+    icon_image_ptr = window->GetProperty(aura::client::kWindowIconKey);
+
+  if (icon_image_ptr) return *icon_image_ptr;

[msw, 2016/08/23 21:37:12]

No return on the same line as if; use 'git cl format'.

[qiangchen, 2016/08/23 23:24:30]

Done.

+
+  // The default icon (for chrome window itself) does not enter window tree

[msw, 2016/08/23 21:37:11]

nit: "(for a Chrome window itself)". Also, I'm not sure what you mean by "does
not enter window tree host", do you mean "is not a member of the window tree
host"? if so, that seems odd... do you just mean that we never call
RegisterAuraWindow for browser windows? if so, is that something we can fix
instead?

[qiangchen, 2016/08/23 23:24:30]

Changed to return empty icon if not aura window.

For chrome window itself, its aura::Window instance does not contain a icon. I
just found the corresponding NativeWidgetAura::SetWindowIcons does not get
called at all, which is the place where we intercept the icon image.

Let me ask aura guys to see whether it is easy to inject icon into aura::Window
for chrome windows, or it is ok to do it here.

+  // host, and thus not registered in aura::Window.
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  icon_image_ptr = rb.GetNativeImageNamed(IDR_PRODUCT_LOGO_32).ToImageSkia();

[msw, 2016/08/23 21:37:12]

Is it possible that something other than a chrome window couldn't be found by
DesktopMediaID::GetAuraWindowById? If so, does it actually make sense to use a
chrome logo for this unidentified window? That seems wrong.

[qiangchen, 2016/08/23 23:24:30]

For ChromeOS, everything is aura window, so no such worry.
For other OS, we do not use this code path, but use OS api directly.

+  if (icon_image_ptr) return *icon_image_ptr;

[msw, 2016/08/23 21:37:12]

Ditto: No return on the same line as if; use 'git cl format'.

[qiangchen, 2016/08/23 23:24:30]

Done.

+  return gfx::ImageSkia();
+#else
+  return gfx::ImageSkia();
+#endif
+}
+
 }  // namespace
 
 DesktopMediaListView::DesktopMediaListView(
@@ -151,6 +179,8 @@ void DesktopMediaListView::OnSourceAdded(DesktopMediaList* list, int index) {
 
   source_view->SetName(source.name);
   source_view->SetGroup(kDesktopMediaSourceViewGroupId);
+  if (source.id.type == DesktopMediaID::TYPE_WINDOW)
+    source_view->SetIcon(GetWindowIconHelper(source.id));
   AddChildViewAt(source_view, index);
 
   if ((child_count() - 1) % active_style_->columns == 0)