Display Window Icon In Picker UI

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/desktop_capture/desktop_media_list_view.h"
 
 #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;
 
 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(
     DesktopMediaPickerDialogView* parent,
     std::unique_ptr<DesktopMediaList> media_list,
     DesktopMediaSourceViewStyle generic_style,
     DesktopMediaSourceViewStyle single_style,
     const base::string16& accessible_name)
     : parent_(parent),
       media_list_(std::move(media_list)),
@@ skipping 111 matching lines @@
 
   // We are going to have a second item, apply the generic style.
   if (child_count() == 1)
     SetStyle(&generic_style_);
 
   DesktopMediaSourceView* source_view =
       new DesktopMediaSourceView(this, source.id, *active_style_);
 
   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)
     parent_->OnMediaListRowsChanged();
 
   // Auto select the first screen.
   if (index == 0 && source.id.type == DesktopMediaID::TYPE_SCREEN)
     source_view->RequestFocus();
 
   PreferredSizeChanged();
@@ skipping 76 matching lines @@
     DesktopMediaSourceView* source_view =
         static_cast<DesktopMediaSourceView*>(child_at(i));
     source_view->SetStyle(*active_style_);
   }
 }
 
 void DesktopMediaListView::GetAccessibleState(ui::AXViewState* state) {
   state->role = ui::AX_ROLE_GROUP;
   state->name = accessible_name_;
 }