Clean up cc/paint interfaces

third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.cpp

 // 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 "platform/graphics/paint/DrawingDisplayItem.h"
 
 #include "platform/graphics/GraphicsContext.h"
 #include "platform/graphics/paint/PaintCanvas.h"
 #include "public/platform/WebDisplayItemList.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkData.h"
 #include "third_party/skia/include/core/SkPictureAnalyzer.h"
 
 namespace blink {
 
 void DrawingDisplayItem::replay(GraphicsContext& context) const {
   if (m_record)
-    context.drawRecord(m_record.get());
+    context.drawRecord(m_record);
 }
 
 void DrawingDisplayItem::appendToWebDisplayItemList(
     const IntRect& visualRect,
     WebDisplayItemList* list) const {
   if (m_record)
     list->appendDrawingItem(visualRect, m_record);
 }
 
 bool DrawingDisplayItem::drawsContent() const {
@@ skipping 16 matching lines @@
   DisplayItem::dumpPropertiesAsDebugString(stringBuilder);
   if (m_record) {
     stringBuilder.append(
         String::format(", rect: [%f,%f %fx%f]", m_record->cullRect().x(),
                        m_record->cullRect().y(), m_record->cullRect().width(),
                        m_record->cullRect().height()));
   }
 }
 #endif
 
-static bool recordsEqual(const PaintRecord* record1,
-                         const PaintRecord* record2) {
+static bool recordsEqual(sk_sp<const PaintRecord> record1,

[danakj, 2017/03/16 20:06:16]

This seems like it should be T* still? ToSkPicture isn't keeping ownership and
we're not storing it's result just using it as a temporary

[enne (OOO), 2017/03/16 20:57:26]

Agreed here.

+                         sk_sp<const PaintRecord> record2) {
   if (record1->approximateOpCount() != record2->approximateOpCount())
     return false;
 
   // TODO(enne): PaintRecord should have an operator==
   sk_sp<SkData> data1 = ToSkPicture(record1)->serialize();

[danakj, 2017/03/16 20:06:17]

move()

   sk_sp<SkData> data2 = ToSkPicture(record2)->serialize();

[danakj, 2017/03/16 20:06:16]

move()

   return data1->equals(data2.get());
 }
 
-static SkBitmap recordToBitmap(const PaintRecord* record) {
+static SkBitmap recordToBitmap(sk_sp<const PaintRecord> record) {
   SkBitmap bitmap;
   SkRect rect = record->cullRect();
   bitmap.allocPixels(SkImageInfo::MakeN32Premul(rect.width(), rect.height()));
   SkiaPaintCanvas canvas(bitmap);
   canvas.clear(SK_ColorTRANSPARENT);
   canvas.translate(-rect.x(), -rect.y());
   canvas.drawPicture(record);

[danakj, 2017/03/16 20:06:16]

mvoe()

   return bitmap;
 }
 
-static bool bitmapsEqual(const PaintRecord* record1,
-                         const PaintRecord* record2) {
+static bool bitmapsEqual(sk_sp<const PaintRecord> record1,

[danakj, 2017/03/16 20:06:17]

This seems like it should be T* also, it's not passing the ownership anywhere

[enne (OOO), 2017/03/16 20:57:26]

Not agreed here.  This is rasterizing via drawPicture which in this patch takes
an sk_sp.  Sure, the SkiaPaintCanvas doesn't take ownership of the PaintRecord
when rasterizing, but the recording version does, and at the PaintCanvas
interface level, I think you don't know that?

[danakj, 2017/03/16 21:02:01]

Oh ok I missed what recordToBitmap does.

+                         sk_sp<const PaintRecord> record2) {
   SkRect rect = record1->cullRect();
   if (rect != record2->cullRect())
     return false;
 
   SkBitmap bitmap1 = recordToBitmap(record1);

[danakj, 2017/03/16 20:06:16]

move()

   SkBitmap bitmap2 = recordToBitmap(record2);

[danakj, 2017/03/16 20:06:16]

move()

   bitmap1.lockPixels();
   bitmap2.lockPixels();
   int mismatchCount = 0;
   const int maxMismatches = 10;
   for (int y = 0; y < rect.height() && mismatchCount < maxMismatches; ++y) {
     for (int x = 0; x < rect.width() && mismatchCount < maxMismatches; ++x) {
       SkColor pixel1 = bitmap1.getColor(x, y);
       SkColor pixel2 = bitmap2.getColor(x, y);
       if (pixel1 != pixel2) {
         LOG(ERROR) << "x=" << x << " y=" << y << " " << std::hex << pixel1
                    << " vs " << std::hex << pixel2;
         ++mismatchCount;
       }
     }
   }
   bitmap1.unlockPixels();
   bitmap2.unlockPixels();
   return !mismatchCount;
 }
 
 bool DrawingDisplayItem::equals(const DisplayItem& other) const {
   if (!DisplayItem::equals(other))
     return false;
 
-  const PaintRecord* record = this->GetPaintRecord();
-  const PaintRecord* otherRecord =
+  sk_sp<const PaintRecord> record = this->GetPaintRecord();

[danakj, 2017/03/16 20:06:16]

const& probably?

+  sk_sp<const PaintRecord> otherRecord =
       static_cast<const DrawingDisplayItem&>(other).GetPaintRecord();
 
   if (!record && !otherRecord)
     return true;
   if (!record || !otherRecord)
     return false;
 
   if (recordsEqual(record, otherRecord))
     return true;
 
   // Sometimes the client may produce different records for the same visual
   // result, which should be treated as equal.
   return bitmapsEqual(record, otherRecord);

[danakj, 2017/03/16 21:02:01]

Then these would be move()s

[enne (OOO), 2017/03/16 21:06:32]

Quite right.

 }
 
 }  // namespace blink