Implement FPDFAction_GetFilePath().

Implement FPDFAction_GetFilePath().

The API is the same as the Foxit version, except the encoding is
specified as UTF-8 instead of local encoding.

Also remove CPDF_LWinParam since it's unused.

BUG=chromium:517713
R=jun_fang@foxitsoftware.com

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

[Lei Zhang, 2015-09-12 01:01:26]

thestig@chromium.org changed reviewers:
+ jun_fang@foxitsoftware.com

[Lei Zhang, 2015-09-12 01:01:27]

Hi Jun, I noticed FPDFAction_GetFilePath() does not exist in PDFium, so I looked
up the FoxIt SDK API and implemented it from scratch. Let me know if this looks
ok, or if you would like me to modify it in some way to be more compatible with
the FoxIt version.

https://codereview.chromium.org/1335373002/diff/1/public/fpdf_doc.h
File public/fpdf_doc.h (right):

https://codereview.chromium.org/1335373002/diff/1/public/fpdf_doc.h#newcode136
public/fpdf_doc.h:136: //          FPDFAction_GetFilePath to
Mentioned here, but does not actually exist.

[jun_fang, 2015-09-14 12:10:43]

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

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:167: if (buffer && buflen >= len)
it should return 0 in 'else' rather than len.

[jun_fang, 2015-09-14 14:18:58]

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

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:149: if (!document)
if (!document || !pDict)
  return NULL;

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:176: if (!document)
if (!document || !pDict)
  return 0;

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:184: if (buffer != NULL && buflen >= len)
It should return 0 in 'else'.

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:184: if (buffer != NULL && buflen >= len)
if (!buffer && buflen >= len)

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:191: if (!document)
The same as above.

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:225: if (!document)
The same as above.

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
File fpdfsdk/src/fpdfdoc_embeddertest.cpp (right):

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
fpdfsdk/src/fpdfdoc_embeddertest.cpp:43: ASSERT_TRUE(page);
EXPECT_NE(nullptr, page);

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
fpdfsdk/src/fpdfdoc_embeddertest.cpp:47: ASSERT_TRUE(link);
EXPECT_NE(nullptr, link);

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
fpdfsdk/src/fpdfdoc_embeddertest.cpp:50: ASSERT_TRUE(action);
EXPECT_NE(nullptr, action);

[Lei Zhang, 2015-09-15 08:24:51]

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

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:149: if (!document)
On 2015/09/14 14:18:57, jun_fang wrote:
> if (!document || !pDict)
>   return NULL;

Done. (note this is existing code that I didn't write)

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:167: if (buffer && buflen >= len)
On 2015/09/14 12:10:43, jun_fang wrote:
> it should return 0 in 'else' rather than len.

Are you saying I should write the following?

if (buffer && buflen >= len) {
  FXSYS_memcpy(buffer, path.c_str(), len);
  return len;
}
return 0;  // (else case)

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:176: if (!document)
On 2015/09/14 14:18:57, jun_fang wrote:
> if (!document || !pDict)
>   return 0;

Done.

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:184: if (buffer != NULL && buflen >= len)
On 2015/09/14 14:18:57, jun_fang wrote:
> if (!buffer && buflen >= len)

Done.

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:191: if (!document)
On 2015/09/14 14:18:57, jun_fang wrote:
> The same as above.

Done.

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:225: if (!document)
On 2015/09/14 14:18:57, jun_fang wrote:
> The same as above.

Done.

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
File fpdfsdk/src/fpdfdoc_embeddertest.cpp (right):

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
fpdfsdk/src/fpdfdoc_embeddertest.cpp:43: ASSERT_TRUE(page);
On 2015/09/14 14:18:58, jun_fang wrote:
> EXPECT_NE(nullptr, page);

I want this test to be straight forward and only continue if all the expected
setup steps before FPDFAction_GetFilePath() works correctly. If anything fails,
and we continue, the rest of the test is going to be, as expected, a miserable
failure as well.

[jun_fang, 2015-09-15 08:35:03]

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

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:167: if (buffer && buflen >= len)
On 2015/09/15 08:24:50, Lei Zhang wrote:
> On 2015/09/14 12:10:43, jun_fang wrote:
> > it should return 0 in 'else' rather than len.
> 
> Are you saying I should write the following?
> 
> if (buffer && buflen >= len) {
>   FXSYS_memcpy(buffer, path.c_str(), len);
>   return len;
> }
> return 0;  // (else case)
>   

yes. For else cases, when buffer is null or buflen is less than len,
the memory that buffer points to is invalid for callers. We need
to inform the callers by the returned value (0).

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
File fpdfsdk/src/fpdfdoc_embeddertest.cpp (right):

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
fpdfsdk/src/fpdfdoc_embeddertest.cpp:43: ASSERT_TRUE(page);
On 2015/09/15 08:24:50, Lei Zhang wrote:
> On 2015/09/14 14:18:58, jun_fang wrote:
> > EXPECT_NE(nullptr, page);
> 
> I want this test to be straight forward and only continue if all the expected
> setup steps before FPDFAction_GetFilePath() works correctly. If anything
fails,
> and we continue, the rest of the test is going to be, as expected, a miserable
> failure as well.

The returned value, page, is handle not boolean value. Do you really want to
compare a handle to boolean value?

[Lei Zhang, 2015-09-15 08:50:57]

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

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
fpdfsdk/src/fpdfdoc.cpp:167: if (buffer && buflen >= len)
On 2015/09/15 08:35:03, jun_fang wrote:
> On 2015/09/15 08:24:50, Lei Zhang wrote:
> > On 2015/09/14 12:10:43, jun_fang wrote:
> > > it should return 0 in 'else' rather than len.
> > 
> > Are you saying I should write the following?
> > 
> > if (buffer && buflen >= len) {
> >   FXSYS_memcpy(buffer, path.c_str(), len);
> >   return len;
> > }
> > return 0;  // (else case)
> >   
> 
> yes. For else cases, when buffer is null or buflen is less than len,
> the memory that buffer points to is invalid for callers. We need
> to inform the callers by the returned value (0). 

But returning 0 contradicts the comments section of the documentation for this
function in the header file. Same for FPDFAction_GetURIPath() below. It's ok to
call FPDFAction_GetFilePath(action, nullptr, 0) and get back a non-zero value,
because that value is then used to allocate memory for a second
FPDFAction_GetFilePath(action, buf, previous_retval) call.

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
File fpdfsdk/src/fpdfdoc_embeddertest.cpp (right):

https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
fpdfsdk/src/fpdfdoc_embeddertest.cpp:43: ASSERT_TRUE(page);
On 2015/09/15 08:35:03, jun_fang wrote:
> On 2015/09/15 08:24:50, Lei Zhang wrote:
> > On 2015/09/14 14:18:58, jun_fang wrote:
> > > EXPECT_NE(nullptr, page);
> > 
> > I want this test to be straight forward and only continue if all the
expected
> > setup steps before FPDFAction_GetFilePath() works correctly. If anything
> fails,
> > and we continue, the rest of the test is going to be, as expected, a
miserable
> > failure as well.
> 
> The returned value, page, is handle not boolean value. Do you really want to
> compare a handle to boolean value?

So handles are really pointers, and the range of values for pointers are:
[nullptr, anything else that's not nullptr], which when passed to
ASSERT_TRUE(ptr), evals to [false, true]. This is fine since we don't care what
the value of the pointer is, as long as it's not nullptr.

This is the same as writing:

if (ptr) {  // pointer evaluated as a boolean on this line.
  ptr->DoSomething();
}

ASSERT_NE(nullptr, link) does the same thing, but it's just more verbose. It's
the equivalent to: if (link != nullptr) { ... }

[jun_fang, 2015-09-15 09:12:11]

On 2015/09/15 08:50:57, Lei Zhang wrote:
> https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp
> File fpdfsdk/src/fpdfdoc.cpp (right):
> 
>
https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
> fpdfsdk/src/fpdfdoc.cpp:167: if (buffer && buflen >= len)
> On 2015/09/15 08:35:03, jun_fang wrote:
> > On 2015/09/15 08:24:50, Lei Zhang wrote:
> > > On 2015/09/14 12:10:43, jun_fang wrote:
> > > > it should return 0 in 'else' rather than len.
> > > 
> > > Are you saying I should write the following?
> > > 
> > > if (buffer && buflen >= len) {
> > >   FXSYS_memcpy(buffer, path.c_str(), len);
> > >   return len;
> > > }
> > > return 0;  // (else case)
> > >   
> > 
> > yes. For else cases, when buffer is null or buflen is less than len,
> > the memory that buffer points to is invalid for callers. We need
> > to inform the callers by the returned value (0). 
> 
> But returning 0 contradicts the comments section of the documentation for this
> function in the header file. Same for FPDFAction_GetURIPath() below. It's ok
to
> call FPDFAction_GetFilePath(action, nullptr, 0) and get back a non-zero value,
> because that value is then used to allocate memory for a second
> FPDFAction_GetFilePath(action, buf, previous_retval) call.
> 
You are right. The caller can know whether this API is called successfully or
not
by comparing the returned len to the input buflen. If the returned len is larger
than the input buflen, the caller needs to call this API again with enough
buffer. 

>
https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
> File fpdfsdk/src/fpdfdoc_embeddertest.cpp (right):
> 
>
https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
> fpdfsdk/src/fpdfdoc_embeddertest.cpp:43: ASSERT_TRUE(page);
> On 2015/09/15 08:35:03, jun_fang wrote:
> > On 2015/09/15 08:24:50, Lei Zhang wrote:
> > > On 2015/09/14 14:18:58, jun_fang wrote:
> > > > EXPECT_NE(nullptr, page);
> > > 
> > > I want this test to be straight forward and only continue if all the
> expected
> > > setup steps before FPDFAction_GetFilePath() works correctly. If anything
> > fails,
> > > and we continue, the rest of the test is going to be, as expected, a
> miserable
> > > failure as well.
> > 
> > The returned value, page, is handle not boolean value. Do you really want to
> > compare a handle to boolean value?
> 
> So handles are really pointers, and the range of values for pointers are:
> [nullptr, anything else that's not nullptr], which when passed to
> ASSERT_TRUE(ptr), evals to [false, true]. This is fine since we don't care
what
> the value of the pointer is, as long as it's not nullptr.
> 
> This is the same as writing:
> 
> if (ptr) {  // pointer evaluated as a boolean on this line.
>   ptr->DoSomething();
> }
> 
> ASSERT_NE(nullptr, link) does the same thing, but it's just more verbose. It's
> the equivalent to: if (link != nullptr) { ... }

I know that ASSERT_TRUE(page) is the same as if(page). However,
ASSERT_TRUE(page)
is easy to make readers confused that the returned page is boolean. It's a
little  
bit tricky.

[jun_fang, 2015-09-15 09:15:07]

On 2015/09/15 09:12:11, jun_fang wrote:
> On 2015/09/15 08:50:57, Lei Zhang wrote:
> > https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp
> > File fpdfsdk/src/fpdfdoc.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc.cpp#new...
> > fpdfsdk/src/fpdfdoc.cpp:167: if (buffer && buflen >= len)
> > On 2015/09/15 08:35:03, jun_fang wrote:
> > > On 2015/09/15 08:24:50, Lei Zhang wrote:
> > > > On 2015/09/14 12:10:43, jun_fang wrote:
> > > > > it should return 0 in 'else' rather than len.
> > > > 
> > > > Are you saying I should write the following?
> > > > 
> > > > if (buffer && buflen >= len) {
> > > >   FXSYS_memcpy(buffer, path.c_str(), len);
> > > >   return len;
> > > > }
> > > > return 0;  // (else case)
> > > >   
> > > 
> > > yes. For else cases, when buffer is null or buflen is less than len,
> > > the memory that buffer points to is invalid for callers. We need
> > > to inform the callers by the returned value (0). 
> > 
> > But returning 0 contradicts the comments section of the documentation for
this
> > function in the header file. Same for FPDFAction_GetURIPath() below. It's ok
> to
> > call FPDFAction_GetFilePath(action, nullptr, 0) and get back a non-zero
value,
> > because that value is then used to allocate memory for a second
> > FPDFAction_GetFilePath(action, buf, previous_retval) call.
> > 
> You are right. The caller can know whether this API is called successfully or
> not
> by comparing the returned len to the input buflen. If the returned len is
larger
> than the input buflen, the caller needs to call this API again with enough
> buffer. 
> 
> >
>
https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
> > File fpdfsdk/src/fpdfdoc_embeddertest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1335373002/diff/1/fpdfsdk/src/fpdfdoc_embedde...
> > fpdfsdk/src/fpdfdoc_embeddertest.cpp:43: ASSERT_TRUE(page);
> > On 2015/09/15 08:35:03, jun_fang wrote:
> > > On 2015/09/15 08:24:50, Lei Zhang wrote:
> > > > On 2015/09/14 14:18:58, jun_fang wrote:
> > > > > EXPECT_NE(nullptr, page);
> > > > 
> > > > I want this test to be straight forward and only continue if all the
> > expected
> > > > setup steps before FPDFAction_GetFilePath() works correctly. If anything
> > > fails,
> > > > and we continue, the rest of the test is going to be, as expected, a
> > miserable
> > > > failure as well.
> > > 
> > > The returned value, page, is handle not boolean value. Do you really want
to
> > > compare a handle to boolean value?
> > 
> > So handles are really pointers, and the range of values for pointers are:
> > [nullptr, anything else that's not nullptr], which when passed to
> > ASSERT_TRUE(ptr), evals to [false, true]. This is fine since we don't care
> what
> > the value of the pointer is, as long as it's not nullptr.
> > 
> > This is the same as writing:
> > 
> > if (ptr) {  // pointer evaluated as a boolean on this line.
> >   ptr->DoSomething();
> > }
> > 
> > ASSERT_NE(nullptr, link) does the same thing, but it's just more verbose.
It's
> > the equivalent to: if (link != nullptr) { ... }
> 
> I know that ASSERT_TRUE(page) is the same as if(page). However,
> ASSERT_TRUE(page)
> is easy to make readers confused that the returned page is boolean. It's a
> little  
> bit tricky.

You can decide it by yourself. LGTM.

[Lei Zhang, 2015-09-15 21:45:25]

On 2015/09/15 09:15:07, jun_fang wrote:
> You can decide it by yourself. LGTM.

Thanks. We do ASSERT_TRUE(ptr) quite often in Chromium code, so I'm leaving the
test as is.

[Lei Zhang, 2015-09-15 21:45:33]

Committed patchset #2 (id:20001) manually as
0b3c8f742613da294f812e2f6e908f4026499f96 (presubmit successful).