Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1751883004:
SkFontHost_FreeType constructor to correctly release resources. (Closed)
|
Created:
4 years, 9 months ago by bungeman-skia Modified:
4 years, 9 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkFontHost_FreeType constructor to correctly release resources.
BUG=chromium:589848
Committed: https://skia.googlesource.com/skia/+/aabd71cc2c3313c51e618224426045f8573e69e6
Patch Set 1 #
Total comments: 4
Messages
Total messages: 14 (7 generated)
Description was changed from ========== SkFontHost_FreeType constructor to correctly release resources. BUG=chromium:589848 ========== to ========== SkFontHost_FreeType constructor to correctly release resources. BUG=chromium:589848 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1751883004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1751883004/1
bungeman@google.com changed reviewers: + benjaminwagner@google.com, herb@google.com
https://codereview.chromium.org/1751883004/diff/1/src/ports/SkFontHost_FreeTy... File src/ports/SkFontHost_FreeType.cpp (left): https://codereview.chromium.org/1751883004/diff/1/src/ports/SkFontHost_FreeTy... src/ports/SkFontHost_FreeType.cpp:905: fFace = nullptr; The related bug was because we went down this path. The fFace is being set to nullptr (so that this->success() would return false) but unref_ft_face(fFace) is not being called. This leaves the face cache dirty. https://codereview.chromium.org/1751883004/diff/1/src/ports/SkFontHost_FreeTy... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/1751883004/diff/1/src/ports/SkFontHost_FreeTy... src/ports/SkFontHost_FreeType.cpp:175: SkASSERT(nullptr == gFaceRecHead); Adding this assert would have helped find this a bit faster. It also documents something assumed anyway, which is that if the library is going away then all the faces should be done. https://codereview.chromium.org/1751883004/diff/1/src/ports/SkFontHost_FreeTy... src/ports/SkFontHost_FreeType.cpp:681: //the total matrix is not taken into account here. All of the 'onFilterRec' implementations should be using this opportunity to call computeMatrices and normalize everything. This would allow for better checking here, as well as result in more cache hits.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== SkFontHost_FreeType constructor to correctly release resources. BUG=chromium:589848 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkFontHost_FreeType constructor to correctly release resources. BUG=chromium:589848 ==========
The CQ bit was checked by herb@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1751883004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1751883004/1
Message was sent while issue was closed.
Description was changed from ========== SkFontHost_FreeType constructor to correctly release resources. BUG=chromium:589848 ========== to ========== SkFontHost_FreeType constructor to correctly release resources. BUG=chromium:589848 Committed: https://skia.googlesource.com/skia/+/aabd71cc2c3313c51e618224426045f8573e69e6 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/aabd71cc2c3313c51e618224426045f8573e69e6
Message was sent while issue was closed.
https://codereview.chromium.org/1751883004/diff/1/src/ports/SkFontHost_FreeTy... File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.chromium.org/1751883004/diff/1/src/ports/SkFontHost_FreeTy... src/ports/SkFontHost_FreeType.cpp:901: return size; ignorable nit: Seems like it would be simpler to default-construct ftSize, change the lambda to a block, and make this line ftSize.reset(size). Then also remove the duplicate nullptr check below. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
