Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277323002:
Simplify some code in CFX_FontMapper. (Closed)
DescriptionSimplify some code in CFX_FontMapper.
Committed: https://pdfium.googlesource.com/pdfium/+/00edbe4b5b47d8007ba2de97722170acffbf8fa2
Patch Set 1 #
Total comments: 11
Patch Set 2 : nits #Messages
Total messages: 18 (11 generated)
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thestig@chromium.org changed reviewers: + npm@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... File core/fxge/ge/cfx_fontmapper.cpp (right): https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:275: const int kExternalFontIndex = 12; Do we have any conventions on the placement of variables on namespace? https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:410: m_pFontMgr->GetBuiltinFont(14, &pFontData, &size); Replace 14 with kExternalFontIndex + 2 https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:419: m_pFontMgr->GetBuiltinFont(15, &pFontData, &size); kExternalFontIndex + 3 https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:444: if (m_FoxitFaces[12]) kExternalFontIndex for this and the 12's just below https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:456: if (m_FoxitFaces[13]) Similarly, kExternalFontIndex+1 here https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:660: if (m_FoxitFaces[12]) kExternalFontIndex here and below https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:790: uint32_t* pBuffer = reinterpret_cast<uint32_t*>(&buffer); Is this the same as uint32_t* pBuffer = reinterpret_cast<uint32_t*>(buffer);
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... File core/fxge/ge/cfx_fontmapper.cpp (right): https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:275: const int kExternalFontIndex = 12; On 2016/08/25 17:43:30, npm wrote: > Do we have any conventions on the placement of variables on namespace? Oh, I can move it to the top if that's what you mean. https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:410: m_pFontMgr->GetBuiltinFont(14, &pFontData, &size); On 2016/08/25 17:43:30, npm wrote: > Replace 14 with kExternalFontIndex + 2 I haven't looked, so I don't know if the index passed to CFX_FontMgr has the same meaning. Let's punt this to later? Same below. https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:790: uint32_t* pBuffer = reinterpret_cast<uint32_t*>(&buffer); On 2016/08/25 17:43:30, npm wrote: > Is this the same as > uint32_t* pBuffer = reinterpret_cast<uint32_t*>(buffer); Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... File core/fxge/ge/cfx_fontmapper.cpp (right): https://codereview.chromium.org/2277323002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:275: const int kExternalFontIndex = 12; On 2016/08/25 18:41:37, Lei Zhang wrote: > On 2016/08/25 17:43:30, npm wrote: > > Do we have any conventions on the placement of variables on namespace? > > Oh, I can move it to the top if that's what you mean. My understanding is there's 14 base fonts: first 12 + Symbol + Zapf Dingbats (last 2 handled separately in the code). And we also have Chrome Sans/Serif to be used as last resort substitute fonts.
On 2016/08/25 19:01:50, npm wrote: > My understanding is there's 14 base fonts: first 12 + Symbol + Zapf Dingbats > (last 2 handled separately in the code). And we also have Chrome Sans/Serif to > be used as last resort substitute fonts. Yep. I'm sure this code could use a few more rounds of cleanups.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Simplify some code in CFX_FontMapper. ========== to ========== Simplify some code in CFX_FontMapper. Committed: https://pdfium.googlesource.com/pdfium/+/00edbe4b5b47d8007ba2de97722170acffbf... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://pdfium.googlesource.com/pdfium/+/00edbe4b5b47d8007ba2de97722170acffbf... |
