[Webkit-unassigned] [Bug 16768] Position and thickness of underline as text size changes

[bugzilla-daemon at webkit.org]

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


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #31915|review?                     |review-
               Flag|                            |




--- Comment #15 from Eric Seidel <eric at webkit.org>  2009-08-06 16:51:13 PDT ---
(From update of attachment 31915)
The ChangeLog detail is very helpful, thank you.

Is it possible to do this in smaller pieces?  That would make review much
easier.

I dont' think we generally use L"" constants.  Maybe we do and I just don't
realize.

 413         // This list is from Firefox.
Depending on the license, we may not be able to just copy the code...

Indent:
 470     if (!fontNameSet) {
 471       fontNameSet = new HashSet<String>;
 472       size_t numElements = sizeof(badFonts) / sizeof(badFonts[0]);
 473       for (size_t i = 0; i < numElements; ++i)
 474           fontNameSet->add(badFonts[i]);
 475     }

 58 bool hasBadUnderlineMetrics(const String& name);
That should be fontName, not just "name", or is it a fontFamily?  If so, we
tend to use AtomicString for font family names, at least that how they're
stored on FontDescription.

Seems all the code you added to paintTextDecorations should be a static inline
function.  Something like paintUnderline(...)  No need to make
paintTextDecorations so much bigger.

Normally we don't use "tmp" in variable names:
 989                             int tmpPosition;
 990                             int tmpThickness;

The extra context save/restores could slow down this code path.

Honestly, this change is rather large, and difficult for me to review, so since
it's languishing in the queue I'm sorta looking for reasons to r- it.  You'll
have more luck with smaller changes.  Also I think you may in the en need to
involve Dave Hyatt or Dan Bernstien in the final review.

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