Summary: | Inserting multiple whitespace using text composition (IME) should insert interleaved nbsp and whitespace. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, buildbot, darin, dglazkov, enrica, gustavo, 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
Hajime Morrita
2011-01-19 21:20:29 PST
Created attachment 79951 [details]
fixes the bug
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 on attachment 79951 [details]
fixes the bug
Whoops, meant to r-.
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"? Created attachment 80006 [details]
fixed typos & comment per ojan's comment
Attachment 80006 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7585300 Attachment 80006 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7645057 Created attachment 80007 [details]
fixed one more typo
Attachment 80006 [details] did not build on qt: Build output: http://queues.webkit.org/results/7542325 Attachment 80006 [details] did not build on win: Build output: http://queues.webkit.org/results/7621313 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 on attachment 80007 [details]
fixed one more typo
Ryosuke informed me that replace copies the string. So the new code is certainly more efficient.
(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. Committed r76565: <http://trac.webkit.org/changeset/76565> |