CURSORINFO.flags should be checked before capturing its bitmap

webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc

Index: webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc
diff --git a/webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc b/webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc
index 204bb00b1602f9d32344e7f0da6145244e1979ca..93f3669c0d43470d8b28b91df801726aabd321ac 100644
--- a/webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc
+++ b/webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc
@@ -11,10 +11,12 @@
 #include "webrtc/modules/desktop_capture/mouse_cursor_monitor.h"
 
 #include <assert.h>
+#include <string.h>
 
 #include <memory>
 
 #include "webrtc/modules/desktop_capture/desktop_frame.h"
+#include "webrtc/modules/desktop_capture/desktop_geometry.h"
 #include "webrtc/modules/desktop_capture/mouse_cursor.h"
 #include "webrtc/modules/desktop_capture/win/cursor.h"
 #include "webrtc/modules/desktop_capture/win/window_capture_utils.h"
@@ -22,6 +24,19 @@
 
 namespace webrtc {
 
+namespace {
+
+bool Equal(const CURSORINFO& left, const CURSORINFO& right) {

[Sergey Ulanov, 2016/05/12 23:23:33]

Rename this to IsSameCursorShape(). This function verifies that the shape is the
same, not that the CURSORINFO are equal (it doesn't check position)

[Hzj_jie, 2016/05/13 01:05:28]

Done.

+  if (left.flags == right.flags) {

[Sergey Ulanov, 2016/05/12 23:23:33]

You can express the whole function with a single return statement:
 return left.flags == right.flags && 
        (left.flags != CURSOR_SHOWING ||
         left.hCursor == right.hCursor);

[Hzj_jie, 2016/05/13 01:05:28]

Done.

+    // If the cursors are not showing, we do not care the hCursor handle.
+    return left.flags != CURSOR_SHOWING ||
+           left.hCursor == right.hCursor;
+  }
+  return false;
+}
+
+}  // namespace
+
 class MouseCursorMonitorWin : public MouseCursorMonitor {
  public:
   explicit MouseCursorMonitorWin(HWND window);
@@ -45,7 +60,8 @@ class MouseCursorMonitorWin : public MouseCursorMonitor {
 
   HDC desktop_dc_;
 
-  HCURSOR last_cursor_;
+  // The last CURSORINFO (converted to MouseCursor) we have sent to the client.
+  CURSORINFO last_cursor_;
 };
 
 MouseCursorMonitorWin::MouseCursorMonitorWin(HWND window)
@@ -53,8 +69,8 @@ MouseCursorMonitorWin::MouseCursorMonitorWin(HWND window)
       screen_(kInvalidScreenId),
       callback_(NULL),
       mode_(SHAPE_AND_POSITION),
-      desktop_dc_(NULL),
-      last_cursor_(NULL) {
+      desktop_dc_(NULL) {
+  memset(&last_cursor_, 0, sizeof(CURSORINFO));
 }
 
 MouseCursorMonitorWin::MouseCursorMonitorWin(ScreenId screen)
@@ -62,9 +78,9 @@ MouseCursorMonitorWin::MouseCursorMonitorWin(ScreenId screen)
       screen_(screen),
       callback_(NULL),
       mode_(SHAPE_AND_POSITION),
-      desktop_dc_(NULL),
-      last_cursor_(NULL) {
+      desktop_dc_(NULL) {
   assert(screen >= kFullDesktopScreenId);
+  memset(&last_cursor_, 0, sizeof(CURSORINFO));
 }
 
 MouseCursorMonitorWin::~MouseCursorMonitorWin() {
@@ -92,13 +108,28 @@ void MouseCursorMonitorWin::Capture() {
     return;
   }
 
-  if (last_cursor_ != cursor_info.hCursor) {
-    last_cursor_ = cursor_info.hCursor;
-    // Note that |cursor_info.hCursor| does not need to be freed.
-    std::unique_ptr<MouseCursor> cursor(
-        CreateMouseCursorFromHCursor(desktop_dc_, cursor_info.hCursor));
-    if (cursor.get())
-      callback_->OnMouseCursor(cursor.release());
+  if (!Equal(cursor_info, last_cursor_)) {
+    if (cursor_info.flags == CURSOR_SUPPRESSED) {
+      // The cursor is intentionally hidden now, send an empty bitmap.
+      last_cursor_ = cursor_info;
+      callback_->OnMouseCursor(new MouseCursor(
+          new BasicDesktopFrame(DesktopSize()), DesktopVector()));
+    } else {
+      // Note that |cursor_info.hCursor| does not need to be freed.

[Sergey Ulanov, 2016/05/12 23:23:33]

move this comment below, where we call CreateMouseCursorFromHCursor()

[Hzj_jie, 2016/05/13 01:05:28]

According to MSDN http://shortn/_FXmsigcgUC (You may find "Before closing, you
must use the DestroyCursor function to destroy any cursors you created with
CreateCursor. It is not necessary to destroy cursors created by other
functions."), we do not need to free HCURSOR created by other functions. So this
comment explains both scenarios (use existing hCursor or use LoadCursor). I will
update the comment to make it more clear.

[Sergey Ulanov, 2016/05/13 21:32:40]

Please don't use shortn links in code reviews - these links don't work
externally.
That comment is about cursors created using CreateCursor(). I don't think it
applies here. Also if I understand this comment correctly it's about destroying
cursor which is different from closing cursor _handle_. The "Before closing"
part actually suggests that HCURSOR instances do need to be closed.
Anyway, MSDN is confusion about it. From LoadCursor() documentation it looks
like we should expect the same handle to be returned every time, so it doesn't
need to be closed.

+      if (cursor_info.flags == 0) {
+        // Host machine does not have a hardware mouse attached, we will send a
+        // default one instead.
+        // Note, Windows automatically caches cursor resource, so we do not need
+        // to cache the result of LoadCursor.
+        cursor_info.hCursor = LoadCursor(nullptr, IDC_ARROW);

[Sergey Ulanov, 2016/05/12 23:23:33]

I think you also need to CloseHandle() for the handle returned by LoadCursor()

[Hzj_jie, 2016/05/13 01:05:28]

Done.

+      }
+      std::unique_ptr<MouseCursor> cursor(
+          CreateMouseCursorFromHCursor(desktop_dc_, cursor_info.hCursor));
+      if (cursor) {
+        last_cursor_ = cursor_info;
+        callback_->OnMouseCursor(cursor.release());
+      }
+    }
   }
 
   if (mode_ != SHAPE_AND_POSITION)