Use string pools in some dictionaries

Use string pools in some dictionaries, names, and strings.

BUG=pdfium:597
Committed: https://pdfium.googlesource.com/pdfium/+/698c5716d005860360527e4cfe15b4a185589117

[Tom Sepez, 2016-09-15 20:46:40]

Description was changed from

==========
Use string pools in some dictionaries

BUG=pdfium:497
==========

to

==========
Use string pools in some dictionaries

BUG=pdfium:597
==========

[Tom Sepez, 2016-09-15 22:11:42]

Description was changed from

==========
Use string pools in some dictionaries

BUG=pdfium:597
==========

to

==========
Use string pools in some dictionaries, names, and strings.

BUG=pdfium:597
==========

[Tom Sepez, 2016-09-15 22:11:42]

tsepez@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Tom Sepez, 2016-09-15 22:11:52]

Dan, for review.

[Tom Sepez, 2016-09-16 00:04:55]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 00:04:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 00:19:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 00:19:48]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-16 16:09:37]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org

[Tom Sepez, 2016-09-16 19:58:39]

https://codereview.chromium.org/2345063002/diff/60001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h (right):

https://codereview.chromium.org/2345063002/diff/60001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h:26:
CPDF_Dictionary(CFX_ByteStringPool* pPool);
note: The question is whether it is better for the dictionary to track the pool
and automatically apply it, or whether to pass a pool to every Set*For() method.

[Tom Sepez, 2016-09-16 23:49:35]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 23:49:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 23:51:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 23:51:36]

Dry run: Try jobs failed on following builders:
  android on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/android/builds/784)

[Tom Sepez, 2016-09-17 00:08:05]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 00:08:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-17 00:10:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-17 00:10:28]

Dry run: Try jobs failed on following builders:
  android on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/android/builds/785)

[dsinclair, 2016-09-19 13:24:34]

https://codereview.chromium.org/2345063002/diff/60001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h (right):

https://codereview.chromium.org/2345063002/diff/60001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h:26:
CPDF_Dictionary(CFX_ByteStringPool* pPool);
On 2016/09/16 19:58:39, Tom Sepez wrote:
> note: The question is whether it is better for the dictionary to track the
pool
> and automatically apply it, or whether to pass a pool to every Set*For()
method.


I think the dictionary should track the pool, unless there is some reason why
the pool would be different for different Set*For() calls.

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/cpdf_syntax_parser.h (right):

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/cpdf_syntax_parser.h:97: CFX_ByteStringPool* m_pPool;
add a // Not owned. comment

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/include/cpdf_document.h (right):

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/include/cpdf_document.h:43: public CFX_ByteStringPool {
I'm not sure about this, should the document just contain the pool instead of
being the pool?

Having more things point to document is going to make the system more confusing,
where as having them point to a pool that the document owns seems a little less
tangled?

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfdoc/include/c...
File core/fpdfdoc/include/cpdf_filespec.h (right):

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfdoc/include/c...
core/fpdfdoc/include/cpdf_filespec.h:17: explicit
CPDF_FileSpec(CFX_ByteStringPool* pDocument);
nit: pPool

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/cpdfsdk_widget...
File fpdfsdk/cpdfsdk_widget.cpp (right):

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/cpdfsdk_widget...
fpdfsdk/cpdfsdk_widget.cpp:1829: CPDF_Document* pDoc =
m_pPageView->GetPDFDocument();
Can we add the ASSERT back in here? We don't de-reference the pDoc in this
method so we'll end up crashing somewhere else when we should be crashing here.

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/fpdf_flatten.cpp
File fpdfsdk/fpdf_flatten.cpp (right):

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/fpdf_flatten.c...
fpdfsdk/fpdf_flatten.cpp:375: pRes = new CPDF_Dictionary(pDocument);
Reading through these, I think see 

  pRes = new CPDF_Dictionary(pDocument->pool())

would be a lot clearer as to what we're passing into the dictionary.

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/fpdf_transform...
File fpdfsdk/fpdf_transformpage.cpp (right):

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/fpdf_transform...
fpdfsdk/fpdf_transformpage.cpp:134: pStream->SetData(textBuf.GetBuffer(),
textBuf.GetSize(), FALSE, FALSE);
Previously this would SetData regardless of the value of pDoc, that is no longer
the case, is that intentional?

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/fpdf_transform...
fpdfsdk/fpdf_transformpage.cpp:310: pStream->SetData(strClip.GetBuffer(),
strClip.GetSize(), FALSE, FALSE);
ditto

[Tom Sepez, 2016-09-19 17:05:06]

On 2016/09/19 13:24:34, dsinclair wrote:
>
https://codereview.chromium.org/2345063002/diff/60001/core/fpdfapi/fpdf_parse...
> File core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h (right):
> 
>
https://codereview.chromium.org/2345063002/diff/60001/core/fpdfapi/fpdf_parse...
> core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h:26:
> CPDF_Dictionary(CFX_ByteStringPool* pPool);
> On 2016/09/16 19:58:39, Tom Sepez wrote:
> > note: The question is whether it is better for the dictionary to track the
> pool
> > and automatically apply it, or whether to pass a pool to every Set*For()
> method.
> 
> 
> I think the dictionary should track the pool, unless there is some reason why
> the pool would be different for different Set*For() calls.
> 
I guess I was concerned whether a given dictionary might outlive the document it
was created against.

[Tom Sepez, 2016-09-19 17:21:33]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[dsinclair, 2016-09-19 17:21:33]

On 2016/09/19 17:05:06, Tom Sepez wrote:
> On 2016/09/19 13:24:34, dsinclair wrote:
> >
>
https://codereview.chromium.org/2345063002/diff/60001/core/fpdfapi/fpdf_parse...
> > File core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h (right):
> > 
> >
>
https://codereview.chromium.org/2345063002/diff/60001/core/fpdfapi/fpdf_parse...
> > core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h:26:
> > CPDF_Dictionary(CFX_ByteStringPool* pPool);
> > On 2016/09/16 19:58:39, Tom Sepez wrote:
> > > note: The question is whether it is better for the dictionary to track the
> > pool
> > > and automatically apply it, or whether to pass a pool to every Set*For()
> > method.
> > 
> > 
> > I think the dictionary should track the pool, unless there is some reason
why
> > the pool would be different for different Set*For() calls.
> > 
> I guess I was concerned whether a given dictionary might outlive the document
it
> was created against.


That is a potential issue. With the string pool, what happens if I get an
Intern'd string from the pool; delete the pool; access the intern'd string, will
that work correctly? If not, we're going to be in trouble either way ...

[commit-bot: I haz the power, 2016-09-19 17:21:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 17:30:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 17:30:22]

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...)

[Tom Sepez, 2016-09-19 17:31:46]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 17:31:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-09-19 17:35:03]

> That is a potential issue. With the string pool, what happens if I get an
> Intern'd string from the pool; delete the pool; access the intern'd string,
will
> that work correctly? If not, we're going to be in trouble either way ...
yes, that will work.  the pool is just an index, the string keeps itself alive
via its own ref-counting.

[dsinclair, 2016-09-19 17:41:40]

On 2016/09/19 17:35:03, Tom Sepez wrote:
> > That is a potential issue. With the string pool, what happens if I get an
> > Intern'd string from the pool; delete the pool; access the intern'd string,
> will
> > that work correctly? If not, we're going to be in trouble either way ...
> yes, that will work.  the pool is just an index, the string keeps itself alive
> via its own ref-counting.


In that case, it's probably safer to pass the pool into the Set* methods instead
of the Document into the constructor.

[commit-bot: I haz the power, 2016-09-19 17:47:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 17:47:47]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-19 18:25:00]

What remains is to prove/disprove the lifespan assumption.

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/cpdf_syntax_parser.h (right):

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/cpdf_syntax_parser.h:97: CFX_ByteStringPool* m_pPool;
On 2016/09/19 13:24:34, dsinclair wrote:
> add a // Not owned. comment

Made CFX_ByteStringPool* const m_pPool; which is even better than a comment.

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/include/cpdf_document.h (right):

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/include/cpdf_document.h:43: public CFX_ByteStringPool {
On 2016/09/19 13:24:34, dsinclair wrote:
> I'm not sure about this, should the document just contain the pool instead of
> being the pool?
> 
> Having more things point to document is going to make the system more
confusing,
> where as having them point to a pool that the document owns seems a little
less
> tangled?

Done.

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfdoc/include/c...
File core/fpdfdoc/include/cpdf_filespec.h (right):

https://codereview.chromium.org/2345063002/diff/100001/core/fpdfdoc/include/c...
core/fpdfdoc/include/cpdf_filespec.h:17: explicit
CPDF_FileSpec(CFX_ByteStringPool* pDocument);
On 2016/09/19 13:24:34, dsinclair wrote:
> nit: pPool

Done.

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/cpdfsdk_widget...
File fpdfsdk/cpdfsdk_widget.cpp (right):

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/cpdfsdk_widget...
fpdfsdk/cpdfsdk_widget.cpp:1829: CPDF_Document* pDoc =
m_pPageView->GetPDFDocument();
On 2016/09/19 13:24:34, dsinclair wrote:
> Can we add the ASSERT back in here? We don't de-reference the pDoc in this
> method so we'll end up crashing somewhere else when we should be crashing
here.

We now de-ref it due to the  has-a change.  Also the if at 1837 is pointless
since we believe |new| can't fail and has been removed.

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/fpdf_flatten.cpp
File fpdfsdk/fpdf_flatten.cpp (right):

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/fpdf_flatten.c...
fpdfsdk/fpdf_flatten.cpp:375: pRes = new CPDF_Dictionary(pDocument);
On 2016/09/19 13:24:34, dsinclair wrote:
> Reading through these, I think see 
> 
>   pRes = new CPDF_Dictionary(pDocument->pool())
> 
> would be a lot clearer as to what we're passing into the dictionary.

Done.

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/fpdf_transform...
File fpdfsdk/fpdf_transformpage.cpp (right):

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/fpdf_transform...
fpdfsdk/fpdf_transformpage.cpp:134: pStream->SetData(textBuf.GetBuffer(),
textBuf.GetSize(), FALSE, FALSE);
On 2016/09/19 13:24:34, dsinclair wrote:
> Previously this would SetData regardless of the value of pDoc, that is no
longer
> the case, is that intentional?

Yes, it would be a leak in that case as the new stream isn't held by anything.

https://codereview.chromium.org/2345063002/diff/100001/fpdfsdk/fpdf_transform...
fpdfsdk/fpdf_transformpage.cpp:310: pStream->SetData(strClip.GetBuffer(),
strClip.GetSize(), FALSE, FALSE);
On 2016/09/19 13:24:34, dsinclair wrote:
> ditto

ditto

[Tom Sepez, 2016-09-19 18:28:54]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 18:28:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dsinclair, 2016-09-19 18:31:22]

This looks a lot clearer with the passing of the explicit pool.

[commit-bot: I haz the power, 2016-09-19 18:32:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 18:32:04]

Dry run: Try jobs failed on following builders:
  mac_no_v8 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_no_v8/build...)

[Tom Sepez, 2016-09-19 18:37:03]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 18:37:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 18:58:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 18:58:47]

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...)

[Tom Sepez, 2016-09-19 19:06:45]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 19:06:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 19:26:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 19:26:05]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-19 19:54:52]

Lei, (non-ooo version), any thoughts?

[Lei Zhang, 2016-09-20 23:48:24]

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/cpdf_syntax_parser.cpp (right):

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/cpdf_syntax_parser.cpp:40: : m_MetadataObjnum(0),
Use a delegate ctor?

: CPDF_SyntaxParser(nullptr) {}

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/cpdf_syntax_parser.h (right):

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/cpdf_syntax_parser.h:25:
CPDF_SyntaxParser(CFX_ByteStringPool* pPool);
explicit

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h (right):

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h:25:
CPDF_Dictionary(CFX_ByteStringPool* pPool);
explicit

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/include/cpdf_document.h (right):

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/include/cpdf_document.h:129: CFX_ByteStringPool
m_ByteStringPool;
Dumb question: Why have a CFX_ByteStringPool per C[FP]DF_Document instead of
just a single global pool? Is it to release everything associated with a
document when it gets destroyed?

[Tom Sepez, 2016-09-27 20:42:57]

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/include/cpdf_document.h (right):

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/include/cpdf_document.h:129: CFX_ByteStringPool
m_ByteStringPool;
On 2016/09/20 23:48:23, Lei Zhang wrote:
> Dumb question: Why have a CFX_ByteStringPool per C[FP]DF_Document instead of
> just a single global pool? Is it to release everything associated with a
> document when it gets destroyed?

Exactly.  I wouldn't want an unloaded document to hold down some idiosyncratic
strings forever.

[Tom Sepez, 2016-09-27 22:36:47]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 22:36:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-09-27 22:36:59]

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/cpdf_syntax_parser.cpp (right):

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/cpdf_syntax_parser.cpp:40: : m_MetadataObjnum(0),
On 2016/09/20 23:48:23, Lei Zhang wrote:
> Use a delegate ctor?
> 
> : CPDF_SyntaxParser(nullptr) {}

done

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h (right):

https://codereview.chromium.org/2345063002/diff/240001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h:25:
CPDF_Dictionary(CFX_ByteStringPool* pPool);
On 2016/09/20 23:48:23, Lei Zhang wrote:
> explicit

Done.

[commit-bot: I haz the power, 2016-09-27 22:41:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 22:41:02]

Dry run: Try jobs failed on following builders:
  mac on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac/builds/2204)
  mac_no_v8 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_no_v8/build...)

[Tom Sepez, 2016-09-27 22:47:00]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 22:47:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 23:05:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 23:05:57]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-28 22:26:55]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 22:27:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-09-28 22:30:03]

Lei, PS#18.  Strings themselves are refcounted and live only as long as needed
independent of the pool; the map underlying the pool is destroyed along with the
document, but any dictionary now referring to the pool are going to get back
nulls; that's ok, any future operations won't intern strings.

[Tom Sepez, 2016-09-28 22:33:03]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 22:33:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 22:51:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 22:51:42]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-09-28 23:26:35]

lgtm

https://codereview.chromium.org/2345063002/diff/340001/core/fpdfapi/fpdf_pars...
File core/fpdfapi/fpdf_parser/include/cpdf_document.h (right):

https://codereview.chromium.org/2345063002/diff/340001/core/fpdfapi/fpdf_pars...
core/fpdfapi/fpdf_parser/include/cpdf_document.h:142:
CFX_WeakPtr<CFX_ByteStringPool> m_pByteStringPool;
Does this have to be last? If yes, please add a comment.

[Tom Sepez, 2016-09-28 23:46:35]

On 2016/09/28 23:26:35, Lei Zhang wrote:
> lgtm
> 
>
https://codereview.chromium.org/2345063002/diff/340001/core/fpdfapi/fpdf_pars...
> File core/fpdfapi/fpdf_parser/include/cpdf_document.h (right):
> 
>
https://codereview.chromium.org/2345063002/diff/340001/core/fpdfapi/fpdf_pars...
> core/fpdfapi/fpdf_parser/include/cpdf_document.h:142:
> CFX_WeakPtr<CFX_ByteStringPool> m_pByteStringPool;
> Does this have to be last? If yes, please add a comment.

No, we can destroy a string pool at any time.

[Tom Sepez, 2016-09-28 23:46:44]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2016-09-28 23:46:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 23:47:12]

Description was changed from

==========
Use string pools in some dictionaries, names, and strings.

BUG=pdfium:597
==========

to

==========
Use string pools in some dictionaries, names, and strings.

BUG=pdfium:597

Committed:
https://pdfium.googlesource.com/pdfium/+/698c5716d005860360527e4cfe15b4a18558...
==========

[commit-bot: I haz the power, 2016-09-28 23:47:14]

Committed patchset #19 (id:360001) as
https://pdfium.googlesource.com/pdfium/+/698c5716d005860360527e4cfe15b4a18558...