Remove implicit conversions and some cleanup

Remove implicit conversions and some cleanup

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/0fc6b25e738c311fd66aa9da8c56de50af5644d5

[Wei Li, 2016-03-01 02:40:37]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-03-01 02:41:55]

Description was changed from

==========
Remove implicit conversions and cleanup

Remove some implicit conversions, and clean up CPDF_BookmarkTree
and CPDF_Bookmark.
==========

to

==========
Remove implicit conversions and some cleanup
==========

[Wei Li, 2016-03-01 02:42:32]

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

[Wei Li, 2016-03-01 02:42:33]

PTAL, thanks

[Wei Li, 2016-03-01 17:29:10]

Patchset #1 (id:20001) has been deleted

[Tom Sepez, 2016-03-01 17:53:35]

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

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:71: CPDF_Document* m_pDocument;
Can this be CPDF_Document* const m_pDocument?

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:89: CPDF_Dictionary* m_pDict;
ditto

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:162: const CPDF_Action* m_pAction;
ditto

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:848: CPDF_IconFit(CPDF_Dictionary* pDict) :
m_pDict(pDict) {}
Did you mean to add "explicit" here?

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:850: operator CPDF_Dictionary*() const { return
m_pDict; }
You might want to try to kill this operator as a follow-up, instead adding a
GetDictionary() method.

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:852: enum ScaleMethod { Always = 0, Bigger,
Smaller, Never };
nit: Maybe put this as the first thing after public while we're at it.

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:841: CFX_ByteString csDA;
Do we have no-arg constructors for CPDF_DefaultAppearance that are equivalent to
the one-arg version called with an empty string?

If so, we can do
... {
  if (!m_pFormDict)
    return CPDF_DefautlAppearance();
  return CPDF_DefautlAppearance(m_pFormDict->GetStringBy("DA"));
}

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_formcontrol.cpp (right):

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_formcontrol.cpp:252: if (m_pWidgetDict) {
Same thing here with no-arg ctor.

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_formfield.cpp (right):

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_formfield.cpp:242: return CPDF_AAction(dict);
Nit: local |dict| only used once, so combine with previous line.

[Wei Li, 2016-03-01 21:16:15]

Patchset #2 (id:60001) has been deleted

[Wei Li, 2016-03-01 21:33:43]

Patchset #2 (id:80001) has been deleted

[Wei Li, 2016-03-01 21:35:58]

Patchset #2 (id:100001) has been deleted

[Wei Li, 2016-03-01 21:43:31]

Patchset #2 (id:120001) has been deleted

[Wei Li, 2016-03-01 21:46:53]

Thanks for your review. Other than your comments, I also cleaned up a couple
similar classes.

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

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:71: CPDF_Document* m_pDocument;
On 2016/03/01 17:53:35, Tom Sepez wrote:
> Can this be CPDF_Document* const m_pDocument?

Yes, done.

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:89: CPDF_Dictionary* m_pDict;
On 2016/03/01 17:53:35, Tom Sepez wrote:
> ditto

This one needs to kept non-const since it is easier to walk the tree in
FindBookmark()

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:162: const CPDF_Action* m_pAction;
On 2016/03/01 17:53:35, Tom Sepez wrote:
> ditto

Done.

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:848: CPDF_IconFit(CPDF_Dictionary* pDict) :
m_pDict(pDict) {}
On 2016/03/01 17:53:35, Tom Sepez wrote:
> Did you mean to add "explicit" here?

yes, thank you for spotting this :)

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:850: operator CPDF_Dictionary*() const { return
m_pDict; }
On 2016/03/01 17:53:35, Tom Sepez wrote:
> You might want to try to kill this operator as a follow-up, instead adding a
> GetDictionary() method.

Done.

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:852: enum ScaleMethod { Always = 0, Bigger,
Smaller, Never };
On 2016/03/01 17:53:34, Tom Sepez wrote:
> nit: Maybe put this as the first thing after public while we're at it.

Done.

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:841: CFX_ByteString csDA;
On 2016/03/01 17:53:35, Tom Sepez wrote:
> Do we have no-arg constructors for CPDF_DefaultAppearance that are equivalent
to
> the one-arg version called with an empty string?
> 
> If so, we can do
> ... {
>   if (!m_pFormDict)
>     return CPDF_DefautlAppearance();
>   return CPDF_DefautlAppearance(m_pFormDict->GetStringBy("DA"));
> }

Done.

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_formcontrol.cpp (right):

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_formcontrol.cpp:252: if (m_pWidgetDict) {
On 2016/03/01 17:53:35, Tom Sepez wrote:
> Same thing here with no-arg ctor.

Done.

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_formfield.cpp (right):

https://codereview.chromium.org/1751753002/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_formfield.cpp:242: return CPDF_AAction(dict);
On 2016/03/01 17:53:35, Tom Sepez wrote:
> Nit: local |dict| only used once, so combine with previous line.

Done.

[Tom Sepez, 2016-03-01 23:43:30]

lgtm

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

https://codereview.chromium.org/1751753002/diff/40001/core/include/fpdfdoc/fp...
core/include/fpdfdoc/fpdf_doc.h:89: CPDF_Dictionary* m_pDict;
On 2016/03/01 21:46:53, Wei Li wrote:
> On 2016/03/01 17:53:35, Tom Sepez wrote:
> > ditto
> 
> This one needs to kept non-const since it is easier to walk the tree in
> FindBookmark()

Acknowledged.

[Wei Li, 2016-03-02 00:29:49]

Description was changed from

==========
Remove implicit conversions and some cleanup
==========

to

==========
Remove implicit conversions and some cleanup

R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/0fc6b25e738c311fd66aa9da8c56de50af56...
==========

[Wei Li, 2016-03-02 00:29:50]

Committed patchset #2 (id:140001) manually as
0fc6b25e738c311fd66aa9da8c56de50af5644d5 (presubmit successful).