Fix hyphenated words not to overflow on Android

kojiishi · Commit bot · commit dd1e6431c28b · 2017-02-19T09:11:00.000Z
This patch fixes to choose the correct hyphenation point when the word has multiple hyphenation points on Android. The line breaker computes the number of characters that can fit and computes that last hyphenation point within the number. The number was not used correctly in the Android hyphenation engine. The fix is about 5 lines in |HyphenationMinikin::lastHyphenLocation|, but the test isn't small. The hyphenation engine loads dictionaries using a mojo service, but since the mojo service is not available in unit tests, the test creates the engine from the dictionary file it loaded. BUG=692094 Review-Url: https://codereview.chromium.org/2692233005 Cr-Commit-Position: refs/heads/master@{#451519}
diff --git a/third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h b/third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
@@ -890,6 +890,7 @@ ALWAYS_INLINE bool BreakingContext::hyphenate(
   unsigned prefixLength = hyphenation.lastHyphenLocation(
       StringView(text.text(), start, len),
       std::min(maxPrefixLength, len - Hyphenation::minimumSuffixLength) + 1);
+  DCHECK_LE(prefixLength, maxPrefixLength);
   if (!prefixLength || prefixLength < Hyphenation::minimumPrefixLength)
     return false;
 
diff --git a/third_party/WebKit/Source/platform/BUILD.gn b/third_party/WebKit/Source/platform/BUILD.gn
@@ -1509,6 +1509,7 @@ component("platform") {
   if (use_minikin_hyphenation) {
     sources += [
       "text/hyphenation/HyphenationMinikin.cpp",
+      "text/hyphenation/HyphenationMinikin.h",
       "text/hyphenation/HyphenatorAOSP.cpp",
       "text/hyphenation/HyphenatorAOSP.h",
     ]
diff --git a/third_party/WebKit/Source/platform/text/HyphenationTest.cpp b/third_party/WebKit/Source/platform/text/HyphenationTest.cpp
@@ -7,6 +7,11 @@
 #include "platform/LayoutLocale.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if OS(ANDROID)
+#include "base/files/file_path.h"
+#include "platform/text/hyphenation/HyphenationMinikin.h"
+#endif
+
 namespace blink {
 
 class NoHyphenation : public Hyphenation {
@@ -28,4 +33,52 @@ TEST(HyphenationTest, Get) {
   LayoutLocale::clearForTesting();
 }
 
+#if OS(ANDROID) || OS(MACOSX)
+TEST(HyphenationTest, LastHyphenLocation) {
+#if OS(ANDROID)
+  // Because the mojo service to open hyphenation dictionaries is not accessible
+  // from the unit test, open the dictionary file directly for testing.
+  base::FilePath path("/system/usr/hyphen-data/hyph-en-us.hyb");
+  base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  if (!file.IsValid()) {
+    // Ignore this test on platforms without hyphenation dictionaries.
+    return;
+  }
+  RefPtr<Hyphenation> hyphenation =
+      HyphenationMinikin::fromFileForTesting(std::move(file));
+#else
+  const LayoutLocale* locale = LayoutLocale::get("en-us");
+  ASSERT_TRUE(locale);
+  Hyphenation* hyphenation = locale->getHyphenation();
+#endif
+  ASSERT_TRUE(hyphenation) << "Cannot find the hyphenation engine";
+
+  // Get all hyphenation points by |hyphenLocations|.
+  const String word("hyphenation");
+  Vector<size_t, 8> locations = hyphenation->hyphenLocations(word);
+  for (unsigned i = 1; i < locations.size(); i++) {
+    ASSERT_GT(locations[i - 1], locations[i])
+        << "hyphenLocations must return locations in the descending order";
+  }
+
+  // Test |lastHyphenLocation| returns all hyphenation points.
+  locations.push_back(0);
+  size_t locationIndex = locations.size() - 1;
+  for (size_t beforeIndex = 0; beforeIndex < word.length(); beforeIndex++) {
+    size_t location = hyphenation->lastHyphenLocation(word, beforeIndex);
+
+    if (location)
+      EXPECT_LT(location, beforeIndex);
+
+    if (locationIndex > 0 && location == locations[locationIndex - 1])
+      locationIndex--;
+    EXPECT_EQ(locations[locationIndex], location) << String::format(
+        "lastHyphenLocation(%s, %zd)", word.utf8().data(), beforeIndex);
+  }
+
+  EXPECT_EQ(locationIndex, 0u)
+      << "Not all locations are found by lastHyphenLocation";
+}
+#endif
+
 }  // namespace blink
diff --git a/third_party/WebKit/Source/platform/text/hyphenation/HyphenationMinikin.cpp b/third_party/WebKit/Source/platform/text/hyphenation/HyphenationMinikin.cpp
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "platform/text/Hyphenation.h"
+#include "platform/text/hyphenation/HyphenationMinikin.h"
 
 #include "base/files/file.h"
 #include "base/files/memory_mapped_file.h"
@@ -18,23 +18,6 @@ namespace blink {
 
 using Hyphenator = android::Hyphenator;
 
-class HyphenationMinikin : public Hyphenation {
- public:
-  bool openDictionary(const AtomicString& locale);
-
-  size_t lastHyphenLocation(const StringView& text,
-                            size_t beforeIndex) const override;
-  Vector<size_t, 8> hyphenLocations(const StringView&) const override;
-
- private:
-  static base::File openDictionaryFile(const AtomicString& locale);
-
-  std::vector<uint8_t> hyphenate(const StringView&) const;
-
-  base::MemoryMappedFile m_file;
-  std::unique_ptr<Hyphenator> m_hyphenator;
-};
-
 static mojom::blink::HyphenationPtr connectToRemoteService() {
   mojom::blink::HyphenationPtr service;
   Platform::current()->interfaceProvider()->getInterface(
@@ -48,17 +31,17 @@ static const mojom::blink::HyphenationPtr& getService() {
   return service;
 }
 
-base::File HyphenationMinikin::openDictionaryFile(const AtomicString& locale) {
+bool HyphenationMinikin::openDictionary(const AtomicString& locale) {
   const mojom::blink::HyphenationPtr& service = getService();
   base::File file;
   base::ElapsedTimer timer;
   service->OpenDictionary(locale, &file);
   UMA_HISTOGRAM_TIMES("Hyphenation.Open", timer.Elapsed());
-  return file;
+
+  return openDictionary(std::move(file));
 }
 
-bool HyphenationMinikin::openDictionary(const AtomicString& locale) {
-  base::File file = openDictionaryFile(locale);
+bool HyphenationMinikin::openDictionary(base::File file) {
   if (!file.IsValid())
     return false;
   if (!m_file.Initialize(std::move(file))) {
@@ -87,14 +70,17 @@ std::vector<uint8_t> HyphenationMinikin::hyphenate(
 
 size_t HyphenationMinikin::lastHyphenLocation(const StringView& text,
                                               size_t beforeIndex) const {
-  if (text.length() < minimumPrefixLength + minimumSuffixLength)
+  if (text.length() < minimumPrefixLength + minimumSuffixLength ||
+      beforeIndex <= minimumPrefixLength)
     return 0;
 
   std::vector<uint8_t> result = hyphenate(text);
-  static_assert(minimumPrefixLength >= 1,
-                "Change the 'if' above if this fails");
-  for (size_t i = text.length() - minimumSuffixLength - 1;
-       i >= minimumPrefixLength; i--) {
+  beforeIndex =
+      std::min<size_t>(beforeIndex, text.length() - minimumSuffixLength);
+  CHECK_LE(beforeIndex, result.size());
+  CHECK_GE(beforeIndex, 1u);
+  static_assert(minimumPrefixLength >= 1, "|beforeIndex - 1| can underflow");
+  for (size_t i = beforeIndex - 1; i >= minimumPrefixLength; i--) {
     if (result[i])
       return i;
   }
@@ -170,4 +156,12 @@ PassRefPtr<Hyphenation> Hyphenation::platformGetHyphenation(
   return nullptr;
 }
 
+PassRefPtr<HyphenationMinikin> HyphenationMinikin::fromFileForTesting(
+    base::File file) {
+  RefPtr<HyphenationMinikin> hyphenation(adoptRef(new HyphenationMinikin));
+  if (hyphenation->openDictionary(std::move(file)))
+    return hyphenation.release();
+  return nullptr;
+}
+
 }  // namespace blink
diff --git a/third_party/WebKit/Source/platform/text/hyphenation/HyphenationMinikin.h b/third_party/WebKit/Source/platform/text/hyphenation/HyphenationMinikin.h
@@ -0,0 +1,39 @@
+// Copyright 2017 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/text/Hyphenation.h"
+
+#include "base/files/memory_mapped_file.h"
+#include "platform/PlatformExport.h"
+
+namespace base {
+class File;
+}  // namespace base
+
+namespace android {
+class Hyphenator;
+}  // namespace andorid
+
+namespace blink {
+
+class PLATFORM_EXPORT HyphenationMinikin : public Hyphenation {
+ public:
+  bool openDictionary(const AtomicString& locale);
+
+  size_t lastHyphenLocation(const StringView& text,
+                            size_t beforeIndex) const override;
+  Vector<size_t, 8> hyphenLocations(const StringView&) const override;
+
+  static PassRefPtr<HyphenationMinikin> fromFileForTesting(base::File);
+
+ private:
+  bool openDictionary(base::File);
+
+  std::vector<uint8_t> hyphenate(const StringView&) const;
+
+  base::MemoryMappedFile m_file;
+  std::unique_ptr<android::Hyphenator> m_hyphenator;
+};
+
+}  // namespace blink
