Implements core logic for Pixel Canvas

ui/views/paint_info.cc

+// Copyright (c) 2017 The Chromium Authors. All rights reserved.

[sky, 2017/07/26 22:12:11]

no (c)

[malaykeshav, 2017/07/28 01:24:37]

Done

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/views/paint_info.h"
+
+namespace views {
+
+PaintInfo::PaintInfo(const ui::PaintContext& context, const gfx::Size& size)
+    : paint_recording_scale_x_(
+          context.is_pixel_canvas() ? context.device_scale_factor() : 1.f),
+      paint_recording_scale_y_(paint_recording_scale_x_),
+      paint_recording_bounds_(
+          gfx::ScaleToRoundedRect(gfx::Rect(size), paint_recording_scale_x_)),
+      context_(context, gfx::Vector2d()),
+      root_context_(&context) {}
+
+PaintInfo::PaintInfo(const PaintInfo& other,
+                     const gfx::Rect& bounds,
+                     const gfx::Rect& parent_bounds,
+                     ScaleType scale_type)
+    : paint_recording_bounds_(

[sky, 2017/07/26 22:12:11]

Make sure to member initialize paint_recording_scale_x_/y_. I realize you reset
them in ComputePaintRecordingScales, but it it's easy to miss that and if an
early return is inserted the value go uninitialized.

[malaykeshav, 2017/07/28 01:24:37]

Done

+          other.GetSnappedRecordingBounds(parent_bounds, bounds)),
+      offset_from_parent_(paint_recording_bounds_.OffsetFromOrigin() -
+                          other.paint_recording_bounds_.OffsetFromOrigin()),
+      context_(other.context(), offset_from_parent_),
+      root_context_(nullptr) {

[sky, 2017/07/26 22:12:11]

How come this doesn't set root_context_? I think the problem with these two
constructors is they are intended for different uses and it isn't clear
immediately clear which to use and when. Factory functions would clear that up.

[malaykeshav, 2017/07/28 01:24:37]

Done

+  ComputePaintRecordingScales(bounds.size(), scale_type);
+}
+
+PaintInfo::PaintInfo(const PaintInfo& other)
+    : paint_recording_scale_x_(other.paint_recording_scale_x_),
+      paint_recording_scale_y_(other.paint_recording_scale_y_),
+      paint_recording_bounds_(other.paint_recording_bounds_),
+      offset_from_parent_(other.offset_from_parent_),
+      context_(other.context(), ui::PaintContext::CLONE_WITHOUT_INVALIDATION),
+      root_context_(nullptr) {}
+
+PaintInfo::~PaintInfo() {}
+
+bool PaintInfo::IsPixelCanvas() const {
+  return context().is_pixel_canvas();
+}
+
+void PaintInfo::ComputePaintRecordingScales(const gfx::Size& size,
+                                            ScaleType scale_type) {
+  if (!IsPixelCanvas()) {
+    // If pixel canvas is disabled, all recordings are done in DIPs. These are
+    // later played back on to the raster canvas at |device_scale_factor_|.
+    paint_recording_scale_x_ = paint_recording_scale_y_ = 1.f;
+    return;
+  }
+
+  paint_recording_scale_x_ = paint_recording_scale_y_ =
+      context().device_scale_factor();
+  if (scale_type == ScaleType::kScaleToScaleFactor)
+    return;
+
+  if (size.width() > 0) {
+    paint_recording_scale_x_ =
+        static_cast<float>(paint_recording_bounds_.width()) /
+        static_cast<float>(size.width());
+  }
+
+  if (size.height() > 0) {
+    paint_recording_scale_y_ =
+        static_cast<float>(paint_recording_bounds_.height()) /
+        static_cast<float>(size.height());
+  }
+}
+
+gfx::Rect PaintInfo::GetSnappedRecordingBounds(
+    const gfx::Rect& parent_bounds,

[sky, 2017/07/26 22:12:11]

Don't the coordinate system of the two rects differ? I suspect parent_bounds
should really be parent_size, or am I misunderstanding what this is?

[malaykeshav, 2017/07/28 01:24:37]

Yes. parent_bounds are not needed. parent_size is enough.
Done.

+    const gfx::Rect& child_bounds) const {
+  if (!IsPixelCanvas())
+    return (child_bounds + paint_recording_bounds_.OffsetFromOrigin());
+
+  const gfx::Vector2d& child_origin = child_bounds.OffsetFromOrigin();
+
+  int right = child_origin.x() + child_bounds.width();
+  int bottom = child_origin.y() + child_bounds.height();
+
+  int new_x = std::round(child_origin.x() * context().device_scale_factor());
+  int new_y = std::round(child_origin.y() * context().device_scale_factor());
+
+  int new_right;
+  int new_bottom;
+
+  if (right == parent_bounds.width()) {

[sky, 2017/07/26 22:12:11]

no {}

[sky, 2017/07/26 22:12:11]

Might rounding give you a size > parent_bounds.width()? Are you sure you don't
need >= ? Same comment below.

[malaykeshav, 2017/07/28 01:24:37]

Both the values are in DIP, so there should be no problem with rounding.

[malaykeshav, 2017/07/28 01:24:37]

Done.

[sky, 2017/07/28 16:02:11]

Actually, I'm a little confused by why you need to special case this. Please add
a comment. Also, remember that there is nothing stopping a child view from going
outside the bounds of it's parent.

+    new_right = paint_recording_bounds_.width();
+  } else {
+    new_right = std::round(right * context().device_scale_factor());
+  }
+
+  if (bottom == parent_bounds.height()) {
+    new_bottom = paint_recording_bounds_.height();
+  } else {
+    new_bottom = std::round(bottom * context().device_scale_factor());
+  }
+
+  return gfx::Rect(new_x + paint_recording_bounds_.x(),
+                   new_y + paint_recording_bounds_.y(), new_right - new_x,
+                   new_bottom - new_y);
+}
+
+}  // namespace views