PDF Viewer - Search results don't work after rotating pdf

PDF Viewer - Search results don't work after rotating pdf

Currently search results are cleared when user rotates
pdf document. As a result, the forward/backward buttons in
find bar do not work if user rotates pdf document after
finding some text in the document.

This happens because |find_results_| is cleared in the method
InvalidateAllPages(), which is called after rotation.

This patch introduces a new method to restore find results state
after invalidate operation is done. New find operation is done
to ensure rects are correctly identified for search results and
correct region is highlighted on pressing forward/backward buttons
in find bar.

BUG=389782
Committed: https://crrev.com/3a8252cdcdf60ad0a6a5ed2ba573801a13117ef0
Cr-Commit-Position: refs/heads/master@{#298083}

[Nikhil, 2014-09-09 10:40:24]

n.bansal@samsung.com changed reviewers:
+ raymes@chromium.org, thestig@chromium.org, vitalybuka@chromium.org

[Nikhil, 2014-09-09 10:40:25]

PTAL at my initial proposal to fix the issue where forward/backward buttons of
find bar don't work after pdf is rotated. 

In addition to making forward/backward buttons work, I see two different
behaviors with which this can be supported -

[1] Maintain the selection of current find result upon rotation.

[2] Do no maintain the selection of current find result upon rotation. Only the
next find result should get highlighted/selected once the user selects
forward/backward button.

Current patch implements [1] and I notice a slight jitter on some pages. I
believe this happens because StartFind() first highlights the first result, and
then I restore original find result index.

FWIW, FF seems to follow similar approach (and I notice jitter there as well).
Adobe seems to be follow approach somewhat similar to [2].

Please let me know which approach do you think sounds better.

Thanks!

[raymes, 2014-09-11 01:44:29]

Could we just StopFind when rotation happens? This seems like reasonable
behavior and it would simplify things.

Just as a general aside:
I find the code in pdfium_engine.cc difficult to read/maintain due to its size
and complexity. Because of that, my preference is to avoid adding new complexity
here wherever possible unless we think about refactoring things at the same
time. That's why I lean toward the opinion above :)

[Nikhil, 2014-09-12 04:59:46]

On 2014/09/11 01:44:29, raymes wrote:
> Could we just StopFind when rotation happens? This seems like reasonable
> behavior and it would simplify things.
> 

I'm afraid that calling StopFind() alone isn't sufficient. StopFind() only
clears the find result state in PDFiumEngine. Browser isn't notified to close
the find session. As a result, find bar will still be displayed and won't work.
So I guess we can do either of the following -

[1] Notify browser to close find session to hide the find bar after doing
rotation.

[2] Maintain find state by starting another find operation after doing rotation.

I defer to you decide which behavior to support. We will need to add code for
both :)

FWIW, Adobe Reader seems to follow [2]. And for [2], we will only need changes
in PDFiumEngine.

> Just as a general aside:
> I find the code in pdfium_engine.cc difficult to read/maintain due to its size
> and complexity. Because of that, my preference is to avoid adding new
complexity
> here wherever possible unless we think about refactoring things at the same
> time. That's why I lean toward the opinion above :)

I agree with you that code in pdfium_engine.cc is difficult to read/maintain
because of its size and complexity. Lack of tests only makes it difficult to
ensure that we don't break anything in an attempt to fix something. But I think
if we document the changes (comments, CL description etc), then we should be
fine for now. 

If you've any plans to refactor the code in pdfium_engine.cc, then do let me
know. I'd be happy to help you out with it :)

[raymes, 2014-09-22 05:16:45]

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

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1848:
StartFind(find_result_state_.find_text_.c_str(), false);
I don't fully understand the find stuff but can we not modify StartFind and
instead to change this code to do:

StartFind(...)
current_find_index_ = ...
SelectFindResult(true)

That way you don't need to add a new member variable or class.

[Nikhil, 2014-09-23 12:18:08]

raymes@ - Thanks for checking this! Please see my response to your comment.

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

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1848:
StartFind(find_result_state_.find_text_.c_str(), false);
On 2014/09/22 05:16:45, raymes wrote:
> I don't fully understand the find stuff but can we not modify StartFind and
> instead to change this code to do:
> 
> StartFind(...)
> current_find_index_ = ...
> SelectFindResult(true)
> 
> That way you don't need to add a new member variable or class.

The problem that I faced earlier with this code flow was that StartFind() is
async and it takes time to complete search. So, I needed a way to store find
index before which rotation happened and update current find index to it once
find operation completes. I introduced new class and member variable for this
purpose.

Updating current find index (once find finishes) to find index prior to rotation
is required to maintain consistency. For example :

[*] Search for text and go to 2nd search item.
[*] Rotate pdf document.
[*] Sarch for previous/next item.
[*] Now we should go to 1st/3rd search item.

Adobe Reader follows the above behavior. 

Please let me know if you see foresee issues with this approach or if there
could be a better way to achieve this.

A few links that might be useful -

[*] Start find :
https://code.google.com/p/chromium/codesearch#chromium/src/pdf/pdfium/pdfium_...

[*] Update find results :
https://code.google.com/p/chromium/codesearch#chromium/src/pdf/pdfium/pdfium_...

[*] End of find :
https://code.google.com/p/chromium/codesearch#chromium/src/pdf/pdfium/pdfium_...

[raymes, 2014-09-24 03:57:50]

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

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1634: if (find_result_state_.find_index_ != -1) {
It doesn't look like we need a new struct, it looks like we can just store the
index to resume the find at?

// When searching is complete, resume finding at a particular index.
if (resume_find_index_ != -1) {
  current_find_index_ = resume_find_index_ - 1;
  SelectFindResult(true);
  resume_find_index_ = -1;
}

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1846: // Restore find state if needed.
// Store the current find index so that we can resume finding at that 
// particular index after we have recomputed the find results.

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1848:
StartFind(find_result_state_.find_text_.c_str(), false);
Ah I see how StartFind is asynchronous now, thanks for explaining.

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1864: find_results_.clear();
These two lines and UpdateTickMarks() are already inside StopFind().

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1866: StopFind();
You might want to move this after CancelPaints() to ensure that it doesn't
interfere with it.

[Nikhil, 2014-09-29 08:22:11]

raymes@ - Sorry for the delay! I've updated the code as per review comments.
PTAL. Thanks.

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

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1634: if (find_result_state_.find_index_ != -1) {
On 2014/09/24 03:57:50, raymes wrote:
> It doesn't look like we need a new struct, it looks like we can just store the
> index to resume the find at?
> 
> // When searching is complete, resume finding at a particular index.
> if (resume_find_index_ != -1) {
>   current_find_index_ = resume_find_index_ - 1;
>   SelectFindResult(true);
>   resume_find_index_ = -1;
> }

Thanks, I've modified the code accordingly. Please see.

Also, I wanted to remove the call to SelectFindResult() to match Adobe Reader's
behavior but it looks like it is required. The way find operation is implemented
is that whenever we start a find, the first index gets highlighted. This
behavior is not desirable in the given use case and so I maintained the call to
SelectFindResult().

The call to SelectFindResult() causes a slight jitter on some pages, depending
on the content, because of the combination of - 

[*] Rotation
[*] First index is highlighted
[*] Desired index is highlighted

Please take a look and let me know WDYT.

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1846: // Restore find state if needed.
On 2014/09/24 03:57:50, raymes wrote:
> // Store the current find index so that we can resume finding at that 
> // particular index after we have recomputed the find results.

Done.

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1848:
StartFind(find_result_state_.find_text_.c_str(), false);
On 2014/09/24 03:57:50, raymes wrote:
> Ah I see how StartFind is asynchronous now, thanks for explaining.

Acknowledged.

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1864: find_results_.clear();
On 2014/09/24 03:57:50, raymes wrote:
> These two lines and UpdateTickMarks() are already inside StopFind(). 

Done.

https://codereview.chromium.org/555803002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1866: StopFind();
On 2014/09/24 03:57:50, raymes wrote:
> You might want to move this after CancelPaints() to ensure that it doesn't
> interfere with it.

Done.

[Nikhil, 2014-09-29 10:03:24]

raymes@ - I've further updated the patch. Please see the details below for the
new behavior :

[1] Search for text in pdf. First item/index gets highlighted.
[2] Use forward/backward buttons on find bar to move to other search items.
[3] Rotate clockwise/counterclockwise.
[4] Search item/index is not highlighted in document. Find bar remains displayed
and search index is maintained.
[5] Use forward/backward buttons on find bar.
[6] Move to correct search item based on search item/index prior to rotation.
[7] New search item is correctly highlighted.

This is consistent with Adobe Reader's behavior. Upon rotation, search index is
maintained but item is not highlighted. The slight jitter that I was
experiencing earlier is also not observed with this approach.

Please take a look and let me know if you've any concerns with this approach.

Thanks!

[raymes, 2014-10-01 20:05:02]

lgtm thanks!

[Nikhil, 2014-10-02 22:05:05]

On 2014/10/01 20:05:02, raymes wrote:
> lgtm thanks!

Thanks raymes@ for reviewing this! Should I go ahead and send this guy to CQ?

[Nikhil, 2014-10-03 08:04:01]

raymes@ - Rebase done! I'll land this once I get go-ahead from you :)

[raymes, 2014-10-03 14:27:27]

Feel free to land after you receive lgtm from all the reviewers. No need to
ask :)

On Fri Oct 03 2014 at 2:04:02 AM <n.bansal@samsung.com> wrote:

> raymes@ - Rebase done! I'll land this once I get go-ahead from you :)
>
> https://codereview.chromium.org/555803002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nikhil, 2014-10-03 16:00:32]

raymes@ - I asked because thestig@ is OOO as of now. So I'm not sure whether to
wait or land. Please let me know WDYT :)

[Nikhil, 2014-10-03 16:10:46]

On 2014/10/03 16:00:32, Nikhil wrote:
> raymes@ - I asked because thestig@ is OOO as of now. So I'm not sure whether
to
> wait or land. Please let me know WDYT :)

Oh, And the other patch is also almost ready
(https://codereview.chromium.org/375253002/) and has all the required LGTMs. But
I don't want to risk changing too many things too quickly (unfortunately, we
don't have tests!). So, I'll go with your judgement (wait or land?) :)

[raymes, 2014-10-03 16:26:13]

I'm ok with landing this.

BTW feel free to think about how we can add more tests for things, that
would be a very valuable thing to do :)
On Fri Oct 03 2014 at 10:10:49 AM <n.bansal@samsung.com> wrote:

> On 2014/10/03 16:00:32, Nikhil wrote:
> > raymes@ - I asked because thestig@ is OOO as of now. So I'm not sure
> > whether
> to
> > wait or land. Please let me know WDYT :)
>
> Oh, And the other patch is also almost ready
> (https://codereview.chromium.org/375253002/) and has all the required
> LGTMs. But
> I don't want to risk changing too many things too quickly (unfortunately,
> we
> don't have tests!). So, I'll go with your judgement (wait or land?) :)
>
> https://codereview.chromium.org/555803002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nikhil, 2014-10-03 19:19:55]

On 2014/10/03 16:26:13, raymes wrote:
> I'm ok with landing this.
> 

Great! CQing this guy then. Thanks for reviewing this! :)

> BTW feel free to think about how we can add more tests for things, that
> would be a very valuable thing to do :)

I'll start an email thread to discuss this. If we can figure out how to add
tests, I can take it up.

> On Fri Oct 03 2014 at 10:10:49 AM <mailto:n.bansal@samsung.com> wrote:
> 
> > On 2014/10/03 16:00:32, Nikhil wrote:
> > > raymes@ - I asked because thestig@ is OOO as of now. So I'm not sure
> > > whether
> > to
> > > wait or land. Please let me know WDYT :)
> >
> > Oh, And the other patch is also almost ready
> > (https://codereview.chromium.org/375253002/) and has all the required
> > LGTMs. But
> > I don't want to risk changing too many things too quickly (unfortunately,
> > we
> > don't have tests!). So, I'll go with your judgement (wait or land?) :)
> >
> > https://codereview.chromium.org/555803002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nikhil, 2014-10-03 19:20:08]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-10-03 19:20:39]

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

[commit-bot: I haz the power, 2014-10-03 19:55:21]

Committed patchset #4 (id:60001) as ddb4b014c13364321bbb68d08b3c19cd4abab506

[commit-bot: I haz the power, 2014-10-03 19:55:57]

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