Bug 183339 - Change the type of SVGToOTFFontConverter::m_weight to be not a char
Summary: Change the type of SVGToOTFFontConverter::m_weight to be not a char
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-05 11:17 PST by Said Abou-Hallawa
Modified: 2018-03-10 17:52 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.62 KB, patch)
2018-03-05 20:16 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2018-03-05 11:17:07 PST
This is a follow up for Darin's comment in https://bugs.webkit.org/show_bug.cgi?id=183165#c2. If m_weight is negative, clampTo<uint16_t> or static_cast<uint16_t> will give the wrong value.
Comment 1 Myles C. Maxfield 2018-03-05 20:03:21 PST
m_result is a Vector<char> because that's what SharedBuffer::create() accepts.
Comment 2 Myles C. Maxfield 2018-03-05 20:16:11 PST
Created attachment 335067 [details]
Patch
Comment 3 WebKit Commit Bot 2018-03-06 10:35:54 PST
Comment on attachment 335067 [details]
Patch

Clearing flags on attachment: 335067

Committed r229328: <https://trac.webkit.org/changeset/229328>
Comment 4 WebKit Commit Bot 2018-03-06 10:36:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Radar WebKit Bug Importer 2018-03-06 10:37:21 PST
<rdar://problem/38185027>
Comment 6 Darin Adler 2018-03-06 20:36:46 PST
(In reply to Myles C. Maxfield from comment #1)
> m_result is a Vector<char> because that's what SharedBuffer::create()
> accepts.

At some point I intend to change Vector<char> to Vector<uint8_t> throughout WebKit for this sort of thing.
Comment 7 Myles C. Maxfield 2018-03-06 20:46:18 PST
(In reply to Darin Adler from comment #6)
> (In reply to Myles C. Maxfield from comment #1)
> > m_result is a Vector<char> because that's what SharedBuffer::create()
> > accepts.
> 
> At some point I intend to change Vector<char> to Vector<uint8_t> throughout
> WebKit for this sort of thing.

Yes! πŸ‘
Comment 8 Darin Adler 2018-03-10 17:52:34 PST
Comment on attachment 335067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335067&action=review

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:528
> +                if (ok && value >= std::numeric_limits<uint8_t>::min() && value <= std::numeric_limits<uint8_t>::max())

If this pattern is common I think we should add a helper function.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:1464
> +                m_weight = std::max(std::min((value + 50) / 100, static_cast<int>(std::numeric_limits<uint8_t>::max())), static_cast<int>(std::numeric_limits<uint8_t>::min()));

This is what clampTo is for, and it’s much easier to read:

    m_weight = clampTo<uint8_t>((value + 50) / 100);