Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2479313005:
Add font substituted metrics for OS other than Linux (Closed)
DescriptionAdd font substituted metrics for OS other than Linux
Created a struct FPDF_SYSFONTINFO_WITHMETRICS to be able to call and
override methods from the default system font infos, for OS's other
than Linux. This was necessary because the function pointers in the
struct contain pointers to the struct. FPDF_GetDefaultSystemFontInfo
returns a pointer to a FPDF_SYSFONTINFO, but in reality it is a
FPDF_SYSFONTINFO_DEFAULT. So the overriden methods need to receive
as input the pointer given by FPDF_GetDefaultSystemFontInfo.
BUG=448584
Committed: https://crrev.com/394c06613bf8080b922094a048567d93135521c0
Cr-Commit-Position: refs/heads/master@{#431103}
Patch Set 1 #Patch Set 2 : Compile on Mac #Patch Set 3 : Override all because struct pointer #
Total comments: 11
Patch Set 4 : Address comments #
Total comments: 2
Patch Set 5 : Use FPDF_FreeDefault #
Total comments: 2
Patch Set 6 : Move ifs inside methods #
Total comments: 6
Patch Set 7 : Nits #
Total comments: 1
Patch Set 8 : Fix Delete! #
Messages
Total messages: 36 (16 generated)
Description was changed from ========== Add font substituted metrics for fonts other than Linux BUG=448584 ========== to ========== Add font substituted metrics for fonts other than Linux Created a struct FPDF_SYSFONTINFO_WITHMETRICS to be able to call and override methods from the default system font infos, for OS's other than Linux. This was necessary because the function pointers in the struct contain pointers to the struct. FPDF_GetDefaultSystemFontInfo returns a pointer to a FPDF_SYSFONTINFO, but in reality it is a FPDF_SYSFONTINFO_DEFAULT. So the overriden methods need to receive as input the pointer given by FPDF_GetDefaultSystemFontInfo. BUG=448584 ==========
npm@chromium.org changed reviewers: + dsinclair@chromium.org, thestig@chromium.org
Description was changed from ========== Add font substituted metrics for fonts other than Linux Created a struct FPDF_SYSFONTINFO_WITHMETRICS to be able to call and override methods from the default system font infos, for OS's other than Linux. This was necessary because the function pointers in the struct contain pointers to the struct. FPDF_GetDefaultSystemFontInfo returns a pointer to a FPDF_SYSFONTINFO, but in reality it is a FPDF_SYSFONTINFO_DEFAULT. So the overriden methods need to receive as input the pointer given by FPDF_GetDefaultSystemFontInfo. BUG=448584 ========== to ========== Add font substituted metrics for OS other than Linux Created a struct FPDF_SYSFONTINFO_WITHMETRICS to be able to call and override methods from the default system font infos, for OS's other than Linux. This was necessary because the function pointers in the struct contain pointers to the struct. FPDF_GetDefaultSystemFontInfo returns a pointer to a FPDF_SYSFONTINFO, but in reality it is a FPDF_SYSFONTINFO_DEFAULT. So the overriden methods need to receive as input the pointer given by FPDF_GetDefaultSystemFontInfo. BUG=448584 ==========
PTAL
https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:311: FPDF_SYSFONTINFO_WITHMETRICS(FPDF_SYSFONTINFO* default_sysfontinfo) { explicit https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:319: void* MapFontWithMetrics(struct _FPDF_SYSFONTINFO* sysfontinfo, Just FPDF_SYSFONTINFO* https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:605: FPDF_SYSFONTINFO_WITHMETRICS* g_font_info = not a global https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:606: new FPDF_SYSFONTINFO_WITHMETRICS(FPDF_GetDefaultSystemFontInfo()); Does this get freed or if it a one time leak? https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:612: if (g_font_info->default_sysfontinfo_->MapFont) This is the only one you actual care about, right? Maybe grab the return value from FPDF_GetDefaultSystemFontInfo(), check this first, and if it's nullptr, then don't even bother with FPDF_SYSFONTINFO_WITHMETRICS? Or is it the case that it's never nullptr?
Do you think we have enough font code that we can move it all to its own file in a clean way?
I could move the font code, except the new file would need to know the PDFiumEngine, as both this and the font code use g_last_instance_id and g_engine_for_fontmapper (and maybe other stuff from the namespace). https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:319: void* MapFontWithMetrics(struct _FPDF_SYSFONTINFO* sysfontinfo, On 2016/11/08 19:16:10, Lei Zhang (slow) wrote: > Just FPDF_SYSFONTINFO* The "_" is used on the Linux overrides in this file, although I don't get why (I assume it makes no difference). https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:605: FPDF_SYSFONTINFO_WITHMETRICS* g_font_info = On 2016/11/08 19:16:10, Lei Zhang (slow) wrote: > not a global What do you mean? Declare pointer in namespace? https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:606: new FPDF_SYSFONTINFO_WITHMETRICS(FPDF_GetDefaultSystemFontInfo()); On 2016/11/08 19:16:10, Lei Zhang (slow) wrote: > Does this get freed or if it a one time leak? We FPDF_SetSystemFontInfo on it, and Release is called on https://cs.chromium.org/chromium/src/third_party/pdfium/fpdfsdk/fpdf_sysfonti... But that doesn't delete the struct itself? I might need to call some struct deleter in ShutdownSDK. https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:612: if (g_font_info->default_sysfontinfo_->MapFont) On 2016/11/08 19:16:10, Lei Zhang (slow) wrote: > This is the only one you actual care about, right? Maybe grab the return value > from FPDF_GetDefaultSystemFontInfo(), check this first, and if it's nullptr, > then don't even bother with FPDF_SYSFONTINFO_WITHMETRICS? Or is it the case that > it's never nullptr? Actually, the default always gets assigned all of these functions, so I think I can just remove all those ifs. But this is because I looked at our internal code... current ifs are based on the documentation. WDYT?
On 2016/11/08 19:54:36, npm wrote: > I could move the font code, except the new file would need > to know the PDFiumEngine, as both this and the font code > use g_last_instance_id and g_engine_for_fontmapper (and > maybe other stuff from the namespace). Sure. You may need some more getters and setters? You don't have to do it now, but just something to consider since the file is getting rather long.
https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:605: FPDF_SYSFONTINFO_WITHMETRICS* g_font_info = On 2016/11/08 19:54:36, npm wrote: > On 2016/11/08 19:16:10, Lei Zhang (slow) wrote: > > not a global > > What do you mean? Declare pointer in namespace? The variable is named g_foo, yet it is a local variable, not a global like the Linux version. https://codereview.chromium.org/2479313005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:612: if (g_font_info->default_sysfontinfo_->MapFont) On 2016/11/08 19:54:36, npm wrote: > On 2016/11/08 19:16:10, Lei Zhang (slow) wrote: > > This is the only one you actual care about, right? Maybe grab the return value > > from FPDF_GetDefaultSystemFontInfo(), check this first, and if it's nullptr, > > then don't even bother with FPDF_SYSFONTINFO_WITHMETRICS? Or is it the case > that > > it's never nullptr? > > Actually, the default always gets assigned all of these functions, so I think > I can just remove all those ifs. But this is because I looked at our > internal code... current ifs are based on the documentation. WDYT? I guess stick with it since we want to follow the documentation.
npm@chromium.org changed reviewers: + tsepez@chromium.org
PTAL (+Tom in case I'm doing something silly with delete)
https://codereview.chromium.org/2479313005/diff/60001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2479313005/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:323: ~FPDF_SYSFONTINFO_WITHMETRICS() { delete default_sysfontinfo_; } The public header says: Application should call FPDF_FreeMemory to free the returned ... Unfortuntely, FPDF_FreeMemory doesn't exist. The delete here is wrong because the FPDF_SYSFONTINFO, being a C-style struct, doesn't have a virtual destructor, and we're deleting it through a pointer to a derived type. I think we should add FPDF_FreeDefaultSystemFontInfo() to that API. It can then static_cast it to FPDF_SYSINFO_DEFAULT, and then delete it because the type will be correct.
https://codereview.chromium.org/2479313005/diff/60001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2479313005/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:323: ~FPDF_SYSFONTINFO_WITHMETRICS() { delete default_sysfontinfo_; } On 2016/11/08 21:25:52, Tom Sepez wrote: > The public header says: > Application should call FPDF_FreeMemory to free the returned ... > > Unfortuntely, FPDF_FreeMemory doesn't exist. > > The delete here is wrong because the FPDF_SYSFONTINFO, being a C-style struct, > doesn't have a virtual destructor, and we're deleting it through a pointer to a > derived type. > > I think we should add FPDF_FreeDefaultSystemFontInfo() to that API. > It can then static_cast it to FPDF_SYSINFO_DEFAULT, and then delete it because > the type will be correct. The Release() method deletes the extra stuff in FPDF_SYSFONTINFO_DEFAULT. So the point of the delete here is to delete the FPDF_SYSFONTINFO part. But that probably leaves holes compared to delete FPDF_SYSFONTINFO_DEFAULT struct, correct? If that is the case, then I'll need to implement that method. Otherwise I don't see a way to delete it, since FPDF_SYSFONTINFO_DEFAULT is not a known struct here.
> Otherwise I don't see a way to delete it, since FPDF_SYSFONTINFO_DEFAULT > is not a known struct here. Exactly. We've painted ourselves into a corner where there's no safe way to delete this at the moment. Deleting via the superclass pointer is undefined behaviour unless there's a virtual dtor, which there isn't. So we need code in some place else that can cast back to the subclass and then delete. Hence the API suggestion.
Rebased, and using FPDF_FreeDefaultSystemFontInfo in pdfium_engine.cc
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2479313005/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2479313005/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:617: if (g_font_info->default_sysfontinfo_->Release) nit: this is OK, but I think I'd be happier if we always set these callbacks, and had each one of them check to see if the underlying one still exists before calling it. The reason I say that is that -- and hopefully this doesn't happen in a sane world -- is that there isn't a reason to believe the pointers in the underlying default struct don't change over time -- they might become null after we've made this check.
LGTM otherwise
https://codereview.chromium.org/2479313005/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2479313005/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:617: if (g_font_info->default_sysfontinfo_->Release) On 2016/11/09 17:11:35, Tom Sepez wrote: > nit: this is OK, but I think I'd be happier if we always set these callbacks, > and had each one of them check to see if the underlying one still exists before > calling it. The reason I say that is that -- and hopefully this doesn't happen > in a sane world -- is that there isn't a reason to believe the pointers in the > underlying default struct don't change over time -- they might become null after > we've made this check. Done.
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...
lgtm https://codereview.chromium.org/2479313005/diff/100001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2479313005/diff/100001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:325: FPDF_SYSFONTINFO* default_sysfontinfo_; nit: In structs, no trailing underscore. https://codereview.chromium.org/2479313005/diff/100001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:352: fontinfo_with_metrics->default_sysfontinfo_->DeleteFont( Should we check the DeleteFont func ptr is not a nullptr for consistency? https://codereview.chromium.org/2479313005/diff/100001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:626: #if !defined(OS_LINUX) Can you flip this around to #if defined(OS_LINUX) ... ?
https://codereview.chromium.org/2479313005/diff/100001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2479313005/diff/100001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:325: FPDF_SYSFONTINFO* default_sysfontinfo_; On 2016/11/09 19:42:56, Lei Zhang (slow) wrote: > nit: In structs, no trailing underscore. Done. https://codereview.chromium.org/2479313005/diff/100001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:352: fontinfo_with_metrics->default_sysfontinfo_->DeleteFont( On 2016/11/09 19:42:56, Lei Zhang (slow) wrote: > Should we check the DeleteFont func ptr is not a nullptr for consistency? Well, like I mentioned, implementation is required according to documentation. But I could add the check. https://codereview.chromium.org/2479313005/diff/100001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:626: #if !defined(OS_LINUX) On 2016/11/09 19:42:56, Lei Zhang (slow) wrote: > Can you flip this around to #if defined(OS_LINUX) ... ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Fixing the delete and landing after testing in Mac https://codereview.chromium.org/2479313005/diff/120001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2479313005/diff/120001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:655: FPDF_FreeDefaultSystemFontInfo(g_font_info); Actually, this should be: FPDF_FreeDefaultSystemFontInfo(g_font_info->default_sysfontinfo); delete gFontInfo;
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, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2479313005/#ps140001 (title: "Fix Delete!")
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 ========== Add font substituted metrics for OS other than Linux Created a struct FPDF_SYSFONTINFO_WITHMETRICS to be able to call and override methods from the default system font infos, for OS's other than Linux. This was necessary because the function pointers in the struct contain pointers to the struct. FPDF_GetDefaultSystemFontInfo returns a pointer to a FPDF_SYSFONTINFO, but in reality it is a FPDF_SYSFONTINFO_DEFAULT. So the overriden methods need to receive as input the pointer given by FPDF_GetDefaultSystemFontInfo. BUG=448584 ========== to ========== Add font substituted metrics for OS other than Linux Created a struct FPDF_SYSFONTINFO_WITHMETRICS to be able to call and override methods from the default system font infos, for OS's other than Linux. This was necessary because the function pointers in the struct contain pointers to the struct. FPDF_GetDefaultSystemFontInfo returns a pointer to a FPDF_SYSFONTINFO, but in reality it is a FPDF_SYSFONTINFO_DEFAULT. So the overriden methods need to receive as input the pointer given by FPDF_GetDefaultSystemFontInfo. BUG=448584 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add font substituted metrics for OS other than Linux Created a struct FPDF_SYSFONTINFO_WITHMETRICS to be able to call and override methods from the default system font infos, for OS's other than Linux. This was necessary because the function pointers in the struct contain pointers to the struct. FPDF_GetDefaultSystemFontInfo returns a pointer to a FPDF_SYSFONTINFO, but in reality it is a FPDF_SYSFONTINFO_DEFAULT. So the overriden methods need to receive as input the pointer given by FPDF_GetDefaultSystemFontInfo. BUG=448584 ========== to ========== Add font substituted metrics for OS other than Linux Created a struct FPDF_SYSFONTINFO_WITHMETRICS to be able to call and override methods from the default system font infos, for OS's other than Linux. This was necessary because the function pointers in the struct contain pointers to the struct. FPDF_GetDefaultSystemFontInfo returns a pointer to a FPDF_SYSFONTINFO, but in reality it is a FPDF_SYSFONTINFO_DEFAULT. So the overriden methods need to receive as input the pointer given by FPDF_GetDefaultSystemFontInfo. BUG=448584 Committed: https://crrev.com/394c06613bf8080b922094a048567d93135521c0 Cr-Commit-Position: refs/heads/master@{#431103} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/394c06613bf8080b922094a048567d93135521c0 Cr-Commit-Position: refs/heads/master@{#431103} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
