Bug 52781

Summary: Inserting multiple whitespace using text composition (IME) should insert interleaved nbsp and whitespace.
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, buildbot, darin, dglazkov, enrica, gns, ojan, rniwa, tkent, tony, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 5241    
Bug Blocks:    
Attachments:
Description Flags
fixes the bug
none
fixed typos & comment per ojan's comment
none
fixed one more typo ojan: review+

Description Hajime Morrita 2011-01-19 21:20:29 PST
Followup fix for Bug 5241.
See https://bugs.webkit.org/show_bug.cgi?id=5241#c34 for detail.
Comment 1 Ryosuke Niwa 2011-01-24 11:47:47 PST
Created attachment 79951 [details]
fixes the bug
Comment 2 Ojan Vafai 2011-01-24 16:54:42 PST
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.
Comment 3 Ojan Vafai 2011-01-24 16:55:02 PST
Comment on attachment 79951 [details]
fixes the bug

Whoops, meant to r-.
Comment 4 Ryosuke Niwa 2011-01-24 17:24:21 PST
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"?
Comment 5 Ryosuke Niwa 2011-01-24 18:28:37 PST
Created attachment 80006 [details]
fixed typos & comment per ojan's comment
Comment 6 WebKit Review Bot 2011-01-24 18:33:23 PST
Attachment 80006 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7585300
Comment 7 WebKit Review Bot 2011-01-24 18:33:55 PST
Attachment 80006 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7645057
Comment 8 Ryosuke Niwa 2011-01-24 18:42:52 PST
Created attachment 80007 [details]
fixed one more typo
Comment 9 Early Warning System Bot 2011-01-24 18:45:36 PST
Attachment 80006 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7542325
Comment 10 Build Bot 2011-01-24 18:51:15 PST
Attachment 80006 [details] did not build on win:
Build output: http://queues.webkit.org/results/7621313
Comment 11 Ojan Vafai 2011-01-24 18:59:26 PST
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?
Comment 12 Ojan Vafai 2011-01-24 19:02:24 PST
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.
Comment 13 Ryosuke Niwa 2011-01-24 19:09:28 PST
(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.
Comment 14 Ryosuke Niwa 2011-01-24 19:27:30 PST
Committed r76565: <http://trac.webkit.org/changeset/76565>