Early return when the value of test-justify is distribute, and the length of line is 0

Source/platform/fonts/Character.cpp

 /*
  * Copyright (C) 2014 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 325 matching lines @@
         0x1F200, 0x1F6FF
     };
 
     return valueInIntervalList(cjkSymbolRanges, c);
 }
 
 unsigned Character::expansionOpportunityCount(const LChar* characters, size_t length, TextDirection direction, bool& isAfterExpansion, const TextJustify textJustify)
 {
     unsigned count = 0;
     if (textJustify == TextJustifyDistribute) {
-        count = length - 1;
+        if (length == 0)
+            return 0;
+        int lastCharacter = (direction == LTR) ? length - 1 : 0;
+        isAfterExpansion = treatAsSpace(characters[lastCharacter]);
+        return length - 1;

[kojii, 2014/12/11 08:47:36]

I jumped in from the mid of review discussions without reading past comments, so
ignore me if other reviewers said differently.

It looks like you reverted non-distribute case back to before the patch, I
personally like that.

This "if" is an optimization of distribute case, which is the assumption of
every call to |treatAsSpace()| returns true in the loop below. By thinking that
way, I think this is not a correct optimization. It should rather be:

isAfterExpansion = true;
return length;

Right?

There's also another edge case where |isAfterExpansion| is |false| on the entry.
In this case, you may want to return |length + 1| so that there's an expansion
opportunity before the first character, but the spec doesn't clearly state
whether there should be an expansion opportunity between non-distribute and
distribute. Since the 99% of use cases for distribute is applying to a whole
block, not to part of a block, and this suggestion applies only to such edge
cases, I suggest you ask this to www-style and then make another bug or CL if it
resolved to differently from what we have with this patch.

+    }
+
+    if (direction == LTR) {
+        for (size_t i = 0; i < length; ++i) {
+            if (treatAsSpace(characters[i])) {
+                count++;
+                isAfterExpansion = true;
+            } else {
+                isAfterExpansion = false;
+            }
+        }
     } else {
-        if (direction == LTR) {
-            for (size_t i = 0; i < length; ++i) {
-                if (treatAsSpace(characters[i]))
-                    count++;
-            }
-        } else {
-            for (size_t i = length; i > 0; --i) {
-                if (treatAsSpace(characters[i - 1]))
-                    count++;
+        for (size_t i = length; i > 0; --i) {
+            if (treatAsSpace(characters[i - 1])) {
+                count++;
+                isAfterExpansion = true;
+            } else {
+                isAfterExpansion = false;
             }
         }
     }
-    int lastCharacter = (direction == LTR) ? length - 1 : 0;
-    isAfterExpansion = treatAsSpace(characters[lastCharacter]);
 
     return count;
 }
 
 unsigned Character::expansionOpportunityCount(const UChar* characters, size_t length, TextDirection direction, bool& isAfterExpansion, const TextJustify textJustify)
 {
     unsigned count = 0;
     if (direction == LTR) {
         for (size_t i = 0; i < length; ++i) {
             UChar32 character = characters[i];
@@ skipping 56 matching lines @@
 {
     return normalizeSpacesInternal(characters, length);
 }
 
 String Character::normalizeSpaces(const UChar* characters, unsigned length)
 {
     return normalizeSpacesInternal(characters, length);
 }
 
 } // namespace blink