Bug 52781 – Inserting multiple whitespace using text composition (IME) should insert interleaved nbsp and whitespace.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Hajime Morrita]

Followup fix for Bug 5241.
See https://bugs.webkit.org/show_bug.cgi?id=5241#c34 for detail.

[Ryosuke Niwa]

Created attachment 79951 [details]
fixes the bug

[Ojan Vafai]

Comment on attachment 79951 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=79951&action=review

> Source/WebCore/editing/htmlediting.cpp:359
> +    Vector<UChar> rebalanacedString;

s/rebalanacedString/rebalancedString

Also, why use a Vector<UChar> instead of  String like the old code did?

> Source/WebCore/editing/htmlediting.h:239
>  // Miscellaneous functions on String
> -    
> +inline bool isWhitespace(UChar c)
> +{
> +    return c == noBreakSpace || c == ' ' || c == '\n' || c == '\t';
> +}

Nit: technically, this is not a function on String, os the comment is not correct.

[Ojan Vafai]

Comment on attachment 79951 [details]
fixes the bug

Whoops, meant to r-.

[Ryosuke Niwa]

Comment on attachment 79951 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=79951&action=review

>> Source/WebCore/editing/htmlediting.cpp:359
>> +    Vector<UChar> rebalanacedString;
> 
> s/rebalanacedString/rebalancedString
> 
> Also, why use a Vector<UChar> instead of  String like the old code did?

As far I checked, we can't modify each character in String directly (because multiple String instances may share single StringImpl).  i.e. String only has UChar operator[](unsigned index) const.

>> Source/WebCore/editing/htmlediting.h:239
>> +}
> 
> Nit: technically, this is not a function on String, os the comment is not correct.

Maybe I should change the comment to "Miscellaneous functions on Text"?

[Ryosuke Niwa]

Created attachment 80006 [details]
fixed typos & comment per ojan's comment

[WebKit Review Bot]

Attachment 80006 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7585300

[WebKit Review Bot]

Attachment 80006 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7645057

[Ryosuke Niwa]

Created attachment 80007 [details]
fixed one more typo

[Early Warning System Bot]

Attachment 80006 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7542325

[Build Bot]

Attachment 80006 [details] did not build on win:
Build output: http://queues.webkit.org/results/7621313

[Ojan Vafai]

Comment on attachment 79951 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=79951&action=review

>>> Source/WebCore/editing/htmlediting.cpp:359
>>> +    Vector<UChar> rebalanacedString;
>> 
>> s/rebalanacedString/rebalancedString
>> 
>> Also, why use a Vector<UChar> instead of  String like the old code did?
> 
> As far I checked, we can't modify each character in String directly (because multiple String instances may share single StringImpl).  i.e. String only has UChar operator[](unsigned index) const.

I don't understand. Isn't that what the old code was doing with the "replace" calls?

[Ojan Vafai]

Comment on attachment 80007 [details]
fixed one more typo

Ryosuke informed me that replace copies the string. So the new code is certainly more efficient.

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 80007 [details])
> Ryosuke informed me that replace copies the string. So the new code is certainly more efficient.

Note: new function creates copy regardless of whether there are whitespace or not while the old implementation doesn't make a copy when there are no whitespace.  However, in practice, almost all text contains whitespace so this shouldn't be a problem.  We can add that optimization later if we're so inclined.

[Ryosuke Niwa]

Committed r76565: <http://trac.webkit.org/changeset/76565>