Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2395883002:
Use another vector for installed fonts in CFX_FontMapper (Closed)
DescriptionSplit m_InstalledTTFonts into two vectors to avoid sketchy logic.
Instead of relying on ' ' to determine whether the CFX_Bytestring
is added on one place or another, use another vector. When trying
to match fonts from the fontmapper, compare with both vectors.
BUG=pdfium:510
Committed: https://pdfium.googlesource.com/pdfium/+/065c35006d96eaca324f49248d20d83709a25fbe
Patch Set 1 #
Total comments: 6
Patch Set 2 : Return to vectors #Patch Set 3 : Fix win #Patch Set 4 : Fix win 2 #
Total comments: 1
Patch Set 5 : Use std::pair #
Total comments: 3
Patch Set 6 : Nits #
Messages
Total messages: 36 (24 generated)
The CQ bit was checked by npm@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...
Description was changed from ========== Use std::map for installed fonts in CFX_FontMapper Changing from std::vector to std::map should do the following, if there are A calls to AddInstalledFont and M to MatchInstalledFonts: - Speed of CFX_FontMapper::AddInstalledFont: O(1) -> O(logA) - Speed of CFX_FontMapper::MatchInstalledFonts: O(A) -> O(logA) - Make unnecessary the sketchy logic stated in the bug below. Although usually A > M, this is probably faster. BUG=pdfium:510 ========== to ========== Use std::map for installed fonts in CFX_FontMapper Changing from std::vector to std::map should do the following, if there are A calls to AddInstalledFont and M to MatchInstalledFonts: - Speed of CFX_FontMapper::AddInstalledFont: O(1) -> O(logA) - Speed of CFX_FontMapper::MatchInstalledFonts: O(A) -> O(logA) - Make unnecessary the sketchy logic stated in the bug below. Although usually A > M, this is probably faster. BUG=pdfium:510 ==========
npm@chromium.org changed reviewers: + dsinclair@chromium.org, thestig@chromium.org, tsepez@chromium.org
PTAL
https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... File core/fxge/ge/cfx_fontmapper.cpp (left): https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:373: CFX_ByteString norm1 = TT_NormalizeName(m_InstalledTTFonts[i].c_str()); Previously if TT_NormalizeName(new_name) matched norm_name we'd return new_name, now we return name, is that intentional? https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... File core/fxge/ge/cfx_fontmapper.cpp (right): https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:351: m_InstalledTTFonts[TT_NormalizeName(new_name.c_str())] = name; Should this name be new_name?
https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... File core/fxge/ge/cfx_fontmapper.cpp (left): https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:372: for (i = pdfium::CollectionSize<int>(m_InstalledTTFonts) - 1; i >= 0; i--) { I worry about the order being significant as we move to maps. This loop would be far simpler if we did it in the other order which makes me wonder why its done this way. Can there be multiple entries per normalize name?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... File core/fxge/ge/cfx_fontmapper.cpp (left): https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:372: for (i = pdfium::CollectionSize<int>(m_InstalledTTFonts) - 1; i >= 0; i--) { On 2016/10/05 21:17:34, Tom Sepez wrote: > I worry about the order being significant as we move to maps. This loop would be > far simpler if we did it in the other order which makes me wonder why its done > this way. Can there be multiple entries per normalize name? The order here is relevant in the sense that we match to the latest added. With the map it should be the same, since I replace the entry without checking if it has already been filled. So the map always contains the latest added. https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:373: CFX_ByteString norm1 = TT_NormalizeName(m_InstalledTTFonts[i].c_str()); On 2016/10/05 21:12:07, dsinclair wrote: > Previously if TT_NormalizeName(new_name) matched norm_name we'd return new_name, > now we return name, is that intentional? On previous line 381, the index is increased by 1. So when matching TT_NormalizeName(new_name), we would return name (at least I think that's the intended logic here). https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... File core/fxge/ge/cfx_fontmapper.cpp (right): https://codereview.chromium.org/2395883002/diff/1/core/fxge/ge/cfx_fontmapper... core/fxge/ge/cfx_fontmapper.cpp:351: m_InstalledTTFonts[TT_NormalizeName(new_name.c_str())] = name; On 2016/10/05 21:12:07, dsinclair wrote: > Should this name be new_name? No. See comment below
Isn't there some use of m_InstalledTTFonts in core/fxge/win32/fx_win32_device.cpp ?
On 2016/10/05 21:27:36, Tom Sepez wrote: > Isn't there some use of m_InstalledTTFonts in > core/fxge/win32/fx_win32_device.cpp ? I completely missed this. There they match in the other order. While I would argue that the order does not matter (repeated font names), maybe for now it's best to not use map for now. I'll just fix the bug.
The CQ bit was checked by npm@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...
Description was changed from ========== Use std::map for installed fonts in CFX_FontMapper Changing from std::vector to std::map should do the following, if there are A calls to AddInstalledFont and M to MatchInstalledFonts: - Speed of CFX_FontMapper::AddInstalledFont: O(1) -> O(logA) - Speed of CFX_FontMapper::MatchInstalledFonts: O(A) -> O(logA) - Make unnecessary the sketchy logic stated in the bug below. Although usually A > M, this is probably faster. BUG=pdfium:510 ========== to ========== Split m_InstalledTTFonts into two vectors to avoid sketchy logic. Instead of relying on ' ' to determine whether the CFX_Bytestring is added on one place or another, use another vector. When trying to match fonts from the fontmapper, compare with both vectors. BUG=pdfium:510 ==========
The CQ bit was checked by npm@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_no_v8 on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/win_no_v8/build...)
The CQ bit was checked by npm@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...
PTAL. Abandoned map and now using another vector.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2395883002/diff/60001/core/fxge/cfx_fontmapper.h File core/fxge/cfx_fontmapper.h (right): https://codereview.chromium.org/2395883002/diff/60001/core/fxge/cfx_fontmappe... core/fxge/cfx_fontmapper.h:48: std::vector<CFX_ByteString> m_LocalizedTTFonts; are we sure we don't just want std::vector<std::pair<CFX_ByteString, CFX_ByteString>> or am I missing something?
The CQ bit was checked by npm@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...
PTAL, using vector of pairs
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is quite good. LGTM after fixing nits. https://codereview.chromium.org/2395883002/diff/80001/core/fxge/ge/cfx_fontma... File core/fxge/ge/cfx_fontmapper.cpp (right): https://codereview.chromium.org/2395883002/diff/80001/core/fxge/ge/cfx_fontma... core/fxge/ge/cfx_fontmapper.cpp:350: if (!new_name.IsEmpty()) { nit: no {} https://codereview.chromium.org/2395883002/diff/80001/core/fxge/ge/cfx_fontma... core/fxge/ge/cfx_fontmapper.cpp:374: break; nit: just return m_InstalledTTFonts[i] here? https://codereview.chromium.org/2395883002/diff/80001/core/fxge/ge/cfx_fontma... core/fxge/ge/cfx_fontmapper.cpp:382: break; nit: same idea here.
The CQ bit was checked by npm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2395883002/#ps100001 (title: "Nits")
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 ========== Split m_InstalledTTFonts into two vectors to avoid sketchy logic. Instead of relying on ' ' to determine whether the CFX_Bytestring is added on one place or another, use another vector. When trying to match fonts from the fontmapper, compare with both vectors. BUG=pdfium:510 ========== to ========== Split m_InstalledTTFonts into two vectors to avoid sketchy logic. Instead of relying on ' ' to determine whether the CFX_Bytestring is added on one place or another, use another vector. When trying to match fonts from the fontmapper, compare with both vectors. BUG=pdfium:510 Committed: https://pdfium.googlesource.com/pdfium/+/065c35006d96eaca324f49248d20d83709a2... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://pdfium.googlesource.com/pdfium/+/065c35006d96eaca324f49248d20d83709a2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
