[webkit-reviews] review denied: [Bug 16768] Position and thickness of underline as text size changes : [Attachment 31915] font_underlining_patch_3

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Yusuke Sato <yusukes at chromium.org>'s
request for review:
Bug 16768: Position and thickness of underline as text size changes
https://bugs.webkit.org/show_bug.cgi?id=16768

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.