[webkit-reviews] review denied: [Bug 124057] Characters too close together in complex Arabic text : [Attachment 216515] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 124057: Characters too close together in complex Arabic text
https://bugs.webkit.org/show_bug.cgi?id=124057

Attachment 216515: Patch
https://bugs.webkit.org/attachment.cgi?id=216515&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=216515&action=review


review- because this puts a Mac-specific result into a platform-independent
test result folder.

> Source/WebCore/ChangeLog:3
> +	   Characters too close together in complex Arabic text

Bug title should say [Mac] since this is a bug and fix in Mac-specific code.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:197
> +	       CGFloat adjustedAdvance = 0;
> +	       if (!index)
> +		   adjustedAdvance += complexTextRun.initialAdvance().width;
> +	       adjustedAdvance += m_adjustedAdvances[index].width;

Since addition is commutative, even for floating point, I’d write it like this
instead:

    CGFloat adjustedAdvance = m_adjustedAdvances[index].width;
    if (!index)
	adjustedAdvance += complexTextRun.initialAdvance().width;

> LayoutTests/ChangeLog:21
> +	   doesn't crash when there is an initial advance. HOWEVER: Note that
with kerning and
> +	   ligatures turned off, CoreText doesn't appear to ever create text
runs with nonzero
> +	   initial advances. This actually causes glyphs to be drawn
differently (Without kerning
> +	   and ligatures, the accent in this test is drawn on the left side of
the character).
> +	   Therefore, this code change can't actually be tested without kerning
and ligatures.
> +	   It's possible to to turn on kerning and ligatures by writing
defaults such that
> +	   [[NSUserDefaults standardUserDefaults]
boolForKey:@"WebKitKerningAndLigaturesEnabledByDefault"]
> +	   returns true in DumpRenderTree.

We need a test that actually runs, not one that an expert could run in some
special way. If necessary, we will need to add a way to turn on kerning and
ligatures from inside a test. Or figure out some good strategy to make sure
that we test both code paths.

> LayoutTests/ChangeLog:27
> +	   *
fast/text/complex-grapheme-cluster-with-initial-advance-expected.html: Added.
> +	   * fast/text/complex-grapheme-cluster-with-initial-advance.html:
Added.
> +	   * fast/text/selection-in-initial-advance-region-expected.txt: added
> +	   * fast/text/selection-in-initial-advance-region.html: added

Is it really OK to put these tests in across platforms if the fix is only for
the Mac? Do these tests pass on the other platforms? We certainly can’t expect
the render tree result to work on other platforms, so it should not be checked
into the fast/text directory; instead it should be in platform/mac/fast/text,
but there may be a better approach.

> LayoutTests/fast/text/selection-in-initial-advance-region.html:2
> +<div id="div" dir="RTL" style="font-size: 100pt;"><span id="c"
style="background: yellow;">&#1604;&#1575;&#1611;</span></div>

Seems strange to use "pt" here instead of "px".

> LayoutTests/fast/text/selection-in-initial-advance-region.html:3
> +<ul id="console"></ul>

A little strange to use a <ul> where we will have a bullet in front of each
line in the console.

> LayoutTests/fast/text/selection-in-initial-advance-region.html:12
> +if (window.testRunner) {

This test doesn’t actually use testRunner, so this if statement should say:

if (window.eventSender)

Also, I prefer to put the test inside a function rather than just having it all
be top level code.

> LayoutTests/fast/text/selection-in-initial-advance-region.html:28
> +    var steps = 20;

Where does the magic number 20 come from?

> LayoutTests/fast/text/selection-in-initial-advance-region.html:34
> +    eventSender.mouseMoveTo(endx, endy);
> +    eventSender.mouseUp();

This test should do something to verify that appropriate text got selected, to
make it harder to accidentally succeed just by not successfully creating a
selection at all.

Then you could use dumpAsText and not require a pixel dump for the result. That
would make the text runnable on non-Mac platforms without requiring unique
results on each.

> LayoutTests/fast/text/selection-in-initial-advance-region.html:38
> +    log("This uses the eventSender to perform a drag select.  To run it
manually, click on the left half of the character and drag to the right half of
the character.");

Should probably say “slowly drag” since you could miss the crash if you drag to
fast. Is that right?