Emit object replacement char to IME

Emit object replacement char to IME

Currently Clank could only delete (with the backspace key) nodes that are
visible to the android IME, and since a replaced element, e.g. <img>, is not
visible to the IME, thus it could not be easily deleted using the backspace key.
Another side effect is that if two text nodes are separated by an image element
alone, then they appear as a single composition to the IME.

To fix these issues, this CL exposes a replaced element as a single object
replacement char 0xFFFC to InputMethodController. For more details please refer
to the atatched bug.

BUG=311448
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175819

[guohui, 2014-05-22 21:25:01]

Hey aurimas,

the CL is obviously not ready for review yet. But i would like to check with you
if it makes sense to always emit a replacement char in
TextIterator::handleReplacedElement. I test the CL with m_emitsReplacementChar
hardcoded to true and it seems to cause no side effect. A more complicated
approach is to create a new behavior flag for emitting a replacement char, and
update all users of TextIterator accordingly. 

Thanks,

Hui

[aurimas (slooooooooow), 2014-05-22 21:43:40]

https://codereview.chromium.org/297003003/diff/20001/Source/core/editing/Text...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/297003003/diff/20001/Source/core/editing/Text...
Source/core/editing/TextIterator.cpp:791: if (m_emitsReplacementChar || true) {
"A || true" is true for any value for A, right?

[guohui, 2014-05-22 21:47:29]

On 2014/05/22 21:43:40, aurimas wrote:
>
https://codereview.chromium.org/297003003/diff/20001/Source/core/editing/Text...
> File Source/core/editing/TextIterator.cpp (right):
> 
>
https://codereview.chromium.org/297003003/diff/20001/Source/core/editing/Text...
> Source/core/editing/TextIterator.cpp:791: if (m_emitsReplacementChar || true)
{
> "A || true" is true for any value for A, right?

right, that is what i meant that i tested with m_emitsReplacementChar hardcoded
to true. The CL is not for review, it just gives an idea how the fix could work,
either we always emit a replacement char or we introduce a new behavior flag and
update callers.

[guohui, 2014-05-22 22:00:37]

+ojan@ and koshi@

The CL is NOT ready for review, but i am new to blink and ime in general, so
before i proceed i would like to confirm with you my approach does make sense.
Basically, i have two options here,
1) always emit a replacement char in TextIterator::handleReplacedElement, it
causes no side effect in my testing so far.
2) add a new behavior flag TextIteratorEmitsReplacementChar and update all
callers of TextIterator so that the flag is only set to true when needed.

I suspect option 2) is much more complicated, so i wonder if   you guys have any
objection against option 1).

thanks!

https://codereview.chromium.org/297003003/diff/20001/Source/core/editing/Text...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/297003003/diff/20001/Source/core/editing/Text...
Source/core/editing/TextIterator.cpp:791: if (m_emitsReplacementChar || true) {
On 2014/05/22 21:43:40, aurimas wrote:
> "A || true" is true for any value for A, right?

Absolutely right, i hardcoded to true for testing purpose, please see my other
comments.

[kochi, 2014-05-23 07:01:17]

On 2014/05/22 22:00:37, guohui wrote:
> +ojan@ and koshi@
> 
> The CL is NOT ready for review, but i am new to blink and ime in general, so
> before i proceed i would like to confirm with you my approach does make sense.
> Basically, i have two options here,
> 1) always emit a replacement char in TextIterator::handleReplacedElement, it
> causes no side effect in my testing so far.
> 2) add a new behavior flag TextIteratorEmitsReplacementChar and update all
> callers of TextIterator so that the flag is only set to true when needed.
> 
> I suspect option 2) is much more complicated, so i wonder if   you guys have
any
> objection against option 1).
> 
> thanks!
> 
>
https://codereview.chromium.org/297003003/diff/20001/Source/core/editing/Text...
> File Source/core/editing/TextIterator.cpp (right):
> 
>
https://codereview.chromium.org/297003003/diff/20001/Source/core/editing/Text...
> Source/core/editing/TextIterator.cpp:791: if (m_emitsReplacementChar || true)
{
> On 2014/05/22 21:43:40, aurimas wrote:
> > "A || true" is true for any value for A, right?
> 
> Absolutely right, i hardcoded to true for testing purpose, please see my other
> comments.

looped in yoshi@, who is more knowledgeable about TextIterator code.

[yosin_UTC9, 2014-05-23 08:17:06]

It seems reasonable to emit replacement character for replaced element.
I think you only need to enable EmitReplacedElement flag for TextIterator only
in PlainTextRange.cpp.

[guohui, 2014-05-23 17:30:09]

On 2014/05/23 08:17:06, yosi wrote:
> It seems reasonable to emit replacement character for replaced element.
> I think you only need to enable EmitReplacedElement flag for TextIterator only
> in PlainTextRange.cpp.

Thanks a lot yosi! Implemented your suggestion in patch 2.

The CL is now ready for review.

Thanks!

[huangs, 2014-05-24 17:18:39]

https://chromiumcodereview.appspot.com/297003003/diff/100001/Source/core/edit...
File Source/core/editing/TextIterator.h (right):

https://chromiumcodereview.appspot.com/297003003/diff/100001/Source/core/edit...
Source/core/editing/TextIterator.h:49: TextIteratorEmitsReplacementChar = 1 <<
7,
Extra comma.

[guohui, 2014-05-26 13:55:01]

Added unit tests.

ping yosin@ and ojan@, could you please take a look at the latest patch?

Thanks!

https://codereview.chromium.org/297003003/diff/100001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.h (right):

https://codereview.chromium.org/297003003/diff/100001/Source/core/editing/Tex...
Source/core/editing/TextIterator.h:49: TextIteratorEmitsReplacementChar = 1 <<
7,
On 2014/05/24 17:18:40, huangs1 wrote:
> Extra comma.

Done.

[guohui, 2014-05-26 22:57:53]

On 2014/05/26 13:55:01, guohui wrote:
> Added unit tests.
> 
> ping yosin@ and ojan@, could you please take a look at the latest patch?
> 
> Thanks!
> 
>
https://codereview.chromium.org/297003003/diff/100001/Source/core/editing/Tex...
> File Source/core/editing/TextIterator.h (right):
> 
>
https://codereview.chromium.org/297003003/diff/100001/Source/core/editing/Tex...
> Source/core/editing/TextIterator.h:49: TextIteratorEmitsReplacementChar = 1 <<
> 7,
> On 2014/05/24 17:18:40, huangs1 wrote:
> > Extra comma.
> 
> Done.

Ping again?

[yosin_UTC9, 2014-05-27 01:35:40]

ACK

Add yutak@ as OWNER and he works on TextIterator.

[guohui, 2014-05-27 20:31:54]

thanks yosi.

@yuta, could you please review the cl?

[leviw_travelin_and_unemployed, 2014-05-27 23:33:45]

Can you please explain a bit more about why you're doing this, and why you're
doing it for only this specific code path?

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (left):

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:790: if (m_entersTextControls &&
renderer->isTextControl()) {
Why are you pulling this up above the m_lastTextNodeEndedWithCollapsedSpace? Is
that necessary?

[guohui, 2014-05-28 01:17:05]

Thanks a lot leviw!

Updated the CL description, for more details， please refer to the crbug.

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (left):

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:790: if (m_entersTextControls &&
renderer->isTextControl()) {
actually it was not clear to me why this was not up above in the first place.
The comment seems to suggest that we should emit nothing for a shadow control,
why do we make an exception when the last text node ends with a collapsed space?

[Yuta Kitamura, 2014-05-28 07:36:07]

I think the general direction of this patch is fine.

* Please wrap the change description in every 80-ish characters;
  unwrapped commit log looks ugly.
* Please use the word "object replacement character" for U+FFFC;
  "replacement character" means a different character U+FFFD,
  which caused a confusion to me when I first looked at this review.

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Pla...
File Source/core/editing/PlainTextRange.h (right):

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Pla...
Source/core/editing/PlainTextRange.h:54: PassRefPtrWillBeRawPtr<Range>
createRangeForInput(const ContainerNode& scope) const;
"ForInput" sounds vague and too generic for this case... Do you have an idea of
alternative names?

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (left):

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:790: if (m_entersTextControls &&
renderer->isTextControl()) {
On 2014/05/28 01:17:06, guohui wrote:
> actually it was not clear to me why this was not up above in the first place.
> The comment seems to suggest that we should emit nothing for a shadow control,
> why do we make an exception when the last text node ends with a collapsed
space?

Text control case does seem like a bug, but I think we need to emit a space
before a replacement character, no?

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.h (right):

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
Source/core/editing/TextIterator.h:49: TextIteratorEmitsReplacementChar = 1 << 7
EmitsReplacementChar -> EmitsObjectReplacementCharacterForImage ?

* We try to avoid abbreviation like "Char" in names;
* "Replacement character" (U+FFFD) is different from "object replacement
  character" (U+FFFC). It's probably good to clarify that here;
* It's probably worth noting that we are replacing images here.

[guohui, 2014-05-28 13:19:11]

Many thanks yuta, change description updated, and all comments addressed except
for one as detailed below.

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Pla...
File Source/core/editing/PlainTextRange.h (right):

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Pla...
Source/core/editing/PlainTextRange.h:54: PassRefPtrWillBeRawPtr<Range>
createRangeForInput(const ContainerNode& scope) const;
On 2014/05/28 07:36:07, Yuta Kitamura wrote:
> "ForInput" sounds vague and too generic for this case... Do you have an idea
of
> alternative names?

Changed to createRangeForInputMethod.

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (left):

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:790: if (m_entersTextControls &&
renderer->isTextControl()) {
reverted, we'd better fix the text control case in a separate CL.

As for the replaced element, do you mean that we should emit an empty space
followed by 0xFFFC when m_lastTextNodeEndedWithCollapsedSpace == true?

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.h (right):

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
Source/core/editing/TextIterator.h:49: TextIteratorEmitsReplacementChar = 1 << 7
On 2014/05/28 07:36:07, Yuta Kitamura wrote:
> EmitsReplacementChar -> EmitsObjectReplacementCharacterForImage ?
> 
> * We try to avoid abbreviation like "Char" in names;
> * "Replacement character" (U+FFFD) is different from "object replacement
>   character" (U+FFFC). It's probably good to clarify that here;
Done.

> * It's probably worth noting that we are replacing images here.

This applies not only to images, but also to any replaced elements.

[guohui, 2014-05-28 13:32:38]

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
File Source/core/editing/TextIterator.cpp (left):

https://codereview.chromium.org/297003003/diff/140001/Source/core/editing/Tex...
Source/core/editing/TextIterator.cpp:790: if (m_entersTextControls &&
renderer->isTextControl()) {
Correction:

when m_lastTextNodeEndedWithCollapsedSpace == true?
->
when m_lastTextNodeEndedWithCollapsedSpace && m_emitsObjectReplacementCharacter?

[guohui, 2014-05-29 18:18:04]

ping yutak@ and leviw@?

[leviw_travelin_and_unemployed, 2014-05-29 20:45:23]

This seems reasonable. LGTM.

[guohui, 2014-05-30 17:58:42]

On 2014/05/29 20:45:23, leviw wrote:
> This seems reasonable. LGTM.

Thanks leviw!

Please let me know if anyone still has concern about the CL, otherwise i would
commit it by the end of today.

[ojan, 2014-05-31 05:35:58]

On 2014/05/30 17:58:42, guohui wrote:
> On 2014/05/29 20:45:23, leviw wrote:
> > This seems reasonable. LGTM.
> 
> Thanks leviw!
> 
> Please let me know if anyone still has concern about the CL, otherwise i would
> commit it by the end of today.

Once any owner LGTMs you can commit unless someone expressed a strong concern.
Although, usually it's the job of the reviewer not to LGTM a patch that has open
concerns from other owners.

[Yuta Kitamura, 2014-06-02 06:21:21]

lgtm

[guohui, 2014-06-09 20:41:38]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-06-09 20:42:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/guohui@chromium.org/297003003/170001

[commit-bot: I haz the power, 2014-06-09 20:57:26]

Change committed as 175819