Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(155)

Issue 2924343005: Add functionality for copying text within form text fields and form combobox text fields (Closed)

Created:
3 years, 6 months ago by drgage
Modified:
3 years, 5 months ago
Reviewers:
Lei Zhang, dsinclair
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add functionality for copying text within PDF form fields. Text can be copied from selected text in text fields and combobox text fields. Selection can be done via holding down and moving the mouse as well as by using shift and arrow keys. BUG=59266 Review-Url: https://codereview.chromium.org/2924343005 Cr-Commit-Position: refs/heads/master@{#482088} Committed: https://chromium.googlesource.com/chromium/src/+/c32fae2601ea9890e50fa34caa8e80de1c007609

Patch Set 1 #

Patch Set 2 : Removed unwanted diff files #

Total comments: 14

Patch Set 3 : Updated style and commenting #

Total comments: 21

Patch Set 4 : Updated commenting and naming, PDFiumPage::FORM_TEXT_AREA changes #

Total comments: 8

Patch Set 5 : Additional updated style and commenting #

Patch Set 6 : Rebase with master #

Patch Set 7 : Add case for PDFiumPage::FORM_TEXT_AREA in switch #

Patch Set 8 : Selection bug fix #

Total comments: 6

Patch Set 9 : Form combobox text selection for copying #

Total comments: 14

Patch Set 10 : Changed combobox selection and fix for text selection bug #

Total comments: 28

Patch Set 11 : Add FormTypeToArea helper, style changes #

Total comments: 12

Patch Set 12 : Rebase #

Patch Set 13 : Update FormTypeToArea() to be static, style changes #

Total comments: 2

Patch Set 14 : Move FormTextFieldFocusChange into SetInFormTextArea, fix compile error with UTF8ToUTF16 #

Patch Set 15 : Add braces around multi-line conditional #

Unified diffs Side-by-side diffs Delta from patch set Stats (+94 lines, -16 lines) Patch
M pdf/pdfium/pdfium_engine.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +16 lines, -4 lines 0 comments Download
M pdf/pdfium/pdfium_engine.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 8 chunks +55 lines, -7 lines 0 comments Download
M pdf/pdfium/pdfium_page.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +10 lines, -3 lines 0 comments Download
M pdf/pdfium/pdfium_page.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +13 lines, -2 lines 0 comments Download

Messages

Total messages: 46 (16 generated)
drgage
Hi Lei and Dan, Please review this CL, which uses the PDFium API FORM_GetSelectedText() implemented ...
3 years, 6 months ago (2017-06-10 00:33:53 UTC) #3
Lei Zhang
https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engine.cc#newcode1634 pdf/pdfium/pdfium_engine.cc:1634: if (form_sel_text_len) { Invert + early return. https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engine.cc#newcode1635 pdf/pdfium/pdfium_engine.cc:1635: ...
3 years, 6 months ago (2017-06-10 01:55:49 UTC) #4
drgage
Hi Lei and Dan, In this patchset, I have addressed most of Lei's comments, but ...
3 years, 6 months ago (2017-06-12 18:57:48 UTC) #5
Lei Zhang
https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engine.cc#newcode1742 pdf/pdfium/pdfium_engine.cc:1742: mouse_down_state_.Set(PDFiumPage::FORM_TEXT_AREA, target); If we click on a form widget ...
3 years, 6 months ago (2017-06-14 07:07:03 UTC) #6
drgage
Hi Lei and Dan, In this patchset, I've made some changes to address Lei's comments ...
3 years, 6 months ago (2017-06-14 22:53:40 UTC) #7
drgage
On 2017/06/14 22:53:40, drgage wrote: > Hi Lei and Dan, > > In this patchset, ...
3 years, 6 months ago (2017-06-14 22:55:58 UTC) #8
Lei Zhang
https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engine.cc#newcode1891 pdf/pdfium/pdfium_engine.cc:1891: default: On 2017/06/14 22:53:39, drgage wrote: > On 2017/06/14 ...
3 years, 6 months ago (2017-06-15 18:11:28 UTC) #9
drgage
Hi Lei and Dan, In this patchset, I've changed my wording in the comments Lei ...
3 years, 6 months ago (2017-06-16 01:49:22 UTC) #10
drgage
Hi Lei and Dan, I realized I forgot to add in the PDFiumPage::FORM_TEXT_AREA case in ...
3 years, 6 months ago (2017-06-16 02:12:36 UTC) #11
Lei Zhang
https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engine.cc#newcode2039 pdf/pdfium/pdfium_engine.cc:2039: uint32_t modifiers = event.GetModifiers(); Try to declare variables as ...
3 years, 6 months ago (2017-06-16 21:34:53 UTC) #12
drgage
Hi Lei and Dan, In this patchset, I've added functionality for selecting text for copying ...
3 years, 6 months ago (2017-06-16 23:15:04 UTC) #13
Lei Zhang
https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engine.h#newcode665 pdf/pdfium/pdfium_engine.h:665: bool in_form_combobox_text_field_; Do you really need a separate boolean? ...
3 years, 6 months ago (2017-06-17 01:55:16 UTC) #16
Lei Zhang
https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engine.cc#newcode1632 pdf/pdfium/pdfium_engine.cc:1632: void PDFiumEngine::SetFormSelectedText(const FPDF_FORMHANDLE& form_handle, We don't do this consistently, ...
3 years, 6 months ago (2017-06-17 02:21:18 UTC) #17
drgage
Hi Lei and Dan, In this patchset, I addressed Lei's comments about how to handle ...
3 years, 6 months ago (2017-06-19 21:53:52 UTC) #18
Lei Zhang
https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_page.cc#newcode268 pdf/pdfium/pdfium_page.cc:268: if (*form_type == FPDF_FORMFIELD_TEXTFIELD) On 2017/06/19 21:53:52, drgage wrote: ...
3 years, 6 months ago (2017-06-20 00:26:24 UTC) #19
dsinclair
https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engine.cc#newcode1638 pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) Is this check needed? Won't ...
3 years, 6 months ago (2017-06-20 15:21:44 UTC) #20
Lei Zhang
https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engine.h#newcode321 pdf/pdfium/pdfium_engine.h:321: // Sets plugin's selected text to be selected text ...
3 years, 6 months ago (2017-06-20 18:34:58 UTC) #21
Lei Zhang
https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engine.cc#newcode1638 pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) On 2017/06/20 15:21:44, dsinclair wrote: ...
3 years, 6 months ago (2017-06-20 21:21:22 UTC) #22
drgage
Hi Lei and Dan, In this patchset, I've added a helper FormTypeToArea() to return the ...
3 years, 6 months ago (2017-06-20 23:14:27 UTC) #23
drgage
Hi Lei and Dan, In this patchset, I've added a helper FormTypeToArea() to return the ...
3 years, 6 months ago (2017-06-20 23:14:28 UTC) #24
Lei Zhang
Getting close. The CL description needs to be updated since it still mentions FORM_COMBOBOX_TEXT_AREA. https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_engine.cc ...
3 years, 6 months ago (2017-06-21 00:19:46 UTC) #25
drgage
Hi Lei and Dan, In this patchset, I have updated the description for the issue, ...
3 years, 6 months ago (2017-06-21 01:30:24 UTC) #28
dsinclair
https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engine.cc#newcode1638 pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) On 2017/06/20 23:14:27, drgage wrote: ...
3 years, 6 months ago (2017-06-21 13:03:29 UTC) #33
Lei Zhang
This is mostly working, though there are still some issues that we can look into ...
3 years, 6 months ago (2017-06-23 19:41:23 UTC) #34
drgage
Hi Lei and Dan, I fixed the bracing around the multi-line conditional that Lei pointed ...
3 years, 6 months ago (2017-06-23 22:16:13 UTC) #35
Lei Zhang
lgtm
3 years, 6 months ago (2017-06-23 22:53:14 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2924343005/270001
3 years, 6 months ago (2017-06-23 22:53:35 UTC) #38
Lei Zhang
I did some editing on the CL description...
3 years, 6 months ago (2017-06-23 23:13:00 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2924343005/270001
3 years, 6 months ago (2017-06-23 23:13:38 UTC) #43
commit-bot: I haz the power
3 years, 6 months ago (2017-06-24 00:18:04 UTC) #46
Message was sent while issue was closed.
Committed patchset #15 (id:270001) as
https://chromium.googlesource.com/chromium/src/+/c32fae2601ea9890e50fa34caa8e...

Powered by Google App Engine
This is Rietveld 408576698