Improved Pinch-Zoom for PDF

pdf/out_of_process_instance.cc

 // Copyright (c) 2012 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 "pdf/out_of_process_instance.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>  // for min/max()
@@ skipping 45 matching lines @@
 const char kAccessibleLoaded[] = "loaded";
 const char kAccessibleCopyable[] = "copyable";
 
 // Constants used in handling postMessage() messages.
 const char kType[] = "type";
 // Viewport message arguments. (Page -> Plugin).
 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).
 const char kJSDocumentDimensionsType[] = "documentDimensions";
 const char kJSDocumentWidth[] = "width";
 const char kJSDocumentHeight[] = "height";
 const char kJSPageDimensions[] = "pageDimensions";
 const char kJSPageX[] = "x";
 const char kJSPageY[] = "y";
 const char kJSPageWidth[] = "width";
@@ skipping 190 matching lines @@
 }
 
 }  // namespace
 
 OutOfProcessInstance::OutOfProcessInstance(PP_Instance instance)
     : pp::Instance(instance),
       pp::Find_Private(this),
       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),
       first_paint_(true),
       document_load_state_(LOAD_STATE_LOADING),
       preview_document_load_state_(LOAD_STATE_COMPLETE),
       uma_(this),
       told_browser_about_unsupported_feature_(false),
       print_preview_page_count_(0),
       last_progress_sent_(0),
       recently_sent_find_update_(false),
       received_viewport_message_(false),
       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);
   print_callback_factory_.Initialize(this);
   engine_.reset(PDFEngine::Create(this));
   pp::Module::Get()->AddPluginInterface(kPPPPdfInterface, &ppp_private);
   AddPerInstanceObject(kPPPPdfInterface, this);
 
   RequestFilteringInputEvents(PP_INPUTEVENT_CLASS_MOUSE);
   RequestFilteringInputEvents(PP_INPUTEVENT_CLASS_KEYBOARD);
@@ skipping 77 matching lines @@
 }
 
 void OutOfProcessInstance::HandleMessage(const pp::Var& message) {
   pp::VarDictionary dict(message);
   if (!dict.Get(kType).is_string()) {
     NOTREACHED();
     return;
   }
 
   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_);
   } else if (type == kJSGetPasswordCompleteType &&
              dict.Get(pp::Var(kJSPassword)).is_string()) {
     if (password_callback_) {
       pp::CompletionCallbackWithOutput<pp::Var> callback = *password_callback_;
       password_callback_.reset();
       *callback.output() = dict.Get(pp::Var(kJSPassword)).pp_var();
       callback.Run(PP_OK);
@@ skipping 148 matching lines @@
   float device_scale = view.GetDeviceScale();
   pp::Size view_device_size(view_rect.width() * device_scale,
                             view_rect.height() * device_scale);
 
   if (view_device_size != plugin_size_ || device_scale != device_scale_) {
     device_scale_ = device_scale;
     plugin_dip_size_ = view_rect.size();
     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_);
     if (new_image_data_size != image_data_.size()) {
       image_data_ = pp::ImageData(this,
                                   PP_IMAGEDATAFORMAT_BGRA_PREMUL,
                                   new_image_data_size,
                                   false);
       first_paint_ = true;
     }
@@ skipping 108 matching lines @@
     DCHECK(plugin_size_.IsEmpty());
     return;
   }
   if (first_paint_) {
     first_paint_ = false;
     pp::Rect rect = pp::Rect(pp::Point(), image_data_.size());
     FillRect(rect, background_color_);
     ready->push_back(PaintManager::ReadyRect(rect, image_data_, true));
   }
 
-  if (!received_viewport_message_)
+  if (!received_viewport_message_ || !do_render_)
     return;
 
   engine_->PrePaint();
 
   for (const auto& paint_rect : paint_rects) {
     // Intersect with plugin area since there could be pending invalidates from
     // when the plugin area was larger.
     pp::Rect rect =
         paint_rect.Intersect(pp::Rect(pp::Point(), plugin_size_));
     if (rect.IsEmpty())
       continue;
 
     pp::Rect pdf_rect = available_area_.Intersect(rect);
     if (!pdf_rect.IsEmpty()) {
       pdf_rect.Offset(available_area_.x() * -1, 0);
 
       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(
             PaintManager::ReadyRect(ready_rect, image_data_, false));
       }
       for (auto& pending_rect : pdf_pending) {
         pending_rect.Offset(available_area_.point());
         pending->push_back(pending_rect);
       }
     }
@@ skipping 127 matching lines @@
   OnGeometryChanged(zoom_, device_scale_);
 }
 
 void OutOfProcessInstance::Invalidate(const pp::Rect& rect) {
   pp::Rect offset_rect(rect);
   offset_rect.Offset(available_area_.point());
   paint_manager_.InvalidateRect(offset_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) {
   pp::VarDictionary position;
   position.Set(kType, kJSSetScrollPositionType);
   position.Set(kJSPositionX, pp::Var(x / device_scale_));
   PostMessage(position);
 }
 
 void OutOfProcessInstance::ScrollToY(int y) {
@@ skipping 426 matching lines @@
 }
 
 void OutOfProcessInstance::OnGeometryChanged(double old_zoom,
                                              float old_device_scale) {
   if (zoom_ != old_zoom || device_scale_ != old_device_scale)
     engine_->ZoomUpdated(zoom_ * device_scale_);
 
   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 =
       GetDocumentPixelHeight() + (top_toolbar_height_ * device_scale_);
   if (bottom_of_document < available_area_.height())
     available_area_.set_height(bottom_of_document);
 
   CalculateBackgroundParts();
   engine_->PageOffsetUpdated(available_area_.point());
   engine_->PluginSizeUpdated(available_area_.size());
@@ skipping 34 matching lines @@
       reinterpret_cast<const PPB_URLLoaderTrusted*>(
           pp::Module::Get()->GetBrowserInterface(
               PPB_URLLOADERTRUSTED_INTERFACE));
   if (trusted_interface)
     trusted_interface->GrantUniversalAccess(loader.pp_resource());
   return loader;
 }
 
 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_);
 }
 
 void OutOfProcessInstance::AppendBlankPrintPreviewPages() {
   if (print_preview_page_count_ == 0)
     return;
   engine_->AppendBlankPages(print_preview_page_count_);
   if (!preview_pages_info_.empty())
     LoadAvailablePreviewPage();
@@ skipping 56 matching lines @@
     const pp::FloatPoint& scroll_offset) {
   float max_x = document_size_.width() * zoom_ - plugin_dip_size_.width();
   float x = std::max(std::min(scroll_offset.x(), max_x), 0.0f);
   float min_y = -top_toolbar_height_;
   float max_y = document_size_.height() * zoom_ - plugin_dip_size_.height();
   float y = std::max(std::min(scroll_offset.y(), max_y), min_y);
   return pp::FloatPoint(x, y);
 }
 
 }  // namespace chrome_pdf