Bug 66498

Summary: Typesetting features need not force complex for single-character runs
Product: WebKit Reporter: Ned Holbrook <ned>
Component: TextAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: jamesr, mitz, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 66733    
Bug Blocks:    
Attachments:
Description Flags
Proposed changes.
mitz: review+
Diffs from failing test.
none
Updated ChangeLog. none

Description Ned Holbrook 2011-08-18 14:33:56 PDT
The only typesetting features are kerning and ligatures, neither of which applies for a single character. As such, there's no need to spool up the complex text machinery for a single-character run just because the font has typesetting features. This is particularly interesting when hyphenation is enabled.
Comment 1 Ned Holbrook 2011-08-18 14:55:34 PDT
Created attachment 104401 [details]
Proposed changes.

In most cases this isn't much of a problem (a percent or less) but every little bit helps.
Comment 2 Ned Holbrook 2011-08-18 17:19:51 PDT
Running the full set of tests shows that fast/writing-mode/text-orientation-basic.html is failing with this patch, I'm investigating now.
Comment 3 Ned Holbrook 2011-08-18 17:46:21 PDT
Created attachment 104428 [details]
Diffs from failing test.
Comment 4 Ned Holbrook 2011-08-23 11:10:16 PDT
(In reply to comment #2)
> Running the full set of tests shows that fast/writing-mode/text-orientation-basic.html is failing with this patch, I'm investigating now.

This should be fixed by 66733.
Comment 5 Ned Holbrook 2011-08-31 14:27:40 PDT
As of r94090, I see no fast or svg pixel test failures due to the proposed changes. I also ran the entire suite of tests without pixel tests and saw no failures.
Comment 6 mitz 2011-09-01 00:59:47 PDT
Comment on attachment 104401 [details]
Proposed changes.

View in context: https://bugs.webkit.org/attachment.cgi?id=104401&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests. (OOPS!)

I think this line will stop the commit bot from landing this patch
Comment 7 Ned Holbrook 2011-09-01 09:06:12 PDT
(In reply to comment #6)
> I think this line will stop the commit bot from landing this patch

Oops, indeed! I will change it to note that the changes are for performance and not expected to change layout.
Comment 8 Ned Holbrook 2011-09-01 09:28:12 PDT
Created attachment 105977 [details]
Updated ChangeLog.
Comment 9 WebKit Review Bot 2011-09-01 10:19:34 PDT
Comment on attachment 105977 [details]
Updated ChangeLog.

Clearing flags on attachment: 105977

Committed r94303: <http://trac.webkit.org/changeset/94303>
Comment 10 WebKit Review Bot 2011-09-01 10:19:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 James Robinson 2011-09-01 11:50:10 PDT
On Chromium Mac on Snow Leopard this patch caused the text metrics on fast/text/international/vertical-text-glyph-test.html to change very, very slightly.  Here's the text diff:

--- /b/build/slave/Webkit_Mac10_6__CG__dbg_/build/layout-test-results/fast/text/international/vertical-text-glyph-test-expected.txt 
+++ /b/build/slave/Webkit_Mac10_6__CG__dbg_/build/layout-test-results/fast/text/international/vertical-text-glyph-test-actual.txt 
@@ -1,8 +1,8 @@
-layer at (0,0) size 785x634
+layer at (0,0) size 785x635
   RenderView at (0,0) size 785x600
-layer at (0,0) size 785x634
-  RenderBlock {HTML} at (0,0) size 785x634
-    RenderBody {BODY} at (8,21) size 769x605
+layer at (0,0) size 785x635
+  RenderBlock {HTML} at (0,0) size 785x635
+    RenderBody {BODY} at (8,21) size 769x606
       RenderBlock {P} at (0,0) size 769x32
         RenderText {#text} at (0,5) size 161x21
           text run at (0,5) width 161: "Simple text path"
@@ -15,8 +15,8 @@
       RenderBlock {P} at (0,320) size 769x32
         RenderText {#text} at (0,5) size 181x21
           text run at (0,5) width 181: "Complex text path"
-      RenderBlock (anonymous) at (0,373) size 769x232
-        RenderBlock {SPAN} at (0,0) size 32x223
-          RenderText {#text} at (5,0) size 21x223
-            text run at (5,0) width 223: "string\x{300C}\x{3042}\x{3001}\x{5909}\x{3063}\x{FF01}\x{300D}\x{3002}"
+      RenderBlock (anonymous) at (0,373) size 769x233
+        RenderBlock {SPAN} at (0,0) size 32x224
+          RenderText {#text} at (5,0) size 21x224
+            text run at (5,0) width 224: "string\x{300C}\x{3042}\x{3001}\x{5909}\x{3063}\x{FF01}\x{300D}\x{3002}"
         RenderText {#text} at (0,0) size 0x0

This also changes the pixel output slightly - the text all renders the same, but the scrollbar looks different because the scrollable area is 1px taller.  I'm planning to rebaseline since this looks harmless, but since the comments indicate that this should not have changed any layout posting as an FYI.
Comment 12 Ned Holbrook 2011-09-01 11:52:37 PDT
Interesting. I wonder if there aren't some minute differences in vertical metrics on SL.