Replace Release() { delete this; } in fde_xml_imp.h

Replace Release() { delete this; } in fde_xml_imp.h

Committed: https://pdfium.googlesource.com/pdfium/+/fa34e805fd03ba81bcfe1148cf96b24fe63b39a0

[Tom Sepez, 2016-05-16 20:06:11]

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

[Tom Sepez, 2016-05-16 20:06:12]

Lei, for review.

[Lei Zhang, 2016-05-16 20:38:09]

https://codereview.chromium.org/1981003002/diff/40001/xfa/fde/xml/fde_xml_imp.h
File xfa/fde/xml/fde_xml_imp.h (right):

https://codereview.chromium.org/1981003002/diff/40001/xfa/fde/xml/fde_xml_imp...
xfa/fde/xml/fde_xml_imp.h:74: ~CFDE_XMLInstruction() override {}
Move the impl to xfa/fde/xml/fde_xml_imp.cpp ? Ditto below.

https://codereview.chromium.org/1981003002/diff/40001/xfa/fde/xml/fde_xml_imp...
xfa/fde/xml/fde_xml_imp.h:205: class CFDE_XMLParser {
BTW, this should be ISomething. It has 2 subclasses, so keep.

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/app/xfa_ffdoc.cpp
File xfa/fxfa/app/xfa_ffdoc.cpp (right):

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/app/xfa_ffdoc....
xfa/fxfa/app/xfa_ffdoc.cpp:268: delete m_pDocument->GetParser();
Feels a bit weird to leave |m_pDocument| with a dangling pointer.

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/parser/xfa_doc...
File xfa/fxfa/parser/xfa_document_serialize.cpp (right):

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/parser/xfa_doc...
xfa/fxfa/parser/xfa_document_serialize.cpp:31: if (!pDataDocumentParser)
Always evals to false?

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/parser/xfa_obj...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/parser/xfa_obj...
xfa/fxfa/parser/xfa_object_imp.cpp:966: if (!pParser)
Always false, right?

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/parser/xfa_obj...
xfa/fxfa/parser/xfa_object_imp.cpp:1060: delete pFakeXMLRoot;
Can this be an unique_ptr?

[Tom Sepez, 2016-05-16 21:29:06]

https://codereview.chromium.org/1981003002/diff/40001/xfa/fde/xml/fde_xml_imp.h
File xfa/fde/xml/fde_xml_imp.h (right):

https://codereview.chromium.org/1981003002/diff/40001/xfa/fde/xml/fde_xml_imp...
xfa/fde/xml/fde_xml_imp.h:74: ~CFDE_XMLInstruction() override {}
On 2016/05/16 20:38:08, Lei Zhang wrote:
> Move the impl to xfa/fde/xml/fde_xml_imp.cpp ? Ditto below.

Done.

https://codereview.chromium.org/1981003002/diff/40001/xfa/fde/xml/fde_xml_imp...
xfa/fde/xml/fde_xml_imp.h:205: class CFDE_XMLParser {
On 2016/05/16 20:38:08, Lei Zhang wrote:
> BTW, this should be ISomething. It has 2 subclasses, so keep.

Renamed to IFDE_.

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/app/xfa_ffdoc.cpp
File xfa/fxfa/app/xfa_ffdoc.cpp (right):

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/app/xfa_ffdoc....
xfa/fxfa/app/xfa_ffdoc.cpp:268: delete m_pDocument->GetParser();
On 2016/05/16 20:38:08, Lei Zhang wrote:
> Feels a bit weird to leave |m_pDocument| with a dangling pointer.
Yeah, ugh.  Made this a method of the xfa doc.

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/parser/xfa_doc...
File xfa/fxfa/parser/xfa_document_serialize.cpp (right):

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/parser/xfa_doc...
xfa/fxfa/parser/xfa_document_serialize.cpp:31: if (!pDataDocumentParser)
On 2016/05/16 20:38:09, Lei Zhang wrote:
> Always evals to false?

Done.

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/parser/xfa_obj...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/parser/xfa_obj...
xfa/fxfa/parser/xfa_object_imp.cpp:966: if (!pParser)
On 2016/05/16 20:38:09, Lei Zhang wrote:
> Always false, right?

Done.

https://codereview.chromium.org/1981003002/diff/40001/xfa/fxfa/parser/xfa_obj...
xfa/fxfa/parser/xfa_object_imp.cpp:1060: delete pFakeXMLRoot;
On 2016/05/16 20:38:09, Lei Zhang wrote:
> Can this be an unique_ptr?
Maybe later.  This gets assigned in a couple of places to pre-existing things,
not sure we always want to clean it.

[Lei Zhang, 2016-05-16 21:34:30]

lgtm

https://codereview.chromium.org/1981003002/diff/60001/xfa/fde/xml/fde_xml_imp...
File xfa/fde/xml/fde_xml_imp.cpp (right):

https://codereview.chromium.org/1981003002/diff/60001/xfa/fde/xml/fde_xml_imp...
xfa/fde/xml/fde_xml_imp.cpp:932: if (!pXMLParser)
I don't think this can be true, but defer to another CL?

[Tom Sepez, 2016-05-16 21:36:06]

https://codereview.chromium.org/1981003002/diff/60001/xfa/fde/xml/fde_xml_imp...
File xfa/fde/xml/fde_xml_imp.cpp (right):

https://codereview.chromium.org/1981003002/diff/60001/xfa/fde/xml/fde_xml_imp...
xfa/fde/xml/fde_xml_imp.cpp:932: if (!pXMLParser)
On 2016/05/16 21:34:30, Lei Zhang wrote:
> I don't think this can be true, but defer to another CL?
Deferred.

[Tom Sepez, 2016-05-16 22:42:34]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-05-16 22:42:34]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1981003002/#ps120001
(title: "Revert accident with editor.")

[commit-bot: I haz the power, 2016-05-16 22:42:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1981003002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1981003002/120001

[commit-bot: I haz the power, 2016-05-16 22:43:01]

Description was changed from

==========
Replace Release() { delete this; } in fde_xml_imp.h
==========

to

==========
Replace Release() { delete this; } in fde_xml_imp.h

Committed:
https://pdfium.googlesource.com/pdfium/+/fa34e805fd03ba81bcfe1148cf96b24fe63b...
==========

[commit-bot: I haz the power, 2016-05-16 22:43:02]

Committed patchset #7 (id:120001) as
https://pdfium.googlesource.com/pdfium/+/fa34e805fd03ba81bcfe1148cf96b24fe63b...

[Lei Zhang, 2016-05-16 22:57:18]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1980223002/ by thestig@chromium.org.

The reason for reverting is: Broke the ASAN Bot.