|
|
|
Created:
13 years, 4 months ago by abodenha Modified:
13 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/src/ Visibility:
Public. |
Normalize font BBox to make sure it's correct when rendering a PDF on the mac.
BUG=crbug.com/124572
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 1
Total messages: 7
|
abodenha
My first Skia change. Please let me know if I've done anything particularly stupid here.
|
13 years, 4 months ago (2012年08月28日 18:57:17 UTC) #1 |
My first Skia change. Please let me know if I've done anything particularly stupid here.
https://codereview.appspot.com/6501049/diff/1/ports/SkFontHost_mac_coretext.cpp File ports/SkFontHost_mac_coretext.cpp (right): https://codereview.appspot.com/6501049/diff/1/ports/SkFontHost_mac_coretext.c... ports/SkFontHost_mac_coretext.cpp:1620: } Use SkTSwap<uint16_t>(info->fBBox.fTop, info->fBBox.fBottom); instead.
https://codereview.appspot.com/6501049/diff/1/ports/SkFontHost_mac_coretext.cpp File ports/SkFontHost_mac_coretext.cpp (right): https://codereview.appspot.com/6501049/diff/1/ports/SkFontHost_mac_coretext.c... ports/SkFontHost_mac_coretext.cpp:1611: CGRect bbox = CTFontGetBoundingBox(ctFont); You may want to look at the other use of CTFontGetBoundingBox in this file. It uses some macros like CGRectGetMaxY_inline to compute the bounds. You might be able to use directly in the call to the SkIRect constructor.
Please take another look. https://codereview.appspot.com/6501049/diff/1/ports/SkFontHost_mac_coretext.cpp File ports/SkFontHost_mac_coretext.cpp (right): https://codereview.appspot.com/6501049/diff/1/ports/SkFontHost_mac_coretext.c... ports/SkFontHost_mac_coretext.cpp:1611: CGRect bbox = CTFontGetBoundingBox(ctFont); Good call. That's much cleaner than what I had here. On 2012年08月28日 19:06:01, Steve VanDeBogart wrote: > You may want to look at the other use of CTFontGetBoundingBox in this file. It > uses some macros like CGRectGetMaxY_inline to compute the bounds. You might be > able to use directly in the call to the SkIRect constructor. https://codereview.appspot.com/6501049/diff/1/ports/SkFontHost_mac_coretext.c... ports/SkFontHost_mac_coretext.cpp:1620: } Cool. I figured there was a swap utility somewhere. Thanks. On 2012年08月28日 19:00:36, caryclark1 wrote: > Use > > SkTSwap<uint16_t>(info->fBBox.fTop, info->fBBox.fBottom); > > instead.
I assume you tested this change and it gave you the values you expected? LGTM I'll land the fix. https://codereview.appspot.com/6501049/diff/4001/ports/SkFontHost_mac_coretex... File ports/SkFontHost_mac_coretext.cpp (right): https://codereview.appspot.com/6501049/diff/4001/ports/SkFontHost_mac_coretex... ports/SkFontHost_mac_coretext.cpp:1613: CGToScalar( CGRectGetMinX_inline(bbox)), // Left nit: extra space.
Landed as r5325 FYI: your patch was one level to high, path should have been src/ports/SkFontHost_mac_coretext.cpp
On Tue, Aug 28, 2012 at 12:40 PM, <vandebo@chromium.org> wrote: > I assume you tested this change and it gave you the values you expected? > > Yep. > LGTM > > I'll land the fix. > > > https://codereview.appspot.**com/6501049/diff/4001/ports/** > SkFontHost_mac_coretext.cpp<https://codereview.appspot.com/6501049/diff/4001/ports/SkFontHost_mac_coretext.cpp> > File ports/SkFontHost_mac_coretext.**cpp (right): > > https://codereview.appspot.**com/6501049/diff/4001/ports/** > SkFontHost_mac_coretext.cpp#**newcode1613<https://codereview.appspot.com/6501049/diff/4001/ports/SkFontHost_mac_coretext.cpp#newcode1613> > ports/SkFontHost_mac_coretext.**cpp:1613: CGToScalar( > CGRectGetMinX_inline(bbox)), // Left > nit: extra space. > > https://codereview.appspot.**com/6501049/<https://codereview.appspot.com/6501... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org