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 869fdf24958af20bbff24f64720e0cae4f5bd3f5..858ca08fbc9c63870cb85a5008e4793b2bcb6768 100644
--- a/pdf/out_of_process_instance.cc
+++ b/pdf/out_of_process_instance.cc
@@ -63,6 +63,11 @@ const char kJSViewportType[] = "viewport";
 const char kJSXOffset[] = "xOffset";
 const char kJSYOffset[] = "yOffset";
 const char kJSZoom[] = "zoom";
+const char kJSRender[] = "render";
+const char kJSPx[] = "px";
+const char kJSPy[] = "py";

[bokan, 2016/04/21 18:20:17]

more descriptive name please

[alessandroa, 2016/04/22 14:33:04]

Acknowledged.

+const char kJSPinchVectorX[] = "pinchVectorX";

[bokan, 2016/04/21 18:20:17]

add a comment to say what pinch vector is (i.e. the amount of panning caused by
the pinch gesture)

[alessandroa, 2016/04/22 14:33:05]

Done.

+const char kJSPinchVectorY[] = "pinchVectorY";
 // Stop scrolling message (Page -> Plugin)
 const char kJSStopScrollingType[] = "stopScrolling";
 // Document dimension arguments (Plugin -> Page).
@@ -273,6 +278,9 @@ OutOfProcessInstance::OutOfProcessInstance(PP_Instance instance)
       pp::Printing_Dev(this),
       cursor_(PP_CURSORTYPE_POINTER),
       zoom_(1.0),
+      do_render_(1),
+      last_current_scroll_(0,0),

[bokan, 2016/04/21 18:20:17]

nit: space after comma: (0,0) -> (0, 0)

[alessandroa, 2016/04/22 14:33:05]

Done.

+      was_smaller_(false),
       device_scale_(1.0),
       full_(false),
       paint_manager_(this, this, true),
@@ -288,7 +296,7 @@ OutOfProcessInstance::OutOfProcessInstance(PP_Instance instance)
       did_call_start_loading_(false),
       stop_scrolling_(false),
       background_color_(0),
-      top_toolbar_height_(0) {
+      top_toolbar_height_(0){
   loader_factory_.Initialize(this);
   timer_factory_.Initialize(this);
   form_factory_.Initialize(this);
@@ -386,20 +394,91 @@ void OutOfProcessInstance::HandleMessage(const pp::Var& message) {
   }
 
   std::string type = dict.Get(kType).AsString();
-
   if (type == kJSViewportType &&
       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()) {
     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 vector that computes fingers panning.

[bokan, 2016/04/21 18:20:17]

These kinds of comments belong where kJSPinchVector is declared. The comment
should also be more detailed. "Is the panning caused due to change in pinch
center between start and end of the gesture". Also, what coordinate space is it
in? (it's in frame coords here) Everywhere you have vectors/points you should be
sure to document either in comments or, even better, in the name, what the
coordinates are.

[alessandroa, 2016/04/22 14:33:05]

Acknowledged.

+    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 we have the grey parts we will use paint_offset to anchor the paint

[wjmaclean, 2016/04/21 14:11:02]

"have the grey parts" -> "the rendered document doesn't fill the display area"
(someday we might change the background colour to something other than grey)

[alessandroa, 2016/04/22 14:33:05]

Done.

+    // 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.

[wjmaclean, 2016/04/21 14:11:02]

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

[bokan, 2016/04/21 18:20:17]

+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).

+    if (!do_render && do_render_) {

[bokan, 2016/04/21 18:20:17]

We can also get into this block if we switch from pinch in to pinch out during a
gesture, no?

+      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());

[bokan, 2016/04/21 18:20:17]

I don't understand what we're doing here. Why are we centering vertically?

[alessandroa, 2016/04/22 14:33:05]

We anchor the paint on the Oy axis and after that we apply the scroll delta.

[bokan, 2016/04/22 15:45:35]

Acknowledged.

+      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_) {

[wjmaclean, 2016/04/21 14:11:02]

space after 'if'

[alessandroa, 2016/04/22 14:33:05]

Done.

+        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.

[wjmaclean, 2016/04/21 14:11:01]

See comment about tracking pinch state above ...

[bokan, 2016/04/21 18:20:17]

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

+    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.

[wjmaclean, 2016/04/21 14:11:01]

PinchEnd state.

+    if (do_render && !do_render_) {
+      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_);
@@ -568,7 +647,6 @@ void OutOfProcessInstance::DidChangeView(const pp::View& view) {
     plugin_size_ = view_device_size;
 
     paint_manager_.SetSize(view_device_size, device_scale_);
-

[wjmaclean, 2016/04/21 14:11:02]

Undo white-space changes.

     pp::Size new_image_data_size = PaintManager::GetNewContextSize(
         image_data_.size(),
         plugin_size_);
@@ -697,7 +775,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();
@@ -717,6 +795,7 @@ void OutOfProcessInstance::OnPaint(
       std::vector<pp::Rect> pdf_ready;
       std::vector<pp::Rect> pdf_pending;
       engine_->Paint(pdf_rect, &image_data_, &pdf_ready, &pdf_pending);
+

[wjmaclean, 2016/04/21 14:11:01]

Ditto.

       for (auto& ready_rect : pdf_ready) {
         ready_rect.Offset(available_area_.point());
         ready->push_back(
@@ -864,8 +943,9 @@ void OutOfProcessInstance::Invalidate(const pp::Rect& rect) {
 }
 
 void OutOfProcessInstance::Scroll(const pp::Point& point) {
-  if (!image_data_.is_null())
+  if (!image_data_.is_null()) {

[wjmaclean, 2016/04/21 14:11:02]

You don't need the curly braces here. I assume this crept in during development,
but it should be restored to its original state.

[alessandroa, 2016/04/22 14:33:05]

Done.

     paint_manager_.ScrollRect(available_area_, point);
+  }
 }
 
 void OutOfProcessInstance::ScrollToX(int x) {
@@ -1312,7 +1392,7 @@ void OutOfProcessInstance::OnGeometryChanged(double old_zoom,
   available_area_ = pp::Rect(plugin_size_);
   int doc_width = GetDocumentPixelWidth();
   if (doc_width < available_area_.width()) {
-    available_area_.Offset((available_area_.width() - doc_width) / 2, 0);
+    available_area_.Offset((available_area_.width() - doc_width) / 2 , 0);

[wjmaclean, 2016/04/21 14:11:01]

Undo this change.

[alessandroa, 2016/04/22 14:33:05]

Done.

     available_area_.set_width(doc_width);
   }
   int bottom_of_document =
@@ -1367,6 +1447,7 @@ pp::URLLoader OutOfProcessInstance::CreateURLLoaderInternal() {
 
 void OutOfProcessInstance::SetZoom(double scale) {
   double old_zoom = zoom_;
+

[wjmaclean, 2016/04/21 14:11:01]

Undo.

[alessandroa, 2016/04/22 14:33:04]

Done.

   zoom_ = scale;
   OnGeometryChanged(old_zoom, device_scale_);
 }