PDF: Fix potential bad indexes in find results.

PDF: Fix potential bad indexes in find results.

Add a new FindTextData class to store the index to avoid all the size_t
to int casts.

BUG=416696
Committed: https://crrev.com/4da86e9519681acee27ec47c63685eb2804d7144
Cr-Commit-Position: refs/heads/master@{#302532}

[Lei Zhang, 2014-10-31 03:33:12]

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

[raymes, 2014-11-03 05:17:31]

Thank you a ton for taking a look. lgtm with some minor comments.

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:1796: size_t find_index =
current_find_index_.IncrementIndex();
It might be a bit simpler to just have IncrementIndex not return a value.

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:1825: }
optional: Alternatively I guess you could have Increment(size_t size) and do the
wraparound inside the function (you would need a Decrement too).

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:1834: if (current_find_index_.valid()) {
Since we call SetIndex right above, won't this always be valid?

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine.h
File pdf/pdfium/pdfium_engine.h (right):

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.h:155: class FindTextData {
Since this seems like an abstraction over the index, should we call it
FindTextIndex (or just FindIndex)?

[Lei Zhang, 2014-11-03 23:25:42]

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:1796: size_t find_index =
current_find_index_.IncrementIndex();
On 2014/11/03 05:17:30, raymes wrote:
> It might be a bit simpler to just have IncrementIndex not return a value.

It's not that much more work.
void IncrementIndex() { ++index_; }
vs
size_t IncrementIndex() { return ++index_; }

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:1825: }
On 2014/11/03 05:17:31, raymes wrote:
> optional: Alternatively I guess you could have Increment(size_t size) and do
the
> wraparound inside the function (you would need a Decrement too).

The class doesn't know about the size of |find_results_|, so the wrap around
code is staying outside.

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:1834: if (current_find_index_.valid()) {
On 2014/11/03 05:17:30, raymes wrote:
> Since we call SetIndex right above, won't this always be valid?

Indeed it is.

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine.h
File pdf/pdfium/pdfium_engine.h (right):

https://codereview.chromium.org/691273003/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.h:155: class FindTextData {
On 2014/11/03 05:17:31, raymes wrote:
> Since this seems like an abstraction over the index, should we call it
> FindTextIndex (or just FindIndex)?

FindTextIndex it is.

[Lei Zhang, 2014-11-03 23:28:51]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-11-03 23:30:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/691273003/60001

[commit-bot: I haz the power, 2014-11-04 01:00:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-04 01:01:02]

Patchset 4 (id:??) landed as
https://crrev.com/4da86e9519681acee27ec47c63685eb2804d7144
Cr-Commit-Position: refs/heads/master@{#302532}