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

[bugzilla-daemon at webkit.org]

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





------- Comment #2 from eric at webkit.org  2009-05-14 22:12 PDT -------
(From update of attachment 29298)
Various style issues.

1.  "helper" is not a very discriptive name. :)  Maybe TextRunWalkerHarfbuzz 
TexRunWalker TextRunIterator?  Since the comment seems to imply its job is to
walk text runs. :)

Lots of foo_bar style violations.  fooBar in WK

It seems HarfbuzzHelper (renamed?) could have its own header (and cpp?) file. 
It's big enough.

Sad that HB_ShaperItem can't use OwnPtr. :(

Style:
 264     bool NextRTLScriptRun()

 307     ssize_t m_iter;  // indexes the next word in |m_run|
We generally try to use full words like m_indexOfNextWord?

 310     unsigned m_width;  // width (in px) of the current script run
m_widthInPx;  m_pixelWidth;

It seems all these things which pertain to m_run should be grouped with it, no?
(and least in ordering, if not also in structure)

I assume this is in no way temporary?
 321     const FontPlatformData& font = fontDataForCharacters(run.characters(),
run.length())->fontDataForCharacter(' ')->platformData();

You probably want to grab off gc->platformContext() since you use it so much:
 323     SkCanvas* canvas = gc->platformContext()->canvas();

I've not seen other parts of the WebCore code use "const" for local variables:
 325     const bool fill = textMode & cTextFill;

Really?
377     // (Mac code ignores includePartialGlyphs, and they don't know what
it's
 378     // supposed to do, so we just ignore it as well.)
ignored arguments generally cause warnings in the compiler, no?

What is "i" here?
 424             int i;
 425             if (harfbuzz.rtl()) {
Needs a better name.


-- 
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.