Remove usage of slice_chars in script #6854

SimonSapin · 2015-07-30T18:31:51Z

It’s deprecated in the #6850 rustup.

The first commit changes some behavior which was previously incorrect: the spec says indices in DOM strings are UTF-16 code units, not char code points.

The second commit should not change behavior, unless I made a mistake.

r? @jdm

Ms2ger · 2015-07-30T19:56:24Z

Needs tests

SimonSapin · 2015-07-30T20:34:44Z

I’ll tweak tests/wpt/web-platform-tests/dom/nodes/CharacterData-* to exercise these changes, but I don’t know what to do about textinput.

jdm · 2015-07-30T20:49:16Z

There are unit tests in test/unit/script/textinput.rs - can you add some there?

SimonSapin · 2015-07-30T21:19:46Z

I’ll look into that tomorrow

bors-servo · 2015-07-31T20:00:01Z

☔ The latest upstream changes (presumably #6876) made this pull request unmergeable. Please resolve the merge conflicts.

SimonSapin · 2015-08-01T08:46:09Z

Rebased and added some tests, including a new file marked expected: CRASH because of #6873.

jdm · 2015-08-04T19:23:01Z

So if a text input contains "🌠" with the cursor at the left side, what happens now if you press right and insert or delete a character? Can we write tests for that?

-S-awaiting-review +S-awaiting-answer +S-needs-code-changes

Reviewed 2 of 2 files at r1, 13 of 13 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

^{components/script/textinput.rs, line 260 [r2] (raw file):}
s/adjusting/adjust/

^{components/script/textinput.rs, line 280 [r2] (raw file):}
adjust_selection_for_horizontal_change

^{components/script/textinput.rs, line 292 [r2] (raw file):}
This is a change in behaviour; previously performing a horizonal adjustment without pressing shift while a selection was present would warp the cursor to one end up of the selection, but no further. Do the tests in test/unit/script catch this?

^{components/script/textinput.rs, line 297 [r2] (raw file):}
perform_horizontal_adjustment

Comments from the review on Reviewable.io

bors-servo · 2015-08-07T23:52:20Z

☔ The latest upstream changes (presumably #7073) made this pull request unmergeable. Please resolve the merge conflicts.

SimonSapin · 2015-08-17T16:10:28Z

Added a couple tests for the scenarios described.

Review status: 7 of 12 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

^{components/script/textinput.rs, line 260 [r2] (raw file):}
Done.

^{components/script/textinput.rs, line 280 [r2] (raw file):}
Done.

^{components/script/textinput.rs, line 292 [r2] (raw file):}
Tests still pass with this code change, but I don’t understand how it is a behavior change. Could you explain a bit more?

^{components/script/textinput.rs, line 297 [r2] (raw file):}
Done.

Comments from the review on Reviewable.io

SimonSapin · 2015-08-20T07:46:20Z

Added the tests that I had apparently forgotten to commit.

bors-servo · 2015-08-25T21:13:12Z

💔 Test failed - mac1

jdm · 2015-08-25T21:51:38Z

tests/wpt/metadata/MANIFEST.json

@@ -29106,6 +29106,12 @@
      "shadow-dom/shadow-trees/hosting-multiple-shadow-trees-005.html"
    ],
    "items": {},
+        ],


Rebase error?

bors-servo · 2015-08-27T21:50:39Z

☔ The latest upstream changes (presumably #7416) made this pull request unmergeable. Please resolve the merge conflicts.


        Refactor script::textinput to count UTF-8 bytes rather than code points.


        Fix CharacterDataMethods to count UTF-16 code units, not code points.


        Add tests for CharacterDataMethods with non-BMP code points

SimonSapin · 2015-08-28T10:25:35Z

Rebased and ran mach test-wpt --manifest-update again, but some unrelated tests have been marked as "deleted" in tests/wpt/metadata/MANIFEST.json

r? for the changes in that file.

SimonSapin · 2015-08-28T11:16:01Z

@bors-servo r=jdm+Ms2ger

bors-servo · 2015-08-28T11:16:02Z

📌 Commit dcc8f63 has been approved by jdm+Ms2ger

bors-servo · 2015-08-28T11:16:04Z

⌛ Testing commit dcc8f63 with merge 2ca48ca...


        Auto merge of #6854 - servo:slice_chars, r=jdm+Ms2ger

@jdm

Remove usage of slice_chars in script It’s deprecated in the #6850 rustup. The first commit changes some behavior which was previously incorrect: the spec says indices in DOM strings are UTF-16 code units, not `char` code points. The second commit should not change behavior, unless I made a mistake. r? @jdm  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6854)

SimonSapin · 2015-08-28T11:19:19Z

@bors-servo r=jdm+Ms2ger

bors-servo · 2015-08-28T11:19:19Z

📌 Commit dcc8f63 has been approved by jdm+Ms2ger

bors-servo · 2015-08-28T11:53:07Z

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2, mac3

highfive added the S-awaiting-review label Jul 30, 2015

jdm added the S-needs-tests label Jul 30, 2015

SimonSapin mentioned this pull request Jul 31, 2015

Splitting surrogate pairs in CharacterData DOM APIs #6873

Open

metajack removed the S-awaiting-review label Jul 31, 2015

metajack added the S-needs-rebase label Jul 31, 2015

SimonSapin force-pushed the slice_chars branch from fe75801 to 4daaff4 Aug 1, 2015

highfive added the S-awaiting-review label Aug 1, 2015

metajack assigned jdm Aug 3, 2015

metajack removed S-needs-rebase S-needs-tests labels Aug 3, 2015

jdm added S-awaiting-answer S-needs-code-changes and removed S-awaiting-review labels Aug 4, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Aug 7, 2015

SimonSapin force-pushed the slice_chars branch from 4daaff4 to 817aa05 Aug 17, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 17, 2015

SimonSapin removed S-awaiting-answer S-needs-rebase labels Aug 17, 2015

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 25, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 25, 2015

jdm reviewed Aug 25, 2015
View changes

tests/wpt/metadata/MANIFEST.json Outdated

@@ -29106,6 +29106,12 @@

"shadow-dom/shadow-trees/hosting-multiple-shadow-trees-005.html"

],

"items": {},

],

This comment has been minimized.

Sign in to view

Copy link

jdm Aug 25, 2015
Member

Rebase error?

nox added the S-needs-code-changes label Aug 27, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Aug 27, 2015

SimonSapin added 3 commits Jul 30, 2015

Refactor script::textinput to count UTF-8 bytes rather than code points.

95a252a

Fix CharacterDataMethods to count UTF-16 code units, not code points.

006dd10

Add tests for CharacterDataMethods with non-BMP code points

Loading status checks…

dcc8f63

SimonSapin force-pushed the slice_chars branch from 758cb33 to dcc8f63 Aug 28, 2015

highfive added S-awaiting-review and removed S-tests-failed S-needs-code-changes S-needs-rebase labels Aug 28, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 28, 2015

bors-servo merged commit dcc8f63 into master Aug 28, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

SimonSapin deleted the slice_chars branch Aug 28, 2015

servo / servo

Remove usage of slice_chars in script #6854

Remove usage of slice_chars in script #6854

SimonSapin commented Jul 30, 2015

Ms2ger commented Jul 30, 2015

SimonSapin commented Jul 30, 2015

jdm commented Jul 30, 2015

SimonSapin commented Jul 30, 2015

bors-servo commented Jul 31, 2015

SimonSapin commented Aug 1, 2015

jdm commented Aug 4, 2015

bors-servo commented Aug 7, 2015

SimonSapin commented Aug 17, 2015

SimonSapin commented Aug 20, 2015

bors-servo commented Aug 25, 2015

This comment has been minimized.

bors-servo commented Aug 27, 2015

SimonSapin commented Aug 28, 2015

SimonSapin commented Aug 28, 2015

bors-servo commented Aug 28, 2015

bors-servo commented Aug 28, 2015

SimonSapin commented Aug 28, 2015

bors-servo commented Aug 28, 2015

bors-servo commented Aug 28, 2015

servo / servo

Join GitHub today

Remove usage of slice_chars in script #6854

Remove usage of slice_chars in script #6854

Conversation

SimonSapin commented Jul 30, 2015

Ms2ger commented Jul 30, 2015

SimonSapin commented Jul 30, 2015

jdm commented Jul 30, 2015

SimonSapin commented Jul 30, 2015

bors-servo commented Jul 31, 2015

SimonSapin commented Aug 1, 2015

jdm commented Aug 4, 2015

bors-servo commented Aug 7, 2015

SimonSapin commented Aug 17, 2015

SimonSapin commented Aug 20, 2015

bors-servo commented Aug 25, 2015

This comment has been minimized.

jdm Aug 25, 2015

bors-servo commented Aug 27, 2015

SimonSapin commented Aug 28, 2015

SimonSapin commented Aug 28, 2015

bors-servo commented Aug 28, 2015

bors-servo commented Aug 28, 2015

SimonSapin commented Aug 28, 2015

bors-servo commented Aug 28, 2015

bors-servo commented Aug 28, 2015