Icon Capture Utility For Linux, Windows and Mac

chrome/browser/media/window_icon_util_x11.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/media/window_icon_util.h"
+
+#include <X11/Xatom.h>
+#include <X11/Xutil.h>
+
+#include "third_party/webrtc/modules/desktop_capture/x11/shared_x_display.h"
+
+class WindowIconUtilLinux : public WindowIconUtil {
+ public:
+  explicit WindowIconUtilLinux(const webrtc::DesktopCaptureOptions& options);
+  gfx::ImageSkia GetWindowIcon(content::DesktopMediaID id) override;
+
+ private:
+  webrtc::SharedXDisplay* x_display_;
+};
+
+WindowIconUtilLinux::WindowIconUtilLinux(
+    const webrtc::DesktopCaptureOptions& options) {
+  x_display_ = options.x_display();
+}
+
+gfx::ImageSkia WindowIconUtilLinux::GetWindowIcon(content::DesktopMediaID id) {
+  Atom property = XInternAtom(x_display_->display(), "_NET_WM_ICON", True);
+  Atom actual_type;
+  int actual_format;
+  unsigned long bytes_after;  // NOLINT: type required by XGetWindowProperty
+  unsigned long size;
+  long* data;
+
+  int status = XGetWindowProperty(x_display_->display(), id.id, property, 0L,
+                                  ~0L, False, AnyPropertyType, &actual_type,
+                                  &actual_format, &size, &bytes_after,
+                                  reinterpret_cast<unsigned char**>(&data));
+  if (status != Success) {
+    return gfx::ImageSkia();
+  }
+
+  long width, height;

[Sergey Ulanov, 2016/08/15 22:28:39]

These are not set before being used below

[qiangchen, 2016/08/16 22:29:53]

Done.

+  int start = 0;
+  unsigned long i = 0;

[Sergey Ulanov, 2016/08/15 22:28:39]

Please don't use types like "unsigned long" and "long". See
https://google.github.io/styleguide/cppguide.html#Integer_Types

I think simple int would be appropriate for all these variables.

[qiangchen, 2016/08/16 22:29:53]

It is due to weird setting of linux for XGetProperty. For 32-bit machine, it
uses an int32_t to represent a pixel, while for 64-bit machine, it uses an
int64_t to represent a pixel, though only the lower 32 bits really contain
information. That's why we need to set |data| to "long *".

We can change the type for width and height, and then a cast will be needed to
take out data.

+  while (i < size) {
+    if (i == 0 || data[i] * data[i + 1] > width * height) {
+      width = data[i];
+      height = data[i + 1];
+      start = i + 2;
+    }
+    i = i + 2 + data[i] * data[i + 1];

[Sergey Ulanov, 2016/08/15 22:28:39]

It's very confusing what happens here. Is it that the returned value may contain
multiple icons and we want the last one? At very least this logic deserves a
comment above, but I think this code can be simplified as well. There are some
error cases that are not handled properly. For example with size = 0
uninitialized width/height are used below. When size=1 this code will try to use
data[1], which is outside of the buffer. Why is it always the last icon in the
list? Is it guaranteed to be the biggest?

[qiangchen, 2016/08/16 22:29:52]

The |size| is not how many icons are there, but just tells us how big the data
section is.

The data structure is [width1, height1, image_data for icon1, width2, height2,
image data for icon2, .....]

The code here took the biggest icon, not the last one.

+  }
+
+  SkBitmap result;
+  result.allocN32Pixels(width, height, false);
+  result.lockPixels();
+
+  uint32_t* pixels_data = reinterpret_cast<uint32_t*>(result.getPixels());
+
+  for (long y = 0; y < height; ++y) {
+    for (long x = 0; x < width; ++x) {
+      pixels_data[result.rowBytesAsPixels() * y + x] = SkPreMultiplyColor(
+          static_cast<uint32_t>(data[start + width * y + x]));
+    }
+  }
+
+  XFree(data);
+  return gfx::ImageSkia::CreateFrom1xBitmap(result);
+}
+
+std::unique_ptr<WindowIconUtil> WindowIconUtil::Create(
+    const webrtc::DesktopCaptureOptions& options) {
+  return std::unique_ptr<WindowIconUtil>(new WindowIconUtilLinux(options));
+}