[Webkit-unassigned] [Bug 25068] [Chromium] Complex text support for Chromium Linux

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25068


agl at chromium.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #31430|0                           |1
        is obsolete|                            |




------- Comment #9 from agl at chromium.org  2009-06-24 14:13 PDT -------
Created an attachment (id=31811)
 --> (https://bugs.webkit.org/attachment.cgi?id=31811&action=view)
patch

On Tue, Jun 23, 2009 at 5:47 PM, <bugzilla-daemon at webkit.org> wrote:
> Even though I have tried to verify the logic, it would be great if you got
> someone on the chromium team more familiar with text handling to look over the
> code.  Perhaps jshin or playmobil would be good choices.

jshin declined to review it. At this point, having something is better than
nothing. We're have security bugs which rely on our non-painting of complex
text.

> Why does this store m_paint and then never use it?

It does use it, but the code got moved to the constructor. I've removed the
member variable.

> 6 is scattered through this code.  (dividing by 64.)  I'm not sure why.
> It would be good to create a static const int/unsigned for this which will help
> explain what this is by its name and get rid of the 6's all over the place.

Have added a static function. I hope it doesn't introduce any signness bugs.

> Avoid abbreviations "pgc".
> Maybe graphicsContext instead?

I only did this because Eric asked. Changing it back then.

> (If you leave it like this, then the continuation lines should only be indented
> 4 spaces.)

The style guide has the operators lining up on the left hand side.

> reinterpret_cast<uint8_t*>(glyphs) + sizeof(uint16_t) * i
>
> This looks wrong, but I get it. What do you think about this instead?

Sorry, it is wrong. It should a char*, I thought that unsigned char* was
special cased in the strict alias rules but, now that I check, it doesn't
appear so.

>> +static void GlyphsToAdvances(HB_Font hbFont, const HB_Glyph* glyphs, hb_uint32 numGlyphs, HB_Fixed* advances, int flags)
>
> flags appears unused.  Is that a bug?

It's not used in the Harfbuzz reference code either.

> Why the use of fastMalloc vs plain old new?
> And if you switch to new, it would be good to adopt OwnPtr (from wtf/OwnPtr.h).
> You can think of this like a scoped_ptr.

I think I can use OwnArrayPtr.

>> +    SkPath path;
>> +    paint.getTextPath(&glyph16, sizeof(glyph16), 0, 0, &path);
>> +    int numPoints = path.getPoints(NULL, 0);
>
> Use 0 instead of NULL.

I know that style guide says this, but this is so manifestly wrong in this case
that I'm going to leave it.

>> +    switch (metric) {
>> +    case HB_FontAscent:
>> +        return SkiaScalarToHarfbuzzFixed(-skiaMetrics.fAscent);
>> +    default:
>> +        return 0;
>> +    }
>
> This looks odd to me.  Why a switch instead of an "if"? (since there is only
> one case statement)

I'm going to leave it as a switch statement because it makes it clearer that we
may need to support other values in the future.


AGL


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.