Improved Pinch-Zoom for PDF.

pdf/out_of_process_instance.cc

Index: pdf/out_of_process_instance.cc
diff --git a/pdf/out_of_process_instance.cc b/pdf/out_of_process_instance.cc
index 92511185c9715dd610adc6353cae440616666284..c26d32666bb618382c45c30d55220e0577816e61 100644
--- a/pdf/out_of_process_instance.cc
+++ b/pdf/out_of_process_instance.cc
@@ -56,6 +56,13 @@ const char kJSViewportType[] = "viewport";
 const char kJSXOffset[] = "xOffset";
 const char kJSYOffset[] = "yOffset";
 const char kJSZoom[] = "zoom";
+const char kJSRender[] = "render";
+// kJSPx and kJSPy represent the center of the pinch gesture.
+const char kJSPx[] = "px";
+const char kJSPy[] = "py";

[Kevin McNee - google account, 2016/10/06 21:53:17]

Comment from previous review:
bokan: more descriptive name please

[Kevin McNee - google account, 2016/10/13 18:19:24]

Done.

+// kJSPinchVector represents the amount of panning caused by the pinch gesture.
+const char kJSPinchVectorX[] = "pinchVectorX";
+const char kJSPinchVectorY[] = "pinchVectorY";
 // Stop scrolling message (Page -> Plugin)
 const char kJSStopScrollingType[] = "stopScrolling";
 // Document dimension arguments (Plugin -> Page).
@@ -274,6 +281,9 @@ OutOfProcessInstance::OutOfProcessInstance(PP_Instance instance)
       pp::Printing_Dev(this),
       cursor_(PP_CURSORTYPE_POINTER),
       zoom_(1.0),
+      do_render_(true),
+      last_current_scroll_(0, 0),
+      was_smaller_(false),
       device_scale_(1.0),
       full_(false),
       paint_manager_(this, this, true),
@@ -391,20 +401,93 @@ void OutOfProcessInstance::HandleMessage(const pp::Var& message) {
   }
 
   std::string type = dict.Get(kType).AsString();
-
   if (type == kJSViewportType &&

[Kevin McNee - google account, 2016/10/06 21:53:17]

Unnecessary whitespace change.

[Kevin McNee - google account, 2016/10/13 18:19:24]

Done.

       dict.Get(pp::Var(kJSXOffset)).is_number() &&
       dict.Get(pp::Var(kJSYOffset)).is_number() &&
-      dict.Get(pp::Var(kJSZoom)).is_number()) {
+      dict.Get(pp::Var(kJSZoom)).is_number() &&
+      dict.Get(pp::Var(kJSRender)).is_bool() &&
+      dict.Get(pp::Var(kJSPx)).is_number() &&
+      dict.Get(pp::Var(kJSPy)).is_number()) {

[Kevin McNee - google account, 2016/10/13 18:04:02]

Missing check for kJSPinchVectorX and kJSPinchVectorY.

[Kevin McNee - google account, 2016/10/13 18:19:24]

Done.

     received_viewport_message_ = true;
     stop_scrolling_ = false;
+    bool do_render = dict.Get(pp::Var(kJSRender)).AsBool();
     double zoom = dict.Get(pp::Var(kJSZoom)).AsDouble();
+    double zoom_delta = zoom / zoom_;
+
+    // Pinch vector is the panning caused due to change in pinch
+    // center between start and end of the gesture.
+    pp::Point pinch_vector =
+        pp::Point(dict.Get(kJSPinchVectorX).AsDouble() * zoom_delta,
+                  dict.Get(kJSPinchVectorY).AsDouble() * zoom_delta);
     pp::FloatPoint scroll_offset(dict.Get(pp::Var(kJSXOffset)).AsDouble(),
                                  dict.Get(pp::Var(kJSYOffset)).AsDouble());
+    pp::Point pinch_center(dict.Get(pp::Var(kJSPx)).AsDouble(),
+                           dict.Get(pp::Var(kJSPy)).AsDouble());
+    pp::Point scroll_delta(0,0);
+
+    // If the rendered document doesn't fill the display area we will
+    // use paint_offset to anchor the paint vertically into the same place. We
+    // use the scroll bars instead of the pinch vector to get the actual
+    // position on screen of the paint.
+    pp::Point paint_offset(0,0);
+
+    // Pinch start.

[Kevin McNee - google account, 2016/10/06 21:53:18]

Comments from previous review:
wjmaclean: Is it worth having an enum like "enum PinchPhase { PinchStart,
PinchUpdate, PinchEnd}" to make tracking of current phase more explicit?

bokan: +1. In fact, I would argue you should be passing that as an arg into this
class rather than doRender and we should figure out the state of doRender in
here based on the pinch phase. (And you may want to split up update into
PinchInUpdate and PinchOutUpdate since behavior seems to differ based on the two
cases).

[wjmaclean, 2016/10/07 12:30:21]

+1 to bokan's comments here. If we split into two functions though, we need to
make sure that transitioning between them mid-gesture is handled cleanly (i.e.
from a code-readability point of view).

[Kevin McNee - google account, 2016/10/13 18:19:24]

Done.

+    if (!do_render && do_render_) {

[Kevin McNee - google account, 2016/10/06 21:53:17]

Comment from previous review:
bokan: We can also get into this block if we switch from pinch in to pinch out
during a gesture, no?
	
bokan: We should replace these with an actual state enum or something of the
like. Also, can't we get in here if we switch from pinch-in (do_render_ == true)
to pinch-out (!do_render) in one gesture? In that case, it's not really a
pinch-start.

[wjmaclean, 2016/10/07 12:30:21]

Presumably that's true. I guess the re-raster status is most important at the
*end* of the gesture, when we release.

[Kevin McNee - google account, 2016/10/13 18:19:24]

Done.

+      last_current_scroll_ = scroll_offset;
+      old_zoom_ = zoom;
+      initial_zoom_delta_ = zoom / zoom_;
+      was_smaller_ = false;
+      do_render_ = false;
+      return;
+    }
+
+    if (!do_render &&
+        plugin_size_.width() > GetDocumentPixelWidth() * zoom_delta) {
+      // We want to keep the paint in the middle but it must stay in the same
+      // position relative to the scroll bars.
+      paint_offset = pp::Point(0, (1 - zoom_delta) * pinch_center.y());
+      scroll_delta = pp::Point(
+          0, (scroll_offset.y() -
+              last_current_scroll_.y() * zoom_delta / initial_zoom_delta_));
+
+      pinch_vector = pp::Point();
+      old_zoom_ = zoom;
+      was_smaller_ = true;
+    } else {
+      if (was_smaller_) {
+        pinch_center = pp::Point((plugin_size_.width() / device_scale_) / 2,
+                                 (plugin_size_.height() / device_scale_) / 2);
+        paint_offset = pp::Point((1 - zoom / old_zoom_) * pinch_center.x(),
+                                 (1 - zoom_delta) * pinch_center.y());
+        pinch_vector = pp::Point(0, 0);
+        scroll_delta = pp::Point(
+            (scroll_offset.x() -
+             last_current_scroll_.x() * zoom_delta / initial_zoom_delta_),
+            (scroll_offset.y() -
+             last_current_scroll_.y() * zoom_delta / initial_zoom_delta_));
+      }
+    }
+
+    // While pinching.

[Kevin McNee - google account, 2016/10/06 21:53:18]

Comments from previous review:	
wjmaclean: See comment about tracking pinch state above ...

bokan: The if ... else branch above is also part of "while pinching" right?

bokan: I think the above if..else is part of "While Pinching" also.

[Kevin McNee - google account, 2016/10/13 18:19:24]

Done.

+    if (!do_render) {
+      paint_manager_.SetTransform(zoom_delta, pinch_center,
+                              pinch_vector + paint_offset + scroll_delta);
+      do_render_ = do_render;
+      return;
+    }
+
+    // On pinch end the scale is again 1.f and we request a render in the new
+    // position.
+    if (do_render && !do_render_) {

[Kevin McNee - google account, 2016/10/06 21:53:18]

Comment from previous review:
wjmaclean: PinchEnd state.

[Kevin McNee - google account, 2016/10/13 18:19:24]

Done.

+      paint_manager_.SetTransform(1.f);
+      was_smaller_ = false;
+    }
 
     // Bound the input parameters.
     zoom = std::max(kMinZoom, zoom);
     SetZoom(zoom);
+
+    do_render_ = do_render;
     scroll_offset = BoundScrollOffsetToDocument(scroll_offset);
     engine_->ScrolledToXPosition(scroll_offset.x() * device_scale_);
     engine_->ScrolledToYPosition(scroll_offset.y() * device_scale_);
@@ -552,7 +635,6 @@ void OutOfProcessInstance::DidChangeView(const pp::View& view) {
     plugin_size_ = view_device_size;
 
     paint_manager_.SetSize(view_device_size, device_scale_);

[Kevin McNee - google account, 2016/10/06 21:53:17]

Unnecessary whitespace change.

[Kevin McNee - google account, 2016/10/13 18:19:24]

Done.

-
     pp::Size new_image_data_size = PaintManager::GetNewContextSize(
         image_data_.size(),
         plugin_size_);
@@ -788,7 +870,7 @@ void OutOfProcessInstance::OnPaint(
     ready->push_back(PaintManager::ReadyRect(rect, image_data_, true));
   }
 
-  if (!received_viewport_message_)
+  if (!received_viewport_message_ || !do_render_)
     return;
 
   engine_->PrePaint();