Bug 111422 - Fix selection handling in a couple of editing tests
Summary: Fix selection handling in a couple of editing tests
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-05 04:16 PST by Claudio Saavedra
Modified: 2013-03-06 00:09 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.62 KB, patch)
2013-03-05 04:17 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (9.65 KB, patch)
2013-03-05 04:28 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (16.65 KB, patch)
2013-03-05 05:47 PST, Claudio Saavedra
rniwa: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2013-03-05 04:16:57 PST
Fix selection handling in pasteboard test
Comment 1 Claudio Saavedra 2013-03-05 04:17:28 PST
Created attachment 191462 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-05 04:24:45 PST
Attachment 191462 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/pasteboard/8145-2.html', u'LayoutTests/platform/chromium-win/editing/pasteboard/8145-2-expected.txt', u'LayoutTests/platform/chromium/editing/pasteboard/8145-2-expected.txt', u'LayoutTests/platform/efl/editing/pasteboard/8145-2-expected.txt', u'LayoutTests/platform/gtk/editing/pasteboard/8145-2-expected.txt', u'LayoutTests/platform/mac/editing/pasteboard/8145-2-expected.txt', u'LayoutTests/platform/qt/editing/pasteboard/8145-2-expected.txt']" exit_code: 1
LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Claudio Saavedra 2013-03-05 04:28:23 PST
Created attachment 191466 [details]
Patch
Comment 4 Claudio Saavedra 2013-03-05 05:47:25 PST
Created attachment 191476 [details]
Patch
Comment 5 Alexey Proskuryakov 2013-03-05 09:51:37 PST
Comment on attachment 191476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191476&action=review

> LayoutTests/ChangeLog:11
> +        for this it's been using move by word and move by character. The
> +        outcome of this might depend on the platform, so use move by
> +        paragraph instead.

Do we have a way to force specific platform behavior in the test? Many editing behaviors are overridable via internals.

It is better to have a way to test all platform behaviors in regression tests, not to avoid things that are platform dependent.
Comment 6 Ryosuke Niwa 2013-03-05 10:26:38 PST
Comment on attachment 191476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191476&action=review

>> LayoutTests/ChangeLog:11
>> +        paragraph instead.
> 
> Do we have a way to force specific platform behavior in the test? Many editing behaviors are overridable via internals.
> 
> It is better to have a way to test all platform behaviors in regression tests, not to avoid things that are platform dependent.

I agree with Alexey. r-.
Comment 7 Claudio Saavedra 2013-03-05 11:56:51 PST
(In reply to comment #5)
> (From update of attachment 191476 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191476&action=review
> 
> > LayoutTests/ChangeLog:11
> > +        for this it's been using move by word and move by character. The
> > +        outcome of this might depend on the platform, so use move by
> > +        paragraph instead.
> 
> Do we have a way to force specific platform behavior in the test? Many editing behaviors are overridable via internals.

Yes, it is possible.

> It is better to have a way to test all platform behaviors in regression tests, not to avoid things that are platform dependent.

I agree with you, just that in this case what it's been tested is not how the selection is changed but what happens afterwards. There are other tests that cover selection manipulation.
Comment 8 Alexey Proskuryakov 2013-03-05 13:00:57 PST
> I agree with you, just that in this case what it's been tested is not how the selection is changed but what happens afterwards.

Yes, it's true that this is not what is being tested here. But wouldn't it be better to make a less invasive change anyway?

Additionally, moving by paragraph is an even less established operation than moving by word, so this patch just makes the test to rely on another assumption.
Comment 9 Claudio Saavedra 2013-03-05 22:51:55 PST
(In reply to comment #8)
> > I agree with you, just that in this case what it's been tested is not how the selection is changed but what happens afterwards.
> 
> Yes, it's true that this is not what is being tested here. But wouldn't it be better to make a less invasive change anyway?
> 
> Additionally, moving by paragraph is an even less established operation than moving by word, so this patch just makes the test to rely on another assumption.

I see. So, if I understand this correctly, instead of fixing this here what I should do is to just force a non-Windows behavior in these tests when landing bug 110487. Is that correct?
Comment 10 Alexey Proskuryakov 2013-03-05 23:33:41 PST
Yes, this sounds right to me.
Comment 11 Claudio Saavedra 2013-03-06 00:09:52 PST
(In reply to comment #10)
> Yes, this sounds right to me.


Thanks for your input. I'll mark this as WONTFIX then.