[Ozone-DRI] Do proper bounds checks when moving the cursor

ui/ozone/platform/dri/dri_cursor.cc

 // Copyright 2014 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 "ui/ozone/platform/dri/dri_cursor.h"
 
 #include "ui/base/cursor/ozone/bitmap_cursor_factory_ozone.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/geometry/point_f.h"
 #include "ui/ozone/platform/dri/dri_surface_factory.h"
+#include "ui/ozone/platform/dri/dri_window.h"
+#include "ui/ozone/platform/dri/dri_window_manager.h"
 #include "ui/ozone/platform/dri/hardware_cursor_delegate.h"
 
 namespace ui {
 
-DriCursor::DriCursor(HardwareCursorDelegate* hardware) : hardware_(hardware) {
-  // TODO(dnicoara) Assume the first widget since at this point there are no
-  // widgets initialized.
-  cursor_window_ = DriSurfaceFactory::kDefaultWidgetHandle;
-  cursor_location_ = gfx::PointF(2560 / 2, 1700 / 2);  // TODO(spang): Argh!
+DriCursor::DriCursor(HardwareCursorDelegate* hardware,
+                     DriWindowManager* window_manager)
+    : hardware_(hardware),
+      window_manager_(window_manager),
+      cursor_widget_(gfx::kNullAcceleratedWidget),
+      cursor_window_(NULL) {
 }
 
 DriCursor::~DriCursor() {
 }
 
 void DriCursor::SetCursor(gfx::AcceleratedWidget widget,
                           PlatformCursor platform_cursor) {
   scoped_refptr<BitmapCursorOzone> cursor =
       BitmapCursorFactoryOzone::GetBitmapCursor(platform_cursor);
-  if (cursor_ == cursor || cursor_window_ != widget)
+  if (cursor_ == cursor || cursor_widget_ != widget)
     return;
 
   cursor_ = cursor;
   ShowCursor();
 }
 
 void DriCursor::ShowCursor() {
+  DCHECK_NE(cursor_widget_, gfx::kNullAcceleratedWidget);
    if (cursor_.get())
-    hardware_->SetHardwareCursor(cursor_window_,
+    hardware_->SetHardwareCursor(cursor_widget_,
                                  cursor_->bitmaps(),
                                  bitmap_location(),
                                  cursor_->frame_delay_ms());
   else
     HideCursor();
 }
 
 void DriCursor::HideCursor() {
+  DCHECK_NE(cursor_widget_, gfx::kNullAcceleratedWidget);
   hardware_->SetHardwareCursor(
-      cursor_window_, std::vector<SkBitmap>(), gfx::Point(), 0);
+      cursor_widget_, std::vector<SkBitmap>(), gfx::Point(), 0);
 }
 
 void DriCursor::MoveCursorTo(gfx::AcceleratedWidget widget,
                              const gfx::PointF& location) {
-  if (widget != cursor_window_)
-    HideCursor();
+  DCHECK_NE(widget, gfx::kNullAcceleratedWidget);
+  if (widget != cursor_widget_) {
+    if (cursor_widget_ != gfx::kNullAcceleratedWidget)

[spang, 2014/09/12 19:28:39]

Can you move this into HideCursor and lose the DCHECK?

[dnicoara, 2014/09/12 20:24:17]

I'd prefer to keep this in here since it is only needed in the initialization
process.

It should never be the NullAcceleratedWidget.

+      HideCursor();
 
-  cursor_window_ = widget;
+    cursor_widget_ = widget;

[spang, 2014/09/12 19:28:39]

There's no reason for these assignments to be done conditionally. Assigning
everything is simpler.

[dnicoara, 2014/09/12 20:24:17]

I felt like cursor_widget_ and cursor_window_ should only be modified at the
same time to make it clearer that they are related. And I didn't want to ask the
window_manager_ for the window every time.

+    cursor_window_ = window_manager_->GetWindow(widget);

[spang, 2014/09/12 19:28:39]

How do you ensure this ptr doesn't become dangling?

[dnicoara, 2014/09/12 20:24:17]

Setting/moving the cursor on an invalid widget is erroneous. Currently the
cursor is warped to another window when the active WTH is deleted. But I do
understand the paranoia. Would you prefer to get the window every time this
function is called? Or just (D)CHECK that?

[spang, 2014/09/12 21:05:47]

What if you delete the window under the cursor, while moving the mouse? You
can't say that evdev is doing erroneous things when it says move cursor 2px
right. It has no concept of windows.

We should also gracefully handle the case where there simply aren't any open
windows.

[dnicoara, 2014/09/12 21:24:52]

Can't happen at the same time since all the changes are done on the UI thread in
the browser.
I think this case is unreasonable. In fact I've had to adjust the way DriCursor
is initialized since everything assumes the cursor is displayed on the first
display (by default).

+  }
+
   cursor_location_ = location;
 
-  gfx::Size size = gfx::Size(2560, 1700);  // TODO(spang): Fix.
+  const gfx::Size& size = cursor_window_->GetBounds().size();

[spang, 2014/09/12 19:28:39]

gfx::Size size =

   cursor_location_.SetToMax(gfx::PointF(0, 0));
   cursor_location_.SetToMin(gfx::PointF(size.width(), size.height()));
 
   if (cursor_.get())
-    hardware_->MoveHardwareCursor(cursor_window_, bitmap_location());
+    hardware_->MoveHardwareCursor(cursor_widget_, bitmap_location());
 }
 
 void DriCursor::MoveCursor(const gfx::Vector2dF& delta) {
-  MoveCursorTo(cursor_window_, cursor_location_ + delta);
+  MoveCursorTo(cursor_widget_, cursor_location_ + delta);
 }
 
 gfx::AcceleratedWidget DriCursor::GetCursorWindow() {
-  return cursor_window_;
+  return cursor_widget_;
 }
 
 bool DriCursor::IsCursorVisible() {
   return cursor_.get();
 }
 
 gfx::PointF DriCursor::location() {
   return cursor_location_;
 }
 
 gfx::Point DriCursor::bitmap_location() {
   return gfx::ToFlooredPoint(cursor_location_) -
          cursor_->hotspot().OffsetFromOrigin();
 }
 
 }  // namespace ui