Resize images for hi-dpi based on a custom PNG chunk added by GRIT r78, and roll GRIT r78

ui/base/resource/resource_bundle.cc

Index: ui/base/resource/resource_bundle.cc
diff --git a/ui/base/resource/resource_bundle.cc b/ui/base/resource/resource_bundle.cc
index f8d214fe81f4509781175a1cebe6a0a4498ba46f..13096d55fc1cd7d84bd60549225b60a04c59a688 100644
--- a/ui/base/resource/resource_bundle.cc
+++ b/ui/base/resource/resource_bundle.cc
@@ -49,88 +49,65 @@ bool ShouldHighlightMissingScaledResources() {
 
 }  // namespace
 
-// An ImageSkiaSource that loads bitmaps for requested scale factor from
-// ResourceBundle on demand for given resource_id. It falls back
-// to the 1x bitmap if the bitmap for the requested scale factor does not
-// exist. If the resource for the requested scale factor is not exactly
-// |scale_factor| * the size of the 1x resource, it will end up with
-// broken UI because it will be drawn as if the bitmap was the correct size.
-// When --highlight-missing-scaled-resources flag is specified, it
-// will show the scaled image blended with red instead.
+// An ImageSkiaSource that loads bitmaps for the requested scale factor from
+// ResourceBundle on demand for a given |resource_id|. If the bitmap for the
+// requested scale factor does not exist, it will return the 1x bitmap scaled
+// by the scale factor. This may lead to broken UI if the correct size of the
+// scaled image is not exactly |scale_factor| * the size of the 1x resource.
+// When --highlight-missing-scaled-resources flag is specified, scaled 1x images
+// are higlighted by blending them with red.
 class ResourceBundle::ResourceBundleImageSource : public gfx::ImageSkiaSource {
  public:
-  ResourceBundleImageSource(ResourceBundle* rb,
-                            int resource_id,
-                            const gfx::Size& size_in_dip)
-      : rb_(rb),
-        resource_id_(resource_id),
-        size_in_dip_(size_in_dip) {
-  }
+  ResourceBundleImageSource(ResourceBundle* rb, int resource_id)
+      : rb_(rb), resource_id_(resource_id) {}
   virtual ~ResourceBundleImageSource() {}
 
   // gfx::ImageSkiaSource overrides:
   virtual gfx::ImageSkiaRep GetImageForScale(
       ui::ScaleFactor scale_factor) OVERRIDE {
-    scoped_ptr<SkBitmap> result(rb_->LoadBitmap(resource_id_, scale_factor));
-    float scale = ui::GetScaleFactorScale(scale_factor);
-    gfx::Size size_in_pixel = size_in_dip_.Scale(scale);
-
-    if (scale_factor != SCALE_FACTOR_100P &&
-        (!result.get() ||
-         result->width() != size_in_pixel.width() ||
-         result->height() != size_in_pixel.height())) {
-
-      // If non 1x resource is missing from |image| or is the incorrect
-      // size and --highlight-missing-scaled-resources is specified, logs
-      // the resource id and creates a version of the resource at the correct
-      // size. Blends the created resource with red to make it
-      // distinguishable from bitmaps in the resource pak.
+    SkBitmap image;
+    bool scale_fallback = false;

[sail, 2012/10/09 22:01:37]

same, naming could be better

[benrg, 2012/10/10 02:18:13]

Done.

+    bool found = rb_->LoadBitmap(resource_id_, scale_factor,
+                                 &image, &scale_fallback);
+
+    if (!found) {
+      // Some images (e.g. wallpapers) exist only at 100%. Return the unscaled
+      // image with the correct scale factor.
+      DCHECK_NE(scale_factor, SCALE_FACTOR_100P);

[sail, 2012/10/09 22:01:37]

This DCHECK says that we shouldn't be missing images at 1x scale factory.
Then in the very next line we check if the image is missing and return an empty
image.

Since the calling code will return a red image if anything is missing I think
this DCHECK is unnecessary.

[benrg, 2012/10/10 02:18:13]

You're right; rewritten.

+      if (!rb_->LoadBitmap(resource_id_, SCALE_FACTOR_100P, &image, NULL))
+        return gfx::ImageSkiaRep();
+      return gfx::ImageSkiaRep(image, SCALE_FACTOR_100P);
+    }
+
+    if (scale_fallback) {
+      // GRIT fell back to the 100% image, so rescale it to the correct size.
+      float scale = GetScaleFactorScale(scale_factor);
+      image = skia::ImageOperations::Resize(
+          image,
+          skia::ImageOperations::RESIZE_LANCZOS3,
+          static_cast<int>(image.width() * scale + 0.5f),

[sail, 2012/10/09 22:01:37]

the cast  and + 0.5 is for rounding right? can you put that in a helper
function?
even better can we move that to a util file, specially since we use this
everywhere

Hm.. this CL might be interesting:
https://chromiumcodereview.appspot.com/11081007/
It uses ToFlooredSize()

[benrg, 2012/10/10 02:18:13]

I'm not sure I even want to make a helper function for this because it is
fundamentally a hack. The nearest integer is just a guess, not necessarily the
right answer. A helper function would need a name and description implying that
it did something that made sense.

There should be a round-to-int function in util, but it's pretty hard to do
right (floorf(x+0.5) fails for x = 2^23+1 for example). I think it's beyond the
scope of this CL.

C99 and C++11 have lroundf(). It's used in a couple of files in cc/. Does that
mean it's fair game? Probably not.

[sail, 2012/10/10 03:09:55]

This is still a rounding function. Duplicated.
Even if you don't put in a public file you should still move it to a function at
the top of the file.

[benrg, 2012/10/10 21:03:56]

Done.

+          static_cast<int>(image.height() * scale + 0.5f));
+      // If --highlight-missing-scaled-resources is specified, log the resource
+      // id and blend the created resource with red.
       if (ShouldHighlightMissingScaledResources()) {
-        if (!result.get()) {
-          LOG(ERROR) << "Missing " << scale << "x resource. id="
-                     << resource_id_;
-        } else {
-          LOG(ERROR) << "Incorrectly sized " << scale << "x resource. id="
-                     << resource_id_;
-        }
-
-        scoped_ptr<SkBitmap> bitmap1x(
-            rb_->LoadBitmap(resource_id_, SCALE_FACTOR_100P));
-        DCHECK(bitmap1x.get());
-        SkBitmap bitmap_scaled = skia::ImageOperations::Resize(
-            *bitmap1x,
-            skia::ImageOperations::RESIZE_LANCZOS3,
-            size_in_pixel.width(),
-            size_in_pixel.height());
+        LOG(ERROR) << "Missing " << scale << "x scaled resource. id="
+                   << resource_id_;
 
         SkBitmap mask;
         mask.setConfig(SkBitmap::kARGB_8888_Config,
-                       bitmap_scaled.width(),
-                       bitmap_scaled.height());
+                       image.width(), image.height());
         mask.allocPixels();
         mask.eraseColor(SK_ColorRED);
-        result.reset(new SkBitmap());
-        *result.get() = SkBitmapOperations::CreateBlendedBitmap(
-            bitmap_scaled, mask, 0.2);
-      } else if (!result.get() || result->width() == size_in_dip_.width()) {
-        // The scaled resource pack may have the 1x image if its grd file
-        // points to 1x image. Fallback to 1x by returning empty image
-        // in this case. This 1x image will be scaled when drawn.
-        return gfx::ImageSkiaRep();
+        image = SkBitmapOperations::CreateBlendedBitmap(image, mask, 0.2);
       }
-      // If the size of scaled image isn't exactly |scale| * 1x version,
-      // create ImageSkia as usual. This will end up with
-      // corrupted visual representation as the size of image doesn't
-      // match the expected size.
     }
-    DCHECK(result.get());
-    return gfx::ImageSkiaRep(*result.get(), scale_factor);
+
+    return gfx::ImageSkiaRep(image, scale_factor);
   }
 
  private:
   ResourceBundle* rb_;
   const int resource_id_;
-  const gfx::Size size_in_dip_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourceBundleImageSource);
 };
@@ -344,24 +321,27 @@ gfx::Image& ResourceBundle::GetImageNamed(int resource_id) {
     DCHECK(!delegate_ && !data_packs_.empty()) <<
         "Missing call to SetResourcesDataDLL?";
 
-    // TODO(oshima): Pick the scale factor from currently used scale factors.
-    scoped_ptr<SkBitmap> bitmap(LoadBitmap(resource_id, SCALE_FACTOR_100P));
-    if (!bitmap.get()) {
+    // TODO(benrg/oshima): GetPrimaryDisplay() crashes at startup.
+    // ScaleFactor primary_scale_factor = GetScaleFactorFromScale(

[sail, 2012/10/09 22:01:37]

there's no point leaving commented out code here.
You can put  a TODO if you like and explain that the code is not ready yet due
to crash at startup

[benrg, 2012/10/10 02:18:13]

Fixed by fixing the crash (courtesy Oshima).

+    //     gfx::Screen::GetPrimaryDisplay().device_scale_factor());
+    ScaleFactor primary_scale_factor = SCALE_FACTOR_100P;
+    scoped_ptr<ResourceBundleImageSource> image_source(
+        new ResourceBundleImageSource(this, resource_id));
+    gfx::ImageSkiaRep scaled_image =
+        image_source->GetImageForScale(primary_scale_factor);
+    if (scaled_image.is_null()) {
       LOG(WARNING) << "Unable to load image with id " << resource_id;
       NOTREACHED();  // Want to assert in debug mode.
       // The load failed to retrieve the image; show a debugging red square.
       return GetEmptyImage();
     }
+    gfx::Size size_in_dip(scaled_image.GetWidth(), scaled_image.GetHeight());
 
     // ResourceBundle::GetSharedInstance() is destroyed after the
     // BrowserMainLoop has finished running. |image_skia| is guaranteed to be
     // destroyed before the resource bundle is destroyed.
-    gfx::Size size_in_dip(bitmap->width(), bitmap->height());
-    gfx::ImageSkia image_skia(
-        new ResourceBundleImageSource(this, resource_id, size_in_dip),
-        size_in_dip);
-    image_skia.AddRepresentation(gfx::ImageSkiaRep(*bitmap.get(),
-                                                   SCALE_FACTOR_100P));
+    gfx::ImageSkia image_skia(image_source.release(), size_in_dip);
+    image_skia.AddRepresentation(scaled_image);
     image_skia.SetReadOnly();
     image = gfx::Image(image_skia);
   }
@@ -589,37 +569,43 @@ void ResourceBundle::LoadFontsIfNecessary() {
   }
 }
 
-SkBitmap* ResourceBundle::LoadBitmap(const ResourceHandle& data_handle,
-                                     int resource_id) const {
+bool ResourceBundle::LoadBitmap(const ResourceHandle& data_handle,
+                                int resource_id,
+                                SkBitmap* bitmap,
+                                bool* scale_fallback) const {
   scoped_refptr<base::RefCountedMemory> memory(
       data_handle.GetStaticMemory(resource_id));
   if (!memory)
-    return NULL;
+    return false;
 
-  SkBitmap bitmap;
-  if (gfx::PNGCodec::Decode(memory->front(), memory->size(), &bitmap))
-    return new SkBitmap(bitmap);
+  if (gfx::PNGCodec::Decode(memory->front(), memory->size(),
+                            bitmap, scale_fallback)) {
+    return true;
+  }
 
   // 99% of our assets are PNGs, however fallback to JPEG.
-  SkBitmap* allocated_bitmap =
-      gfx::JPEGCodec::Decode(memory->front(), memory->size());
-  if (allocated_bitmap)
-    return allocated_bitmap;
+  scoped_ptr<SkBitmap> jpeg_bitmap(
+      gfx::JPEGCodec::Decode(memory->front(), memory->size()));
+  if (jpeg_bitmap.get()) {
+    bitmap->swap(*jpeg_bitmap.get());
+    return true;
+  }
 
   NOTREACHED() << "Unable to decode theme image resource " << resource_id;
-  return NULL;
+  return false;
 }
 
-SkBitmap* ResourceBundle::LoadBitmap(int resource_id,
-                                     ScaleFactor scale_factor) const {
+bool ResourceBundle::LoadBitmap(int resource_id,
+                                ScaleFactor scale_factor,
+                                SkBitmap* bitmap,
+                                bool* scale_fallback) const {
   for (size_t i = 0; i < data_packs_.size(); ++i) {
     if (data_packs_[i]->GetScaleFactor() == scale_factor) {
-      SkBitmap* bitmap = LoadBitmap(*data_packs_[i], resource_id);
-      if (bitmap)
-        return bitmap;
+      if (LoadBitmap(*data_packs_[i], resource_id, bitmap, scale_fallback))
+        return true;
     }
   }
-  return NULL;
+  return false;
 }
 
 gfx::Image& ResourceBundle::GetEmptyImage() {