Bug 111422 – Fix selection handling in a couple of editing tests

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Claudio Saavedra]

Fix selection handling in pasteboard test

[Claudio Saavedra]

Created attachment 191462 [details]
Patch

[WebKit Review Bot]

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.

[Claudio Saavedra]

Created attachment 191466 [details]
Patch

[Claudio Saavedra]

Created attachment 191476 [details]
Patch

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

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-.

[Claudio Saavedra]

(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.

[Alexey Proskuryakov]

> 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.

[Claudio Saavedra]

(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?

[Alexey Proskuryakov]

Yes, this sounds right to me.

[Claudio Saavedra]

(In reply to comment #10)
> Yes, this sounds right to me.


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