76041 – fast/text/unicode-variation-selector.html doesn't pass on Lion

Bug 76041 - fast/text/unicode-variation-selector.html doesn't pass on Lion

Summary: fast/text/unicode-variation-selector.html doesn't pass on Lion

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Text (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Kenichi Ishibashi

URL:
Keywords:

Depends on:
Blocks:

Reported:	2012-01-11 02:26 PST by Kenichi Ishibashi
Modified:	2012-01-25 04:33 PST (History)
CC List:	3 users (show)

See Also:

Attachments
Patch (2.88 KB, patch) 2012-01-11 18:44 PST, Kenichi Ishibashi	no flags	Details \| Formatted Diff \| Diff
crash log (54.21 KB, text/plain) 2012-01-19 00:43 PST, Kenichi Ishibashi	no flags	Details
Patched font (4.43 KB, application/x-font-ttf) 2012-01-20 09:08 PST, mitz	no flags	Details
Patch (patched font added) (3.94 KB, patch) 2012-01-22 17:03 PST, Kenichi Ishibashi	no flags	Details \| Formatted Diff \| Diff
Patch (3.36 KB, patch) 2012-01-24 20:47 PST, Kenichi Ishibashi	no flags	Details \| Formatted Diff \| Diff
Patch (3.11 KB, patch) 2012-01-24 22:29 PST, Kenichi Ishibashi	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Kenichi Ishibashi 2012-01-11 02:26:09 PST

<http://trac.webkit.org/changeset/104545> broke fast/text/unicode-variation-selector.html on OS X Lion.

Comment 1 Alexey Proskuryakov 2012-01-11 12:05:59 PST

How does the failure look like?

Comment 2 mitz 2012-01-11 12:09:04 PST

Unmodified CJK glyph followed by a square.

Comment 3 Kenichi Ishibashi 2012-01-11 18:44:13 PST

Created attachment 122149 [details]
Patch

Comment 4 Kenichi Ishibashi 2012-01-11 18:48:34 PST

(In reply to comment #3)
> Created an attachment (id=122149) [details]
> Patch

I think this patch will fix the regression, but DRT often crashed when I tested. It looks like a bug of CoreText that I reported at <https://bugs.webkit.org/show_bug.cgi?id=74662>. I checked the input was valid. Here is a gdb log:

(gdb) bt
#0  0x00007fff8c74b9dc in std::equal_range<TFormat14UVSTable::UnicodeValueRange const*, int> ()
#1  0x00007fff8c74af7f in TFormat14UVSTable::Map ()
#2  0x00007fff8c74bf59 in TFormat12UTF16cmapTable::MapT<true> ()
#3  0x00007fff8c6f5d67 in TcmapTable::Map ()
#4  0x00007fff8c6f5afb in TBaseFont::GetGlyphsForCharacters ()
#5  0x00007fff8c700b35 in TUnicodeEncoder::EncodePortion ()
#6  0x00007fff8c700aca in TUnicodeEncoder::Encode ()
#7  0x00007fff8c716139 in TGlyphEncoder::RunUnicodeEncoderRecursively ()
#8  0x00007fff8c7164c4 in TGlyphEncoder::RunUnicodeEncoder ()
#9  0x00007fff8c716b2b in TGlyphEncoder::EncodeChars ()
#10 0x00007fff8c722c89 in TTypesetterUniChar::Initialize ()
#11 0x00007fff8c7183e5 in CTLineCreateWithUniCharProvider ()
#12 0x0000000102730efe in WebCore::SimpleFontData::canRenderCombiningCharacterSequence (this=0x10782d600, characters=0x108e56a40, length=3) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:451
#13 0x000000010198260c in WebCore::Font::fontDataForCombiningCharacterSequence (this=0x106d4f7c0, characters=0x108e56a40, length=3, variant=WebCore::NormalVariant) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/platform/graphics/mac/FontComplexTextMac.cpp:152
#14 0x00000001014c2ce4 in WebCore::ComplexTextController::collectComplexTextRuns (this=0x7fff5fbf8608) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:278
#15 0x00000001014c2a55 in WebCore::ComplexTextController::ComplexTextController (this=0x7fff5fbf8608, font=0x106d4f7c0, run=@0x7fff5fbf9cb8, mayUseNaturalWritingDirection=true, fallbackFonts=0x7fff5fbf9fd0, forTextEmphasis=false) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:97
#16 0x00000001014c2774 in WebCore::ComplexTextController::ComplexTextController (this=0x7fff5fbf8608, font=0x106d4f7c0, run=@0x7fff5fbf9cb8, mayUseNaturalWritingDirection=true, fallbackFonts=0x7fff5fbf9fd0, forTextEmphasis=false) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:82
#17 0x00000001019820cd in WebCore::Font::floatWidthForComplexText (this=0x106d4f7c0, run=@0x7fff5fbf9cb8, fallbackFonts=0x7fff5fbf9fd0, glyphOverflow=0x7fff5fbf9fb8) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/platform/graphics/mac/FontComplexTextMac.cpp:112
#18 0x000000010196f76a in WebCore::Font::width (this=0x106d4f7c0, run=@0x7fff5fbf9cb8, fallbackFonts=0x7fff5fbf9fd0, glyphOverflow=0x7fff5fbf9fb8) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/platform/graphics/Font.cpp:188
#19 0x00000001026124f1 in WebCore::RenderText::widthFromCache (this=0x108e4b438, f=@0x106d4f7c0, start=0, len=3, xPos=0, fallbackFonts=0x7fff5fbf9fd0, glyphOverflow=0x7fff5fbf9fb8) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderText.cpp:766
#20 0x000000010260e4cd in WebCore::RenderText::computePreferredLogicalWidths (this=0x108e4b438, leadWidth=0, fallbackFonts=@0x7fff5fbf9fd0, glyphOverflow=@0x7fff5fbf9fb8) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderText.cpp:999
#21 0x000000010260da8b in WebCore::RenderText::computePreferredLogicalWidths (this=0x108e4b438, leadWidth=0) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderText.cpp:883
#22 0x000000010260da10 in WebCore::RenderText::maxLogicalWidth (this=0x108e4b438) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderText.cpp:874
#23 0x00000001026107fd in WebCore::RenderText::width (this=0x108e4b438, from=0, len=3, f=@0x106d4f7c0, xPos=269, fallbackFonts=0x0, glyphOverflow=0x0) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderText.cpp:1460
#24 0x00000001024687ea in textWidth (text=0x108e4b438, from=0, len=3, font=@0x106d4f7c0, xPos=269, isFixedPitch=false, collapseWhiteSpace=true) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:1921
#25 0x000000010246465a in WebCore::RenderBlock::LineBreaker::nextLineBreak (this=0x7fff5fbfaef8, resolver=@0x7fff5fbfb168, lineInfo=@0x7fff5fbfb4a8, lineBreakIteratorInfo=@0x7fff5fbfafe0, lastFloatFromPreviousLine=0x0, consecutiveHyphenatedLines=0) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:2514
#26 0x0000000102460230 in WebCore::RenderBlock::layoutRunsAndFloatsInRange (this=0x108e44c58, layoutState=@0x7fff5fbfb480, resolver=@0x7fff5fbfb168, cleanLineStart=@0x7fff5fbfb120, cleanLineBidiStatus=@0x7fff5fbfb108, consecutiveHyphenatedLines=0) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:1240
#27 0x000000010245efcc in WebCore::RenderBlock::layoutRunsAndFloats (this=0x108e44c58, layoutState=@0x7fff5fbfb480, hasInlineChild=true) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:1206
#28 0x0000000102465d8a in WebCore::RenderBlock::layoutInlineChildren (this=0x108e44c58, relayoutChildren=false, repaintLogicalTop=@0x7fff5fbfb76c, repaintLogicalBottom=@0x7fff5fbfb768) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:1497
#29 0x000000010241743c in WebCore::RenderBlock::layoutBlock (this=0x108e44c58, relayoutChildren=false, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1308
#30 0x0000000102416b07 in WebCore::RenderBlock::layout (this=0x108e44c58) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1184
#31 0x000000010241fa9a in WebCore::RenderBlock::layoutBlockChild (this=0x108e2a218, child=0x108e44c58, marginInfo=@0x7fff5fbfbac0, previousFloatLogicalBottom=@0x7fff5fbfbab4, maxFloatLogicalBottom=@0x7fff5fbfbdc4) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2090
#32 0x00000001024197b3 in WebCore::RenderBlock::layoutBlockChildren (this=0x108e2a218, relayoutChildren=false, maxFloatLogicalBottom=@0x7fff5fbfbdc4) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2027
#33 0x000000010241745c in WebCore::RenderBlock::layoutBlock (this=0x108e2a218, relayoutChildren=false, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1310
#34 0x0000000102416b07 in WebCore::RenderBlock::layout (this=0x108e2a218) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1184
#35 0x000000010241fa9a in WebCore::RenderBlock::layoutBlockChild (this=0x106d30da8, child=0x108e2a218, marginInfo=@0x7fff5fbfc120, previousFloatLogicalBottom=@0x7fff5fbfc114, maxFloatLogicalBottom=@0x7fff5fbfc424) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2090
#36 0x00000001024197b3 in WebCore::RenderBlock::layoutBlockChildren (this=0x106d30da8, relayoutChildren=false, maxFloatLogicalBottom=@0x7fff5fbfc424) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2027
#37 0x000000010241745c in WebCore::RenderBlock::layoutBlock (this=0x106d30da8, relayoutChildren=false, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1310
#38 0x0000000102416b07 in WebCore::RenderBlock::layout (this=0x106d30da8) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1184
#39 0x000000010241fa9a in WebCore::RenderBlock::layoutBlockChild (this=0x106d1bce8, child=0x106d30da8, marginInfo=@0x7fff5fbfc780, previousFloatLogicalBottom=@0x7fff5fbfc774, maxFloatLogicalBottom=@0x7fff5fbfca84) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2090
#40 0x00000001024197b3 in WebCore::RenderBlock::layoutBlockChildren (this=0x106d1bce8, relayoutChildren=false, maxFloatLogicalBottom=@0x7fff5fbfca84) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2027
#41 0x000000010241745c in WebCore::RenderBlock::layoutBlock (this=0x106d1bce8, relayoutChildren=false, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1310
#42 0x0000000102416b07 in WebCore::RenderBlock::layout (this=0x106d1bce8) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1184
#43 0x000000010263acf5 in WebCore::RenderView::layout (this=0x106d1bce8) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/rendering/RenderView.cpp:136
#44 0x00000001019e2b70 in WebCore::FrameView::layout (this=0x1089404c0, allowSubtree=true) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/page/FrameView.cpp:1125
#45 0x00000001019ea865 in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive (this=0x1089404c0) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/page/FrameView.cpp:2974
#46 0x0000000100da6360 in -[WebHTMLView(WebInternal) _web_updateLayoutAndStyleIfNeededRecursive] (self=0x10893f750, _cmd=0x7fff8be6e263) at /Users/bashi/c/src/third_party/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:5393
#47 0x0000000100d8f322 in -[WebHTMLView(WebPrivate) viewWillDraw] (self=0x10893f750, _cmd=0x7fff835f0e1e) at /Users/bashi/c/src/third_party/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:1271
#48 0x00007fff82d37c11 in -[NSView viewWillDraw] ()
#49 0x00007fff82d37c11 in -[NSView viewWillDraw] ()
#50 0x00007fff82d384a2 in -[NSScrollView viewWillDraw] ()
#51 0x00007fff82d37c11 in -[NSView viewWillDraw] ()
#52 0x00007fff82d37c11 in -[NSView viewWillDraw] ()
#53 0x00007fff82d37c11 in -[NSView viewWillDraw] ()
#54 0x00007fff82d37c11 in -[NSView viewWillDraw] ()
#55 0x00007fff82d36952 in -[NSView _sendViewWillDrawInRect:clipRootView:suppressRecursion:] ()
#56 0x00007fff82d356c1 in -[NSView displayIfNeeded] ()
#57 0x0000000100023ca5 in -[FrameLoadDelegate webView:didFinishLoadForFrame:] (self=0x10b20f9e0, _cmd=0x7fff835ca473, sender=0x108913960, frame=0x108921660) at /Users/bashi/c/src/third_party/WebKit/Tools/DumpRenderTree/mac/FrameLoadDelegate.mm:228
#58 0x0000000100d2ccdc in CallDelegate (implementation=0x100023ac0 <-[FrameLoadDelegate webView:didFinishLoadForFrame:]>, self=0x108913960, delegate=0x10b20f9e0, selector=0x7fff835ca473, object=0x108921660) at /Users/bashi/c/src/third_party/WebKit/Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:285
#59 0x0000000100d2cc1d in CallFrameLoadDelegate (implementation=0x100023ac0 <-[FrameLoadDelegate webView:didFinishLoadForFrame:]>, self=0x108913960, selector=0x7fff835ca473, object=0x108921660) at /Users/bashi/c/src/third_party/WebKit/Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:504
#60 0x0000000100d54c76 in WebFrameLoaderClient::dispatchDidFinishLoad (this=0x108921c60) at /Users/bashi/c/src/third_party/WebKit/Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:654
#61 0x00000001019bf915 in WebCore::FrameLoader::checkLoadCompleteForThisFrame (this=0x1080448d8) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:2282
#62 0x00000001019b7123 in WebCore::FrameLoader::checkLoadComplete (this=0x1080448d8) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:2457
#63 0x00000001017602f7 in WebCore::DocumentLoader::removeSubresourceLoader (this=0x10804d000, loader=0x107858a00) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:776
#64 0x000000010279cf01 in WebCore::SubresourceLoader::releaseResources (this=0x107858a00) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/loader/SubresourceLoader.cpp:313
#65 0x0000000102667665 in WebCore::ResourceLoader::didFinishLoading (this=0x107858a00, finishTime=0) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:313
#66 0x000000010279c9ec in WebCore::SubresourceLoader::didFinishLoading (this=0x107858a00, finishTime=0) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/loader/SubresourceLoader.cpp:268
#67 0x0000000102667edc in WebCore::ResourceLoader::didFinishLoading (this=0x107858a00, finishTime=0) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:451
#68 0x0000000102664805 in -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] (self=0x106d2b7a0, _cmd=0x7fff888b9916, connection=0x106d28200) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/platform/network/mac/ResourceHandleMac.mm:886
#69 0x00007fff8e731712 in ___NSURLConnectionDidFinishLoading_block_invoke_1 ()
#70 0x00007fff8e731692 in _NSURLConnectionDidFinishLoading ()
#71 0x00007fff8b64eee2 in URLConnectionClient::_clientDidFinishLoading ()
#72 0x00007fff8b6fed0e in URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload ()
#73 0x00007fff8b629dfd in URLConnectionClient::processEvents ()
#74 0x00007fff8b629ca2 in MultiplexerSource::perform ()
#75 0x00007fff8828ab51 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ ()
#76 0x00007fff8828a3bd in __CFRunLoopDoSources0 ()
#77 0x00007fff882b11a9 in __CFRunLoopRun ()
#78 0x00007fff882b0ae6 in CFRunLoopRunSpecific ()
#79 0x00007fff8e6d504f in -[NSRunLoop(NSRunLoop) runMode:beforeDate:] ()
#80 0x00000001000164be in runTest (testPathOrURL=@0x7fff5fbff7a8) at /Users/bashi/c/src/third_party/WebKit/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1332
#81 0x0000000100015205 in dumpRenderTree (argc=3, argv=0x7fff5fbff830) at /Users/bashi/c/src/third_party/WebKit/Tools/DumpRenderTree/mac/DumpRenderTree.mm:859
#82 0x0000000100016c4c in main (argc=3, argv=0x7fff5fbff830) at /Users/bashi/c/src/third_party/WebKit/Tools/DumpRenderTree/mac/DumpRenderTree.mm:893
(gdb) f 12
#12 0x0000000102730efe in WebCore::SimpleFontData::canRenderCombiningCharacterSequence (this=0x10782d600, characters=0x108e56a40, length=3) at /Users/bashi/c/src/third_party/WebKit/Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:451
451	    RetainPtr<CTLineRef> line(AdoptCF, wkCreateCTLineWithUniCharProvider(&provideStringAndAttributes, 0, &info));
(gdb) p info
$5 = {
  characters = 0x108e56a40, 
  length = 3, 
  attributes = 0x108985a70
}
(gdb) p/x *characters@length
$6 = {0x845b, 0xdb40, 0xdd00}

Comment 5 Kenichi Ishibashi 2012-01-15 16:24:15 PST

Hi mitz,

Do you have any ideas about the crash? I'd like to fix the problem.

Comment 6 Kenichi Ishibashi 2012-01-18 17:08:58 PST

Hi mitz,

Please let me know if you are not the right person to ask.

(In reply to comment #5)
> Hi mitz,
> 
> Do you have any ideas about the crash? I'd like to fix the problem.

Comment 7 mitz 2012-01-19 00:37:43 PST

Can you attach the crash log file from ~/Library/Logs/DiagnosticMessages?

Comment 8 Kenichi Ishibashi 2012-01-19 00:43:32 PST

Created attachment 123087 [details]
crash log

Comment 9 mitz 2012-01-19 11:34:02 PST

(In reply to comment #8)
> Created an attachment (id=123087) [details]
> crash log

Thank you!

Comment 10 mitz 2012-01-20 09:08:42 PST

Created attachment 123336 [details]
Patched font

The crash is caused by a Core Text issue, which Apple is tracking internally. Meanwhile, I am attaching a modified version of gw432047.ttf that works around the crash, but otherwise behaves identically.

Comment 11 Kenichi Ishibashi 2012-01-22 17:03:25 PST

Created attachment 123502 [details]
Patch (patched font added)

Comment 12 Kenichi Ishibashi 2012-01-22 17:05:34 PST

(In reply to comment #10)
> The crash is caused by a Core Text issue, which Apple is tracking internally. Meanwhile, I am attaching a modified version of gw432047.ttf that works around the crash, but otherwise behaves identically.

Thank you so much for your help! I checked the font works around the crash and DRT passes fast/text/unicode-variation-selector.html.

Mitz, could you please review the patch?

Comment 13 mitz 2012-01-24 13:24:04 PST

Comment on attachment 123502 [details]
Patch (patched font added)

Thanks for the patch. All variation selectors are combining marks, so there is no need to treat them specially. In particular, advanceByCombiningCharacterSequence() already handles to BMP variation selectors (U+180B, U+180C, U+180D, U+FE00…U+FE0F) correctly. It is just not set up to handle non-BMP marks. You should change the loop in advanceByCombiningCharacterSequence() so that it gets and iterates by full Unicode character instead of UTF-16 code unit.

Comment 14 Kenichi Ishibashi 2012-01-24 20:47:39 PST

Created attachment 123878 [details]
Patch

Comment 15 Kenichi Ishibashi 2012-01-24 20:49:23 PST

(In reply to comment #13)
> (From update of attachment 123502 [details])
> Thanks for the patch. All variation selectors are combining marks, so there is no need to treat them specially. In particular, advanceByCombiningCharacterSequence() already handles to BMP variation selectors (U+180B, U+180C, U+180D, U+FE00…U+FE0F) correctly. It is just not set up to handle non-BMP marks. You should change the loop in advanceByCombiningCharacterSequence() so that it gets and iterates by full Unicode character instead of UTF-16 code unit.

Updated the patch. Thank you for review and detailed suggestion!

Comment 16 mitz 2012-01-24 21:14:14 PST

Comment on attachment 123878 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:217
> +        if (U16_IS_SURROGATE(nextCharacter)) {
> +            if (!U16_IS_LEAD(nextCharacter) || markIterator == end)
> +                return false;
> +            UChar trail = *markIterator++;
> +            if (!U16_IS_TRAIL(trail))
> +                return false;
> +            nextCharacter = U16_GET_SUPPLEMENTARY(nextCharacter, trail);
> +        }

I think this can be written more succinctly, as long as you’re adding a local variable, using U16_NEXT.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:220
>          markCount++;

I think despite markCount’s misleading name, if you look at how it’s used, you’ll see that it should be incremented by the number of UChars, i.e. 2 in the surrogate-pair case.

Comment 17 Kenichi Ishibashi 2012-01-24 22:29:17 PST

Created attachment 123885 [details]
Patch

Comment 18 Kenichi Ishibashi 2012-01-24 22:30:33 PST

Comment on attachment 123878 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:217
>> +        }
> 
> I think this can be written more succinctly, as long as you’re adding a local variable, using U16_NEXT.

Modified to use U16_NEXT.

>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:220
>>          markCount++;
> 
> I think despite markCount’s misleading name, if you look at how it’s used, you’ll see that it should be incremented by the number of UChars, i.e. 2 in the surrogate-pair case.

I misunderstood how it is used. Thank you for correction.

Comment 19 WebKit Review Bot 2012-01-25 04:33:05 PST

Comment on attachment 123885 [details]
Patch

Clearing flags on attachment: 123885

Committed r105866: <http://trac.webkit.org/changeset/105866>

Comment 20 WebKit Review Bot 2012-01-25 04:33:10 PST

All reviewed patches have been landed.  Closing bug.