Summary: | [chromium] Can't handle deletion of characters beyond U+10000. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Su <suzhe> | ||||||||||
Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, shinyak, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
James Su
2010-06-08 22:13:25 PDT
I reproduced it with the latest Chrome. But seems that Safari 4.0 doesn't have this problem. Yes, I also cannot reproduce this in Safari. Created attachment 106334 [details]
Patch
Comment on attachment 106334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106334&action=review Looks good. > Source/WebCore/rendering/RenderText.cpp:1671 > +#if PLATFORM(MAC) || defined(OS_MACOSX) I don't know if all other ports want this behavior when running on OS X, although I would certainly encourage them to. > Source/WebCore/rendering/RenderText.cpp:1761 > + StringImpl& text = *m_text.impl(); > + if (U16_IS_TRAIL(text[--current])) > + --current; Why did you store StringImpl in a local variable? Just accessing the text through m_text seems like it would be better for readability: if (U16_IS_TRAIL(m_text[--current])) Comment on attachment 106334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106334&action=review >> Source/WebCore/rendering/RenderText.cpp:1671 >> +#if PLATFORM(MAC) || defined(OS_MACOSX) > > I don't know if all other ports want this behavior when running on OS X, although I would certainly encourage them to. OS_MACOSX is a macro used only in Chromium, and we should not use it here. We should write #if PLATFORM(MAC) || PLATFORM(CHROMIUM) && OS(MAC_OS_X) or #if OS(DARWIN) if you'd like to include iOS and Qt/Mac. Comment on attachment 106334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106334&action=review > LayoutTests/editing/deleting/delete-surrogatepair.html:34 > + if (!window.layoutTestController) > + continue; This check should be at the outside of the 'for' loop. We had better show a message like "This test requires layoutTestController" or a manual test instruction if there is no layoutTestController. > LayoutTests/editing/deleting/delete-surrogatepair.html:40 > + layoutTestController.execCommand("MoveForward"); > + > + layoutTestController.execCommand("DeleteBackward"); Do you need layoutTestController.execCommand()? Doesn't document.execCommand() work? Created attachment 106382 [details]
Patch
(In reply to comment #6) > (From update of attachment 106334 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106334&action=review > > > LayoutTests/editing/deleting/delete-surrogatepair.html:34 > > + if (!window.layoutTestController) > > + continue; > > This check should be at the outside of the 'for' loop. > We had better show a message like "This test requires layoutTestController" or a manual test instruction if there is no layoutTestController. > > > LayoutTests/editing/deleting/delete-surrogatepair.html:40 > > + layoutTestController.execCommand("MoveForward"); > > + > > + layoutTestController.execCommand("DeleteBackward"); > > Do you need layoutTestController.execCommand()? Doesn't document.execCommand() work? Unfortunately, document.execCommand() didn't work in my environment. Comment on attachment 106382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106382&action=review > LayoutTests/editing/deleting/delete-surrogatepair.html:10 > +<script src="../editing.js" language="JavaScript" type="text/JavaScript" ></script> nit: language and type attributes are not needed. > LayoutTests/editing/deleting/delete-surrogatepair.html:38 > + //adocument.execCommand("MoveForward"); Unused code should be removed. Created attachment 106386 [details]
Patch
(In reply to comment #9) > (From update of attachment 106382 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106382&action=review > > > LayoutTests/editing/deleting/delete-surrogatepair.html:10 > > +<script src="../editing.js" language="JavaScript" type="text/JavaScript" ></script> > > nit: language and type attributes are not needed. > > > LayoutTests/editing/deleting/delete-surrogatepair.html:38 > > + //adocument.execCommand("MoveForward"); > > Unused code should be removed. Sorry, fixed them. Comment on attachment 106386 [details]
Patch
ok
Comment on attachment 106386 [details] Patch Rejecting attachment 106386 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Last 500 characters of output: pp M Source/WebCore/bindings/v8/OptionsObject.h M Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp r94946 = 2009367ae0a0b4f627ac5aecb39e8cab82b59735 (refs/remotes/origin/master) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Updating chromium port dependencies using gclient... ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/9631958 Comment on attachment 106386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106386&action=review > Source/WebCore/ChangeLog:6 > + https://bugs.webkit.org/show_bug.cgi?id=40351 > + > + If a character is the trail part of unicode surrogate pair, the lead part of it There is no "Reviewed by NOBODY (OOPS!)" line. (In reply to comment #14) > (From update of attachment 106386 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106386&action=review > > > Source/WebCore/ChangeLog:6 > > + https://bugs.webkit.org/show_bug.cgi?id=40351 > > + > > + If a character is the trail part of unicode surrogate pair, the lead part of it > > There is no "Reviewed by NOBODY (OOPS!)" line. !! Sorry, I will fix it soon. Created attachment 107024 [details]
Patch
Comment on attachment 107024 [details] Patch Clearing flags on attachment: 107024 Committed r94947: <http://trac.webkit.org/changeset/94947> All reviewed patches have been landed. Closing bug. |