[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





--- Comment #22 from Yusuke Sato <yusukes at chromium.org>  2009-08-30 03:34:31 PDT ---
> Is it possible to do this in smaller pieces?  That would make review much
> easier.

I've split the previous change into 5 pieces. Could you take another look?

 1. get_underline_metrics_interface_v1
 2. get_underline_metrics_mac_impl_v1 (depends on 1. Safari Mac implementation
of 1.)
 3. calc_underline_metrics_for_a_textrun_v1 (depends on 1.)
 4. draw_underline_v1 (depends on 3.)
 5. underline_rebaseline_v1 (rebaselined pixel tests)

I've dropped Safari Win and Chromium Win implementations from the changes above
this time for simplicity. After having submitted the changes, I'll ask you to
review these extra implementations.

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

I read the document of Mozilla
(http://kb.mozillazine.org/Font.blacklist.underline_offset) and reused the list
of blacklisted font family names, but didn't copy the code. So I guess there's
no license issue.

That said, this time I've dropped changes against OpenTypeUtilities.cpp (for
simplicity, either), since the hack I added to the OpenTypeUtilities seems not
to be indispensable for most Safari/Mac users. I'll add the blacklisting hack
later if necessary.

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

Done.

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

Fixed.

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

Done. Removed all saves and restores. I think the change list #4 is now fairly
small.

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