Flip document and parser ownership

Flip document and parser ownership

This Cl switches the ownership between the parser and the document. Previously
the parser owned the document and we'd jump through hoops during cleanup to
delete the right object. This Cl flips the ownership so the document owns
the parser and simplifies the cleanup logic where needed.

BUG=pdfium:565
Committed: https://pdfium.googlesource.com/pdfium/+/cedaa557316a3f5c436814e69d67f19795f471d7

[dsinclair, 2016-08-24 15:31:18]

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

[commit-bot: I haz the power, 2016-08-24 15:31:20]

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

[dsinclair, 2016-08-24 15:40:37]

dsinclair@chromium.org changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org, weili@chromium.org

[dsinclair, 2016-08-24 15:40:37]

PTAL.

[commit-bot: I haz the power, 2016-08-24 15:48:30]

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

[commit-bot: I haz the power, 2016-08-24 15:48:31]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-08-24 16:38:52]

https://codereview.chromium.org/2275773003/diff/1/core/fpdfapi/fpdf_parser/in...
File core/fpdfapi/fpdf_parser/include/cpdf_parser.h (right):

https://codereview.chromium.org/2275773003/diff/1/core/fpdfapi/fpdf_parser/in...
core/fpdfapi/fpdf_parser/include/cpdf_parser.h:120: CPDF_Document* m_pDocument; 
// not owned
pity this can't be CPDF_Document* const m_pDocument and we pass the doc in the
ctor ... or can it?

https://codereview.chromium.org/2275773003/diff/1/fpdfsdk/fpdf_dataavail.cpp
File fpdfsdk/fpdf_dataavail.cpp (right):

https://codereview.chromium.org/2275773003/diff/1/fpdfsdk/fpdf_dataavail.cpp#...
fpdfsdk/fpdf_dataavail.cpp:151: CheckUnSupportError(pDocument.get(),
FPDF_ERR_SUCCESS);
nit: can this be a document method?

https://codereview.chromium.org/2275773003/diff/1/fpdfsdk/fpdfview.cpp
File fpdfsdk/fpdfview.cpp (right):

https://codereview.chromium.org/2275773003/diff/1/fpdfsdk/fpdfview.cpp#newcod...
fpdfsdk/fpdfview.cpp:376: CPDFXFA_App* pProvider = CPDFXFA_App::GetInstance();
nit: pre-existing, local not needed.

[dsinclair, 2016-08-24 17:30:43]

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

[dsinclair, 2016-08-24 17:30:48]

https://codereview.chromium.org/2275773003/diff/1/core/fpdfapi/fpdf_parser/in...
File core/fpdfapi/fpdf_parser/include/cpdf_parser.h (right):

https://codereview.chromium.org/2275773003/diff/1/core/fpdfapi/fpdf_parser/in...
core/fpdfapi/fpdf_parser/include/cpdf_parser.h:120: CPDF_Document* m_pDocument; 
// not owned
On 2016/08/24 16:38:52, Tom Sepez wrote:
> pity this can't be CPDF_Document* const m_pDocument and we pass the doc in the
> ctor ... or can it?


At the moment, no.

Two reasons:
CPDF_Document accepts CPDF_Parser in it's ctor as a unique_ptr, so this can't
accept the document in it's ctor.

The CloseParser method sets the document to a nullptr. So, in theory, we can use
the parser multiple times given a different document to Start*Parse each time. I
don't know if we do it in practice, but needs to be tracked down and
CloseParser() either removed or made to stop changing the document.

https://codereview.chromium.org/2275773003/diff/1/fpdfsdk/fpdf_dataavail.cpp
File fpdfsdk/fpdf_dataavail.cpp (right):

https://codereview.chromium.org/2275773003/diff/1/fpdfsdk/fpdf_dataavail.cpp#...
fpdfsdk/fpdf_dataavail.cpp:151: CheckUnSupportError(pDocument.get(),
FPDF_ERR_SUCCESS);
On 2016/08/24 16:38:52, Tom Sepez wrote:
> nit: can this be a document method?

CheckUnSupportError calls into FPDF_UnSupportError which is part of fpdfsdk.
FPDF_UnSupportError can't easily be moved into Document as there are other
callers which would need to be worked out.

https://codereview.chromium.org/2275773003/diff/1/fpdfsdk/fpdfview.cpp
File fpdfsdk/fpdfview.cpp (right):

https://codereview.chromium.org/2275773003/diff/1/fpdfsdk/fpdfview.cpp#newcod...
fpdfsdk/fpdfview.cpp:376: CPDFXFA_App* pProvider = CPDFXFA_App::GetInstance();
On 2016/08/24 16:38:52, Tom Sepez wrote:
> nit: pre-existing, local not needed.

Done.

[commit-bot: I haz the power, 2016-08-24 17:30:48]

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-08-24 17:38:13]

lgtm

[dsinclair, 2016-08-24 17:46:36]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-08-24 17:46:37]

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/2275773003/#ps40001
(title: "Rebase to master")

[commit-bot: I haz the power, 2016-08-24 17:46:39]

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-08-24 18:12:22]

Description was changed from

==========
Flip document and parser ownership

This Cl switches the ownership between the parser and the document. Previously
the parser owned the document and we'd jump through hoops during cleanup to
delete the right object. This Cl flips the ownership so the document owns
the parser and simplifies the cleanup logic where needed.

BUG=pdfium:565
==========

to

==========
Flip document and parser ownership

This Cl switches the ownership between the parser and the document. Previously
the parser owned the document and we'd jump through hoops during cleanup to
delete the right object. This Cl flips the ownership so the document owns
the parser and simplifies the cleanup logic where needed.

BUG=pdfium:565

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

[commit-bot: I haz the power, 2016-08-24 18:12:22]

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