Image Search: Move thumbnail JPEG encoding to the renderer.

chrome/renderer/chrome_render_frame_observer.cc

Index: chrome/renderer/chrome_render_frame_observer.cc
diff --git a/chrome/renderer/chrome_render_frame_observer.cc b/chrome/renderer/chrome_render_frame_observer.cc
index 2a9b594a734e40d6cf846592e82fbc400598e211..e49103aec6e1716551afba331e369cf28d767b6d 100644
--- a/chrome/renderer/chrome_render_frame_observer.cc
+++ b/chrome/renderer/chrome_render_frame_observer.cc
@@ -4,6 +4,10 @@
 
 #include "chrome/renderer/chrome_render_frame_observer.h"
 
+#include <limits>
+#include <string>
+#include <vector>
+
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/common/prerender_messages.h"
 #include "chrome/common/print_messages.h"
@@ -12,10 +16,11 @@
 #include "chrome/renderer/printing/print_web_view_helper.h"
 #include "content/public/renderer/render_frame.h"
 #include "skia/ext/image_operations.h"
-#include "skia/ext/platform_canvas.h"
 #include "third_party/WebKit/public/platform/WebImage.h"
 #include "third_party/WebKit/public/web/WebElement.h"
 #include "third_party/WebKit/public/web/WebNode.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/gfx/codec/jpeg_codec.h"
 
 using blink::WebElement;
 using blink::WebNode;
@@ -117,8 +122,31 @@ void ChromeRenderFrameObserver::OnRequestThumbnailForContextNode(
                           thumbnail_min_area_pixels,
                           thumbnail_max_size_pixels);
   }
+
+  SkBitmap bitmap;
+  if (thumbnail.colorType() == kN32_SkColorType) {

[Peter Kasting, 2014/12/11 20:11:30]

Nit: {} unnecessary

[jbroman, 2014/12/12 00:08:57]

Done.

+    bitmap = thumbnail;
+  } else {
+    thumbnail.copyTo(&bitmap, kN32_SkColorType);
+  }
+
+  std::string thumbnail_data;
+  {
+    SkAutoLockPixels lock(bitmap);

[Peter Kasting, 2014/12/11 20:11:30]

Is it important that this lock be released before the Send() call below?  If
not, I'd avoid placing this in a new block scope.  If so, I'd add a comment as
to why.

[jbroman, 2014/12/12 00:08:57]

It's not important, no. There was just no reason for it to continue to be valid
during Send. Changed.

+    if (bitmap.getPixels() != nullptr &&

[Peter Kasting, 2014/12/11 20:11:30]

Nit: "!= nullptr" unnecessary

[jbroman, 2014/12/12 00:08:57]

Done.

I had some vague idea that Chromium style preferred this, but I can find no
supporting evidence. :D

+        static_cast<int>(bitmap.rowBytes()) > 0) {

[Peter Kasting, 2014/12/11 20:11:30]

Why is this cast to int necessary?

[jbroman, 2014/12/12 00:08:57]

The rowBytes argument to gfx::JPEGCodec::Encode is an int, but
SkBitmap::rowBytes returns a size_t.

Other call sites seem to use a cast in the call (but I don't remember whether
this is necessary with our warning flags on various platforms). I suppose I can
remove this check (it doesn't seem to be done elsewhere), though I'm not certain
whether it's actually impossible to have rowBytes between INT_MAX and SIZE_MAX.

[Peter Kasting, 2014/12/12 00:35:51]

I see.  You need some kind of cast here, then.  The question is whether it
should be a static_cast or a checked_cast.  A checked_cast will terminate
execution (basically CHECK(false)) if the source value doesn't fit in the
destination type.  We use that when we aren't sure the result will succeed, and
it might be a security issue if the cast were to pass the wrong value.

I think in this case a static_cast is OK, since I think it would just result in
the JPEG encoder not reading all the data, but you should make sure of my
reasoning.

[jbroman, 2014/12/12 01:10:46]

If this were to be negative after a cast, it would result in JPEGCodec reading
before the beginning of the data (it does computes a row's address as
&input[next_scanline * row_byte_width).

But Skia does say where SkBitmap::fRowBytes is set that "require[s] that
rowBytes fit in 31bits", so I suppose this assumption is reasonable (and we have
it elsewhere).

So I'm inclined to leave this as-is unless someone has another preference.

+      const int kDefaultQuality = 90;
+      std::vector<unsigned char> data;
+      if (gfx::JPEGCodec::Encode(
+              reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)),
+              gfx::JPEGCodec::FORMAT_SkBitmap, bitmap.width(), bitmap.height(),
+              static_cast<int>(bitmap.rowBytes()), kDefaultQuality, &data))
+        thumbnail_data = std::string(data.begin(), data.end());
+    }
+  }
+
   Send(new ChromeViewHostMsg_RequestThumbnailForContextNode_ACK(
-      routing_id(), thumbnail, original_size));
+      routing_id(), thumbnail_data, original_size));
 }
 
 void ChromeRenderFrameObserver::OnPrintNodeUnderContextMenu() {