Cleanup: Refactor some code into its own function in fpdf_text_int.cpp.

Cleanup: Refactor some code into its own function in fpdf_text_int.cpp.

Also use stdlib algorithms in a few places.

R=tsepez@chromium.org

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

[Lei Zhang, 2014-12-18 03:37:26]

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

[Tom Sepez, 2014-12-18 19:33:36]

LGTM % comments.

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:37: FX_FLOAT _NormalizeThreshold(FX_FLOAT
threshold)
Ok, lets just follow their naming convention even if identifiers arting with
underbar capital-letter are reserved for the compiler in the same way __ is.

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:39: int nDivide = 6;
Why we need nDivide local?  Why is it int? I realize you're just copying code,
but

if (threshold < 300 {
  return threshold / 2.0
}
...
return threshold / 6.0;

seems clearer.

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1740: FX_FLOAT spacing = 0;
nit: 0.0

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1851: int index = m_TempCharList.GetSize() -
n;
why not just count down instead? Is n used somewhere I didn't spot?

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1852: PAGECHAR_INFO* charinfo1 =
(PAGECHAR_INFO*)m_TempCharList.GetAt(index);
Doesn't the first iteration do ... .GetAt(... .GetSize()) which would be out of
bounds even for their custom arrays?  I bet there's a bug lurking here.

[Lei Zhang, 2014-12-18 21:07:10]

PTAL and sanity check my indexing.

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:37: FX_FLOAT _NormalizeThreshold(FX_FLOAT
threshold)
On 2014/12/18 19:33:36, Tom Sepez wrote:
> Ok, lets just follow their naming convention even if identifiers arting with
> underbar capital-letter are reserved for the compiler in the same way __ is.

I'm a bit unclear what the style guide is exactly, so just trying to stay
consistent as much as I can. Maybe we'll just clang-format the whole repo one
day. ;)

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:39: int nDivide = 6;
On 2014/12/18 19:33:36, Tom Sepez wrote:
> Why we need nDivide local?  Why is it int? I realize you're just copying code,
> but
> 
> if (threshold < 300 {
>   return threshold / 2.0
> }
> ...
> return threshold / 6.0;
> 
> seems clearer.

Done. I was just shuffling code around.

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1740: FX_FLOAT spacing = 0;
On 2014/12/18 19:33:36, Tom Sepez wrote:
> nit: 0.0 

Does that actually make a difference? There's too many of these everywhere.

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1851: int index = m_TempCharList.GetSize() -
n;
On 2014/12/18 19:33:36, Tom Sepez wrote:
> why not just count down instead? Is n used somewhere I didn't spot?

Done.

https://codereview.chromium.org/810223003/diff/1/core/src/fpdftext/fpdf_text_...
core/src/fpdftext/fpdf_text_int.cpp:1852: PAGECHAR_INFO* charinfo1 =
(PAGECHAR_INFO*)m_TempCharList.GetAt(index);
On 2014/12/18 19:33:36, Tom Sepez wrote:
> Doesn't the first iteration do ... .GetAt(... .GetSize()) which would be out
of
> bounds even for their custom arrays?  I bet there's a bug lurking here.

No, it's confusing because the first thing we do when we enter the loop is to
increment |n| from 0 to 1. Thus we start with |size() - 1| here.

[Tom Sepez, 2014-12-18 21:24:44]

https://codereview.chromium.org/810223003/diff/20001/core/src/fpdftext/fpdf_t...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/810223003/diff/20001/core/src/fpdftext/fpdf_t...
core/src/fpdftext/fpdf_text_int.cpp:1848: n > m_TempCharList.GetSize() - count -
1;
Hmmm.  This is just as confusing.  I think the way you had it before was better.
 For small sizes, the comparision is n > -1, which is trouble if GetSize() was
ever changed to returned a size_t (unsigned) (which it doesn't because the code
is int-happy). Sorry.

[Lei Zhang, 2014-12-18 21:49:03]

https://codereview.chromium.org/810223003/diff/20001/core/src/fpdftext/fpdf_t...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/810223003/diff/20001/core/src/fpdftext/fpdf_t...
core/src/fpdftext/fpdf_text_int.cpp:1848: n > m_TempCharList.GetSize() - count -
1;
On 2014/12/18 21:24:43, Tom Sepez wrote:
> Hmmm.  This is just as confusing.  I think the way you had it before was
better.
>  For small sizes, the comparision is n > -1, which is trouble if GetSize() was
> ever changed to returned a size_t (unsigned) (which it doesn't because the
code
> is int-happy). Sorry.

How about:

for (int n = m_TempCharList.GetSize(); n > m_TempCharList.GetSize() - count;
n--) {
  GetAt(n - 1);
}

Then |n| never underflows.

[Tom Sepez, 2014-12-18 21:55:52]

> Then |n| never underflows.
Yep.  That's it.

[Tom Sepez, 2014-12-18 21:56:46]

LGTM++ with that change.

[Lei Zhang, 2014-12-18 22:03:21]

On 2014/12/18 21:56:46, Tom Sepez wrote:
> LGTM++ with that change.

Patch set 3.

[Lei Zhang, 2014-12-18 22:03:50]

Committed patchset #3 (id:40001) manually as
ba2dc4a8111448f509e53fc58616b2dad3e742e9 (presubmit successful).