Remove static declared SymbolDictCache.

Remove static declared SymbolDictCache.

This is a follow up CL on https://codereview.chromium.org/761313004/

BUG=https://code.google.com/p/pdfium/issues/detail?id=93
R=brucedawson@chromium.org, tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/17b5cf451cfcd5ec614d1d4e1afd8daa8b5d8699

[Bo Xu, 2014-12-31 22:02:04]

bo_xu@foxitsoftware.com changed reviewers:
+ breidenbach@gmail.com, brucedawson@chromium.org, thestig@chromium.org,
tsepez@chromium.org

[Bo Xu, 2014-12-31 22:02:05]

Hi all, please take a look at this CL.

Besides removing the static variable, also add a check to deal with the
situation of |kSymbolDictCacheMaxSize| == 0;

https://codereview.chromium.org/828183002/diff/1/core/src/fxcodec/jbig2/JBig2...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/828183002/diff/1/core/src/fxcodec/jbig2/JBig2...
core/src/fxcodec/jbig2/JBig2_Context.cpp:839: if (value &&
kSymbolDictCacheMaxSize > 0) {
Add a index check here, to accommodate the condition when
|kSymbolDictCacheMaxSize| == 0

[brucedawson, 2014-12-31 23:00:10]

I definitely prefer this solution to mine -- getting rid of globals entirely is
preferable.

See my comments about *(). versus ->.

https://codereview.chromium.org/828183002/diff/20001/core/src/fxcodec/jbig2/J...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/828183002/diff/20001/core/src/fxcodec/jbig2/J...
core/src/fxcodec/jbig2/JBig2_Context.cpp:810: (*m_pSymbolDictCache).begin(); it
!= (*m_pSymbolDictCache).end(); ++it) {
Why not m_pSymbolDictCache-> instead of (*)? Ditto for all of the changes below.

[Bo Xu, 2015-01-01 00:04:39]

https://codereview.chromium.org/828183002/diff/20001/core/src/fxcodec/jbig2/J...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/828183002/diff/20001/core/src/fxcodec/jbig2/J...
core/src/fxcodec/jbig2/JBig2_Context.cpp:810: (*m_pSymbolDictCache).begin(); it
!= (*m_pSymbolDictCache).end(); ++it) {
On 2014/12/31 23:00:10, brucedawson wrote:
> Why not m_pSymbolDictCache-> instead of (*)? Ditto for all of the changes
below.

Done.

[Tom Sepez, 2015-01-01 00:43:36]

https://codereview.chromium.org/828183002/diff/40001/core/src/fxcodec/jbig2/J...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/828183002/diff/40001/core/src/fxcodec/jbig2/J...
core/src/fxcodec/jbig2/JBig2_Context.cpp:839: if (value &&
kSymbolDictCacheMaxSize > 0) {
nit: it seems odd to be comparing a constant against 0. 0 means unlimited? If
so, we should add that fact to the comment where the constant is defined.

https://codereview.chromium.org/828183002/diff/40001/core/src/fxcodec/jbig2/J...
File core/src/fxcodec/jbig2/JBig2_Context.h (right):

https://codereview.chromium.org/828183002/diff/40001/core/src/fxcodec/jbig2/J...
core/src/fxcodec/jbig2/JBig2_Context.h:16: typedef std::pair<FX_BYTE*,
CJBig2_SymbolDict*> CJBig2_CachePair;
nit: this file should #include <utiltiy> so that the std::pair declaration will
exist for us no matter how the file is later included.

[Bo Xu, 2015-01-01 00:53:27]

https://codereview.chromium.org/828183002/diff/40001/core/src/fxcodec/jbig2/J...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/828183002/diff/40001/core/src/fxcodec/jbig2/J...
core/src/fxcodec/jbig2/JBig2_Context.cpp:839: if (value &&
kSymbolDictCacheMaxSize > 0) {
On 2015/01/01 00:43:36, Tom Sepez (vacated ho ho ho) wrote:
> nit: it seems odd to be comparing a constant against 0. 0 means unlimited? If
> so, we should add that fact to the comment where the constant is defined.

0 means no cache used. If someone set kSymbolDictCacheMaxSize to 0, previous
code will crash. Do you suggest adding the comment that kSymbolDictCacheMaxSize
should be larger than 0 and getting rid of this check? For situations like
comparing the performance, it is good to keep the 0 scenario usable.

https://codereview.chromium.org/828183002/diff/40001/core/src/fxcodec/jbig2/J...
File core/src/fxcodec/jbig2/JBig2_Context.h (right):

https://codereview.chromium.org/828183002/diff/40001/core/src/fxcodec/jbig2/J...
core/src/fxcodec/jbig2/JBig2_Context.h:16: typedef std::pair<FX_BYTE*,
CJBig2_SymbolDict*> CJBig2_CachePair;
On 2015/01/01 00:43:36, Tom Sepez (vacated ho ho ho) wrote:
> nit: this file should #include <utiltiy> so that the std::pair declaration
will
> exist for us no matter how the file is later included.

Done.

[brucedawson, 2015-01-02 21:16:46]

lgtm

[jab, 2015-01-05 16:47:09]

Thank you for doing this.

[Tom Sepez, 2015-01-05 18:16:29]

> core/src/fxcodec/jbig2/JBig2_Context.cpp:839: if (value &&
> kSymbolDictCacheMaxSize > 0) {
> On 2015/01/01 00:43:36, Tom Sepez (vacated ho ho ho) wrote:
> > nit: it seems odd to be comparing a constant against 0. 0 means unlimited?
If
> > so, we should add that fact to the comment where the constant is defined.
> 
> 0 means no cache used. If someone set kSymbolDictCacheMaxSize to 0, previous
> code will crash. Do you suggest adding the comment that
kSymbolDictCacheMaxSize
> should be larger than 0 and getting rid of this check? For situations like
> comparing the performance, it is good to keep the 0 scenario usable.
> 
Ok. Lets keep it. LGTM.

[Bo Xu, 2015-01-05 18:48:32]

Committed patchset #4 (id:60001) manually as
17b5cf451cfcd5ec614d1d4e1afd8daa8b5d8699 (presubmit successful).