Fix another discrepancy in text selection between PDF and HTML forms.

Fix another discrepancy in text selection between PDF and HTML forms.

Added fix for issue in which two notification messages of selected text
were being sent to the selection clipboard. Now, if a user selects text
in a PDF form text area and then selects text in a regular PDF text
area, only the regular selected text is sent. The same is true in the
reverse scenario - only one text selection is sent (the most recent one).

BUG=59266
Review-Url: https://codereview.chromium.org/2963403002
Cr-Commit-Position: refs/heads/master@{#485799}
Committed: https://chromium.googlesource.com/chromium/src/+/cb54de2bccfe08ba07d74435496c592d6289d480

[drgage, 2017-07-01 00:50:02]

Patchset #1 (id:1) has been deleted

[drgage, 2017-07-01 00:51:48]

Patchset #1 (id:20001) has been deleted

[drgage, 2017-07-01 00:53:38]

Description was changed from

==========
Fix another discrepancy in text selection between PDF and HTML forms.

Added fix for issue in which two notification messages of selected text
were being sent to the selection clipboard. Now, if a user selects text
in a PDF form text area and then selects text in a regular PDF text
area, only the regular selected text is sent. The same is true in the
reverse scenario - only one text selection is sent (the most recent one).

BUG=59266
==========

to

==========
Fix another discrepancy in text selection between PDF and HTML forms.

Added fix for issue in which two notification messages of selected text
were being sent to the selection clipboard. Now, if a user selects text
in a PDF form text area and then selects text in a regular PDF text
area, only the regular selected text is sent. The same is true in the
reverse scenario - only one text selection is sent (the most recent one).

BUG=59266
==========

[drgage, 2017-07-01 00:53:38]

drgage@google.com changed reviewers:
+ dsinclair@chromium.org, thestig@chromium.org

[drgage, 2017-07-01 00:58:34]

Hi Lei and Dan,

This CL fixes the selection bug where the clipboard received two selections from
the PDF plugin. With this change, it now only receives the expected result (the
most recently selected text).

Thanks for reviewing.

Best,
Diana

[Lei Zhang, 2017-07-07 01:10:42]

Here's a scenario that doesn't work:

1) Highlight some regular text on a PDF. e.g. the selection is "foo".
2) Click inside a form text field with pre-existing text. e.g. the pre-existing
text is "bar".
3) Hold down shift and use arrow keys to highlight "bar".
4) Middle click paste somewhere.

Expected: Pasted text should be "bar".
Actual: Pasted text is "foo".

When (2) occurs, OnSelectionChanged() gets called, but it still thinks the
selected text is "foo". Thus after (3), one renderer thinks the selection
clipboard contains "bar" but the other renderer still thinks it contains "foo".

[drgage, 2017-07-10 21:47:32]

Patchset #2 (id:60001) has been deleted

[drgage, 2017-07-10 21:54:14]

Patchset #2 (id:80001) has been deleted

[drgage, 2017-07-10 22:02:35]

On 2017/07/07 01:10:42, Lei Zhang wrote:
> Here's a scenario that doesn't work:
> 
> 1) Highlight some regular text on a PDF. e.g. the selection is "foo".
> 2) Click inside a form text field with pre-existing text. e.g. the
pre-existing
> text is "bar".
> 3) Hold down shift and use arrow keys to highlight "bar".
> 4) Middle click paste somewhere.
> 
> Expected: Pasted text should be "bar".
> Actual: Pasted text is "foo".
> 
> When (2) occurs, OnSelectionChanged() gets called, but it still thinks the
> selected text is "foo". Thus after (3), one renderer thinks the selection
> clipboard contains "bar" but the other renderer still thinks it contains
"foo".

Thanks, I think this is fixed now. Please let me know if you have any
edits/suggestions.

[Lei Zhang, 2017-07-10 23:08:21]

https://codereview.chromium.org/2963403002/diff/120001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2963403002/diff/120001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1730:
std::unique_ptr<chrome_pdf::PDFiumEngine::SelectionChangeInvalidator>
Since the return value from base::MakeUnique() (which will become
std::make_unique() eventually) is well known. You can just replace this long
line with "auto".

https://codereview.chromium.org/2963403002/diff/120001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1761: selection_invalidator.reset();
Can you add a comment to explain why this has to be called here?

https://codereview.chromium.org/2963403002/diff/120001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:3630: // If focus previously in form text area,
clear form text selection.
// If focus was previously in the form text area, clear the form text selection.
Otherwise ... ?

[drgage, 2017-07-11 17:19:35]

Patchset #4 (id:140001) has been deleted

[drgage, 2017-07-11 17:25:37]

Hi Lei and Dan,

In this patchset, I added a comment to explain why the |selection_invalidator|
is reset before the code that handles the form text area focus change, and used
the "auto" keyword to avoid spelling out the type name for the
SelectionChangeInvalidator unique pointer.

Thanks for reviewing.

Best,
Diana

https://codereview.chromium.org/2963403002/diff/120001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2963403002/diff/120001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1730:
std::unique_ptr<chrome_pdf::PDFiumEngine::SelectionChangeInvalidator>
On 2017/07/10 23:08:20, Lei Zhang wrote:
> Since the return value from base::MakeUnique() (which will become
> std::make_unique() eventually) is well known. You can just replace this long
> line with "auto".

Done.

https://codereview.chromium.org/2963403002/diff/120001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1761: selection_invalidator.reset();
On 2017/07/10 23:08:21, Lei Zhang wrote:
> Can you add a comment to explain why this has to be called here?

Done.

https://codereview.chromium.org/2963403002/diff/120001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:3630: // If focus previously in form text area,
clear form text selection.
On 2017/07/10 23:08:20, Lei Zhang wrote:
> // If focus was previously in the form text area, clear the form text
selection.
> Otherwise ... ?

Otherwise, nothing is cleared; the focus is changed and the in_form_text_area_
variable is set (like in the other case). Do you want me to add a comment that
explains this? I didn't think it was necessary because the lines 3634 and 3635
always run.

[Lei Zhang, 2017-07-11 20:37:39]

lgtm

https://codereview.chromium.org/2963403002/diff/120001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2963403002/diff/120001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:3630: // If focus previously in form text area,
clear form text selection.
On 2017/07/11 17:25:37, drgage wrote:
> On 2017/07/10 23:08:20, Lei Zhang wrote:
> > // If focus was previously in the form text area, clear the form text
> selection.
> > Otherwise ... ?
> 
> Otherwise, nothing is cleared; the focus is changed and the in_form_text_area_
> variable is set (like in the other case). Do you want me to add a comment that
> explains this? I didn't think it was necessary because the lines 3634 and 3635
> always run.

Otherwise, nothing is cleared and if |in_form_text_area_| flips to false, then
there is no way to clear the renderer clipboard selection. All the
pp::PDF::SetSelectedText() calls while |in_form_text_area_| is false goes to the
MimeHandler.

[drgage, 2017-07-11 22:32:51]

The CQ bit was checked by drgage@google.com

[drgage, 2017-07-11 22:32:51]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2963403002/#ps180001
(title: "Make comments more informative")

[commit-bot: I haz the power, 2017-07-11 22:33:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-12 02:20:39]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1499812371300400,
"parent_rev": "8130390255328a1d6e922d8cfd281304935f556b", "commit_rev":
"cb54de2bccfe08ba07d74435496c592d6289d480"}

[commit-bot: I haz the power, 2017-07-12 02:20:54]

Description was changed from

==========
Fix another discrepancy in text selection between PDF and HTML forms.

Added fix for issue in which two notification messages of selected text
were being sent to the selection clipboard. Now, if a user selects text
in a PDF form text area and then selects text in a regular PDF text
area, only the regular selected text is sent. The same is true in the
reverse scenario - only one text selection is sent (the most recent one).

BUG=59266
==========

to

==========
Fix another discrepancy in text selection between PDF and HTML forms.

Added fix for issue in which two notification messages of selected text
were being sent to the selection clipboard. Now, if a user selects text
in a PDF form text area and then selects text in a regular PDF text
area, only the regular selected text is sent. The same is true in the
reverse scenario - only one text selection is sent (the most recent one).

BUG=59266

Review-Url: https://codereview.chromium.org/2963403002
Cr-Commit-Position: refs/heads/master@{#485799}
Committed:
https://chromium.googlesource.com/chromium/src/+/cb54de2bccfe08ba07d74435496c...
==========

[commit-bot: I haz the power, 2017-07-12 02:20:55]

Committed patchset #5 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/cb54de2bccfe08ba07d74435496c...