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 38 matching lines @@
 const char kChromeExtension[] =
     "chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai";
 
 // 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";
+// 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).
 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 198 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_(true),
+      last_current_scroll_(0, 0),
+      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),
@@ skipping 97 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 &&

[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_);
   } 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 126 matching lines @@
   float old_device_scale = device_scale_;
   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_);

[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_);
     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 215 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())
@@ skipping 732 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