Fix nits in CPDF_PageOrganizer.

Fix nits in CPDF_PageOrganizer.

Committed: https://pdfium.googlesource.com/pdfium/+/88d87c13c25fbd20fd2963e8ff4b383c5d64b5f4

[Lei Zhang, 2016-11-12 01:20:46]

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

[commit-bot: I haz the power, 2016-11-12 01:20:51]

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-11-12 01:41:55]

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

[commit-bot: I haz the power, 2016-11-12 01:41:55]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-11-12 02:06:43]

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

[Tom Sepez, 2016-11-14 17:31:35]

lgtm

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp
File fpdfsdk/fpdfppo.cpp (right):

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp#newcode21
fpdfsdk/fpdfppo.cpp:21: #include "third_party/base/stl_util.h"
nit: include ptr_util.h

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp#newcode108
fpdfsdk/fpdfppo.cpp:108: if (cbSrcKeyStr.Compare(("Type")) &&
cbSrcKeyStr.Compare(("Parent"))) {
nit: overparenthesized, also how about
  if (cbSrcKeyStr == "Type" || cbSrcKeyStr == "Parent")
    continue;

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp#newcode109
fpdfsdk/fpdfppo.cpp:109: if (pCurPageDict->KeyExist(cbSrcKeyStr))
No need to remove as dicts clean themselves.

Wonder if we need a CPDF_Dictionary::DeepCopyExceptForKeys() method. we have
this pattern in a couple places I think.

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp#newcode141
fpdfsdk/fpdfppo.cpp:141: CPDF_Object* pInheritable =
Helper method?

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp#newcode297
fpdfsdk/fpdfppo.cpp:297: FPDF_BOOL ParserPageRangeString(CFX_ByteString
rangstring,
nit: bool ?

[Lei Zhang, 2016-11-14 19:54:55]

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

[commit-bot: I haz the power, 2016-11-14 19:54:56]

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

[Lei Zhang, 2016-11-14 19:57:01]

Please take another look. I shuffled the code around a bit more.

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp
File fpdfsdk/fpdfppo.cpp (right):

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp#newcode21
fpdfsdk/fpdfppo.cpp:21: #include "third_party/base/stl_util.h"
On 2016/11/14 17:31:35, Tom Sepez wrote:
> nit: include ptr_util.h

Done.

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp#newcode108
fpdfsdk/fpdfppo.cpp:108: if (cbSrcKeyStr.Compare(("Type")) &&
cbSrcKeyStr.Compare(("Parent"))) {
On 2016/11/14 17:31:35, Tom Sepez wrote:
> nit: overparenthesized, also how about
>   if (cbSrcKeyStr == "Type" || cbSrcKeyStr == "Parent")
>     continue;

Done.

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp#newcode109
fpdfsdk/fpdfppo.cpp:109: if (pCurPageDict->KeyExist(cbSrcKeyStr))
On 2016/11/14 17:31:35, Tom Sepez wrote:
> No need to remove as dicts clean themselves.

Done.

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp#newcode141
fpdfsdk/fpdfppo.cpp:141: CPDF_Object* pInheritable =
On 2016/11/14 17:31:35, Tom Sepez wrote:
> Helper method?

Done.

https://codereview.chromium.org/2493283003/diff/1/fpdfsdk/fpdfppo.cpp#newcode297
fpdfsdk/fpdfppo.cpp:297: FPDF_BOOL ParserPageRangeString(CFX_ByteString
rangstring,
On 2016/11/14 17:31:35, Tom Sepez wrote:
> nit: bool ?

Done.

[commit-bot: I haz the power, 2016-11-14 20:11:55]

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

[commit-bot: I haz the power, 2016-11-14 20:11:56]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-11-14 21:27:36]

lgtm

https://codereview.chromium.org/2493283003/diff/20001/fpdfsdk/fpdfppo.cpp
File fpdfsdk/fpdfppo.cpp (right):

https://codereview.chromium.org/2493283003/diff/20001/fpdfsdk/fpdfppo.cpp#new...
fpdfsdk/fpdfppo.cpp:368: return false;
nit: or just return pageOrg.PDFDocInit() && pageOrg.ExportPage(pageArray,
index);

[Lei Zhang, 2016-11-14 21:37:14]

https://codereview.chromium.org/2493283003/diff/20001/fpdfsdk/fpdfppo.cpp
File fpdfsdk/fpdfppo.cpp (right):

https://codereview.chromium.org/2493283003/diff/20001/fpdfsdk/fpdfppo.cpp#new...
fpdfsdk/fpdfppo.cpp:368: return false;
On 2016/11/14 21:27:36, Tom Sepez wrote:
> nit: or just return pageOrg.PDFDocInit() && pageOrg.ExportPage(pageArray,
> index);

Done.

[Lei Zhang, 2016-11-14 21:37:21]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-11-14 21:37:21]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2493283003/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2016-11-14 21:37:32]

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-11-14 22:06:23]

Description was changed from

==========
Fix nits in CPDF_PageOrganizer.
==========

to

==========
Fix nits in CPDF_PageOrganizer.

Committed:
https://pdfium.googlesource.com/pdfium/+/88d87c13c25fbd20fd2963e8ff4b383c5d64...
==========

[commit-bot: I haz the power, 2016-11-14 22:06:24]

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