|
|
DescriptionAdd 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 #
Messages
Total messages: 46 (16 generated)
Description was changed from ========== Add functionality for copying text within form text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextField() for setting status of whether we're in form text field 3. New Area within PDFiumPage, FORM_TEXT_AREA to represent form text field 4. New member variable in_form_text_field_ which is TRUE if we're within a form text field BUG=59266 ========== to ========== Add functionality for copying text within form text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextField() for setting status of whether we're in form text field 3. New Area within PDFiumPage, FORM_TEXT_AREA to represent form text field 4. New member variable in_form_text_field_ which is TRUE if we're within a form text field BUG=59266 ==========
drgage@google.com changed reviewers: + dsinclair@chromium.org, thestig@chromium.org
Hi Lei and Dan, Please review this CL, which uses the PDFium API FORM_GetSelectedText() implemented in my previous CL to copy selected text within form text fields. Thanks, Diana
https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1634: if (form_sel_text_len) { Invert + early return. https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1635: base::string16 selected_form_text; How about calling the std::string version selected_form_text, and this selected_form_text16? https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1642: if (str_selected_form_text.c_str() && str_selected_form_text != "") { I think you just want to check: !str_selected_form_text.empty() https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1750: SetInFormTextField(is_valid_control); Isn't this also going to be true for a combo box? https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2025: if (in_form_text_field_ && shift_key) { If |in_form_text_field_| is false, then it is not necessary to even look up the keyboard modifier. So this can be written as: if (in_form_text_field_) { uint32_t modifiers = event.GetModifiers(); if (modifiers & PP_INPUTEVENT_MODIFIER_SHIFTKEY) SetFormSelectedText(form_, pages_[last_page_mouse_down_]->GetPage()); } https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:448: // Sets status of whether or not we're in a form field text field. Try to avoid pronouns. Who does "we" refer to? You and I are not trapped in a form field. There's only a single PDFiumEngine... https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_page.h File pdf/pdfium/pdfium_page.h (right): https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_page.... pdf/pdfium/pdfium_page.h:57: FORM_TEXT_AREA, // Area is a form field text field. It may be helpful to add a comment to TEXT_AREA above to explain the difference.
Hi Lei and Dan, In this patchset, I have addressed most of Lei's comments, but I'm not sure of the best way to proceed with setting is_valid_control - I can think of a few possible solutions, but I'd like to hear what you think is best. Thanks, Diana https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1634: if (form_sel_text_len) { On 2017/06/10 01:55:49, Lei Zhang (OOO) wrote: > Invert + early return. Done. https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1635: base::string16 selected_form_text; On 2017/06/10 01:55:49, Lei Zhang (OOO) wrote: > How about calling the std::string version selected_form_text, and this > selected_form_text16? Done. https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1642: if (str_selected_form_text.c_str() && str_selected_form_text != "") { On 2017/06/10 01:55:49, Lei Zhang (OOO) wrote: > I think you just want to check: !str_selected_form_text.empty() Done. https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1750: SetInFormTextField(is_valid_control); On 2017/06/10 01:55:49, Lei Zhang (OOO) wrote: > Isn't this also going to be true for a combo box? Yeah, you're right. Do you think I should change how is_valid_control is set so that it's clear which form type it is? https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2025: if (in_form_text_field_ && shift_key) { On 2017/06/10 01:55:49, Lei Zhang (OOO) wrote: > If |in_form_text_field_| is false, then it is not necessary to even look up the > keyboard modifier. So this can be written as: > > if (in_form_text_field_) { > uint32_t modifiers = event.GetModifiers(); > if (modifiers & PP_INPUTEVENT_MODIFIER_SHIFTKEY) > SetFormSelectedText(form_, pages_[last_page_mouse_down_]->GetPage()); > } Done. https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:448: // Sets status of whether or not we're in a form field text field. On 2017/06/10 01:55:49, Lei Zhang (OOO) wrote: > Try to avoid pronouns. Who does "we" refer to? You and I are not trapped in a > form field. There's only a single PDFiumEngine... Done. Wouldn't it be strange if we were trapped in a form field? :) https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_page.h File pdf/pdfium/pdfium_page.h (right): https://codereview.chromium.org/2924343005/diff/20001/pdf/pdfium/pdfium_page.... pdf/pdfium/pdfium_page.h:57: FORM_TEXT_AREA, // Area is a form field text field. On 2017/06/10 01:55:49, Lei Zhang (OOO) wrote: > It may be helpful to add a comment to TEXT_AREA above to explain the difference. Done.
https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1742: mouse_down_state_.Set(PDFiumPage::FORM_TEXT_AREA, target); If we click on a form widget that is not a form text area, is this the right type to set? https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1750: // TODO (drgage): come back here and figure out what to do with I think this is actually correct as is in the non-XFA case. BTW, I referred to the combo box as drop down control. It looks like Acrobat Reader can copy text from these two types of fields, so we should as well. We can work on making combo boxes actually work separately. (XFA will be even more work to figure out. Maybe leave a TODO(bug_62400) here for XFA?) https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1860: if (!selecting_) The existing code like this member variable and SetSelecting() can use some updated comments to better explain what they select. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1891: default: Let's handle FORM_TEXT_AREA explicitly here too. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2026: if (modifiers & PP_INPUTEVENT_MODIFIER_SHIFTKEY) I tried this code out and noticed if I: 1) used the shift + arrow keys to select some text 2) stopped holding shift 3) used the arrow keys to move around Then the selection disappears visually, but remains as far as the selection code for copying is concerned. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:448: // Sets status of whether or not focus is in form field text field. As mentioned in the PDFium-side CL, how about just "form text field" ? Ditto on line 657. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_page.... pdf/pdfium/pdfium_page.cc:266: *form_type = control; Should we check the form type here as well, and return FORM_TEXT_AREA when appropriate? https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_page.... pdf/pdfium/pdfium_page.cc:287: if (*form_type == FPDF_FORMFIELD_TEXTFIELD) { We may want to check the combo box type when we get around to improving selection for that form field type.
Hi Lei and Dan, In this patchset, I've made some changes to address Lei's comments on the previous patchset regarding when FORM_TEXT_AREA is set, as well as commenting. I am planning on looking further into the bug with the visual selection not matching the selection code, and I plan to update this so that text selection and copying is possible in comboboxes. Thanks for reviewing. Best, Diana https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1742: mouse_down_state_.Set(PDFiumPage::FORM_TEXT_AREA, target); On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > If we click on a form widget that is not a form text area, is this the right > type to set? Done. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1742: mouse_down_state_.Set(PDFiumPage::FORM_TEXT_AREA, target); On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > If we click on a form widget that is not a form text area, is this the right > type to set? Probably not - I'll change this so that it only is set to FORM_TEXT_AREA if the form_type == FPDF_FORMFIELD_TEXTFIELD. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1750: // TODO (drgage): come back here and figure out what to do with On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > I think this is actually correct as is in the non-XFA case. BTW, I referred to > the combo box as drop down control. > > It looks like Acrobat Reader can copy text from these two types of fields, so we > should as well. We can work on making combo boxes actually work separately. (XFA > will be even more work to figure out. Maybe leave a TODO(bug_62400) here for > XFA?) Done. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1860: if (!selecting_) On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > The existing code like this member variable and SetSelecting() can use some > updated comments to better explain what they select. Done. Do you think it's sufficient to add an explanation for these in pdfium_engine.h? I can also add a comment here, but I can make it clear that selection_ is used for text selection. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1891: default: On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > Let's handle FORM_TEXT_AREA explicitly here too. Do you think I should just add another case without a break statement? Did you have anything else in mind? https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2026: if (modifiers & PP_INPUTEVENT_MODIFIER_SHIFTKEY) On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > I tried this code out and noticed if I: > 1) used the shift + arrow keys to select some text > 2) stopped holding shift > 3) used the arrow keys to move around > > Then the selection disappears visually, but remains as far as the selection code > for copying is concerned. I see what you mean. I hadn't tried this combination of actions. The same thing happens if you press any other key, like if you type another letter and erase what was there, the old selected text is still available for copying just because the shift key was released. The same thing also happens with the mouse selection. If you have something selected with the mouse, then press a key, the visual selection disappears but you can still copy the old selected text. I can look into how the visual selection code works and try to fix this. As far as text selection (i.e. via the selection_ vector for PDFiumPage::TEXT_AREA text) is concerned, you can't select using the shift and arrow keys at all, and if you select with the mouse and then press the arrow keys, the current selection doesn't visually change/disappear, so this isn't a problem there. I'm thinking another option for handling this bug would be to mimic this behavior and prevent the visual selection from changing if you press a key within a form text field when you have text selected, but I don't think this is a great choice for user experience, so I'm going to look into how the visual selection code works. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:448: // Sets status of whether or not focus is in form field text field. On 2017/06/14 07:07:03, Lei Zhang (OOO) wrote: > As mentioned in the PDFium-side CL, how about just "form text field" ? Ditto on > line 657. Done. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_page.... pdf/pdfium/pdfium_page.cc:266: *form_type = control; On 2017/06/14 07:07:03, Lei Zhang (OOO) wrote: > Should we check the form type here as well, and return FORM_TEXT_AREA when > appropriate? Done. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_page.... pdf/pdfium/pdfium_page.cc:287: if (*form_type == FPDF_FORMFIELD_TEXTFIELD) { On 2017/06/14 07:07:03, Lei Zhang (OOO) wrote: > We may want to check the combo box type when we get around to improving > selection for that form field type. Done.
On 2017/06/14 22:53:40, drgage wrote: > Hi Lei and Dan, > > In this patchset, I've made some changes to address Lei's comments on the > previous patchset regarding when FORM_TEXT_AREA is set, as well as commenting. > > I am planning on looking further into the bug with the visual selection not > matching the selection code, and I plan to update this so that text selection > and copying is possible in comboboxes. > > Thanks for reviewing. > > Best, > Diana > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... > File pdf/pdfium/pdfium_engine.cc (right): > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.cc:1742: > mouse_down_state_.Set(PDFiumPage::FORM_TEXT_AREA, target); > On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > > If we click on a form widget that is not a form text area, is this the right > > type to set? > > Done. > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.cc:1742: > mouse_down_state_.Set(PDFiumPage::FORM_TEXT_AREA, target); > On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > > If we click on a form widget that is not a form text area, is this the right > > type to set? > > Probably not - I'll change this so that it only is set to FORM_TEXT_AREA if the > form_type == FPDF_FORMFIELD_TEXTFIELD. > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.cc:1750: // TODO (drgage): come back here and figure > out what to do with > On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > > I think this is actually correct as is in the non-XFA case. BTW, I referred to > > the combo box as drop down control. > > > > It looks like Acrobat Reader can copy text from these two types of fields, so > we > > should as well. We can work on making combo boxes actually work separately. > (XFA > > will be even more work to figure out. Maybe leave a TODO(bug_62400) here for > > XFA?) > > Done. > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.cc:1860: if (!selecting_) > On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > > The existing code like this member variable and SetSelecting() can use some > > updated comments to better explain what they select. > > Done. Do you think it's sufficient to add an explanation for these in > pdfium_engine.h? I can also add a comment here, but I can make it clear that > selection_ is used for text selection. > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.cc:1891: default: > On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > > Let's handle FORM_TEXT_AREA explicitly here too. > > Do you think I should just add another case without a break statement? Did you > have anything else in mind? > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.cc:2026: if (modifiers & > PP_INPUTEVENT_MODIFIER_SHIFTKEY) > On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > > I tried this code out and noticed if I: > > 1) used the shift + arrow keys to select some text > > 2) stopped holding shift > > 3) used the arrow keys to move around > > > > Then the selection disappears visually, but remains as far as the selection > code > > for copying is concerned. > > I see what you mean. I hadn't tried this combination of actions. The same thing > happens if you press any other key, like if you type another letter and erase > what was there, the old selected text is still available for copying just > because the shift key was released. The same thing also happens with the mouse > selection. If you have something selected with the mouse, then press a key, the > visual selection disappears but you can still copy the old selected text. > > I can look into how the visual selection code works and try to fix this. > > As far as text selection (i.e. via the selection_ vector for > PDFiumPage::TEXT_AREA text) is concerned, you can't select using the shift and > arrow keys at all, and if you select with the mouse and then press the arrow > keys, the current selection doesn't visually change/disappear, so this isn't a > problem there. I'm thinking another option for handling this bug would be to > mimic this behavior and prevent the visual selection from changing if you press > a key within a form text field when you have text selected, but I don't think > this is a great choice for user experience, so I'm going to look into how the > visual selection code works. > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engine.h > File pdf/pdfium/pdfium_engine.h (right): > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... > pdf/pdfium/pdfium_engine.h:448: // Sets status of whether or not focus is in > form field text field. > On 2017/06/14 07:07:03, Lei Zhang (OOO) wrote: > > As mentioned in the PDFium-side CL, how about just "form text field" ? Ditto > on > > line 657. > > Done. > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_page.cc > File pdf/pdfium/pdfium_page.cc (right): > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_page.... > pdf/pdfium/pdfium_page.cc:266: *form_type = control; > On 2017/06/14 07:07:03, Lei Zhang (OOO) wrote: > > Should we check the form type here as well, and return FORM_TEXT_AREA when > > appropriate? > > Done. > > https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_page.... > pdf/pdfium/pdfium_page.cc:287: if (*form_type == FPDF_FORMFIELD_TEXTFIELD) { > On 2017/06/14 07:07:03, Lei Zhang (OOO) wrote: > > We may want to check the combo box type when we get around to improving > > selection for that form field type. > > Done. Also, you'll notice in pdfium_engine.cc on line 1636 that I changed the condition from "if (!form_sel_text_len)" to "if (form_sel_text_len <= 2)" because I noticed that an empty selection actually has a length of 2 instead of 0.
https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1891: default: On 2017/06/14 22:53:39, drgage wrote: > On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > > Let's handle FORM_TEXT_AREA explicitly here too. > > Do you think I should just add another case without a break statement? Did you > have anything else in mind? Add another case without a break. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2026: if (modifiers & PP_INPUTEVENT_MODIFIER_SHIFTKEY) On 2017/06/14 22:53:39, drgage wrote: > On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > > I tried this code out and noticed if I: > > 1) used the shift + arrow keys to select some text > > 2) stopped holding shift > > 3) used the arrow keys to move around > > > > Then the selection disappears visually, but remains as far as the selection > code > > for copying is concerned. > > I see what you mean. I hadn't tried this combination of actions. The same thing > happens if you press any other key, like if you type another letter and erase > what was there, the old selected text is still available for copying just > because the shift key was released. The same thing also happens with the mouse > selection. If you have something selected with the mouse, then press a key, the > visual selection disappears but you can still copy the old selected text. > > I can look into how the visual selection code works and try to fix this. > > As far as text selection (i.e. via the selection_ vector for > PDFiumPage::TEXT_AREA text) is concerned, you can't select using the shift and > arrow keys at all, and if you select with the mouse and then press the arrow > keys, the current selection doesn't visually change/disappear, so this isn't a > problem there. I'm thinking another option for handling this bug would be to > mimic this behavior and prevent the visual selection from changing if you press > a key within a form text field when you have text selected, but I don't think > this is a great choice for user experience, so I'm going to look into how the > visual selection code works. I don't think you need to actually know how the visual selection works. Just look at how pp::PDF::SetSelectedText() gets called when there is a non-form text selection, and one does a mouse click to clear that selection. https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1745: (form_type == FPDF_FORMFIELD_TEXTFIELD) style: With a ternary operator, write this as: mouse_down_state_.Set(form_type == FPDF_FORMFIELD_TEXTFIELD ? PDFiumPage::FORM_TEXT_AREA : PDFiumPage::NONSELECTABLE_AREA, target); so there's no need to repeat most of the statment. Otherwise, write: if (form_type == FPDF_FORMFIELD_TEXTFIELD) ... else ... https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:445: // Setting text selection status of document. I don't think most readers would understand form text is not included when they read this comment. Same with line 655. https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:651: // Used for text selection in PDFiumPage::TEXT_AREA areas, not within Can we use PDFiumPage::TEXT_AREA and PDFiumPage::FORM_TEXT_AREA in this comment? Or don't refer to the enum values as all and explain in plain English. https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_page.... pdf/pdfium/pdfium_page.cc:291: // TODO (drgage): remember to add in case for combobox later style: no space after the word "TODO". Ditto in pdfium_engine.cc. There's no need to write "remember to" or "come back and" either.
Hi Lei and Dan, In this patchset, I've changed my wording in the comments Lei flagged and changed the way I set mouse_down_state_ with the ternary operator. Thanks for reviewing. Diana https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1745: (form_type == FPDF_FORMFIELD_TEXTFIELD) On 2017/06/15 18:11:28, Lei Zhang wrote: > style: With a ternary operator, write this as: > > mouse_down_state_.Set(form_type == FPDF_FORMFIELD_TEXTFIELD > ? PDFiumPage::FORM_TEXT_AREA : PDFiumPage::NONSELECTABLE_AREA, target); > > so there's no need to repeat most of the statment. Otherwise, write: > > if (form_type == FPDF_FORMFIELD_TEXTFIELD) > ... > else > ... Done. https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:445: // Setting text selection status of document. On 2017/06/15 18:11:28, Lei Zhang wrote: > I don't think most readers would understand form text is not included when they > read this comment. Same with line 655. Done. https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:651: // Used for text selection in PDFiumPage::TEXT_AREA areas, not within On 2017/06/15 18:11:28, Lei Zhang wrote: > Can we use PDFiumPage::TEXT_AREA and PDFiumPage::FORM_TEXT_AREA in this comment? > Or don't refer to the enum values as all and explain in plain English. Done. https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2924343005/diff/60001/pdf/pdfium/pdfium_page.... pdf/pdfium/pdfium_page.cc:291: // TODO (drgage): remember to add in case for combobox later On 2017/06/15 18:11:28, Lei Zhang wrote: > style: no space after the word "TODO". Ditto in pdfium_engine.cc. There's no > need to write "remember to" or "come back and" either. Done.
Hi Lei and Dan, I realized I forgot to add in the PDFiumPage::FORM_TEXT_AREA case in the switch statement that Lei pointed out, so this is all this patchset is. Sorry about that! Thanks for reviewing. Diana https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1891: default: On 2017/06/15 18:11:28, Lei Zhang wrote: > On 2017/06/14 22:53:39, drgage wrote: > > On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > > > Let's handle FORM_TEXT_AREA explicitly here too. > > > > Do you think I should just add another case without a break statement? Did you > > have anything else in mind? > > Add another case without a break. Done. https://codereview.chromium.org/2924343005/diff/40001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2026: if (modifiers & PP_INPUTEVENT_MODIFIER_SHIFTKEY) On 2017/06/15 18:11:28, Lei Zhang wrote: > On 2017/06/14 22:53:39, drgage wrote: > > On 2017/06/14 07:07:02, Lei Zhang (OOO) wrote: > > > I tried this code out and noticed if I: > > > 1) used the shift + arrow keys to select some text > > > 2) stopped holding shift > > > 3) used the arrow keys to move around > > > > > > Then the selection disappears visually, but remains as far as the selection > > code > > > for copying is concerned. > > > > I see what you mean. I hadn't tried this combination of actions. The same > thing > > happens if you press any other key, like if you type another letter and erase > > what was there, the old selected text is still available for copying just > > because the shift key was released. The same thing also happens with the mouse > > selection. If you have something selected with the mouse, then press a key, > the > > visual selection disappears but you can still copy the old selected text. > > > > I can look into how the visual selection code works and try to fix this. > > > > As far as text selection (i.e. via the selection_ vector for > > PDFiumPage::TEXT_AREA text) is concerned, you can't select using the shift and > > arrow keys at all, and if you select with the mouse and then press the arrow > > keys, the current selection doesn't visually change/disappear, so this isn't a > > problem there. I'm thinking another option for handling this bug would be to > > mimic this behavior and prevent the visual selection from changing if you > press > > a key within a form text field when you have text selected, but I don't think > > this is a great choice for user experience, so I'm going to look into how the > > visual selection code works. > > I don't think you need to actually know how the visual selection works. Just > look at how pp::PDF::SetSelectedText() gets called when there is a non-form text > selection, and one does a mouse click to clear that selection. Done.
https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2039: uint32_t modifiers = event.GetModifiers(); Try to declare variables as close to where they are used, within the right scope. In this case, maybe even just get rid of the variable since it'll still fit on 1 line. https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2044: SelectionChangeInvalidator selection_invalidator(this); SelectionChangeInvalidator seems to do a lot of work internally. Do you really it to do all that work here? https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:323: void SetFormSelectedText(const FPDF_FORMHANDLE& hHandle, hHandle -> PDFium variable naming style leaking out.
Hi Lei and Dan, In this patchset, I've added functionality for selecting text for copying within form combobox text fields. I also simplified the fix for the selection bug. Thanks for reviewing. Diana https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2039: uint32_t modifiers = event.GetModifiers(); On 2017/06/16 21:34:53, Lei Zhang wrote: > Try to declare variables as close to where they are used, within the right > scope. In this case, maybe even just get rid of the variable since it'll still > fit on 1 line. Done. https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2044: SelectionChangeInvalidator selection_invalidator(this); On 2017/06/16 21:34:53, Lei Zhang wrote: > SelectionChangeInvalidator seems to do a lot of work internally. Do you really > it to do all that work here? Done. https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/140001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:323: void SetFormSelectedText(const FPDF_FORMHANDLE& hHandle, On 2017/06/16 21:34:53, Lei Zhang wrote: > hHandle -> PDFium variable naming style leaking out. Done.
Description was changed from ========== Add functionality for copying text within form text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextField() for setting status of whether we're in form text field 3. New Area within PDFiumPage, FORM_TEXT_AREA to represent form text field 4. New member variable in_form_text_field_ which is TRUE if we're within a form text field BUG=59266 ========== to ========== Add functionality for copying text within form text fields and form combobox text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextField() for setting status of whether we're in form text field - PDFiumEngine::SetInFormComboboxTextField() for setting status of whether we're in form combobox text field 3. New Area within PDFiumPage are as follows: - FORM_TEXT_AREA to represent form text field - FORM_COMBOBOX_TEXT_AREA to represent form combobox text field 4. New member variables are as follows: - in_form_text_field_ which is TRUE if focus is within a form text field - in_form_combobox_text_field_ which is TRUE if focus is within a form combobox text field BUG=59266 ==========
Description was changed from ========== Add functionality for copying text within form text fields and form combobox text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextField() for setting status of whether we're in form text field - PDFiumEngine::SetInFormComboboxTextField() for setting status of whether we're in form combobox text field 3. New Area within PDFiumPage are as follows: - FORM_TEXT_AREA to represent form text field - FORM_COMBOBOX_TEXT_AREA to represent form combobox text field 4. New member variables are as follows: - in_form_text_field_ which is TRUE if focus is within a form text field - in_form_combobox_text_field_ which is TRUE if focus is within a form combobox text field BUG=59266 ========== to ========== Add functionality for copying text within form text fields and form combobox text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextField() for setting status of whether we're in form text field - PDFiumEngine::SetInFormComboboxTextField() for setting status of whether we're in form combobox text field 3. New Areas within PDFiumPage are as follows: - FORM_TEXT_AREA to represent form text field - FORM_COMBOBOX_TEXT_AREA to represent form combobox text field 4. New member variables are as follows: - in_form_text_field_ which is TRUE if focus is within a form text field - in_form_combobox_text_field_ which is TRUE if focus is within a form combobox text field BUG=59266 ==========
https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:665: bool in_form_combobox_text_field_; Do you really need a separate boolean? There's only 1 place that checks its value, and it's checking (in_form_text_field_ || in_form_combobox_text_field_).
https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1632: void PDFiumEngine::SetFormSelectedText(const FPDF_FORMHANDLE& form_handle, We don't do this consistently, so let's try to be consistent and pass by value, since all of these types are void* underneath. https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1637: // Initial length = 2 (not 0) because text is CFX_WideString and CFX_WideString is suppose to be an implementation detail. Shhh! https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1638: // sentinel char is included in count. By "sentinel char" do you really mean NUL character? https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2052: pp::PDF::SetSelectedText(GetPluginInstance(), ""); Are you sure the key up event is the best time to do this? With a regular HTML text form, if there is selected text, and I let go of the shift key. The moment I press an arrow key, the cursor starts moving and the visual selection goes away. It does not wait for the arrow key to be released. 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... pdf/pdfium/pdfium_page.cc:268: if (*form_type == FPDF_FORMFIELD_TEXTFIELD) We now have 3 (or more?) places that are trying to decide between (FORM_TEXT_AREA, FORM_COMBOBOX_TEXT_AREA, NONSELECTABLE_AREA). Can we write a helper function?
Hi Lei and Dan, In this patchset, I addressed Lei's comments about how to handle the text selection bug (i.e. not clearing selection in OnKeyUp(), but instead doing so in OnKeyDown() when appropriate), as well as how to handle text selection in form comboboxes. Thanks for reviewing. Best, Diana https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1632: void PDFiumEngine::SetFormSelectedText(const FPDF_FORMHANDLE& form_handle, On 2017/06/17 02:21:17, Lei Zhang wrote: > We don't do this consistently, so let's try to be consistent and pass by value, > since all of these types are void* underneath. Done. https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1637: // Initial length = 2 (not 0) because text is CFX_WideString and On 2017/06/17 02:21:18, Lei Zhang wrote: > CFX_WideString is suppose to be an implementation detail. Shhh! Done. https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1638: // sentinel char is included in count. On 2017/06/17 02:21:17, Lei Zhang wrote: > By "sentinel char" do you really mean NUL character? Done. https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2052: pp::PDF::SetSelectedText(GetPluginInstance(), ""); On 2017/06/17 02:21:17, Lei Zhang wrote: > Are you sure the key up event is the best time to do this? With a regular HTML > text form, if there is selected text, and I let go of the shift key. The moment > I press an arrow key, the cursor starts moving and the visual selection goes > away. It does not wait for the arrow key to be released. Done. https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/150001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:665: bool in_form_combobox_text_field_; On 2017/06/17 01:55:16, Lei Zhang wrote: > Do you really need a separate boolean? There's only 1 place that checks its > value, and it's checking (in_form_text_field_ || in_form_combobox_text_field_). Done. 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... pdf/pdfium/pdfium_page.cc:268: if (*form_type == FPDF_FORMFIELD_TEXTFIELD) On 2017/06/17 02:21:18, Lei Zhang wrote: > We now have 3 (or more?) places that are trying to decide between > (FORM_TEXT_AREA, FORM_COMBOBOX_TEXT_AREA, NONSELECTABLE_AREA). Can we write a > helper function? Do you still think this will be necessary after removing the FORM_COMBOBOX_TEXT_AREA value? In pdfium_engine.cc where this decision is made, I've gone back to using the ternary operator.
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... pdf/pdfium/pdfium_page.cc:268: if (*form_type == FPDF_FORMFIELD_TEXTFIELD) On 2017/06/19 21:53:52, drgage wrote: > On 2017/06/17 02:21:18, Lei Zhang wrote: > > We now have 3 (or more?) places that are trying to decide between > > (FORM_TEXT_AREA, FORM_COMBOBOX_TEXT_AREA, NONSELECTABLE_AREA). Can we write a > > helper function? > > Do you still think this will be necessary after removing the > FORM_COMBOBOX_TEXT_AREA value? In pdfium_engine.cc where this decision is made, > I've gone back to using the ternary operator. Given the pdfium_engine.cc code in question forgot to check for the combobox type, probably yes. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1636: // Initial length = 2 (not 0) because chars are two bytes and This statement is confusing. It can be read as: char (the variable type) is two bytes, but that's clearly not the case. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1747: mouse_down_state_.Set(form_type == FPDF_FORMFIELD_TEXTFIELD What about FPDF_FORMFIELD_COMBOBOX? https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2027: if (FORM_GetSelectedText(form_, pages_[last_page_mouse_down_]->GetPage(), Since the conditional is evaluated from left to right, FORM_GetSelectedText() will be called even if |in_form_text_area_| is false. Calling FORM_GetSelectedText() is much more expensive than checking |in_form_text_area_|. So reversing the order of the two checks means the code will run faster when |in_form_text_area_| is false. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:321: // Sets plugin's selected text to be selected text in form text field I don't know why I didn't notice earlier. What does it mean for selected text to be "available" ? https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:448: // Sets status of whether or not focus is in form text field or form combobox You can probably omit "status of" here. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:658: // True if focus is on form text field or form combobox text field. Line 448 says "focus is in". Be consistent?
https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) Is this check needed? Won't we just end up creating a std::string on 1647 which is empty and then do nothing because of the following if? https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1848: SetInFormTextArea(false); Should we add a SetFormTextAreaStatus(bool) or something that calls these two methods as they appear to get called together with the same value? https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.cc:269: *form_type == FPDF_FORMFIELD_COMBOBOX) This and the one below should have {}s as the conditional splits multiple lines.
https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:321: // Sets plugin's selected text to be selected text in form text field On 2017/06/20 00:26:24, Lei Zhang wrote: > I don't know why I didn't notice earlier. What does it mean for selected text to > be "available" ? How about this? Checks if |page| has selected text in a form element. If so, sets that as the plugin's text selection. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:444: // Setting text selection status of document. This does not include text Let's use "Sets ..." here for consistency.
https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) On 2017/06/20 15:21:44, dsinclair wrote: > Is this check needed? Won't we just end up creating a std::string on 1647 which > is empty and then do nothing because of the following if? Might as well bale out early if we knows continuing onwards is going to do more work for nothing.
Hi Lei and Dan, In this patchset, I've added a helper FormTypeToArea() to return the correct Area value given a form_type, and addressed comments on style. Thanks for reviewing. Diana 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... pdf/pdfium/pdfium_page.cc:268: if (*form_type == FPDF_FORMFIELD_TEXTFIELD) On 2017/06/20 00:26:24, Lei Zhang wrote: > On 2017/06/19 21:53:52, drgage wrote: > > On 2017/06/17 02:21:18, Lei Zhang wrote: > > > We now have 3 (or more?) places that are trying to decide between > > > (FORM_TEXT_AREA, FORM_COMBOBOX_TEXT_AREA, NONSELECTABLE_AREA). Can we write > a > > > helper function? > > > > Do you still think this will be necessary after removing the > > FORM_COMBOBOX_TEXT_AREA value? In pdfium_engine.cc where this decision is > made, > > I've gone back to using the ternary operator. > > Given the pdfium_engine.cc code in question forgot to check for the combobox > type, probably yes. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1636: // Initial length = 2 (not 0) because chars are two bytes and On 2017/06/20 00:26:24, Lei Zhang wrote: > This statement is confusing. It can be read as: char (the variable type) is two > bytes, but that's clearly not the case. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) On 2017/06/20 15:21:44, dsinclair wrote: > Is this check needed? Won't we just end up creating a std::string on 1647 which > is empty and then do nothing because of the following if? Yeah, you're right. I just wasn't sure if I should be trying to return as soon as possible, which could potentially be on 1639. I guess by that logic, that would make the check on 1648 unnecessary, but I put it there just in case. Do you think it's okay to remove the check on 1638 and let it go until 1648? https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) On 2017/06/20 21:21:22, Lei Zhang wrote: > On 2017/06/20 15:21:44, dsinclair wrote: > > Is this check needed? Won't we just end up creating a std::string on 1647 > which > > is empty and then do nothing because of the following if? > > Might as well bale out early if we knows continuing onwards is going to do more > work for nothing. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1747: mouse_down_state_.Set(form_type == FPDF_FORMFIELD_TEXTFIELD On 2017/06/20 00:26:24, Lei Zhang wrote: > What about FPDF_FORMFIELD_COMBOBOX? I think I got confused while refactoring this yesterday, I'll fix that. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1848: SetInFormTextArea(false); On 2017/06/20 15:21:43, dsinclair wrote: > Should we add a SetFormTextAreaStatus(bool) or something that calls these two > methods as they appear to get called together with the same value? I could do this, or I could add the line "client_->FormTextFieldFocusChange(in_form_text_area)" to my existing function SetInFormTextArea(). What do you think would be better? https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2027: if (FORM_GetSelectedText(form_, pages_[last_page_mouse_down_]->GetPage(), On 2017/06/20 00:26:24, Lei Zhang wrote: > Since the conditional is evaluated from left to right, FORM_GetSelectedText() > will be called even if |in_form_text_area_| is false. Calling > FORM_GetSelectedText() is much more expensive than checking > |in_form_text_area_|. So reversing the order of the two checks means the code > will run faster when |in_form_text_area_| is false. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:321: // Sets plugin's selected text to be selected text in form text field On 2017/06/20 00:26:24, Lei Zhang wrote: > I don't know why I didn't notice earlier. What does it mean for selected text to > be "available" ? What I meant by available is if selected text in a form text field or form combobox text field exists, then this function will set the plugin's selected text to be that. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:321: // Sets plugin's selected text to be selected text in form text field On 2017/06/20 18:34:57, Lei Zhang wrote: > On 2017/06/20 00:26:24, Lei Zhang wrote: > > I don't know why I didn't notice earlier. What does it mean for selected text > to > > be "available" ? > > How about this? > > Checks if |page| has selected text in a form element. If so, sets that as the > plugin's text selection. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:444: // Setting text selection status of document. This does not include text On 2017/06/20 18:34:58, Lei Zhang wrote: > Let's use "Sets ..." here for consistency. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:448: // Sets status of whether or not focus is in form text field or form combobox On 2017/06/20 00:26:24, Lei Zhang wrote: > You can probably omit "status of" here. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:658: // True if focus is on form text field or form combobox text field. On 2017/06/20 00:26:24, Lei Zhang wrote: > Line 448 says "focus is in". Be consistent? Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.cc:269: *form_type == FPDF_FORMFIELD_COMBOBOX) On 2017/06/20 15:21:44, dsinclair wrote: > This and the one below should have {}s as the conditional splits multiple lines. Done - I actually created a helper to convert from a form_type to its corresponding Area value, so these conditionals aren't here anymore.
Hi Lei and Dan, In this patchset, I've added a helper FormTypeToArea() to return the correct Area value given a form_type, and addressed comments on style. Thanks for reviewing. Diana 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... pdf/pdfium/pdfium_page.cc:268: if (*form_type == FPDF_FORMFIELD_TEXTFIELD) On 2017/06/20 00:26:24, Lei Zhang wrote: > On 2017/06/19 21:53:52, drgage wrote: > > On 2017/06/17 02:21:18, Lei Zhang wrote: > > > We now have 3 (or more?) places that are trying to decide between > > > (FORM_TEXT_AREA, FORM_COMBOBOX_TEXT_AREA, NONSELECTABLE_AREA). Can we write > a > > > helper function? > > > > Do you still think this will be necessary after removing the > > FORM_COMBOBOX_TEXT_AREA value? In pdfium_engine.cc where this decision is > made, > > I've gone back to using the ternary operator. > > Given the pdfium_engine.cc code in question forgot to check for the combobox > type, probably yes. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1636: // Initial length = 2 (not 0) because chars are two bytes and On 2017/06/20 00:26:24, Lei Zhang wrote: > This statement is confusing. It can be read as: char (the variable type) is two > bytes, but that's clearly not the case. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) On 2017/06/20 15:21:44, dsinclair wrote: > Is this check needed? Won't we just end up creating a std::string on 1647 which > is empty and then do nothing because of the following if? Yeah, you're right. I just wasn't sure if I should be trying to return as soon as possible, which could potentially be on 1639. I guess by that logic, that would make the check on 1648 unnecessary, but I put it there just in case. Do you think it's okay to remove the check on 1638 and let it go until 1648? https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) On 2017/06/20 21:21:22, Lei Zhang wrote: > On 2017/06/20 15:21:44, dsinclair wrote: > > Is this check needed? Won't we just end up creating a std::string on 1647 > which > > is empty and then do nothing because of the following if? > > Might as well bale out early if we knows continuing onwards is going to do more > work for nothing. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1747: mouse_down_state_.Set(form_type == FPDF_FORMFIELD_TEXTFIELD On 2017/06/20 00:26:24, Lei Zhang wrote: > What about FPDF_FORMFIELD_COMBOBOX? I think I got confused while refactoring this yesterday, I'll fix that. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1848: SetInFormTextArea(false); On 2017/06/20 15:21:43, dsinclair wrote: > Should we add a SetFormTextAreaStatus(bool) or something that calls these two > methods as they appear to get called together with the same value? I could do this, or I could add the line "client_->FormTextFieldFocusChange(in_form_text_area)" to my existing function SetInFormTextArea(). What do you think would be better? https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2027: if (FORM_GetSelectedText(form_, pages_[last_page_mouse_down_]->GetPage(), On 2017/06/20 00:26:24, Lei Zhang wrote: > Since the conditional is evaluated from left to right, FORM_GetSelectedText() > will be called even if |in_form_text_area_| is false. Calling > FORM_GetSelectedText() is much more expensive than checking > |in_form_text_area_|. So reversing the order of the two checks means the code > will run faster when |in_form_text_area_| is false. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:321: // Sets plugin's selected text to be selected text in form text field On 2017/06/20 00:26:24, Lei Zhang wrote: > I don't know why I didn't notice earlier. What does it mean for selected text to > be "available" ? What I meant by available is if selected text in a form text field or form combobox text field exists, then this function will set the plugin's selected text to be that. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:321: // Sets plugin's selected text to be selected text in form text field On 2017/06/20 18:34:57, Lei Zhang wrote: > On 2017/06/20 00:26:24, Lei Zhang wrote: > > I don't know why I didn't notice earlier. What does it mean for selected text > to > > be "available" ? > > How about this? > > Checks if |page| has selected text in a form element. If so, sets that as the > plugin's text selection. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:444: // Setting text selection status of document. This does not include text On 2017/06/20 18:34:58, Lei Zhang wrote: > Let's use "Sets ..." here for consistency. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:448: // Sets status of whether or not focus is in form text field or form combobox On 2017/06/20 00:26:24, Lei Zhang wrote: > You can probably omit "status of" here. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:658: // True if focus is on form text field or form combobox text field. On 2017/06/20 00:26:24, Lei Zhang wrote: > Line 448 says "focus is in". Be consistent? Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.cc:269: *form_type == FPDF_FORMFIELD_COMBOBOX) On 2017/06/20 15:21:44, dsinclair wrote: > This and the one below should have {}s as the conditional splits multiple lines. Done - I actually created a helper to convert from a form_type to its corresponding Area value, so these conditionals aren't here anymore.
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_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1636: // Initial length = 2 (not 0) because an empty NUL-terminated wide string is Not sure why I didn't mention this earlier, but the "Initial length = 2" part sounds weird too. How about: Check to see if there is selected text in the form. When |form_sel_text_len| is 2, that represents a wide string with just a NUL-terminator. https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1747: mouse_down_state_.Set(pages_[page_index]->FormTypeToArea(form_type), If PDFiumPage::FormTypeToArea() became a static method, then you can just write: PDFiumPage::FormTypeToArea(form_type) since it no longer needs to be tied to any particular instance of PDFiumPage. https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.cc:295: PDFiumPage::Area PDFiumPage::FormTypeToArea(int form_type) { If we make this a static method, we like to make a note of that as follows: // static PDFiumPage::Area PDFiumPage::FormTypeToArea(int form_type) { https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.cc:298: return PDFiumPage::FORM_TEXT_AREA; We can omit this return and just let it fall through. https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.cc:299: case FPDF_FORMFIELD_COMBOBOX: Let's put this value first, since it appears earlier both by alphabetica order, and by the form field type value. https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page.h File pdf/pdfium/pdfium_page.h (right): https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.h:81: Area FormTypeToArea(int form_type); Let's make this a static method, since its implementation only depends on the |form_type| parameter, and not any part of a class instance.
Description was changed from ========== Add functionality for copying text within form text fields and form combobox text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextField() for setting status of whether we're in form text field - PDFiumEngine::SetInFormComboboxTextField() for setting status of whether we're in form combobox text field 3. New Areas within PDFiumPage are as follows: - FORM_TEXT_AREA to represent form text field - FORM_COMBOBOX_TEXT_AREA to represent form combobox text field 4. New member variables are as follows: - in_form_text_field_ which is TRUE if focus is within a form text field - in_form_combobox_text_field_ which is TRUE if focus is within a form combobox text field BUG=59266 ========== to ========== Add functionality for copying text within form text fields and form combobox text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextArea() for setting status of whether focus is in a form text field or form combobox text field 3. New Area within PDFiumPage FORM_TEXT_AREA to represent form text fields and form combobox text fields 4. New member variable in_form_text_field_, which is TRUE if focus is within a form text field BUG=59266 ==========
Description was changed from ========== Add functionality for copying text within form text fields and form combobox text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextArea() for setting status of whether focus is in a form text field or form combobox text field 3. New Area within PDFiumPage FORM_TEXT_AREA to represent form text fields and form combobox text fields 4. New member variable in_form_text_field_, which is TRUE if focus is within a form text field BUG=59266 ========== to ========== Add functionality for copying text within form text fields and form combobox text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextArea() for setting status of whether focus is in a form text field or form combobox text field 3. New Area within PDFiumPage FORM_TEXT_AREA to represent form text fields and form combobox text fields 4. New member variable in_form_text_field_, which is TRUE if focus is within a form text field BUG=59266 ==========
Hi Lei and Dan, In this patchset, I have updated the description for the issue, changed the FormTypeToArea() to be a static method in PDFiumPage, and made some style changes. Thanks for reviewing. Best, Diana https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1636: // Initial length = 2 (not 0) because an empty NUL-terminated wide string is On 2017/06/21 00:19:45, Lei Zhang wrote: > Not sure why I didn't mention this earlier, but the "Initial length = 2" part > sounds weird too. How about: > > Check to see if there is selected text in the form. When |form_sel_text_len| is > 2, that represents a wide string with just a NUL-terminator. Done. https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1747: mouse_down_state_.Set(pages_[page_index]->FormTypeToArea(form_type), On 2017/06/21 00:19:45, Lei Zhang wrote: > If PDFiumPage::FormTypeToArea() became a static method, then you can just write: > PDFiumPage::FormTypeToArea(form_type) since it no longer needs to be tied to any > particular instance of PDFiumPage. Done. https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.cc:295: PDFiumPage::Area PDFiumPage::FormTypeToArea(int form_type) { On 2017/06/21 00:19:46, Lei Zhang wrote: > If we make this a static method, we like to make a note of that as follows: > > // static > PDFiumPage::Area PDFiumPage::FormTypeToArea(int form_type) { Done. https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.cc:298: return PDFiumPage::FORM_TEXT_AREA; On 2017/06/21 00:19:45, Lei Zhang wrote: > We can omit this return and just let it fall through. Done. https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.cc:299: case FPDF_FORMFIELD_COMBOBOX: On 2017/06/21 00:19:45, Lei Zhang wrote: > Let's put this value first, since it appears earlier both by alphabetica order, > and by the form field type value. Done. https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page.h File pdf/pdfium/pdfium_page.h (right): https://codereview.chromium.org/2924343005/diff/190001/pdf/pdfium/pdfium_page... pdf/pdfium/pdfium_page.h:81: Area FormTypeToArea(int form_type); On 2017/06/21 00:19:46, Lei Zhang wrote: > Let's make this a static method, since its implementation only depends on the > |form_type| parameter, and not any part of a class instance. Done.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) On 2017/06/20 23:14:27, drgage wrote: > On 2017/06/20 15:21:44, dsinclair wrote: > > Is this check needed? Won't we just end up creating a std::string on 1647 > which > > is empty and then do nothing because of the following if? > > Yeah, you're right. I just wasn't sure if I should be trying to return as soon > as possible, which could potentially be on 1639. I guess by that logic, that > would make the check on 1648 unnecessary, but I put it there just in case. Do > you think it's okay to remove the check on 1638 and let it go until 1648? Leaving it is fine, early exiting does make it a bit clearer what's happening. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1848: SetInFormTextArea(false); On 2017/06/20 23:14:27, drgage wrote: > On 2017/06/20 15:21:43, dsinclair wrote: > > Should we add a SetFormTextAreaStatus(bool) or something that calls these two > > methods as they appear to get called together with the same value? > > I could do this, or I could add the line > "client_->FormTextFieldFocusChange(in_form_text_area)" to my existing function > SetInFormTextArea(). What do you think would be better? Moving the FormTextFieldFocusChange into SetInFormTextArea makes sense to me.
This is mostly working, though there are still some issues that we can look into those in follow up CLs. - The copy to selection clipboard behavior is not exactly the same as for webpages. - Sometimes the PDF viewer has the wrong selected text. We are probably missing a corner case check somewhere? https://codereview.chromium.org/2924343005/diff/230001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/230001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2030: nullptr, 0) <= 2) style: Should add braces.
Hi Lei and Dan, I fixed the bracing around the multi-line conditional that Lei pointed out and also . Please let me know if there is anything else you think should be addressed with this CL. Lei, when you get the chance to show me what the issues are so I can address them in follow up CLs that would be great. Thanks for reviewing. Best, Diana https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1638: if (form_sel_text_len <= 2) On 2017/06/21 13:03:28, dsinclair wrote: > On 2017/06/20 23:14:27, drgage wrote: > > On 2017/06/20 15:21:44, dsinclair wrote: > > > Is this check needed? Won't we just end up creating a std::string on 1647 > > which > > > is empty and then do nothing because of the following if? > > > > Yeah, you're right. I just wasn't sure if I should be trying to return as soon > > as possible, which could potentially be on 1639. I guess by that logic, that > > would make the check on 1648 unnecessary, but I put it there just in case. Do > > you think it's okay to remove the check on 1638 and let it go until 1648? > > Leaving it is fine, early exiting does make it a bit clearer what's happening. Done. https://codereview.chromium.org/2924343005/diff/170001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1848: SetInFormTextArea(false); On 2017/06/21 13:03:28, dsinclair wrote: > On 2017/06/20 23:14:27, drgage wrote: > > On 2017/06/20 15:21:43, dsinclair wrote: > > > Should we add a SetFormTextAreaStatus(bool) or something that calls these > two > > > methods as they appear to get called together with the same value? > > > > I could do this, or I could add the line > > "client_->FormTextFieldFocusChange(in_form_text_area)" to my existing function > > SetInFormTextArea(). What do you think would be better? > > Moving the FormTextFieldFocusChange into SetInFormTextArea makes sense to me. Done. https://codereview.chromium.org/2924343005/diff/230001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2924343005/diff/230001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:2030: nullptr, 0) <= 2) On 2017/06/23 19:41:23, Lei Zhang wrote: > style: Should add braces. Done.
The CQ bit was checked by thestig@chromium.org
lgtm
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by thestig@chromium.org
Description was changed from ========== Add functionality for copying text within form text fields and form combobox text fields 1. Copying can be done with selection via holding down and moving the mouse as well as by using shift and arrow keys. 2. New methods are as follows: - PDFiumEngine::SetFormSelectedText() for setting PDF plugin selection to be selected text in form text field - PDFiumEngine::SetInFormTextArea() for setting status of whether focus is in a form text field or form combobox text field 3. New Area within PDFiumPage FORM_TEXT_AREA to represent form text fields and form combobox text fields 4. New member variable in_form_text_field_, which is TRUE if focus is within a form text field BUG=59266 ========== to ========== 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 ==========
I did some editing on the CL description...
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 270001, "attempt_start_ts": 1498259599715320, "parent_rev": "13792b8b17e88e1ab36594a6592f4fed7ea824fe", "commit_rev": "c32fae2601ea9890e50fa34caa8e80de1c007609"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/c32fae2601ea9890e50fa34caa8e... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270001) as https://chromium.googlesource.com/chromium/src/+/c32fae2601ea9890e50fa34caa8e... |