Clean up bookmark related codes.

Clean up bookmark related codes.

Remove CPDF_Dictionary*() operator in CPDF_Bookmark class.
Unify naming conventions and coding styles.
Change some functions to const.

Change the name of function argument to |pDict| for FPDF_xxx type variable.
This makes the code more clear and gives better variable naming

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/263842843f4b817b9713eb3a081e297e1708f582

[Bo Xu, 2015-01-01 06:40:57]

bo_xu@foxitsoftware.com changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2015-01-05 18:08:32]

https://codereview.chromium.org/828203002/diff/40001/core/include/fpdfdoc/fpd...
File core/include/fpdfdoc/fpdf_doc.h (right):

https://codereview.chromium.org/828203002/diff/40001/core/include/fpdfdoc/fpd...
core/include/fpdfdoc/fpdf_doc.h:81: CPDF_Bookmark		GetFirstChild(CPDF_Bookmark
parent) const;
nit: args here should probably of type const CPDF_Bookmark&, but the bookmark is
small so the copy is likely to be insignificant.

https://codereview.chromium.org/828203002/diff/40001/core/include/fpdfdoc/fpd...
core/include/fpdfdoc/fpdf_doc.h:98: CPDF_Dictionary* GetDict() const {return
m_pDict;}
nit: space after { and before }.

https://codereview.chromium.org/828203002/diff/40001/core/src/fpdfdoc/doc_boo...
File core/src/fpdfdoc/doc_bookmark.cpp (right):

https://codereview.chromium.org/828203002/diff/40001/core/src/fpdfdoc/doc_boo...
core/src/fpdfdoc/doc_bookmark.cpp:59: int len = title.GetLength(), i;
nit: one var delcaration per line.

https://codereview.chromium.org/828203002/diff/40001/core/src/fpdfdoc/doc_boo...
core/src/fpdfdoc/doc_bookmark.cpp:60: for (i = 0; i < len; i++) {
nit: for (int i =0; ...

https://codereview.chromium.org/828203002/diff/40001/fpdfsdk/src/fpdfdoc.cpp
File fpdfsdk/src/fpdfdoc.cpp (right):

https://codereview.chromium.org/828203002/diff/40001/fpdfsdk/src/fpdfdoc.cpp#...
fpdfsdk/src/fpdfdoc.cpp:14: if (bookmark.GetDict() &&
bookmark.GetTitle().CompareNoCase(title) == 0) {
while having a CPDF_Dictionary* operator is ugly, having a bool operator for
your class will make this a lot cleaner.

[Bo Xu, 2015-01-05 18:45:09]

https://codereview.chromium.org/828203002/diff/40001/core/include/fpdfdoc/fpd...
File core/include/fpdfdoc/fpdf_doc.h (right):

https://codereview.chromium.org/828203002/diff/40001/core/include/fpdfdoc/fpd...
core/include/fpdfdoc/fpdf_doc.h:81: CPDF_Bookmark		GetFirstChild(CPDF_Bookmark
parent) const;
On 2015/01/05 18:08:32, Tom Sepez (vacated ho ho ho) wrote:
> nit: args here should probably of type const CPDF_Bookmark&, but the bookmark
is
> small so the copy is likely to be insignificant.

Done.

https://codereview.chromium.org/828203002/diff/40001/core/include/fpdfdoc/fpd...
core/include/fpdfdoc/fpdf_doc.h:98: CPDF_Dictionary* GetDict() const {return
m_pDict;}
On 2015/01/05 18:08:32, Tom Sepez (vacated ho ho ho) wrote:
> nit: space after { and before }.

Done.

https://codereview.chromium.org/828203002/diff/40001/core/src/fpdfdoc/doc_boo...
File core/src/fpdfdoc/doc_bookmark.cpp (right):

https://codereview.chromium.org/828203002/diff/40001/core/src/fpdfdoc/doc_boo...
core/src/fpdfdoc/doc_bookmark.cpp:59: int len = title.GetLength(), i;
On 2015/01/05 18:08:32, Tom Sepez (vacated ho ho ho) wrote:
> nit: one var delcaration per line. 

Done.

https://codereview.chromium.org/828203002/diff/40001/core/src/fpdfdoc/doc_boo...
core/src/fpdfdoc/doc_bookmark.cpp:60: for (i = 0; i < len; i++) {
On 2015/01/05 18:08:32, Tom Sepez (vacated ho ho ho) wrote:
> nit: for (int i =0; ...

Done.

https://codereview.chromium.org/828203002/diff/40001/fpdfsdk/src/fpdfdoc.cpp
File fpdfsdk/src/fpdfdoc.cpp (right):

https://codereview.chromium.org/828203002/diff/40001/fpdfsdk/src/fpdfdoc.cpp#...
fpdfsdk/src/fpdfdoc.cpp:14: if (bookmark.GetDict() &&
bookmark.GetTitle().CompareNoCase(title) == 0) {
On 2015/01/05 18:08:32, Tom Sepez (vacated ho ho ho) wrote:
> while having a CPDF_Dictionary* operator is ugly, having a bool operator for
> your class will make this a lot cleaner.

Done.

[Tom Sepez, 2015-01-05 19:24:56]

LGTM with nits.

https://codereview.chromium.org/828203002/diff/60001/core/include/fpdfdoc/fpd...
File core/include/fpdfdoc/fpdf_doc.h (right):

https://codereview.chromium.org/828203002/diff/60001/core/include/fpdfdoc/fpd...
core/include/fpdfdoc/fpdf_doc.h:96: CPDF_Bookmark(CPDF_Dictionary* pDict = NULL)
: m_pDict(pDict) {}
nit:  I'd probably be happier if we had a separate empty construtor and this
constructor (and it probably should be marked explicit for the case when it only
takes one argument).

https://codereview.chromium.org/828203002/diff/60001/fpdfsdk/src/fpdfdoc.cpp
File fpdfsdk/src/fpdfdoc.cpp (right):

https://codereview.chromium.org/828203002/diff/60001/fpdfsdk/src/fpdfdoc.cpp#...
fpdfsdk/src/fpdfdoc.cpp:27: return CPDF_Bookmark(NULL);
nit: return CPDF_Bookmark(), since we have an empty constructor (either via
default args or if you make the previous suggested change).

[Bo Xu, 2015-01-05 20:07:13]

A few more changes with the explicit constructor, please take a look again.

https://codereview.chromium.org/828203002/diff/60001/core/include/fpdfdoc/fpd...
File core/include/fpdfdoc/fpdf_doc.h (right):

https://codereview.chromium.org/828203002/diff/60001/core/include/fpdfdoc/fpd...
core/include/fpdfdoc/fpdf_doc.h:96: CPDF_Bookmark(CPDF_Dictionary* pDict = NULL)
: m_pDict(pDict) {}
On 2015/01/05 19:24:56, Tom Sepez (vacated ho ho ho) wrote:
> nit:  I'd probably be happier if we had a separate empty construtor and this
> constructor (and it probably should be marked explicit for the case when it
only
> takes one argument).

I removed the default args as we have empty constructor now.

[Tom Sepez, 2015-01-05 20:21:09]

lgtm

[Bo Xu, 2015-01-05 20:40:05]

Committed patchset #5 (id:80001) manually as
263842843f4b817b9713eb3a081e297e1708f582 (presubmit successful).