Remove CFX_ArrayTemplate from CPDF_LinkExtract

Remove CFX_ArrayTemplate from CPDF_LinkExtract

Use unqiue_ptrs while we're at it, also better ctor.

Committed: https://pdfium.googlesource.com/pdfium/+/691411873cb16eb82d5912d0f73b96310f632932

[Tom Sepez, 2016-04-19 23:25:01]

Description was changed from

==========
Less usage of c_str(), misc tidy.
==========

to

==========
Remove CFX_ArrayTemplate from CPDF_LinkExtract

Use unqiue_ptrs while we're at it, also better ctor.
==========

[Tom Sepez, 2016-04-19 23:25:01]

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

[Tom Sepez, 2016-04-19 23:25:16]

Lei, for review.

[Lei Zhang, 2016-04-19 23:52:07]

lgtm

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
File core/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
core/fpdftext/fpdf_text_int.cpp:2491: if (!m_bIsParsed || index < 0 ||
Helper function instead of repeating this thrice?

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/include/c...
File core/fpdftext/include/cpdf_linkextract.h (right):

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/include/c...
core/fpdftext/include/cpdf_linkextract.h:22: CPDF_LinkExtract(const
CPDF_TextPage* pTextPage);
explicit

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/include/c...
core/fpdftext/include/cpdf_linkextract.h:53: std::vector<std::unique_ptr<Item>>
m_LinkList;
It's not actually a linked list.

[dsinclair, 2016-04-20 13:05:18]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org

[dsinclair, 2016-04-20 13:05:19]

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
File core/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
core/fpdftext/fpdf_text_int.cpp:2490: CFX_WideString
CPDF_LinkExtract::GetURL(int index) const {
Can index be changed from an int to a size_t so we know it's > 0?

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
core/fpdftext/fpdf_text_int.cpp:2498: void
CPDF_LinkExtract::GetBoundedSegment(int index,
size_t?

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
core/fpdftext/fpdf_text_int.cpp:2509: void CPDF_LinkExtract::GetRects(int index,
CFX_RectArray& rects) const {
size_t?

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/include/c...
File core/fpdftext/include/cpdf_linkextract.h (right):

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/include/c...
core/fpdftext/include/cpdf_linkextract.h:53: std::vector<std::unique_ptr<Item>>
m_LinkList;
Why unique_ptr<Item> instead of just Item?

[Tom Sepez, 2016-04-20 19:07:40]

Reworked. Let's try again.

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
File core/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
core/fpdftext/fpdf_text_int.cpp:2490: CFX_WideString
CPDF_LinkExtract::GetURL(int index) const {
On 2016/04/20 13:05:19, dsinclair wrote:
> Can index be changed from an int to a size_t so we know it's > 0?

Done.  Pushed negative value checking up to API calls.

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
core/fpdftext/fpdf_text_int.cpp:2491: if (!m_bIsParsed || index < 0 ||
On 2016/04/19 23:52:06, Lei Zhang wrote:
> Helper function instead of repeating this thrice?
Nah, short enough to prefer transparency over consolidation.

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
core/fpdftext/fpdf_text_int.cpp:2498: void
CPDF_LinkExtract::GetBoundedSegment(int index,
On 2016/04/20 13:05:19, dsinclair wrote:
> size_t?

Done.

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/fpdf_text...
core/fpdftext/fpdf_text_int.cpp:2509: void CPDF_LinkExtract::GetRects(int index,
CFX_RectArray& rects) const {
On 2016/04/20 13:05:19, dsinclair wrote:
> size_t?

Done.

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/include/c...
File core/fpdftext/include/cpdf_linkextract.h (right):

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/include/c...
core/fpdftext/include/cpdf_linkextract.h:53: std::vector<std::unique_ptr<Item>>
m_LinkList;
On 2016/04/19 23:52:06, Lei Zhang wrote:
> It's not actually a linked list.

Renamed.

https://codereview.chromium.org/1896303002/diff/60001/core/fpdftext/include/c...
core/fpdftext/include/cpdf_linkextract.h:53: std::vector<std::unique_ptr<Item>>
m_LinkList;
On 2016/04/20 13:05:19, dsinclair wrote:
> Why unique_ptr<Item> instead of just Item?

Even better.  Then I can get rid of the ctor and just use brace initialization
in the one push_back() that constructs these.

[dsinclair, 2016-04-20 19:12:55]

lgtm

[Tom Sepez, 2016-04-20 19:15:37]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-04-20 19:15:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/1896303002/#ps140001
(title: "<memory> no longer required.")

[commit-bot: I haz the power, 2016-04-20 19:15:40]

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

[commit-bot: I haz the power, 2016-04-20 19:23:50]

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

[commit-bot: I haz the power, 2016-04-20 19:23:51]

Try jobs failed on following builders:
  linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)

[Tom Sepez, 2016-04-20 21:26:57]

Ok, re-review required as I've fixed the test diff by
1) Clarifying the public API
2) Making the behaviour match it.

[Lei Zhang, 2016-04-20 21:36:49]

https://codereview.chromium.org/1896303002/diff/160001/fpdfsdk/fpdftext_embed...
File fpdfsdk/fpdftext_embeddertest.cpp (right):

https://codereview.chromium.org/1896303002/diff/160001/fpdfsdk/fpdftext_embed...
fpdfsdk/fpdftext_embeddertest.cpp:279: // Zero returned for bogus links.
Isn't this changing the public API behavior? Won't that potentially confuse some
clients?

[Tom Sepez, 2016-04-20 22:58:23]

On 2016/04/20 21:36:49, Lei Zhang wrote:
>
https://codereview.chromium.org/1896303002/diff/160001/fpdfsdk/fpdftext_embed...
> File fpdfsdk/fpdftext_embeddertest.cpp (right):
> 
>
https://codereview.chromium.org/1896303002/diff/160001/fpdfsdk/fpdftext_embed...
> fpdfsdk/fpdftext_embeddertest.cpp:279: // Zero returned for bogus links.
> Isn't this changing the public API behavior? Won't that potentially confuse
some
> clients?

Maybe.  But we never said what it was.  The difference is 0 vs "", or even -1
(!!!) in some case.
Are we ok?

[Tom Sepez, 2016-04-20 23:05:24]

On 2016/04/20 22:58:23, Tom Sepez wrote:
> On 2016/04/20 21:36:49, Lei Zhang wrote:
> >
>
https://codereview.chromium.org/1896303002/diff/160001/fpdfsdk/fpdftext_embed...
> > File fpdfsdk/fpdftext_embeddertest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1896303002/diff/160001/fpdfsdk/fpdftext_embed...
> > fpdfsdk/fpdftext_embeddertest.cpp:279: // Zero returned for bogus links.
> > Isn't this changing the public API behavior? Won't that potentially confuse
> some
> > clients?
> 
> Maybe.  But we never said what it was.  The difference is 0 vs "", or even -1
> (!!!) in some case.
> Are we ok?

Argh, I'll put it back the way it was. Stay tuned.

[Tom Sepez, 2016-04-20 23:38:47]

Ok, time for re-review.

[Lei Zhang, 2016-04-20 23:59:14]

lgtm

https://codereview.chromium.org/1896303002/diff/200001/core/fpdftext/fpdf_tex...
File core/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1896303002/diff/200001/core/fpdftext/fpdf_tex...
core/fpdftext/fpdf_text_int.cpp:265: CFX_RectArray* rectArray) const {
BTW, I have a feeling you can just return the array, but that'll be for another
day. (And convert to std::vector<CFX_FloatRect> as well)

[Tom Sepez, 2016-04-21 00:10:19]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-04-21 00:10:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/1896303002/#ps200001
(title: "Old API behaviour; funnel empty sring processing.")

[commit-bot: I haz the power, 2016-04-21 00:10:28]

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

[commit-bot: I haz the power, 2016-04-21 01:59:23]

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

[commit-bot: I haz the power, 2016-04-21 01:59:24]

Try jobs failed on following builders:
  win_no_v8 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_no_v8/build...)

[dsinclair, 2016-04-21 13:11:05]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-04-21 13:11:17]

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

[dsinclair, 2016-04-21 14:51:36]

On 2016/04/21 13:11:17, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1896303002/200001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1896303002/200001


Looks like the test is hanging: FPDFTextEmbeddertest.WebLinks

[commit-bot: I haz the power, 2016-04-21 15:29:38]

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

[commit-bot: I haz the power, 2016-04-21 15:29:40]

Try jobs failed on following builders:
  win_no_v8 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_no_v8/build...)

[Tom Sepez, 2016-04-21 16:25:50]

On 2016/04/21 15:29:40, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_no_v8 on master.tryserver.client.pdfium (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_no_v8/build...)

Swell.  The exact test corresponding to what I changed.  Looking.

[Tom Sepez, 2016-04-21 16:35:02]

On 2016/04/21 16:25:50, Tom Sepez wrote:
> On 2016/04/21 15:29:40, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   win_no_v8 on master.tryserver.client.pdfium (JOB_FAILED,
> >
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_no_v8/build...)
> 
> Swell.  The exact test corresponding to what I changed.  Looking.

Probably trying to return that rect array by value - copy ctor defaulted, I
guess not duplicating buffer?

[Tom Sepez, 2016-04-21 17:40:10]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-04-21 17:40:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/1896303002/#ps160002
(title: "No RVO for you.")

[commit-bot: I haz the power, 2016-04-21 17:40:25]

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

[commit-bot: I haz the power, 2016-04-21 17:43:41]

Description was changed from

==========
Remove CFX_ArrayTemplate from CPDF_LinkExtract

Use unqiue_ptrs while we're at it, also better ctor.
==========

to

==========
Remove CFX_ArrayTemplate from CPDF_LinkExtract

Use unqiue_ptrs while we're at it, also better ctor.

Committed:
https://pdfium.googlesource.com/pdfium/+/691411873cb16eb82d5912d0f73b96310f63...
==========

[commit-bot: I haz the power, 2016-04-21 17:43:42]

Committed patchset #12 (id:160002) as
https://pdfium.googlesource.com/pdfium/+/691411873cb16eb82d5912d0f73b96310f63...