Summary: | Ctrl+Shift+Right in Windows should select the spacing after the word | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Claudio Saavedra <csaavedra> | ||||||||||||||||||||||||||||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | ap, benjamin, buildbot, cmarcelo, commit-queue, dglazkov, enrica, gyuyoung.kim, mifenton, ojan.autocc, rakuco, rniwa, shezbaig.wk, tony, webkit-ews, webkit.review.bot, xji | ||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||||||||||
OS: | Windows 7 | ||||||||||||||||||||||||||||||||||||
Bug Depends on: | 111321, 113383, 113414, 113424, 113490 | ||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Claudio Saavedra
2013-02-21 09:38:27 PST
Created attachment 189549 [details]
Patch
Comment on attachment 189549 [details] Patch Attachment 189549 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16696186 New failing tests: editing/selection/extend-selection-enclosing-block.html editing/selection/extend-selection-word.html Comment on attachment 189549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review r- due to lack of tests and failing tests. Why did this change the results of a test on Linux? > Source/WebCore/editing/FrameSelection.cpp:603 > +#if USE(ICU_UNICODE) Why does this depend on ICU_UNICODE? Comment on attachment 189549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review > Source/WebCore/editing/FrameSelection.cpp:605 > + bool skipsSpaceWhenMovingRight = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight(); > + pos = rightWordPosition(pos, skipsSpaceWhenMovingRight); This should be testable. Also, be sure to test this with bidirectional text. Furthermore, there should be an equivalent change in modifyExtendingLeft. (In reply to comment #4) > (From update of attachment 189549 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review > > > Source/WebCore/editing/FrameSelection.cpp:605 > > + bool skipsSpaceWhenMovingRight = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight(); > > + pos = rightWordPosition(pos, skipsSpaceWhenMovingRight); > > This should be testable. Also, be sure to test this with bidirectional text. > Furthermore, there should be an equivalent change in modifyExtendingLeft. I tested the ctrl+shift+left case on my Windows machine (notepad.exe) and it does not include the preceding space. (In reply to comment #3) > (From update of attachment 189549 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review > > r- due to lack of tests and failing tests. Why did this change the results of a test on Linux? > > > Source/WebCore/editing/FrameSelection.cpp:603 > > +#if USE(ICU_UNICODE) > > Why does this depend on ICU_UNICODE? It is commented somewhere else in the same file. I think there's one method that is not available for all platforms. (In reply to comment #5) > (In reply to comment #4) > > > > This should be testable. Also, be sure to test this with bidirectional text. > > Furthermore, there should be an equivalent change in modifyExtendingLeft. > > I tested the ctrl+shift+left case on my Windows machine (notepad.exe) and it does not include the preceding space. It probably does if your text is RTL. Ryosuke is probably right. (In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #4) > > > > > > > This should be testable. Also, be sure to test this with bidirectional text. > > > Furthermore, there should be an equivalent change in modifyExtendingLeft. > > > > I tested the ctrl+shift+left case on my Windows machine (notepad.exe) and it does not include the preceding space. > > It probably does if your text is RTL. Ryosuke is probably right. Ah, yes, if the text is RTL, it probably should select the trailing space when pressing ctrl+shift+left. (In reply to comment #3) > (From update of attachment 189549 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review > > r- due to lack of tests and failing tests. Why did this change the results of a test on Linux? This changed the results of the tests in Linux because what my initial patch did was to use rightWordPosition() for the cases where the enclosing paragraph is LTR. Now, this fails because when the text is using RTL, this is wrong. It seems to me that the proper fix would be to change nextWordPosition() to take into account whether spacing should be skipped, just like it was done with rightWordPosition(). Created attachment 190316 [details]
Patch
Comment on attachment 190316 [details] Patch Attachment 190316 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16770324 Comment on attachment 190316 [details] Patch Attachment 190316 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16773304 Comment on attachment 190316 [details] Patch Attachment 190316 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16778291 Comment on attachment 190316 [details] Patch Attachment 190316 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16780269 Comment on attachment 190316 [details] Patch Attachment 190316 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16775390 Comment on attachment 190316 [details] Patch Attachment 190316 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16774461 New failing tests: editing/execCommand/remove-formatting-2.html editing/execCommand/toggle-unlink.html editing/execCommand/createLink.html editing/execCommand/button.html editing/pasteboard/8145-2.html editing/inserting/insert-paragraph-03.html editing/pasteboard/5006779.html editing/selection/4932260-2.html editing/inserting/4960120-2.html editing/deleting/list-item-1.html editing/deleting/smart-delete-across-editable-boundaries.html editing/execCommand/remove-format-multiple-elements.html editing/execCommand/toggle-link.html editing/execCommand/hilitecolor.html editing/inserting/insert-before-link-1.html editing/pasteboard/4806874.html editing/deleting/delete-cell-contents.html editing/deleting/smart-editing-disabled.html editing/execCommand/unlink.html editing/execCommand/indent-pre.html editing/deleting/delete-by-word-001.html editing/deleting/non-smart-delete.html editing/deleting/delete-by-word-002.html editing/inserting/insert-paragraph-04.html editing/deleting/smart-delete-002.html editing/pasteboard/bad-placeholder.html editing/pasteboard/4242293-1.html http/tests/cache/subresource-failover-to-network.html editing/execCommand/query-command-state.html Created attachment 190503 [details]
Patch
(In reply to comment #17) > Created an attachment (id=190503) [details] > Patch This will probably fail tests but it should build in Mac and Qt. Comment on attachment 190503 [details] Patch Attachment 190503 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16789278 Created attachment 190519 [details]
Patch
(In reply to comment #20) > Created an attachment (id=190519) [details] > Patch And another attempt at fixing the build in mac. Comment on attachment 190519 [details] Patch Attachment 190519 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16807280 Created attachment 190528 [details]
Patch
Comment on attachment 190528 [details] Patch Attachment 190528 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16810045 Comment on attachment 190528 [details] Patch Attachment 190528 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16803101 Comment on attachment 190528 [details] Patch Attachment 190528 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16835089 New failing tests: editing/execCommand/remove-formatting-2.html editing/execCommand/toggle-unlink.html editing/execCommand/createLink.html editing/execCommand/button.html editing/pasteboard/8145-2.html editing/inserting/insert-paragraph-03.html editing/pasteboard/5006779.html editing/selection/4932260-2.html editing/inserting/4960120-2.html editing/deleting/list-item-1.html editing/deleting/smart-delete-across-editable-boundaries.html editing/execCommand/remove-format-multiple-elements.html editing/execCommand/toggle-link.html editing/inserting/paragraph-outside-nested-divs.html editing/inserting/insert-before-link-1.html editing/pasteboard/4806874.html editing/deleting/delete-cell-contents.html editing/deleting/smart-editing-disabled.html editing/deleting/non-smart-delete.html editing/execCommand/unlink.html editing/execCommand/indent-pre.html editing/deleting/delete-by-word-001.html editing/execCommand/hilitecolor.html editing/pasteboard/bad-placeholder.html editing/inserting/insert-paragraph-04.html editing/deleting/smart-delete-002.html editing/deleting/delete-by-word-002.html editing/pasteboard/4242293-1.html http/tests/cache/subresource-failover-to-network.html editing/execCommand/query-command-state.html Comment on attachment 190528 [details] Patch Attachment 190528 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16779485 Created attachment 190658 [details]
Patch
Comment on attachment 190658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review > Source/WebCore/platform/text/TextBoundaries.cpp:65 > -int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward) > +int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward, bool skipSpaces) It's not a good idea to modify findNextWordFromIndex like this. Please follow what we did for visualWordPosition. (In reply to comment #29) > (From update of attachment 190658 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review > > > Source/WebCore/platform/text/TextBoundaries.cpp:65 > > -int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward) > > +int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward, bool skipSpaces) > > It's not a good idea to modify findNextWordFromIndex like this. Please follow what we did for visualWordPosition. Thanks for the comments. Not sure I understand though. visualWordPosition() is used from left/rightWordPosition(). For the platforms where the wordbreak is not available, the fallback is going to lead to findNextWordFromIndex() too.. would you mind clarifying what exactly you mean? Comment on attachment 190658 [details] Patch Attachment 190658 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16822318 New failing tests: editing/execCommand/remove-formatting-2.html editing/execCommand/toggle-unlink.html editing/execCommand/createLink.html editing/execCommand/button.html editing/pasteboard/8145-2.html editing/inserting/insert-paragraph-03.html editing/pasteboard/5006779.html editing/selection/4932260-2.html editing/inserting/4960120-2.html editing/deleting/list-item-1.html editing/deleting/smart-delete-across-editable-boundaries.html editing/execCommand/remove-format-multiple-elements.html editing/execCommand/toggle-link.html editing/execCommand/hilitecolor.html editing/pasteboard/copy-text-with-backgroundcolor.html editing/inserting/insert-before-link-1.html editing/pasteboard/4806874.html editing/deleting/delete-cell-contents.html editing/deleting/smart-editing-disabled.html editing/execCommand/unlink.html editing/execCommand/indent-pre.html editing/deleting/delete-by-word-001.html editing/deleting/non-smart-delete.html editing/deleting/delete-by-word-002.html editing/inserting/insert-paragraph-04.html editing/deleting/smart-delete-002.html editing/pasteboard/bad-placeholder.html editing/pasteboard/4242293-1.html http/tests/cache/subresource-failover-to-network.html editing/execCommand/query-command-state.html (In reply to comment #30) > (In reply to comment #29) > > (From update of attachment 190658 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review > > > > > Source/WebCore/platform/text/TextBoundaries.cpp:65 > > > -int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward) > > > +int findNextWordFromIndex(const UChar* chars, int len, int position, bool forward, bool skipSpaces) > > > > It's not a good idea to modify findNextWordFromIndex like this. Please follow what we did for visualWordPosition. > > Thanks for the comments. Not sure I understand though. visualWordPosition() is used from left/rightWordPosition(). For the platforms where the wordbreak is not available, the fallback is going to lead to findNextWordFromIndex() too.. would you mind clarifying what exactly you mean? The technique we used there is to move forward twice and go back. (In reply to comment #32) > (In reply to comment #30) > > (In reply to comment #29) > The technique we used there is to move forward twice and go back. I might be overlooking something but that's not what I read in the code. What I see is that in visualWordPosition() it is iterated until finding the start or end of the next word, depending on whether we're in LTR/RTL and whether it is desired to skip spaces or not. I don't think there's any moving forward twice and then back.. (In reply to comment #3) > (From update of attachment 189549 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review > > r- due to lack of tests and failing tests. Why did this change the results of a test on Linux? Some of the tests break in cr-linux because Chromium's TestRunner uses WebSettings::EditingBehaviorWin in all platforms but Linux. Since this changes the outcome of extending the selection in Windows, many test expectations will need to be updated. (In reply to comment #34) > (In reply to comment #3) > > (From update of attachment 189549 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=189549&action=review > > > > r- due to lack of tests and failing tests. Why did this change the results of a test on Linux? > > Chromium's TestRunner uses WebSettings::EditingBehaviorWin in all platforms but Linux. Excuse my thinko -- in all platforms but Mac. Comment on attachment 190658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review > Source/WebCore/editing/FrameSelection.cpp:609 > + if (directionOfEnclosingBlock() == LTR) { > +#if PLATFORM(MAC) > pos = nextWordPosition(pos); > - else > +#else > + bool skipsSpaceWhenMovingForward = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight(); > + pos = nextWordPosition(pos, skipsSpaceWhenMovingForward); > +#endif I don't want to see all these if-defs here. (In reply to comment #36) > (From update of attachment 190658 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review > > > Source/WebCore/editing/FrameSelection.cpp:609 > > + if (directionOfEnclosingBlock() == LTR) { > > +#if PLATFORM(MAC) > > pos = nextWordPosition(pos); > > - else > > +#else > > + bool skipsSpaceWhenMovingForward = m_frame && m_frame->editor()->behavior().shouldSkipSpaceWhenMovingRight(); > > + pos = nextWordPosition(pos, skipsSpaceWhenMovingForward); > > +#endif > > I don't want to see all these if-defs here. I am not sure how to implement a version of findNextWordFromIndex() for mac that would take into account whether spaces should be skipped or not. The existing method seems to use NSAttributedString attributes. Since Mac doesn't follow the Windows behavior, this doesn't seem absolutely mandatory, but if some dev from the Mac port is kind enough to jump in we might not need the ifdef's. Comment on attachment 190658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review I think that Ryosuke's request is not to implement Windows behavior using NSString APIs, but to make sure that editing behavior is not determined at compile time. We want to test all platforms behavior in regression tests, using a window.internals switch to choose editing behavior in each test. This way, running tests locally would be a better indication that a patch is correct. Since the patch still does not have tests, it's automatically r-. > Source/WTF/wtf/unicode/icu/UnicodeIcu.h:179 > +inline bool isSpace(UChar32 c) > +{ > + return !!u_isWhitespace(c); > +} This is not a good name for this function (isWhitespace is not so great either, but it's OK in ICU context). There are multiple definitions of whitespace used in the Web platform. They may or may not include Unicode spaces, form feeds etc. Thanks for your comments! (In reply to comment #38) > (From update of attachment 190658 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190658&action=review > > I think that Ryosuke's request is not to implement Windows behavior using NSString APIs, but to make sure that editing behavior is not determined at compile time. Editing behavior is not determined at compile-time. I'll explain why there are ifdef's more carefully: the way the next stop is determined, in all platforms, is through a call to the findNextWordFromIndex() method. What my patch is doing is to add a boolean parameter to the signature of this method, so that it looks like this: findNextWordFromIndex(UChar const* buffer, int len, int position, bool forward, bool skipSpaces); This way, the calling methods can decide whether to request from this method to skip spaces when searching for the next word. Calling methods, of course, rely on the run-time property EditingBehavior. As you can see, the editing behavior is never determined at run-time. As to the ifdefs: This method has different implementations: a generic one, one for the Qt port (using QString), and one for the Mac one (using NSString). My patch modifies both the generic implementation and the one for the Qt port. To make the patch complete one would need to modify the Mac implementation as well so that it works as we now need. Admittedly, since I am not familiar with NSString at all I've left this out. I am open to suggestions as to how to complete this patch so that findNextWordFromIndex() in Mac works in the same way and the ifdefs are not necessary. > > We want to test all platforms behavior in regression tests, using a window.internals switch to choose editing behavior in each test. This way, running tests locally would be a better indication that a patch is correct. > > Since the patch still does not have tests, it's automatically r-. I will add tests soon, of course. However, at this point I am mostly concerned in what we're discussing above. > > > Source/WTF/wtf/unicode/icu/UnicodeIcu.h:179 > > +inline bool isSpace(UChar32 c) > > +{ > > + return !!u_isWhitespace(c); > > +} > > This is not a good name for this function (isWhitespace is not so great either, but it's OK in ICU context). > > There are multiple definitions of whitespace used in the Web platform. They may or may not include Unicode spaces, form feeds etc. Do you have any suggestion? Maybe just isWhitespace() or isSpacing()? (In reply to comment #39) > Thanks for your comments! > > Editing behavior is not determined at compile-time. I'll explain why there are ifdef's more carefully: the way the next stop is determined, in all platforms, is through a call to the findNextWordFromIndex() method. What my patch is doing is to add a boolean parameter to the signature of this method, so that it looks like this: > > findNextWordFromIndex(UChar const* buffer, int len, int position, bool forward, bool skipSpaces); > > This way, the calling methods can decide whether to request from this method to skip spaces when searching for the next word. Calling methods, of course, rely on the run-time property EditingBehavior. As you can see, the editing behavior is never determined at run-time. What I've been telling you ever since this this bug is filed is that we shouldn't do this. Instead, call nextWordPosition twice and call previousWordPosition that'll move the caret to the position after the whitespace. This way, new behavior can be implemented and exposed in DRT everywhere. Created attachment 194359 [details]
Patch
Comment on attachment 194359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194359&action=review > Source/WebCore/editing/FrameSelection.cpp:598 > + pos = nextWordPosition(pos); There is a bug here if c is in the middle of the word. In that case pos will be in a previous position and this won't work. I will attach a new patch correcting this. Created attachment 194615 [details]
Patch
Comment on attachment 194615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194615&action=review I would have preferred test changes to be submitted separately on a separate bug so that we can assess the effect of the code change more closely. > Source/WebCore/editing/FrameSelection.cpp:584 > +VisiblePosition FrameSelection::nextWordPositionForPlatform(const VisiblePosition &c) Please don't use one-letter variables like c. > Source/WebCore/editing/FrameSelection.cpp:586 > + VisiblePosition pos = nextWordPosition(c); Please don't use abbreviations like "pos". Also, "position" is a very generic name that doesn't really tell us the semantics. How about "currentPosition"? > Source/WebCore/editing/FrameSelection.cpp:590 > + // In Windows, moving by word moves until the beginning of the > + // next word. We emulate that behavior here by moving twice > + // forward (intermediatePosition) and then once back. I'm not sure if this comment is all that helpful. Given that this is an editing behavior, it's no longer tied to Windows or that it may change in the future. Given that the code already tells us we're moving forward twice and going backwards once, we're probably repeating ourselves needlessly. In WebKit, we try to avoid adding comments like this that merely explains what the code does as much as possible. If we could explain *why* we do it, that's useful. > Source/WebCore/editing/FrameSelection.cpp:592 > + VisiblePosition intermediatePosition = nextWordPosition(pos); "intermediate" is not a descriptive name either. Something like positionAfterCurrentWord would better. In particular, this emphasizes the fact it's not after the whitespace. > Source/WebCore/editing/FrameSelection.cpp:597 > + // When moving back, it is possible that we end up in the > + // start of the current word. In that case, just move until > + // the intermediate position. Instead of adding a long comment line this, we can give the variable a more describe name. > Source/WebCore/editing/FrameSelection.cpp:599 > + VisiblePosition wordStart = previousWordPosition(nextWordPosition(c)); > + if (pos == wordStart) e.g. we can do something like bool movingBackwardsTwiceMovedPositionToStartOfCurrentWord = intermiediatePosition == previousWordPosition(nextWordPosition(c)); > LayoutTests/ChangeLog:8 > + This is a massive review of all tests that use word selection for Massive review? or massive rebaseline? > LayoutTests/ChangeLog:28 > + uncovered a bug in execCommand(), reported now as wkb112240, that Please use a regular URL like http://webkit.org/b/112240. "wkb" is not a thing. > LayoutTests/ChangeLog:32 > + * editing/deleting/smart-editing-disabled-mac-expected.txt: Copied from LayoutTests/editing/deleting/smart-editing-disabled-expected.txt. > + * editing/deleting/smart-editing-disabled-mac.html: Copied from LayoutTests/editing/deleting/smart-editing-disabled.html. These lines are really long. Please line break somewhere. And ditto for the following lines. > LayoutTests/TestExpectations:12 > +# pending bug fix > +webkit.org/b/112240 editing/execCommand/remove-format-multiple-elements-win.html [ Skip ] Please don't skip an existing test on all platforms. r- because of this change. You may add [ Failure ] expectation on some platform where this test fails but it's not acceptable to disable a test on all platforms. If we need to fix a test, then please fix it before submitting a patch on this bug. Comment on attachment 194615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194615&action=review Thanks for the review. A few comments follow. >> Source/WebCore/editing/FrameSelection.cpp:599 >> + if (pos == wordStart) > > e.g. we can do something like > bool movingBackwardsTwiceMovedPositionToStartOfCurrentWord = intermiediatePosition == previousWordPosition(nextWordPosition(c)); It would be movingBackwardsMovedPositionToStasrtOfCurrentWord, as we never move backward twice, only once. >> LayoutTests/ChangeLog:8 >> + This is a massive review of all tests that use word selection for > > Massive review? or massive rebaseline? Some tests have been rebaselined, others fixed differently. >> LayoutTests/TestExpectations:12 >> +webkit.org/b/112240 editing/execCommand/remove-format-multiple-elements-win.html [ Skip ] > > Please don't skip an existing test on all platforms. r- because of this change. > You may add [ Failure ] expectation on some platform where this test fails but it's not acceptable to disable a test on all platforms. > If we need to fix a test, then please fix it before submitting a patch on this bug. It is not an existing test. It is a new test (see the ChangeLog entry fot that test.) It fails because of a completely different bug I have found that is unrelated to this one (bug 112240, already existing and affecting many editor commands). The test fails in all platforms because, as specified by the filename, the test forces a windows editing behavior regardless of the platform where it runs. Does Failure runs the test anyway? I think that would make sense, but as I said this will fail in all platforms until the bug is addressed. Another option is not to add it or to add an -expected file that takes into consideration the failure. Created attachment 195074 [details]
Patch without tests, in order to assess effects of code changes.
This patch has no tests so I don't expect it to be committed. It addresses your comments regarding variable naming and code comments. We can check with this what breaks.
Created attachment 195080 [details]
Patch without tests without format issues.
Comment on attachment 195080 [details] Patch without tests without format issues. Attachment 195080 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17309297 (In reply to comment #45) > >> LayoutTests/TestExpectations:12 > >> +webkit.org/b/112240 editing/execCommand/remove-format-multiple-elements-win.html [ Skip ] > > > > Please don't skip an existing test on all platforms. r- because of this change. > > You may add [ Failure ] expectation on some platform where this test fails but it's not acceptable to disable a test on all platforms. > > If we need to fix a test, then please fix it before submitting a patch on this bug. > > It is not an existing test. It is a new test (see the ChangeLog entry fot that test.) It fails because of a completely different bug I have found that is unrelated to this one (bug 112240, already existing and affecting many editor commands). The test fails in all platforms because, as specified by the filename, the test forces a windows editing behavior regardless of the platform where it runs. No. The test was passing in Windows editing behavior prior to this patch. This patch introduces new behavior and reveals the bug that had previously not encountered on Windows. Unfortunately, that's not acceptable in WebKit. We need to fix the said bug prior to fixing this bug. Comment on attachment 195080 [details] Patch without tests without format issues. Attachment 195080 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17318227 New failing tests: editing/selection/selection-extend-should-not-move-across-caret-on-mac.html fast/repaint/japanese-rl-selection-repaint-in-regions.html editing/execCommand/query-command-state.html Created attachment 195110 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Comment on attachment 195080 [details] Patch without tests without format issues. Attachment 195080 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17112914 New failing tests: editing/execCommand/remove-formatting-2.html editing/inserting/paragraph-separator-01.html editing/execCommand/createLink.html editing/execCommand/button.html editing/pasteboard/8145-2.html editing/inserting/insert-paragraph-03.html editing/pasteboard/5006779.html editing/selection/4932260-2.html editing/inserting/4960120-2.html editing/deleting/list-item-1.html editing/deleting/smart-delete-across-editable-boundaries.html editing/execCommand/remove-format-multiple-elements.html editing/execCommand/toggle-link.html editing/inserting/paragraph-outside-nested-divs.html editing/inserting/insert-before-link-1.html editing/pasteboard/4806874.html editing/deleting/delete-cell-contents.html editing/deleting/smart-editing-disabled.html editing/deleting/non-smart-delete.html editing/execCommand/unlink.html editing/execCommand/indent-pre.html editing/deleting/delete-by-word-001.html editing/execCommand/hilitecolor.html editing/pasteboard/bad-placeholder.html editing/inserting/insert-paragraph-04.html editing/deleting/smart-delete-002.html editing/deleting/delete-by-word-002.html editing/pasteboard/4242293-1.html http/tests/cache/subresource-failover-to-network.html editing/execCommand/query-command-state.html Created attachment 195158 [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment on attachment 195080 [details] Patch without tests without format issues. Attachment 195080 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17221808 New failing tests: editing/selection/selection-extend-should-not-move-across-caret-on-mac.html editing/execCommand/query-command-state.html Created attachment 195230 [details]
Archive of layout-test-results from webkit-ews-04 for mac-future
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-future Platform: Mac OS X 10.8.2
Comment on attachment 195080 [details]
Patch without tests without format issues.
Flipping to get updated test results after the patches to tests that have landed.
Created attachment 196141 [details]
Patch without tests
Reuploaded to get EWS to check it again.
Comment on attachment 196141 [details] Patch without tests Attachment 196141 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17304705 New failing tests: editing/style/remove-underline-across-paragraph-in-bold.html editing/selection/selection-extend-should-not-move-across-caret-on-mac.html editing/execCommand/createLink.html editing/style/remove-underline-from-stylesheet.html editing/execCommand/button.html editing/pasteboard/merge-end-table-2.html editing/pasteboard/page-zoom.html editing/style/remove-underline-across-paragraph.html editing/inserting/insert-before-link-1.html editing/style/remove-underline.html editing/deleting/delete-cell-contents.html editing/undo/replace-text-in-node-preserving-markers-crash.html editing/execCommand/unlink.html editing/execCommand/indent-pre.html editing/style/unbold-in-bold.html editing/style/apply-through-end-of-document.html editing/selection/extend-selection-word.html editing/pasteboard/merge-end-list-2.html editing/deleting/delete-by-word-002.html editing/pasteboard/display-block-on-spans.html editing/style/remove-underline-in-bold.html editing/execCommand/query-command-state.html Created attachment 196331 [details]
Archive of layout-test-results from gce-cr-linux-07 for chromium-linux-x86_64
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
(In reply to comment #49) > (In reply to comment #45) > > It is not an existing test. It is a new test (see the ChangeLog entry fot that test.) It fails because of a completely different bug I have found that is unrelated to this one (bug 112240, already existing and affecting many editor commands). The test fails in all platforms because, as specified by the filename, the test forces a windows editing behavior regardless of the platform where it runs. > > No. The test was passing in Windows editing behavior prior to this patch. This patch introduces new behavior and reveals the bug that had previously not encountered on Windows. Unfortunately, that's not acceptable in WebKit. We need to fix the said bug prior to fixing this bug. Bug 112240 was fixed! I'll prepare a new patch updating the tests now. It might take a bit of time with all the changes in the WK tree post-blink. Created attachment 198583 [details]
Patch
Comment on attachment 198583 [details] Patch Clearing flags on attachment: 198583 Committed r148987: <http://trac.webkit.org/changeset/148987> All reviewed patches have been landed. Closing bug. |