commit 498a9b244901602661b848e6dde1db6cc7f00aa0
author Khushal <khushalsagar@chromium.org> Wed Sep 04 16:19:22 2019 -0700
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Thu Sep 05 20:39:12 2019 +0000
tree 3eddfa91c5147e8df60d1eb55b7a6fee2b5e7f29
parent f0c681e459405291ac2db87d536d94d81c177317

Reland: Only send the RemoteStrikes that have pending glyphs.

This reverts commit 94c66475560de551a499334818df85966db7681c. The issue
was that we were not clearing the set of locked strikes, if there is no
data to send. As a result the client was assuming strikes are locked
even after they were purged on the service side.

R=herb@google.com

Bug:999682
Change-Id: I767dd0cab81e085123058201dab042628ac4e241
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/239343
Auto-Submit: Khushal Sagar <khushalsagar@chromium.org>
Commit-Queue: Herb Derby <herb@google.com>
Reviewed-by: Herb Derby <herb@google.com>

src/core/SkRemoteGlyphCache.cpp

diff --git a/src/core/SkRemoteGlyphCache.cpp b/src/core/SkRemoteGlyphCache.cpp
index b3af9b7..3e4fc4d 100644
--- a/src/core/SkRemoteGlyphCache.cpp
+++ b/src/core/SkRemoteGlyphCache.cpp
@@ -221,14 +221,16 @@
 
     void onAboutToExitScope() override {}
 
-private:
     bool hasPendingGlyphs() const {
         return !fPendingGlyphImages.empty() || !fPendingGlyphPaths.empty();
     }
+
+    void resetScalerContext();
+
+private:
     void writeGlyphPath(const SkPackedGlyphID& glyphID, Serializer* serializer) const;
 
     void ensureScalerContext();
-    void resetScalerContext();
 
     // The set of glyphs cached on the remote client.
     SkTHashSet<SkPackedGlyphID> fCachedGlyphImages;
@@ -425,7 +427,17 @@
 }
 
 void SkStrikeServer::writeStrikeData(std::vector<uint8_t>* memory) {
-    if (fRemoteStrikesToSend.empty() && fTypefacesToSend.empty()) {
+    size_t strikesToSend = 0;
+    fRemoteStrikesToSend.foreach ([&strikesToSend](RemoteStrike* strike) {
+        if (strike->hasPendingGlyphs()) {
+            strikesToSend++;
+        } else {
+            strike->resetScalerContext();
+        }
+    });
+
+    if (strikesToSend == 0 && fTypefacesToSend.empty()) {
+        fRemoteStrikesToSend.reset();
         return;
     }
 
@@ -436,11 +448,14 @@
     }
     fTypefacesToSend.clear();
 
-    serializer.emplace<uint64_t>(SkTo<uint64_t>(fRemoteStrikesToSend.count()));
-    fRemoteStrikesToSend.foreach(
+    serializer.emplace<uint64_t>(SkTo<uint64_t>(strikesToSend));
+    fRemoteStrikesToSend.foreach (
 #ifdef SK_DEBUG
             [&serializer, this](RemoteStrike* strike) {
-                strike->writePendingGlyphs(&serializer);
+                if (strike->hasPendingGlyphs()) {
+                    strike->writePendingGlyphs(&serializer);
+                    strike->resetScalerContext();
+                }
                 auto it = fDescToRemoteStrike.find(&strike->getDescriptor());
                 SkASSERT(it != fDescToRemoteStrike.end());
                 SkASSERT(it->second.get() == strike);
@@ -448,7 +463,10 @@
 
 #else
             [&serializer](RemoteStrike* strike) {
-                strike->writePendingGlyphs(&serializer);
+                if (strike->hasPendingGlyphs()) {
+                    strike->writePendingGlyphs(&serializer);
+                    strike->resetScalerContext();
+                }
             }
 #endif
     );
@@ -569,12 +587,8 @@
 }
 
 void SkStrikeServer::RemoteStrike::writePendingGlyphs(Serializer* serializer) {
-    // TODO(khushalsagar): Write a strike only if it has any pending glyphs.
     serializer->emplace<bool>(this->hasPendingGlyphs());
-    if (!this->hasPendingGlyphs()) {
-        this->resetScalerContext();
-        return;
-    }
+    SkASSERT(this->hasPendingGlyphs());
 
     // Write the desc.
     serializer->emplace<StrikeSpec>(fContext->getTypeface()->uniqueID(), fDiscardableHandleId);
@@ -618,7 +632,6 @@
         writeGlyphPath(glyphID, serializer);
     }
     fPendingGlyphPaths.clear();
-    this->resetScalerContext();
 }
 
 void SkStrikeServer::RemoteStrike::ensureScalerContext() {

tests/SkRemoteGlyphCacheTest.cpp

diff --git a/tests/SkRemoteGlyphCacheTest.cpp b/tests/SkRemoteGlyphCacheTest.cpp
index 8b35768..92e1fec 100644
--- a/tests/SkRemoteGlyphCacheTest.cpp
+++ b/tests/SkRemoteGlyphCacheTest.cpp
@@ -314,6 +314,13 @@
     // handles.
     std::vector<uint8_t> fontData;
     server.writeStrikeData(&fontData);
+
+    // Another analysis pass, to ensure that deleting handles after a complete cache hit still
+    // works. This is a regression test for crbug.com/999682.
+    cache_diff_canvas.drawTextBlob(serverBlob.get(), 0, 0, paint);
+    server.writeStrikeData(&fontData);
+    REPORTER_ASSERT(reporter, discardableManager->handleCount() == 1u);
+
     discardableManager->unlockAndDeleteAll();
     cache_diff_canvas.drawTextBlob(serverBlob.get(), 0, 0, paint);
     REPORTER_ASSERT(reporter, discardableManager->handleCount() == 2u);