[WIP] Add support for Android SuggestionSpans when editing text

Add support for Android SuggestionSpans when editing text

This allows people entering text using the Android Voice IME to tap on the underlined suggestion spans and pick from a set of potential replacements in a dropdown menu.

This still needs to be split up into smaller patches and cleaned up a bit, I am
uploading what I have so far so we can discuss the best way to split it up.

BUG=672259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[rlanday, 2017-01-25 01:30:20]

Description was changed from

==========
Add support for Android SuggestionSpans when editing text

This allows people entering text using the Android Voice IME to tap on the
underlined suggestion spans and pick from a set of potential replacements in a
dropdown menu.

This still needs to be split up into smaller patches and cleaned up a bit, I am
uploading what I have so far so we can discuss the best way to split it up.

BUG=672259
==========

to

==========
Add support for Android SuggestionSpans when editing text

This allows people entering text using the Android Voice IME to tap on the
underlined suggestion spans and pick from a set of potential replacements in a
dropdown menu.

This still needs to be split up into smaller patches and cleaned up a bit, I am
uploading what I have so far so we can discuss the best way to split it up.

BUG=672259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[rlanday, 2017-01-25 02:27:22]

rlanday@chromium.org changed reviewers:
+ aelias@chromium.org

[rlanday, 2017-01-25 02:27:23]

Uploading this up to discuss how to split it up into smaller patches, there's
still some cleanup I need to do

https://codereview.chromium.org/2650113004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/2650113004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/ime_adapter_android.cc:33: #include
"third_party/WebKit/public/web/WebTextSuggestionInfo.h"
One thing I'm curious about is that there's a lint rule complaining about me
adding an include from third_party/WebKit here (and in a couple other files as
well)...but clearly there are already three other WebKit includes here, so I'm
not sure what I'm supposed to do...

[aelias_OOO_until_Jul13, 2017-01-25 03:34:27]

I can't think of any good split point for this, I guess we can carry on
reviewing it as a blob.  Looks decent overall, here are some initial comments.

https://codereview.chromium.org/2650113004/diff/1/content/common/input_messag...
File content/common/input_messages.h (right):

https://codereview.chromium.org/2650113004/diff/1/content/common/input_messag...
content/common/input_messages.h:199:
IPC_MESSAGE_ROUTED2(InputMsg_ImeApplySuggestionReplacement,
These do not originate from the IME per se so I'd prefer "Ime" not be in the
name.

https://codereview.chromium.org/2650113004/diff/1/content/common/input_messag...
content/common/input_messages.h:203:
IPC_MESSAGE_ROUTED0(InputMsg_ImeDeleteSuggestionHighlight)
Can this take a marker ID as well instead of inferring it from current
selection?  Or, if the ID doesn't provide any information on top of what Blink
has to keep state on anyway, I guess neither message should have it.

https://codereview.chromium.org/2650113004/diff/1/content/common/input_messag...
content/common/input_messages.h:205:
IPC_MESSAGE_ROUTED0(InputMsg_ImeCloseSuggestionMenu)
This is more of a notification for something that already happened, so I'd
prefer "InputMsg_SuggestionMenuClosed"

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java
(left):

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:198:
if (mHasPendingImmediateRequest
These conditions are important to preserve for performance reasons.  You might
want to turn on "monitor mode" at any time a SuggestionSpan is present, or
(perhaps better) send an "immediate request" at time of creating the menu.

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:773:
private class SuggestionsPopupWindow implements OnItemClickListener,
OnDismissListener {
Please move this into a new .java file.

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:951:
// certain operations, e.g. rotating the device while the menu
Seems this is no longer needed after changwan@'s fix?

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:986:
mDismissedByItemTap = true;
Can we instead not dismiss at all at just yet?  We can replace it when the new
menu comes in from Blink instead.

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:990:
public void closeMenu() {
This has no callsites, please remove.

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:1025:
// Use getUnderlineColor method by reflection to avoid having to reimplement
The method is public, it's just @hide.  You can probably just call it directly
and suppress the build warning if you get one.

https://codereview.chromium.org/2650113004/diff/1/content/renderer/input/rend...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/2650113004/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:424: if
(input_event.type() == WebInputEvent::GestureTap) {
This handler is too generica place to put this, please place it in the handler
responsible for actually moving the caret around, after already hit tested into
a textbox.

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:314:
marker->startOffset() <= markerPtr->endOffset()) ||
Isn't it equivalent to say "marker->startOffset() > markerPtr->startOffset() &&
marker->endOffset() < markerPtr->endOffset()"?  You can DCHECK_LT(startOffset,
endOffset); at some point to make that assumption guaranteed.

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:333:
markersToExpand.push_back(marker);
Seems a bit weird, though I don't understand the scenario well enough, perhaps
we can discuss it more later.

I notice SpellChecker::updateMarkersForWordsAffectedByEditing has a very general
approach to solve the same problem.  I'd suggest harmonizing the approach
(ideally using the same code, perhaps by refactoring out the code in
SpellChecker).  And note that performance is pretty negligible here, I'd rather
keep the logic simple and general.

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:373: if
(suggestionHighlightMarkers.size() == 0)
isEmpty()

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:382:
HeapVector<Member<DocumentMarker>> markersToExpand;
unused

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:410:
plainText(nextCharacterEphemeralRange, TextIteratorEmitsOriginalText);
EmitsOriginalText is for password fields not to be reported as dot symbols, not
really needed here.

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:412: if
(WTF::isASCIISpace(nextCharacterStr[0])) {
Probably want to consider non-breaking spaces as well.  The text editing code
likes to use them in many cases in contenteditable elements.

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:413: if
(suggestionHighlightMarkerPtr->startOffset() == 0) {
The range code works even if this is 0, right?  Could you delete this
special-case branch to simplify the logic, then?  Performance is a negligible
concern here.

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:432:
setCompositionFromExistingText(
We don't want to send composition events to Javascript here, so I'd prefer
setting the selection and doing TypingCommand directly (likewise above).

Note
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/s...
does frame().editor().replaceSelectionWithText

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:440: LOG(INFO)
<< "command type: " << m_commandType;
Leftover LOG

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:527:
// For zero-length ranges, include spans whose endpoints touch the range
That's not very principled.  I'd prefer you introduce a new method
markersInRangeInclusive.  From a comment in InputMethodController, it sounds
like that would benefit non-zero-length cases as well?

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp (right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp:165: void
WebInputMethodControllerImpl::prepareForTextSuggestionMenuToBeShown() {
This seems to have no callers?

[rlanday, 2017-01-25 18:48:10]

https://codereview.chromium.org/2650113004/diff/1/content/common/input_messag...
File content/common/input_messages.h (right):

https://codereview.chromium.org/2650113004/diff/1/content/common/input_messag...
content/common/input_messages.h:203:
IPC_MESSAGE_ROUTED0(InputMsg_ImeDeleteSuggestionHighlight)
On 2017/01/25 at 03:34:26, aelias wrote:
> Can this take a marker ID as well instead of inferring it from current
selection?  Or, if the ID doesn't provide any information on top of what Blink
has to keep state on anyway, I guess neither message should have it.

This message doesn't take an ID because:
  - At any point there's only one span of text highlighted in this manner
  - The highlighted span is actually a union of SuggestionSpan ranges rather
than necessarily corresponding to any individual range

The messages to replace an individual SuggestionSpan do need the ID because
neither of these two reasons applies.

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:986:
mDismissedByItemTap = true;
On 2017/01/25 at 03:34:26, aelias wrote:
> Can we instead not dismiss at all at just yet?  We can replace it when the new
menu comes in from Blink instead.

I don't understand what you're asking here, the menu is supposed to close when
you pick a suggestion

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:990:
public void closeMenu() {
On 2017/01/25 at 03:34:26, aelias wrote:
> This has no callsites, please remove.

ah, I added this when I was thinking we might want to close the menu on
backgrounding the app/rotation before I got the menu tracking the cursor
properly in those cases

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:1025:
// Use getUnderlineColor method by reflection to avoid having to reimplement
On 2017/01/25 at 03:34:26, aelias wrote:
> The method is public, it's just @hide.  You can probably just call it directly
and suppress the build warning if you get one.

Hmm I think I tried that but I'll look into it again

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:314:
marker->startOffset() <= markerPtr->endOffset()) ||
On 2017/01/25 at 03:34:27, aelias wrote:
> Isn't it equivalent to say "marker->startOffset() > markerPtr->startOffset()
&& marker->endOffset() < markerPtr->endOffset()"?  You can
DCHECK_LT(startOffset, endOffset); at some point to make that assumption
guaranteed.

I don't think so, because your check doesn't handle the case where the markers
share either a start or end position?

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:333:
markersToExpand.push_back(marker);
On 2017/01/25 at 03:34:26, aelias wrote:
> Seems a bit weird, though I don't understand the scenario well enough, perhaps
we can discuss it more later.
> 
> I notice SpellChecker::updateMarkersForWordsAffectedByEditing has a very
general approach to solve the same problem.  I'd suggest harmonizing the
approach (ideally using the same code, perhaps by refactoring out the code in
SpellChecker).  And note that performance is pretty negligible here, I'd rather
keep the logic simple and general.

I'll look into that code

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:333:
markersToExpand.push_back(marker);
On 2017/01/25 at 03:34:26, aelias wrote:
> Seems a bit weird, though I don't understand the scenario well enough, perhaps
we can discuss it more later.
> 
> I notice SpellChecker::updateMarkersForWordsAffectedByEditing has a very
general approach to solve the same problem.  I'd suggest harmonizing the
approach (ideally using the same code, perhaps by refactoring out the code in
SpellChecker).  And note that performance is pretty negligible here, I'd rather
keep the logic simple and general.

I'll look into that code

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:412: if
(WTF::isASCIISpace(nextCharacterStr[0])) {
On 2017/01/25 at 03:34:26, aelias wrote:
> Probably want to consider non-breaking spaces as well.  The text editing code
likes to use them in many cases in contenteditable elements.

I was having trouble finding a function that seemed to include just spaces and
non-breaking spaces, I'll check again

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:413: if
(suggestionHighlightMarkerPtr->startOffset() == 0) {
On 2017/01/25 at 03:34:27, aelias wrote:
> The range code works even if this is 0, right?  Could you delete this
special-case branch to simplify the logic, then?  Performance is a negligible
concern here.

This is not for performance, this is so if you have a word (or a series of
words) highlighted at the beginning of the text box, and hit the "Delete" menu
option, we also delete the space following the word (which is not part of the
range). This is behavior I'm copying over from the native Android
implementation.

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:61:
SuggestionBackgroundHighlight = 1
lol @ git cl format...

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:527:
// For zero-length ranges, include spans whose endpoints touch the range
On 2017/01/25 at 03:34:27, aelias wrote:
> That's not very principled.  I'd prefer you introduce a new method
markersInRangeInclusive.  From a comment in InputMethodController, it sounds
like that would benefit non-zero-length cases as well?

That's a good idea; I got the idea to do it the way I currently have it by
looking at the Android SpannableString implementation, which actually does
special case zero-length ranges and queries:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp (right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp:165: void
WebInputMethodControllerImpl::prepareForTextSuggestionMenuToBeShown() {
On 2017/01/25 at 03:34:27, aelias wrote:
> This seems to have no callers?

Oh, I think I should add a call somewhere because the insertion caret currently
visible when the menu's open...

[rlanday, 2017-01-25 19:21:56]

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java
(left):

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:198:
if (mHasPendingImmediateRequest
On 2017/01/25 at 03:34:26, aelias wrote:
> These conditions are important to preserve for performance reasons.  You might
want to turn on "monitor mode" at any time a SuggestionSpan is present, or
(perhaps better) send an "immediate request" at time of creating the menu.

I think we probably want to turn on "monitor mode" when the menu is open? That
way we can keep the menu positioned properly, but we don't need to request any
updates just because a SuggestionSpan is present.

[rlanday, 2017-01-26 00:23:05]

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/2650113004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:1025:
// Use getUnderlineColor method by reflection to avoid having to reimplement
On 2017/01/25 at 18:48:10, rlanday wrote:
> On 2017/01/25 at 03:34:26, aelias wrote:
> > The method is public, it's just @hide.  You can probably just call it
directly and suppress the build warning if you get one.
> 
> Hmm I think I tried that but I'll look into it again

Trying to call span.getUnderlineColor() directly results in the compiler saying
"error: cannot find symbol"...the actual implementation is here:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...

Since I didn't end up implementing any of the flags, I think we can just hard
code the gray color and avoid having to call this

[rlanday, 2017-01-26 01:29:07]

https://codereview.chromium.org/2650113004/diff/1/content/renderer/input/rend...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/2650113004/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:424: if
(input_event.type() == WebInputEvent::GestureTap) {
On 2017/01/25 at 03:34:26, aelias wrote:
> This handler is too generica place to put this, please place it in the handler
responsible for actually moving the caret around, after already hit tested into
a textbox.

That would be SelectionController::handleMousePressEventSingleClick() I
guess...I think I stuck this here because it was the last point at which we have
access to the RenderWidgetInputHandlerDelegate which we need to have send the
IPC call up. After this we're in third_party/WebKit. I can't get access to the
delegate from there, can I?

[aelias_OOO_until_Jul13, 2017-01-26 02:00:30]

https://codereview.chromium.org/2650113004/diff/1/content/renderer/input/rend...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/2650113004/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:424: if
(input_event.type() == WebInputEvent::GestureTap) {
On 2017/01/26 at 01:29:07, rlanday wrote:
> On 2017/01/25 at 03:34:26, aelias wrote:
> > This handler is too generica place to put this, please place it in the
handler responsible for actually moving the caret around, after already hit
tested into a textbox.
> 
> That would be SelectionController::handleMousePressEventSingleClick() I
guess...I think I stuck this here because it was the last point at which we have
access to the RenderWidgetInputHandlerDelegate which we need to have send the
IPC call up. After this we're in third_party/WebKit. I can't get access to the
delegate from there, can I?

You can just add a new abstract method in third_party/WebKit/Source/public/ to
access RenderWidget stuff.  There is no hard boundary here, the distinction
between what goes in chromium and what goes in WebKit is somewhat historic.

[rlanday, 2017-01-26 18:28:53]

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:410:
plainText(nextCharacterEphemeralRange, TextIteratorEmitsOriginalText);
On 2017/01/25 at 03:34:26, aelias wrote:
> EmitsOriginalText is for password fields not to be reported as dot symbols,
not really needed here.

I copied this from the composingText() method, are you sure it's not needed?
What happens if you're editing text inside a password field?

[aelias_OOO_until_Jul13, 2017-01-27 19:56:36]

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:410:
plainText(nextCharacterEphemeralRange, TextIteratorEmitsOriginalText);
On 2017/01/26 at 18:28:52, rlanday wrote:
> On 2017/01/25 at 03:34:26, aelias wrote:
> > EmitsOriginalText is for password fields not to be reported as dot symbols,
not really needed here.
> 
> I copied this from the composingText() method, are you sure it's not needed?
What happens if you're editing text inside a password field?

It doesn't really make sense for password fields to have voice input and even
less so voice suggestions.  If we are want to write code specific to password
fields it should probably be along the lines of disabling SuggestionSpans on
purpose for them instead of trying to make bits of our implementations
compatible.

[rlanday, 2017-01-28 02:35:28]

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:333:
markersToExpand.push_back(marker);
On 2017/01/25 at 18:48:10, rlanday wrote:
> On 2017/01/25 at 03:34:26, aelias wrote:
> > Seems a bit weird, though I don't understand the scenario well enough,
perhaps we can discuss it more later.
> > 
> > I notice SpellChecker::updateMarkersForWordsAffectedByEditing() has a very
general approach to solve the same problem.  I'd suggest harmonizing the
approach (ideally using the same code, perhaps by refactoring out the code in
SpellChecker).  And note that performance is pretty negligible here, I'd rather
keep the logic simple and general.
> 
> I'll look into that code

Ok, there are actually two methods involved with updating the spell check
markers:

- SpellChecker::updateMarkersForWordsAffectedByEditing() clears spellcheck
markers from words that are being edited
- SpellChecker::markMisspellingsAfterApplyingCommand() adds them back once
you're finished typing a word

We can't reuse markMisspellingsAfterApplyingCommand() at all because the stuff
we need to do to keep the suggestion markers up to date is totally different
from spell check markers (we can't just clear on every edit operation and re-add
because we can't re-generate the suggestion markers from the text the same way
we re-generate spellcheck markers).

I'm not sure we can re-use updateMarkersForWordsAffectedByEditing() because it's
overly aggressive (e.g. it seems that just tapping around on different words and
making the suggestion menu sometimes clears the suggestion span). It might be
that we need to call it from a different place,
updateMarkersForWordsAffectedByEditing() is called rather aggressively, from all
of these places:

- Editor::respondToChangedContents()
- Editor::insertTextWithoutSendingTextEvent()
- Editor::cut()
- Editor::paste()
- Editor::pasteAsPlainText()
- TypingCommand::insertText()
- TypingCommand::deleteKeyPressed()
- TypingCommand::forwardDeleteKeyPressed()

I'm not quite sure why this needs to be called from so many different places; I
think we need to clear the suggestion markers only in response to user-generated
edits, and not stuff like tapping around in the text box and the IME sending
stuff as a response, and we need to have custom logic when we actually perform a
replacement/delete from the suggestion menu. What I currently have works for the
most common cases, but there might be stuff like pasting text right after a
suggestion range, or the text being updated by JavaScript that doesn't work
quite as expected. I will have to think about this some more, but I think what I
currently have is pretty close to being correct in most cases. Ideally I think
we'll be able to pull some more of this code out of InputMethodController
though.

[rlanday, 2017-01-28 02:48:49]

On 2017/01/28 at 02:35:28, rlanday wrote:
>
https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
> 
>
https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:333:
markersToExpand.push_back(marker);
> On 2017/01/25 at 18:48:10, rlanday wrote:
> > On 2017/01/25 at 03:34:26, aelias wrote:
> > > Seems a bit weird, though I don't understand the scenario well enough,
perhaps we can discuss it more later.
> > > 
> > > I notice SpellChecker::updateMarkersForWordsAffectedByEditing() has a very
general approach to solve the same problem.  I'd suggest harmonizing the
approach (ideally using the same code, perhaps by refactoring out the code in
SpellChecker).  And note that performance is pretty negligible here, I'd rather
keep the logic simple and general.
> > 
> > I'll look into that code
> 
> Ok, there are actually two methods involved with updating the spell check
markers:
> 
> - SpellChecker::updateMarkersForWordsAffectedByEditing() clears spellcheck
markers from words that are being edited
> - SpellChecker::markMisspellingsAfterApplyingCommand() adds them back once
you're finished typing a word
> 
> We can't reuse markMisspellingsAfterApplyingCommand() at all because the stuff
we need to do to keep the suggestion markers up to date is totally different
from spell check markers (we can't just clear on every edit operation and re-add
because we can't re-generate the suggestion markers from the text the same way
we re-generate spellcheck markers).
> 
> I'm not sure we can re-use updateMarkersForWordsAffectedByEditing() because
it's overly aggressive (e.g. it seems that just tapping around on different
words and making the suggestion menu sometimes clears the suggestion span). It
might be that we need to call it from a different place,
updateMarkersForWordsAffectedByEditing() is called rather aggressively, from all
of these places:
> 
> - Editor::respondToChangedContents()
> - Editor::insertTextWithoutSendingTextEvent()
> - Editor::cut()
> - Editor::paste()
> - Editor::pasteAsPlainText()
> - TypingCommand::insertText()
> - TypingCommand::deleteKeyPressed()
> - TypingCommand::forwardDeleteKeyPressed()
> 
> I'm not quite sure why this needs to be called from so many different places;
I think we need to clear the suggestion markers only in response to
user-generated edits, and not stuff like tapping around in the text box and the
IME sending stuff as a response, and we need to have custom logic when we
actually perform a replacement/delete from the suggestion menu. What I currently
have works for the most common cases, but there might be stuff like pasting text
right after a suggestion range, or the text being updated by JavaScript that
doesn't work quite as expected. I will have to think about this some more, but I
think what I currently have is pretty close to being correct in most cases.
Ideally I think we'll be able to pull some more of this code out of
InputMethodController though.

[rlanday, 2017-01-28 02:55:07]

On 2017/01/28 at 02:48:49, rlanday wrote:
> On 2017/01/28 at 02:35:28, rlanday wrote:
> >
https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
(right):
> > 
> >
https://codereview.chromium.org/2650113004/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/editing/InputMethodController.cpp:333:
markersToExpand.push_back(marker);
> > On 2017/01/25 at 18:48:10, rlanday wrote:
> > > On 2017/01/25 at 03:34:26, aelias wrote:
> > > > Seems a bit weird, though I don't understand the scenario well enough,
perhaps we can discuss it more later.
> > > > 
> > > > I notice SpellChecker::updateMarkersForWordsAffectedByEditing() has a
very general approach to solve the same problem.  I'd suggest harmonizing the
approach (ideally using the same code, perhaps by refactoring out the code in
SpellChecker).  And note that performance is pretty negligible here, I'd rather
keep the logic simple and general.
> > > 
> > > I'll look into that code
> > 
> > Ok, there are actually two methods involved with updating the spell check
markers:
> > 
> > - SpellChecker::updateMarkersForWordsAffectedByEditing() clears spellcheck
markers from words that are being edited
> > - SpellChecker::markMisspellingsAfterApplyingCommand() adds them back once
you're finished typing a word
> > 
> > We can't reuse markMisspellingsAfterApplyingCommand() at all because the
stuff we need to do to keep the suggestion markers up to date is totally
different from spell check markers (we can't just clear on every edit operation
and re-add because we can't re-generate the suggestion markers from the text the
same way we re-generate spellcheck markers).
> > 
> > I'm not sure we can re-use updateMarkersForWordsAffectedByEditing() because
it's overly aggressive (e.g. it seems that just tapping around on different
words and making the suggestion menu sometimes clears the suggestion span). It
might be that we need to call it from a different place,
updateMarkersForWordsAffectedByEditing() is called rather aggressively, from all
of these places:
> > 
> > - Editor::respondToChangedContents()
> > - Editor::insertTextWithoutSendingTextEvent()
> > - Editor::cut()
> > - Editor::paste()
> > - Editor::pasteAsPlainText()
> > - TypingCommand::insertText()
> > - TypingCommand::deleteKeyPressed()
> > - TypingCommand::forwardDeleteKeyPressed()
> > 
> > I'm not quite sure why this needs to be called from so many different
places; I think we need to clear the suggestion markers only in response to
user-generated edits, and not stuff like tapping around in the text box and the
IME sending stuff as a response, and we need to have custom logic when we
actually perform a replacement/delete from the suggestion menu. What I currently
have works for the most common cases, but there might be stuff like pasting text
right after a suggestion range, or the text being updated by JavaScript that
doesn't work quite as expected. I will have to think about this some more, but I
think what I currently have is pretty close to being correct in most cases.
Ideally I think we'll be able to pull some more of this code out of
InputMethodController though.

I apologize, I accidentally posted an empty comment there...

But yeah, basically I think InputMethodController needs to be in charge of
clearing the suggestion markers touching a word you're editing, because if we
try to move this logic into e.g. TypingCommand or Editor, I think we lose all
the information about what sort of text insertion we're doing, and we clearly
need to handle text being inserted as a result of a suggestion replacement
operation differently than text being typed on the keyboard. I don't think
trying to e.g. tag every single EditCommand with information about whether or
not it originated as part of a suggestion replacement so we can do different
marker update code inside there would be be a clean solution.

[rlanday, 2017-01-31 02:06:45]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 02:07:07]

Dry run: 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-01-31 02:07:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 02:07:09]

Dry run: CLs for remote refs other than refs/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[rlanday, 2017-01-31 19:37:46]

rlanday@chromium.org changed reviewers:
+ dcheng@chromium.org, esprehn@chromium.org, kbr@chromium.org,
tedchoc@chromium.org

[rlanday, 2017-01-31 19:37:46]

rlanday@chromium.org changed required reviewers:
+ aelias@chromium.org, dcheng@chromium.org, esprehn@chromium.org,
kbr@chromium.org, tedchoc@chromium.org

[rlanday, 2017-01-31 19:50:22]

I noted a few changes I'm planning to make, and I'm planning to add some test
cases, but I think this CL is essentially ready to review. I'm happy to split it
up but there's probably not a lot of stuff here that makes sense to do in
isolation.

This CL depends on these other two pending CLs for correct behavior:
- https://codereview.chromium.org/2370663002/ : fix composition underlines being
jumpy when the selection is changed, and fix a bug where the cursor location no
longer updates properly once the device has been rotated or the app has been
backgrounded
- https://codereview.chromium.org/2665923002 : make
DocumentMarkerController::removeMarkers() properly handle the case where markers
can be nested


There's an intermittent bug where the suggestion menu will not open in the right
place, I'm going to try to fix this.

Double-tap to select text is currently not supported, is this something we need?

https://codereview.chromium.org/2650113004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebTextSuggestionInfo.h (right):

https://codereview.chromium.org/2650113004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebTextSuggestionInfo.h:18: int suggestionStart;
There's some information in here (suggestionStart, suggestionEnd, spanStart,
spanEnd) we only need inside blink to compute the union of the spans we're
working with, that doesn't need to get send up to the browser; should I add a
second struct with just the information the browser needs?

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
File
content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml
(right):

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml:5:
-->
It seems we use inconsistent styles for these comment headers...is the -->
supposed to be on its own line, or on the previous line?

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
File content/public/android/java/strings/android_content_strings.grd (right):

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
content/public/android/java/strings/android_content_strings.grd:105: <message
name="IDS_DELETE_TEXT" desc="Drop-down menu item to delete the currently
selected text">
Is there any issue with adding a new string and using it right away? Do we need
to wait for translation or something?

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/CharacterData.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CharacterData.cpp:161:
document().didRemoveText(this, offset + data.length(), realCount);
I hope this doesn't break anything...presumably we have test cases that would
catch if this messes up DOM range handling, and I don't think any of the other
DocumentMarker types persist long enough for this to be an issue...

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:53: const
double kSuggestionUnderlineAlphaMultiplier = 0.4;
The suggestion highlight background seems kind of hard to see on the yellow
background of the webpage I've been using for testing (editpad.org), but this is
the multiplier the native Android text box uses...

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:542:
applyCommandToComposite(DeleteFromTextNodeCommand::create(
Again, hope this doesn't break anything...

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:222:
backgroundColor);
note: the suggestionMarkerID has already been incremented in this case and we
just don't use it, I don't think it really matters, but it felt a little sloppy

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:124: int32_t
suggestionMarkerID);
Should this be int, int32_t, or maybe an unsigned int? I wasn't really sure when
I was writing this, it looks like I made TextSuggestionMarkerDetails just take
an int (I think these should match)

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:139: int
suggestionMarkerID() const;
note: DocumentMarker already has a concept of a "hash" that can be stored with a
marker, ideally we wouldn't need two different ways to give a marker an ID but I
couldn't figure out a clean way to unify these.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:921:
// algorithm from https://dom.spec.whatwg.org/#concept-cd-replace
This is fancy logic that lets you replace part of the text spanned by a
DocumentMarker and have the marker track it properly...it requires two changes
above where I changed replace to be "insert, then delete" instead of "delete,
then insert" because that's the only way we can get the new text to still be in
the marked range.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1976: if
(visibleSelection.isNone())
I'm going to undo the changes to this file, I was testing if we wanted to hide
the blue selection handle when the suggestion menu opens, but it didn't really
look any better when I did that because the selection still jumps around

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp:162: return
frame()->inputMethodController().getTextSuggestionInfosUnderCaret();
I think all the code in this file that does frame()->inputMethodController()
should just call the helper function inputMethodController()

[rlanday, 2017-01-31 19:51:16]

For reference, these are the files that git cl owners says are owned by each
reviewer:

tedchoc:
  content/public/android/BUILD.gn
  content/public/android/java/res/drawable/floating_popup_background_light.xml
  content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml
  content/public/android/java/res/layout-v21/text_edit_suggestion_item.xml
  content/public/android/java/res/values-v21/styles.xml
 
content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java
 
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
 
content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java
  content/public/android/java/src/org/chromium/content/browser/input/Range.java
 
content/public/android/java/src/org/chromium/content/browser/input/SuggestionInfo.java
 
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java
  content/public/android/java/strings/android_content_strings.grd
 
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
 
content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java
 
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java
kbr:
  content/browser/renderer_host/ime_adapter_android.cc
  content/browser/renderer_host/ime_adapter_android.h
  content/browser/renderer_host/render_widget_host_impl.cc
  content/browser/renderer_host/render_widget_host_impl.h
  content/browser/renderer_host/render_widget_host_view_android.cc
  content/browser/renderer_host/render_widget_host_view_android.h
  content/browser/renderer_host/render_widget_host_view_base.cc
  content/browser/renderer_host/render_widget_host_view_base.h
  third_party/WebKit/Source/core/dom/Document.cpp
  third_party/WebKit/Source/core/editing/BUILD.gn
  third_party/WebKit/Source/core/editing/CompositionUnderline.cpp
  third_party/WebKit/Source/core/editing/CompositionUnderline.h
  third_party/WebKit/Source/core/editing/Editor.cpp
  third_party/WebKit/Source/core/editing/FrameSelection.cpp
  third_party/WebKit/Source/core/editing/InputMethodController.cpp
  third_party/WebKit/Source/core/editing/InputMethodController.h
  third_party/WebKit/Source/core/editing/SelectionController.cpp
  third_party/WebKit/Source/core/editing/TextSuggestionList.cpp
  third_party/WebKit/Source/core/editing/TextSuggestionList.h
  third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp
  third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
  third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
  third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
 
third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp
  third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
  third_party/WebKit/Source/core/frame/FrameView.cpp
  third_party/WebKit/Source/core/loader/EmptyClients.h
  third_party/WebKit/Source/core/page/ChromeClient.h
  third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
  third_party/WebKit/Source/web/ChromeClientImpl.cpp
  third_party/WebKit/Source/web/ChromeClientImpl.h
  third_party/WebKit/Source/web/CompositionUnderlineBuilder.h
  third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp
  third_party/WebKit/Source/web/WebInputMethodControllerImpl.h
aelias:
  content/browser/renderer_host/ime_adapter_android.cc
  content/browser/renderer_host/ime_adapter_android.h
  content/browser/renderer_host/render_widget_host_impl.cc
  content/browser/renderer_host/render_widget_host_impl.h
  content/browser/renderer_host/render_widget_host_view_android.cc
  content/browser/renderer_host/render_widget_host_view_android.h
  content/browser/renderer_host/render_widget_host_view_base.cc
  content/browser/renderer_host/render_widget_host_view_base.h
  content/public/android/BUILD.gn
 
content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java
 
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
 
content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java
  content/public/android/java/src/org/chromium/content/browser/input/Range.java
 
content/public/android/java/src/org/chromium/content/browser/input/SuggestionInfo.java
 
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java
  content/public/android/java/strings/android_content_strings.grd
 
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
 
content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java
 
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java
  content/renderer/render_view_impl.cc
  content/renderer/render_view_impl.h
  content/renderer/render_widget.cc
  content/renderer/render_widget.h
  third_party/WebKit/Source/web/ChromeClientImpl.cpp
  third_party/WebKit/Source/web/ChromeClientImpl.h
  third_party/WebKit/Source/web/CompositionUnderlineBuilder.h
  third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp
  third_party/WebKit/Source/web/WebInputMethodControllerImpl.h
  third_party/WebKit/public/web/WebCompositionUnderline.h
  third_party/WebKit/public/web/WebInputMethodController.h
  third_party/WebKit/public/web/WebTextSuggestionInfo.h
  third_party/WebKit/public/web/WebViewClient.h
  third_party/WebKit/public/web/WebWidgetClient.h
esprehn:
  content/renderer/render_view_impl.cc
  content/renderer/render_view_impl.h
  content/renderer/render_widget.cc
  content/renderer/render_widget.h
  third_party/WebKit/Source/core/dom/Document.cpp
  third_party/WebKit/Source/core/editing/BUILD.gn
  third_party/WebKit/Source/core/editing/CompositionUnderline.cpp
  third_party/WebKit/Source/core/editing/CompositionUnderline.h
  third_party/WebKit/Source/core/editing/Editor.cpp
  third_party/WebKit/Source/core/editing/FrameSelection.cpp
  third_party/WebKit/Source/core/editing/InputMethodController.cpp
  third_party/WebKit/Source/core/editing/InputMethodController.h
  third_party/WebKit/Source/core/editing/SelectionController.cpp
  third_party/WebKit/Source/core/editing/TextSuggestionList.cpp
  third_party/WebKit/Source/core/editing/TextSuggestionList.h
  third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp
  third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
  third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
  third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
 
third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp
  third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
  third_party/WebKit/Source/core/frame/FrameView.cpp
  third_party/WebKit/Source/core/loader/EmptyClients.h
  third_party/WebKit/Source/core/page/ChromeClient.h
  third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
  third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
  third_party/WebKit/Source/web/ChromeClientImpl.cpp
  third_party/WebKit/Source/web/ChromeClientImpl.h
  third_party/WebKit/Source/web/CompositionUnderlineBuilder.h
  third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp
  third_party/WebKit/Source/web/WebInputMethodControllerImpl.h
  third_party/WebKit/public/web/WebCompositionUnderline.h
  third_party/WebKit/public/web/WebInputMethodController.h
  third_party/WebKit/public/web/WebTextSuggestionInfo.h
  third_party/WebKit/public/web/WebViewClient.h
  third_party/WebKit/public/web/WebWidgetClient.h
dcheng:
  content/common/content_param_traits_macros.h
  content/common/input_messages.h
  third_party/WebKit/Source/core/dom/Document.cpp
  third_party/WebKit/Source/core/editing/BUILD.gn
  third_party/WebKit/Source/core/editing/CompositionUnderline.cpp
  third_party/WebKit/Source/core/editing/CompositionUnderline.h
  third_party/WebKit/Source/core/editing/Editor.cpp
  third_party/WebKit/Source/core/editing/FrameSelection.cpp
  third_party/WebKit/Source/core/editing/InputMethodController.cpp
  third_party/WebKit/Source/core/editing/InputMethodController.h
  third_party/WebKit/Source/core/editing/SelectionController.cpp
  third_party/WebKit/Source/core/editing/TextSuggestionList.cpp
  third_party/WebKit/Source/core/editing/TextSuggestionList.h
  third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp
  third_party/WebKit/Source/core/editing/markers/DocumentMarker.h
  third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
  third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
 
third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp
  third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
  third_party/WebKit/Source/core/frame/FrameView.cpp
  third_party/WebKit/Source/core/loader/EmptyClients.h
  third_party/WebKit/Source/core/page/ChromeClient.h
  third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
  third_party/WebKit/Source/web/ChromeClientImpl.cpp
  third_party/WebKit/Source/web/ChromeClientImpl.h
  third_party/WebKit/Source/web/CompositionUnderlineBuilder.h
  third_party/WebKit/Source/web/WebInputMethodControllerImpl.cpp
  third_party/WebKit/Source/web/WebInputMethodControllerImpl.h
  third_party/WebKit/public/web/WebCompositionUnderline.h
  third_party/WebKit/public/web/WebInputMethodController.h
  third_party/WebKit/public/web/WebTextSuggestionInfo.h
  third_party/WebKit/public/web/WebViewClient.h
  third_party/WebKit/public/web/WebWidgetClient.h

[rlanday, 2017-01-31 21:17:12]

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:314:
rootEditableElement->document().markers().markersInRange(
I introduced a bug here when I was refactoring, this should be
markersInRangeInclusive() (to include markers the cursor is immediately before
or immediately after)

[rlanday, 2017-01-31 22:33:45]

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:542:
applyCommandToComposite(DeleteFromTextNodeCommand::create(
On 2017/01/31 at 19:50:21, rlanday wrote:
> Again, hope this doesn't break anything...

I have a test case that requires this fix to work properly, but this change
breaks some other test cases, I will look into this.

Also, one of the CLs I'm depending on to fix a few bugs
(https://codereview.chromium.org/2370663002/ ) currently breaks a couple of
InputMethodController tests as well...

[esprehn, 2017-01-31 22:41:35]

At a high level:

- Use wtf/ inside blink.
- Use mojo, no new legacy IPCs.
- This is adding tons of new machinery, but it seems like we should just
refactor spell check to do what you want.

https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:600: OnApplySuggestionReplacement)
Please use mojo.

https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:2143:
controller->prepareForTextSuggestionMenuToBeShown();
This should all be done with mojo inside blink.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/CharacterData.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CharacterData.cpp:161:
document().didRemoveText(this, offset + data.length(), realCount);
On 2017/01/31 at 19:50:21, rlanday wrote:
> I hope this doesn't break anything...presumably we have test cases that would
catch if this messes up DOM range handling, and I don't think any of the other
DocumentMarker types persist long enough for this to be an issue...

This probably needs unit tests?

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/CompositionUnderline.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/CompositionUnderline.h:65:
std::vector<std::string> m_suggestions;
no std:: in blink. This should be Vector<String>

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:52: const int
kMaxNumberSuggestions = 5;
Why 5?

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:53: const
double kSuggestionUnderlineAlphaMultiplier = 0.4;
On 2017/01/31 at 19:50:21, rlanday wrote:
> The suggestion highlight background seems kind of hard to see on the yellow
background of the webpage I've been using for testing (editpad.org), but this is
the multiplier the native Android text box uses...

Did this get a UI review?

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:308: void
InputMethodController::applySuggestionReplacement(int documentMarkerID,
This change is so huge, we probably need a lot of tests. Why is this different
than how normal spell check suggestions are applied?

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:1289:
std::vector<WebTextSuggestionInfo> suggestionInfos;
WTF Vector

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/TextSuggestionList.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/TextSuggestionList.h:13: struct
TextSuggestionList {
Why do we need all of this new machinery? This just looks like how the spell
check system itself should work.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/TextSuggestionList.h:17:
std::vector<std::string> suggestions;
no std::

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:542:
applyCommandToComposite(DeleteFromTextNodeCommand::create(
On 2017/01/31 at 19:50:21, rlanday wrote:
> Again, hope this doesn't break anything...

What did the code do before and what's different? This seems like it's a web
exposed change in the way editing works. We probably need tests for that.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:137: const
std::vector<std::string>& suggestions,
ditto

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:144: void
replaceSuggestion(int index, const std::string& newSuggestion);
No std::string

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:141:
const std::vector<std::string>& suggestions) {
ditto

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebInputMethodController.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputMethodController.h:68:
getTextSuggestionInfosUnderCaret() = 0;
We shouldn't need this or prepareForTextSuggestionMenuToBeShown.

[rlanday, 2017-01-31 22:58:43]

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:542:
applyCommandToComposite(DeleteFromTextNodeCommand::create(
On 2017/01/31 at 22:41:34, esprehn wrote:
> On 2017/01/31 at 19:50:21, rlanday wrote:
> > Again, hope this doesn't break anything...
> 
> What did the code do before and what's different? This seems like it's a web
exposed change in the way editing works. We probably need tests for that.

Actually this doesn't break any of the webkit_unit_tests, I'd just messed up my
local copy of this file when I was double-checking that all these changes were
necessary :)

The reason I'm making this change is that there's a method
DeleteSelectionCommand:fixupWhitespace():
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c...

I'd written a test (not currently in this CL but I will add it) that does the
following:

- Create an editable div containing the text "Initial text."
- Underline "text"
- Set composition to "Initial"
- Delete the composition (leaving us with " text.")
- Verify that the underline is still attached to "text"

Without this fix, fixupWhitespace() would come along and do some operation (I
think replacing a space with a non-breaking space?) and it would first delete
the space, causing the marker to be shifted left by a character (so it's still
on "text"), and then it would insert the nbsp or whatever and it would get
included at the beginning of the range, which seems clearly broken in this case.
If you always do the insert before the delete, the information about whether or
not the replaced character should be included in the new text gets preserved.

This is a fairly low-level editing operation, if it breaks anything, I suspect
it would be the way DOM ranges get updated, but I think I'm actually updating
the DocumentMarkers now in the same way DOM ranges are updated, and I'm pretty
sure there are tests for DOM ranges (although I think I've only tried running
webkit_unit_tests so far).

[rlanday, 2017-01-31 23:29:56]

Thanks for looking this over @esprehn! Basically the reasons this requires a lot
of new code beyond what we have for spell check is that spell check has a couple
simplifying factors:

- We don't have to be very careful with maintaining the markers because we can
regenerate them at any time by re-running spell check, whereas SuggestionSpans
are written out by the IME and we have to carefully keep them updated
- Spell check spans cannot overlap (I think), whereas SuggestionSpans can be
nested (and if you apply a suggestion nested inside another suggestion, the
outer suggestion remains valid after the replacement)

I will attempt to make the changes you're requesting and will add some more test
cases.

[rlanday, 2017-01-31 23:30:10]

https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:600: OnApplySuggestionReplacement)
On 2017/01/31 at 22:41:34, esprehn wrote:
> Please use mojo.

Ok

https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:2143:
controller->prepareForTextSuggestionMenuToBeShown();
On 2017/01/31 at 22:41:34, esprehn wrote:
> This should all be done with mojo inside blink.

Ok

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/CompositionUnderline.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/CompositionUnderline.h:65:
std::vector<std::string> m_suggestions;
On 2017/01/31 at 22:41:34, esprehn wrote:
> no std:: in blink. This should be Vector<String>

Ok

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:52: const int
kMaxNumberSuggestions = 5;
On 2017/01/31 at 22:41:34, esprehn wrote:
> Why 5?

5 is what the native text box on Android uses. Blink has to be aware of this
number because once we find 5 suggestions, we stop looking for longer and longer
ranges that the cursor is over to pull more suggestions. E.g. if we have:

The quick brown fox jumped over the lazy dog.

and we have the following suggestion ranges:

"The": 1 suggestion
"The quick": 1 suggestion
"The quick brown": 1 suggestion
"The quick brown fox": 1 suggestion
"The quick brown fox jumped:" 1 suggestion
"The quick brown fox jumped over:" 1 suggestion

and you tap on "The", we highlight up to "The quick brown fox jumped" since
that's how far we have to go to get 5 suggestions.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:53: const
double kSuggestionUnderlineAlphaMultiplier = 0.4;
On 2017/01/31 at 22:41:34, esprehn wrote:
> On 2017/01/31 at 19:50:21, rlanday wrote:
> > The suggestion highlight background seems kind of hard to see on the yellow
background of the webpage I've been using for testing (editpad.org), but this is
the multiplier the native Android text box uses...
> 
> Did this get a UI review?

This is just existing Android UI we're porting over, so it didn't seem
necessary. Do you disagree?

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:308: void
InputMethodController::applySuggestionReplacement(int documentMarkerID,
On 2017/01/31 at 22:41:34, esprehn wrote:
> This change is so huge, we probably need a lot of tests. Why is this different
than how normal spell check suggestions are applied?

Yeah I should write some tests for this method.

Basically it's different from spell check in that, for spell check, the markers
do not overlap, and can be regenerated whenever we want by re-running the spell
check. For this, the markers are added by the IME, can overlap and/or be nested,
and cannot be regenerated by blink (the IME can rewrite the text but we can't
control that), so we have to carefully persist them, and keep them updated
properly when performing replacements.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:1289:
std::vector<WebTextSuggestionInfo> suggestionInfos;
On 2017/01/31 at 22:41:34, esprehn wrote:
> WTF Vector

Ok

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/TextSuggestionList.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/TextSuggestionList.h:17:
std::vector<std::string> suggestions;
On 2017/01/31 at 22:41:34, esprehn wrote:
> no std::

Ok

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.cpp:137: const
std::vector<std::string>& suggestions,
On 2017/01/31 at 22:41:35, esprehn wrote:
> ditto

Ok

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:144: void
replaceSuggestion(int index, const std::string& newSuggestion);
On 2017/01/31 at 22:41:35, esprehn wrote:
> No std::string

Ok

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
(right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:141:
const std::vector<std::string>& suggestions) {
On 2017/01/31 at 22:41:35, esprehn wrote:
> ditto

Ok

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebInputMethodController.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputMethodController.h:68:
getTextSuggestionInfosUnderCaret() = 0;
On 2017/01/31 at 22:41:35, esprehn wrote:
> We shouldn't need this or prepareForTextSuggestionMenuToBeShown.

Because everything's going to be in mojo? Ok

[rlanday, 2017-02-01 21:11:59]

The blink_unit_tests seem to be passing, but at least one blink layout test,
external/wpt/dom/ranges/Range-mutations-replaceData.html, is broken by a change
in this (probably CharacterData::replaceData), so I need to figure out how to
make this work without breaking anything

[Ken Russell (switch to Gerrit), 2017-02-04 23:25:26]

Sorry but this is outside my domain of expertise. Elliott's a much better
reviewer for this than I would be.

[Ken Russell (switch to Gerrit), 2017-02-04 23:25:38]

Description was changed from

==========
Add support for Android SuggestionSpans when editing text

This allows people entering text using the Android Voice IME to tap on the
underlined suggestion spans and pick from a set of potential replacements in a
dropdown menu.

This still needs to be split up into smaller patches and cleaned up a bit, I am
uploading what I have so far so we can discuss the best way to split it up.

BUG=672259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add support for Android SuggestionSpans when editing text

This allows people entering text using the Android Voice IME to tap on the
underlined suggestion spans and pick from a set of potential replacements in a
dropdown menu.

This still needs to be split up into smaller patches and cleaned up a bit, I am
uploading what I have so far so we can discuss the best way to split it up.

BUG=672259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Ken Russell (switch to Gerrit), 2017-02-04 23:25:38]

kbr@chromium.org changed reviewers:
- kbr@chromium.org

[Ken Russell (switch to Gerrit), 2017-02-04 23:25:38]

kbr@chromium.org changed required reviewers:
- kbr@chromium.org

[aelias_OOO_until_Jul13, 2017-02-10 21:43:55]

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:343: // Special
case: DocumentMarkerController handles shifting most of the
As a reminder of what we discussed offline, marking marker ranges "inclusive" is
the way to solve this problem, and this special case should be deleted.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:359:
setCompositionFromExistingText(Vector<CompositionUnderline>(),
There's no reason to do anything relating to composition here.  Please select
the word directly without going through composition.  Then it will be easier to
separate this method out from InputMethodController.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:405: // Remove
markers that contain part, but not all, of the text range to be
It can't be correct for this code to live here, since there's no difference
between this situation, and the one where you long-press-select the word and
press backspace.  It should be in a more generic place that covers both cases.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:419: // If the
character immediately following the range to be deleted is a space,
Please split this off into a separate method called
"expandRangeToSurroundingWhitespace"

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:593:
clearSuggestionMarkersTouchingSelection();
If text is inserted by typing on a physical keyboard or by Javascript method, it
doesn't go through this method (it's only for IME).  So this spot is too
high-level for this, it needs to be in lower-level code.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:1262:
InputMethodController::getTextSuggestionInfosUnderCaret() const {
Please move this and most other changes in this file into a new file called
something like MobileSuggestions.cpp

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:1368:
document().markers().addSuggestionBackgroundHighlightMarker(
I don't think a method called "get...()" should be adding a marker, this should
be split off.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:49:
SuggestionMarkerIndex,
This marker type should probably be returned in MisspellingMarkers(), which
itself should probably be renamed to SuggestionMarkers().

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:139: int
suggestionMarkerID() const;
On 2017/01/31 at 19:50:22, rlanday wrote:
> note: DocumentMarker already has a concept of a "hash" that can be stored with
a marker, ideally we wouldn't need two different ways to give a marker an ID but
I couldn't figure out a clean way to unify these.

Let's try to unify it by keeping populating the hash from InputMethodController
only for suggestion markers, keeping it 0 for all other non-spellcheck markers.

[Changwan Ryu, 2017-02-14 00:58:00]

changwan@chromium.org changed reviewers:
+ changwan@chromium.org

[Changwan Ryu, 2017-02-14 00:58:01]

drive-by review:

could you try to add a Robolectric test class named SuggestionSpanTest.java that
tests scenarios such as

1) commit text -> show suggestionspopupwindow -> scroll -> dismiss
(By the way do we support scrolling at all? or do we just dismiss the window?)
2) commit text -> show suggestionspopupwindow -> change input (i.e. restart
input)

Here's an example robolectric test for IME:
https://cs.chromium.org/chromium/src/content/public/android/junit/src/org/chr...

The benefit of this type of tests is to mock out interactions with real keyboard
and with renderer/native, such that you can separate the core Java logic from
the rest.

You can think of this as a good split point in this sense, but I don't mind
keeping this as is if you believe the review is almost finishing.

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/SuggestionInfo.java
(right):

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SuggestionInfo.java:13:
int mDocumentMarkerID;
nit: normally style convention for public variables is camel casing (e.g.
documentMarkerId). Also, you can make the whole class package-private by
removing 'public' from class definition or add public to each variable to be
consistent.

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java
(right):

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:50:
private boolean mListeningForCursorAnchorInfoUpdates = false;
no need to add '= false' or '= 0.0'. here and other places

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:55:
int mPositionX, mPositionY;
nit: you should define one variable in one line

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:82:
mPopupWindow.setWindowLayoutType(WindowManager.LayoutParams.TYPE_APPLICATION_SUB_PANEL);
could you add a comment on the effect of this line?

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:94:
protected void createPopupWindow() {
no need to use protected

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:105:
protected void initContentView() {
no need to use protected

https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:177:
protected void measureContent() {
no need to use protected

[rlanday, 2017-02-16 23:34:27]

On 2017/02/14 at 00:58:01, changwan wrote:
> drive-by review:
> 
> could you try to add a Robolectric test class named SuggestionSpanTest.java
that tests scenarios such as
> 
> 1) commit text -> show suggestionspopupwindow -> scroll -> dismiss
> (By the way do we support scrolling at all? or do we just dismiss the window?)
> 2) commit text -> show suggestionspopupwindow -> change input (i.e. restart
input)
> 
> Here's an example robolectric test for IME:
>
https://cs.chromium.org/chromium/src/content/public/android/junit/src/org/chr...
> 
> The benefit of this type of tests is to mock out interactions with real
keyboard and with renderer/native, such that you can separate the core Java
logic from the rest.
> 
> You can think of this as a good split point in this sense, but I don't mind
keeping this as is if you believe the review is almost finishing.
> 
>
https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
> File
content/public/android/java/src/org/chromium/content/browser/input/SuggestionInfo.java
(right):
> 
>
https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/SuggestionInfo.java:13:
int mDocumentMarkerID;
> nit: normally style convention for public variables is camel casing (e.g.
documentMarkerId). Also, you can make the whole class package-private by
removing 'public' from class definition or add public to each variable to be
consistent.
> 
>
https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
> File
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java
(right):
> 
>
https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:50:
private boolean mListeningForCursorAnchorInfoUpdates = false;
> no need to add '= false' or '= 0.0'. here and other places
> 
>
https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:55:
int mPositionX, mPositionY;
> nit: you should define one variable in one line
> 
>
https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:82:
mPopupWindow.setWindowLayoutType(WindowManager.LayoutParams.TYPE_APPLICATION_SUB_PANEL);
> could you add a comment on the effect of this line?
> 
>
https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:94:
protected void createPopupWindow() {
> no need to use protected
> 
>
https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:105:
protected void initContentView() {
> no need to use protected
> 
>
https://codereview.chromium.org/2650113004/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:177:
protected void measureContent() {
> no need to use protected

[rlanday, 2017-02-16 23:39:48]

Gah, I apologize for another blank comment...

@esprehn, am I supposed to use WebString in WebCompositionUnderline? We
currently create WebCompositionUnderlines in ime_adapter_android.cc and send
them over IPC to Blink. If I try to allocate a WebString in
ime_adapter_android.cc by using blink::WebString::fromUTF8(), I get an error
that PartitionAllocator hasn't been initialized (I guess since we're not in
Blink).

If I rewrite the setCompostion()/commitText() calls to use Mojo instead of IPC,
will that solve this issue?

[rlanday, 2017-02-17 01:46:21]

https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:2143:
controller->prepareForTextSuggestionMenuToBeShown();
On 2017/01/31 at 23:30:09, rlanday wrote:
> On 2017/01/31 at 22:41:34, esprehn wrote:
> > This should all be done with mojo inside blink.
> 
> Ok

Is there a way to get access to the RenderWidget from LocalFrame (which is what
InputMethodController has access to, and it's what my new
TextSuggestionController class has access to) to call UpdateCompositionInfo()?

[dcheng, 2017-02-18 00:04:37]

On 2017/02/17 01:46:21, rlanday wrote:
>
https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
> content/renderer/render_widget.cc:2143:
> controller->prepareForTextSuggestionMenuToBeShown();
> On 2017/01/31 at 23:30:09, rlanday wrote:
> > On 2017/01/31 at 22:41:34, esprehn wrote:
> > > This should all be done with mojo inside blink.
> > 
> > Ok
> 
> Is there a way to get access to the RenderWidget from LocalFrame (which is
what
> InputMethodController has access to, and it's what my new
> TextSuggestionController class has access to) to call UpdateCompositionInfo()?

The current way of doing that is to add a method to ChromeClient (that takes a
LocalFrame*), looks up the WebLocalFrameImpl from the LocalFrame* (using
WebLocalFrameImpl::fromFrame), getting the WebFrameWidget from that, then
talking to the WebWidgetClient to get to RenderWidget. It's rather painful.

+ekaramad as well, as a heads up, since he's done a bunch of IME work for OOPIF
and factored out some of the IME stuff into WebInputMethodController already.

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebViewClient.h (right):

https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebViewClient.h:275: void
handlePotentialTextSuggestionTap() override {}
Is this work supposed to be associated only with the WebView? It seems like it
should probably be associated with the WebFrameWidget, to match how the other
IME stuff works.

(We could/should even move this to a private section, since this is a new
method)

[EhsanK, 2017-02-21 16:47:49]

On 2017/02/18 00:04:37, dcheng wrote:
> On 2017/02/17 01:46:21, rlanday wrote:
> >
>
https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
> > File content/renderer/render_widget.cc (right):
> > 
> >
>
https://codereview.chromium.org/2650113004/diff/40001/content/renderer/render...
> > content/renderer/render_widget.cc:2143:
> > controller->prepareForTextSuggestionMenuToBeShown();
> > On 2017/01/31 at 23:30:09, rlanday wrote:
> > > On 2017/01/31 at 22:41:34, esprehn wrote:
> > > > This should all be done with mojo inside blink.
> > > 
> > > Ok
> > 
> > Is there a way to get access to the RenderWidget from LocalFrame (which is
> what
> > InputMethodController has access to, and it's what my new
> > TextSuggestionController class has access to) to call
UpdateCompositionInfo()?
> 
> The current way of doing that is to add a method to ChromeClient (that takes a
> LocalFrame*), looks up the WebLocalFrameImpl from the LocalFrame* (using
> WebLocalFrameImpl::fromFrame), getting the WebFrameWidget from that, then
> talking to the WebWidgetClient to get to RenderWidget. It's rather painful.
> 
> +ekaramad as well, as a heads up, since he's done a bunch of IME work for
OOPIF
> and factored out some of the IME stuff into WebInputMethodController already.
> 
>
https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/publ...
> File third_party/WebKit/public/web/WebViewClient.h (right):
> 
>
https://codereview.chromium.org/2650113004/diff/40001/third_party/WebKit/publ...
> third_party/WebKit/public/web/WebViewClient.h:275: void
> handlePotentialTextSuggestionTap() override {}
> Is this work supposed to be associated only with the WebView? It seems like it
> should probably be associated with the WebFrameWidget, to match how the other
> IME stuff works.
> 
> (We could/should even move this to a private section, since this is a new
> method)

Thanks for the heads-up!
+1 to dcheng@'s comment above. Specifically given that the IPC is directly sent
through RenderWidgetHostImpl as well as 
being handled by RenderWidget. Also note that we get WebInputMethodController
from WebFrameWidget.

Also to have support for top-document-isolation/site-per-process, the IPC should
be sent to the focused RenderWidgetHostImpl
instead (inside ime_adapter_android.cc). I have an ongoing CL which is doing
that for the current IPCs being sent to
RenderWidgets:
https://codereview.chromium.org/2653283002/.