Fixes for re-enabling more MSVC level 4 warnings: misc edition #2

skia/ext/pixel_ref_utils_unittest.cc

 // Copyright 2014 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 "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #include "cc/test/geometry_test_utils.h"
 #include "skia/ext/pixel_ref_utils.h"
 #include "skia/ext/refptr.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ skipping 18 matching lines @@
  public:
   TestDiscardableShader() {
     CreateBitmap(gfx::Size(50, 50), "discardable", &bitmap_);
   }
 
   TestDiscardableShader(SkFlattenableReadBuffer& flattenable_buffer) {
     SkOrderedReadBuffer& buffer =
         static_cast<SkOrderedReadBuffer&>(flattenable_buffer);
     SkReader32* reader = buffer.getReader32();
 
-    reader->skip(-4);
+    reader->skip(static_cast<size_t>(-4));  //!!? Is this safe?

[Peter Kasting, 2014/07/09 22:19:54]

This needs review.  Static-casting a negative value to an unsigned one and then
actually using it like a negative value seems dangerous to me (means we have
code relying on integer overflow); it suggests the signature of skip() shouldn't
be taking an unsigned value if we want it to be able to move backwards.

[Stephen White, 2014/07/10 02:13:56]

+vmpstr +junov

Much of this code looks dubious. SkOrderedReadBuffer is in Skia's src/core,
which means it's not meant to be #included outside of Skia proper (it's not in
the public API, although it's just a typedef now anyway). Skipping backwards
also looks dubious, as Peter pointed out. It looks like the code is trying to
skip over the shader by sniffing the size written in the already-read int, which
looks fragile.

Anyway, this is only test code, but I think the correct thing here is to call
the base class constructor with the read buffer; something like:

TestDiscardableShader(SkReadBuffer& buffer) : SkShader(buffer) {
    CreateBitmap(gfx::Size(50, 50), "discardable", &bitmap_);
}

Could you try that and see if the test still passes?

(Or Vlad, if there was some reason this was necessary, could you enlighten us?)

     uint32_t toSkip = reader->readU32();
     reader->skip(toSkip);
 
     CreateBitmap(gfx::Size(50, 50), "discardable", &bitmap_);
   }
 
   virtual SkShader::BitmapType asABitmap(SkBitmap* bitmap,
                                          SkMatrix* matrix,
                                          TileMode xy[2]) const OVERRIDE {
     if (bitmap)
@@ skipping 611 matching lines @@
   EXPECT_EQ(3u, pixel_refs.size());
   EXPECT_FLOAT_RECT_EQ(gfx::RectF(10, 10, 90, 90),
                        gfx::SkRectToRectF(pixel_refs[0].pixel_ref_rect));
   EXPECT_FLOAT_RECT_EQ(gfx::RectF(10, 10, 40, 40),
                        gfx::SkRectToRectF(pixel_refs[1].pixel_ref_rect));
   EXPECT_FLOAT_RECT_EQ(gfx::RectF(50, 55, 150, 145),
                        gfx::SkRectToRectF(pixel_refs[2].pixel_ref_rect));
 }
 
 }  // namespace skia