Bug 55341 – Add lineboundary editing/selection/extend-selection test

• Component: New Bugs
• Status: NEW

[Benjamin (Ben) Kalman]

Add lineboundary editing/selection/extend-selection test

[Benjamin (Ben) Kalman]

Created attachment 84008 [details]
Patch

[Benjamin (Ben) Kalman]

I think this test might actually obsolete editing/selection/extend-selection-home-end

[Ryosuke Niwa]

(In reply to comment #2)
> I think this test might actually obsolete editing/selection/extend-selection-home-end

It definitely does.  Please modify extend-selection-home-end.html instead of adding new test that's a superset of the old test.

[Ryosuke Niwa]

Comment on attachment 84008 [details]
Patch

r- per my comment above.

[Benjamin (Ben) Kalman]

Created attachment 84010 [details]
Patch

[Benjamin (Ben) Kalman]

home-end is badly named, it should be lineboundary.

Consider it an obsoletion rather than a modification.

[Ryosuke Niwa]

Comment on attachment 84010 [details]
Patch

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

(In reply to comment #6)
> home-end is badly named, it should be lineboundary.
> 
> Consider it an obsoletion rather than a modification.

I'm not sure.  home-end signifies the fact we're testing the behavior of home and end keys wheres line-boundary doesn't.  I don't think new name is significantly better.  In general, I'm hesitant to rename tests unless there's a really compelling reason to do so.  For example, flakiness dashboard will lose all test results if we did this rename.

> LayoutTests/ChangeLog:32
> [phone]  Benjamin Kalman  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add lineboundary editing/selection/extend-selection test
> +        https://bugs.webkit.org/show_bug.cgi?id=55341
> +
> +        This is the same as most other extend-selection tests where everything is done in the script test.
> +        A lineboundary version of the test didn't initially exist due to RTL bugs, which have now been fixed.
> +
> +        * editing/selection/extend-selection-lineboundary-expected.txt: Added.
> +        * editing/selection/extend-selection-lineboundary.html: Added.
> +

Nit: old change log.

[Benjamin (Ben) Kalman]

Well, lineboundary is actually a disjoint set from home-end.  Home-end is platform dependent, lineboundary isn't.  So actually they're quite distinct.

[Benjamin (Ben) Kalman]

Created attachment 84011 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 84011 [details]
Patch

(In reply to comment #8)
> Well, lineboundary is actually a disjoint set from home-end.  Home-end is platform dependent, lineboundary isn't.  So actually they're quite distinct.

Ok, but I'm still skeptical of the benefit of this rename.  I'm going to wait until some other reviewer can comment on the benefit of renaming this test.

[Ojan Vafai]

(In reply to comment #10)
> (From update of attachment 84011 [details])
> (In reply to comment #8)
> > Well, lineboundary is actually a disjoint set from home-end.  Home-end is platform dependent, lineboundary isn't.  So actually they're quite distinct.
> 
> Ok, but I'm still skeptical of the benefit of this rename.  I'm going to wait until some other reviewer can comment on the benefit of renaming this test.

home-end is a poor name for this test since it actually is only testing lineboundary behavior. The new name is clearly more accurate. But the rename is orthogonal to cleaning up the test to using runSelectionTestsWithGranularity(createAllNodes(), "lineBoundary").

Ben, would you mind making them in two separate patches? Also, as you noted in person, you'll need to use SVN to do the move properly since "git svn" doesn't handle it correctly.

[Ojan Vafai]

Comment on attachment 84011 [details]
Patch

r- per comment 11.

[Benjamin (Ben) Kalman]

(In reply to comment #8)
> Well, lineboundary is actually a disjoint set from home-end.  Home-end is platform dependent, lineboundary isn't.  So actually they're quite distinct.

I'm wrong about this, sorry.  Extend by lineboundary is platform dependent, though home-end is more platform divergent.

That said, as Ojan mentioned, this test isn't testing home-end but rather lineboundary anyway.