Once bug 110487 is resolved, extending selection forward will yield different results depending on the platform. The test editing/execCommand/toggle-unlink.html currently doesn't support this, as the expected results are hardcoded into the script itself, redundantly.
Created attachment 191235 [details] Patch
Comment on attachment 191235 [details] Patch The right fix is to make the behavior dependent on editing behavior and set editing behavior to mac in this test.
Comment on attachment 191235 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191235&action=review > LayoutTests/ChangeLog:11 > + (testSingleToggle): Just output the result of running the test and > + let the test infrastructure check whether the expectations are met > + or not. We should not, then, write out the word PASS when the test may have failed.
(In reply to comment #2) > (From update of attachment 191235 [details]) > The right fix is to make the behavior dependent on editing behavior and set editing behavior to mac in this test. Wouldn't setting the editing behavior to Mac in this test be counter-productive? That would mean that, in this particular test, only the codepaths related to Mac would be tested, in all platforms. If the feature breaks for other platforms this test wouldn't catch it. How can that be better than having separate expectations for platforms that behave differently?
(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 191235 [details] [details]) > > The right fix is to make the behavior dependent on editing behavior and set editing behavior to mac in this test. > > Wouldn't setting the editing behavior to Mac in this test be counter-productive? That would mean that, in this particular test, only the codepaths related to Mac would be tested, in all platforms. If the feature breaks for other platforms this test wouldn't catch it. How can that be better than having separate expectations for platforms that behave differently? You'll then add a new version of the test that tests Windows behavior.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #2) > > > (From update of attachment 191235 [details] [details] [details]) > > > The right fix is to make the behavior dependent on editing behavior and set editing behavior to mac in this test. > > > > Wouldn't setting the editing behavior to Mac in this test be counter-productive? That would mean that, in this particular test, only the codepaths related to Mac would be tested, in all platforms. If the feature breaks for other platforms this test wouldn't catch it. How can that be better than having separate expectations for platforms that behave differently? > > You'll then add a new version of the test that tests Windows behavior. This seems absolutely unnecessary to me. This is what different expectations per platform are for. The test shouldn't be hardcoding the expected outcome in its code. There are -expected.txt files that are meant to be used for that.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > > You'll then add a new version of the test that tests Windows behavior. > > This seems absolutely unnecessary to me. This is what different expectations per platform are for. The test shouldn't be hardcoding the expected outcome in its code. There are -expected.txt files that are meant to be used for that. That's what editing behaviors are for. We used to have all these platform-specific behaviors and it was an absolute nightmare to debug them because you needed access to that platform/port.
Please grep setEditingBehaviors in LayoutTests/editing and see how we're testing various editing features.
(In reply to comment #8) > Please grep setEditingBehaviors in LayoutTests/editing and see how we're testing various editing features. I see. Then I'll add the suffix -mac.html to this test, set edditing behavior to Mac as you suggested, and add a new test for windows when landing bug 110487. Does that sound OK for you?
Created attachment 191456 [details] Patch
Comment on attachment 191456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191456&action=review > LayoutTests/ChangeLog:14 > + * editing/execCommand/script-tests/toggle-unlink-mac.js: Renamed > + from > + LayoutTests/editing/execCommand/script-tests/toggle-unlink.js. > + Also set editing behaviour to Mac. Why was this file not svn mv'ed?
(In reply to comment #11) > (From update of attachment 191456 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191456&action=review > > > LayoutTests/ChangeLog:14 > > + * editing/execCommand/script-tests/toggle-unlink-mac.js: Renamed > > + from > > + LayoutTests/editing/execCommand/script-tests/toggle-unlink.js. > > + Also set editing behaviour to Mac. > > Why was this file not svn mv'ed? It is, to toggle-unlink-mac.js. The ChangeLog word wrapping might be a bit confusing here.
Comment on attachment 191456 [details] Patch Attachment 191456 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17013074 New failing tests: editing/selection/selection-invalid-offset.html
(In reply to comment #13) > (From update of attachment 191456 [details]) > Attachment 191456 [details] did not pass mac-ews (mac): > Output: http://webkit-commit-queue.appspot.com/results/17013074 > > New failing tests: > editing/selection/selection-invalid-offset.html Failing test is unrelated to the patch as far as I can tell.. is it safe to commit?
Comment on attachment 191456 [details] Patch Clearing flags on attachment: 191456 Committed r144883: <http://trac.webkit.org/changeset/144883>
All reviewed patches have been landed. Closing bug.