Record all fonts, not just one per charset.

Record all fonts, not just one per charset.

Attempting to open bug_434.pdf on my Linux box would fail with:

../../xfa/fxfa/app/xfa_fwltheme.cpp:96: virtual FWL_ERR
CXFA_FWLTheme::Initialize(): Assertion `__null != m_pCalendarFont' failed.

I tracked the regression back to [1].

The issue seems to be in CFX_FontManager::AddInstalledFont we will only add
one font for a given Charset. In my case I end up loading 6 charsets, but the
fonts are all strange ones. When I open the PDF, it fails to find 'Arial'
because I've registered these other fonts.

To fix this I changed the m_FaceArray into a struct of {name, chraset}. Then
we record all fonts into this list and search over that list for the charset
when needed. This allows bug_434.pdf to open and the test to pass successfully.

1- https://pdfium.googlesource.com/pdfium/+/fe73e7849b8b4ce49408d2f52f3fc29b370b82b5

Committed: https://pdfium.googlesource.com/pdfium/+/ea98238666e33cd16b69cb23dcaca047c21c9998

[dsinclair, 2016-04-07 19:03:21]

dsinclair@chromium.org changed reviewers:
+ jun_fang@foxitsoftware.com, thestig@chromium.org, tsepez@chromium.org

[dsinclair, 2016-04-07 19:03:22]

PTAL.

This is working, although I don't know if there are unintended consequences.....

https://codereview.chromium.org/1874433002/diff/1/core/fxge/ge/fx_ge_fontmap.cpp
File core/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1874433002/diff/1/core/fxge/ge/fx_ge_fontmap....
core/fxge/ge/fx_ge_fontmap.cpp:749: m_FaceArray.push_back(name);
This bit is the actual code change, m_FaceArray is moved out of the above if()
to always execute.

[Tom Sepez, 2016-04-08 15:36:14]

Description was changed from

==========
Record all fonts, not just one per charset.

Attempting to open bug_434.pdf on my Linux box would fail with:

../../xfa/fxfa/app/xfa_fwltheme.cpp:96: virtual FWL_ERR
CXFA_FWLTheme::Initialize(): Assertion `__null != m_pCalendarFont' failed.

I tracked the regression back to [1].

The issue seems to be in CFX_FontManager::AddInstalledFont we will only add
one font for a given Charset. In my case I end up loading 6 charsets, but the
fonts are all strange ones. When I open the PDF, it fails to find 'Arial'
because I've registered these other fonts.

To fix this I changed AddInstalledFont to record the first font into
m_CharsetArray to use as the fallback but I add all fonts to the face
array list. This allows bug_434.pdf to open and the test to pass successfully.

1-
https://pdfium.googlesource.com/pdfium/+/fe73e7849b8b4ce49408d2f52f3fc29b370b...
==========

to

==========
Record all fonts, not just one per charset.

Attempting to open bug_434.pdf on my Linux box would fail with:

../../xfa/fxfa/app/xfa_fwltheme.cpp:96: virtual FWL_ERR
CXFA_FWLTheme::Initialize(): Assertion `__null != m_pCalendarFont' failed.

I tracked the regression back to [1].

The issue seems to be in CFX_FontManager::AddInstalledFont we will only add
one font for a given Charset. In my case I end up loading 6 charsets, but the
fonts are all strange ones. When I open the PDF, it fails to find 'Arial'
because I've registered these other fonts.

To fix this I changed AddInstalledFont to record the first font into
m_CharsetArray to use as the fallback but I add all fonts to the face
array list. This allows bug_434.pdf to open and the test to pass successfully.

1-
https://pdfium.googlesource.com/pdfium/+/fe73e7849b8b4ce49408d2f52f3fc29b370b...
==========

[Tom Sepez, 2016-04-08 15:36:14]

tsepez@chromium.org changed reviewers:
+ hong_zhang@foxitsoftware.com
- jun_fang@foxitsoftware.com

[Tom Sepez, 2016-04-08 15:36:54]

Adding Hong, who is taking over for Jun.  Can you run this by the FX devs to see
what they think?

[hong_zhang, 2016-04-11 07:06:04]

line 1160 to 1165 are

int index = m_CharsetArray.Find(Charset);

if (index < 0) 
        return UseInternalSubst(pSubstFont, iBaseFont, italic_angle,
old_weight,PitchFamily);

hFont = m_pFontInfo->GetFont(m_FaceArray[index]);

https://codereview.chromium.org/1874433002/diff/1/core/fxge/ge/fx_ge_fontmap.cpp
File core/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1874433002/diff/1/core/fxge/ge/fx_ge_fontmap....
core/fxge/ge/fx_ge_fontmap.cpp:749: m_FaceArray.push_back(name);
On 2016/04/07 19:03:22, dsinclair wrote:
> This bit is the actual code change, m_FaceArray is moved out of the above if()
> to always execute.

Actually we cannot make this change.
See line 1160 to 1165
the element count of m_CharsetArray should equal that of m_FaceArray

[dsinclair, 2016-04-11 13:52:11]

PTAL.

I removed m_CharSet array, and turned m_FaceArray into a struct which has the
charset and the face name. Line 1160 then turns into a std::find_if over the
structs.

https://codereview.chromium.org/1874433002/diff/1/core/fxge/ge/fx_ge_fontmap.cpp
File core/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1874433002/diff/1/core/fxge/ge/fx_ge_fontmap....
core/fxge/ge/fx_ge_fontmap.cpp:749: m_FaceArray.push_back(name);
On 2016/04/11 07:06:04, hong_zhang wrote:
> On 2016/04/07 19:03:22, dsinclair wrote:
> > This bit is the actual code change, m_FaceArray is moved out of the above
if()
> > to always execute.
> 
> Actually we cannot make this change.
> See line 1160 to 1165
> the element count of m_CharsetArray should equal that of m_FaceArray


Ok, I've removed m_CharsetArray and merged it with m_FaceArray.

[dsinclair, 2016-04-11 13:53:14]

Description was changed from

==========
Record all fonts, not just one per charset.

Attempting to open bug_434.pdf on my Linux box would fail with:

../../xfa/fxfa/app/xfa_fwltheme.cpp:96: virtual FWL_ERR
CXFA_FWLTheme::Initialize(): Assertion `__null != m_pCalendarFont' failed.

I tracked the regression back to [1].

The issue seems to be in CFX_FontManager::AddInstalledFont we will only add
one font for a given Charset. In my case I end up loading 6 charsets, but the
fonts are all strange ones. When I open the PDF, it fails to find 'Arial'
because I've registered these other fonts.

To fix this I changed AddInstalledFont to record the first font into
m_CharsetArray to use as the fallback but I add all fonts to the face
array list. This allows bug_434.pdf to open and the test to pass successfully.

1-
https://pdfium.googlesource.com/pdfium/+/fe73e7849b8b4ce49408d2f52f3fc29b370b...
==========

to

==========
Record all fonts, not just one per charset.

Attempting to open bug_434.pdf on my Linux box would fail with:

../../xfa/fxfa/app/xfa_fwltheme.cpp:96: virtual FWL_ERR
CXFA_FWLTheme::Initialize(): Assertion `__null != m_pCalendarFont' failed.

I tracked the regression back to [1].

The issue seems to be in CFX_FontManager::AddInstalledFont we will only add
one font for a given Charset. In my case I end up loading 6 charsets, but the
fonts are all strange ones. When I open the PDF, it fails to find 'Arial'
because I've registered these other fonts.

To fix this I changed the m_FaceArray into a struct of {name, chraset}. Then
we record all fonts into this list and search over that list for the charset
when needed. This allows bug_434.pdf to open and the test to pass successfully.

1-
https://pdfium.googlesource.com/pdfium/+/fe73e7849b8b4ce49408d2f52f3fc29b370b...
==========

[dsinclair, 2016-04-12 13:01:56]

On 2016/04/11 13:52:11, dsinclair wrote:
> PTAL.
> 
> I removed m_CharSet array, and turned m_FaceArray into a struct which has the
> charset and the face name. Line 1160 then turns into a std::find_if over the
> structs.
> 
>
https://codereview.chromium.org/1874433002/diff/1/core/fxge/ge/fx_ge_fontmap.cpp
> File core/fxge/ge/fx_ge_fontmap.cpp (right):
> 
>
https://codereview.chromium.org/1874433002/diff/1/core/fxge/ge/fx_ge_fontmap....
> core/fxge/ge/fx_ge_fontmap.cpp:749: m_FaceArray.push_back(name);
> On 2016/04/11 07:06:04, hong_zhang wrote:
> > On 2016/04/07 19:03:22, dsinclair wrote:
> > > This bit is the actual code change, m_FaceArray is moved out of the above
> if()
> > > to always execute.
> > 
> > Actually we cannot make this change.
> > See line 1160 to 1165
> > the element count of m_CharsetArray should equal that of m_FaceArray
> 
> 
> Ok, I've removed m_CharsetArray and merged it with m_FaceArray.

Ping.

[Tom Sepez, 2016-04-12 16:39:41]

LGTM, but get sign-off from Hong

[hong_zhang, 2016-04-12 17:19:21]

It is good. I confirmed with my team. Feel free to close this review.

https://codereview.chromium.org/1874433002/diff/1/core/fxge/ge/fx_ge_fontmap.cpp
File core/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1874433002/diff/1/core/fxge/ge/fx_ge_fontmap....
core/fxge/ge/fx_ge_fontmap.cpp:749: m_FaceArray.push_back(name);
On 2016/04/11 13:52:11, dsinclair wrote:
> On 2016/04/11 07:06:04, hong_zhang wrote:
> > On 2016/04/07 19:03:22, dsinclair wrote:
> > > This bit is the actual code change, m_FaceArray is moved out of the above
> if()
> > > to always execute.
> > 
> > Actually we cannot make this change.
> > See line 1160 to 1165
> > the element count of m_CharsetArray should equal that of m_FaceArray
> 
> 
> Ok, I've removed m_CharsetArray and merged it with m_FaceArray.

It is good. I confirmed with my team. Feel free to close this review.

[dsinclair, 2016-04-12 17:25:58]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-04-12 17:26:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1874433002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1874433002/40001

[commit-bot: I haz the power, 2016-04-12 17:51:07]

Description was changed from

==========
Record all fonts, not just one per charset.

Attempting to open bug_434.pdf on my Linux box would fail with:

../../xfa/fxfa/app/xfa_fwltheme.cpp:96: virtual FWL_ERR
CXFA_FWLTheme::Initialize(): Assertion `__null != m_pCalendarFont' failed.

I tracked the regression back to [1].

The issue seems to be in CFX_FontManager::AddInstalledFont we will only add
one font for a given Charset. In my case I end up loading 6 charsets, but the
fonts are all strange ones. When I open the PDF, it fails to find 'Arial'
because I've registered these other fonts.

To fix this I changed the m_FaceArray into a struct of {name, chraset}. Then
we record all fonts into this list and search over that list for the charset
when needed. This allows bug_434.pdf to open and the test to pass successfully.

1-
https://pdfium.googlesource.com/pdfium/+/fe73e7849b8b4ce49408d2f52f3fc29b370b...
==========

to

==========
Record all fonts, not just one per charset.

Attempting to open bug_434.pdf on my Linux box would fail with:

../../xfa/fxfa/app/xfa_fwltheme.cpp:96: virtual FWL_ERR
CXFA_FWLTheme::Initialize(): Assertion `__null != m_pCalendarFont' failed.

I tracked the regression back to [1].

The issue seems to be in CFX_FontManager::AddInstalledFont we will only add
one font for a given Charset. In my case I end up loading 6 charsets, but the
fonts are all strange ones. When I open the PDF, it fails to find 'Arial'
because I've registered these other fonts.

To fix this I changed the m_FaceArray into a struct of {name, chraset}. Then
we record all fonts into this list and search over that list for the charset
when needed. This allows bug_434.pdf to open and the test to pass successfully.

1-
https://pdfium.googlesource.com/pdfium/+/fe73e7849b8b4ce49408d2f52f3fc29b370b...

Committed:
https://pdfium.googlesource.com/pdfium/+/ea98238666e33cd16b69cb23dcaca047c21c...
==========

[commit-bot: I haz the power, 2016-04-12 17:51:08]

Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/ea98238666e33cd16b69cb23dcaca047c21c...