Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISSUE-20455: introduce stronger types for textinput indexing #23272

Merged
merged 2 commits into from May 23, 2019

Conversation

Projects
None yet
6 participants
@tdelacour
Copy link
Contributor

commented Apr 26, 2019

Added two new types:

  • ByteOffset
  • UTF16CodeUnitOffset

I've replaced any instance of usize that would be better represented by one or the other of these. I also updated any downstream code, including the unit tests for textinput.rs. Along the way, I tried to add or edit comments to better reflect my understanding of this file - happy to revisit if I have misrepresented anything.

I did not end up finding any places where types were very obviously being mixed, as the issue description suggested I should do... LMK if I should re-audit the file for that (might need a bit of guidance)!


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #20455 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 26, 2019

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon.

@highfive

This comment has been minimized.

Copy link

commented Apr 26, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/htmltextareaelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/textcontrol.rs, components/script/textinput.rs
  • @KiChjang: components/script/dom/htmltextareaelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/textcontrol.rs, components/script/textinput.rs
@@ -137,28 +232,30 @@ pub const CMD_OR_CONTROL: Modifiers = Modifiers::META;
#[cfg(not(target_os = "macos"))]
pub const CMD_OR_CONTROL: Modifiers = Modifiers::CONTROL;

// FIXME this function does not behave as described (if given string has fewer than n

This comment has been minimized.

Copy link
@tdelacour

tdelacour Apr 26, 2019

Author Contributor

happy to change this as a drive-by. Wasn't sure if I should be affected end behavior for this issue or not.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 14, 2019

Member

Based on looking at code around where this function is called it looks like the documented behavior is the desirable one. So the code should change, not the doc. Please fix it in an additional commit. (Avoid modifying the existing commit, to facilitate review.)

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 16, 2019

Member

Sorry I wasn’t clear by commenting about this here rather than at the top level. Avoiding to modify the existing commit was intended for any change after the first review pass, not just this one.

This comment has been minimized.

Copy link
@tdelacour

tdelacour May 16, 2019

Author Contributor

So I should revert the fix for now and add it back in once this commit has been approved? Or maybe an entirely different PR?

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 22, 2019

Member

On closer look your FIXME comment is wrong. This function did and does behave as documented, it returns text.len() if there are fewer than n chars. If iter yields fewer than n items, iter.take(n) acts the same as iter and .last() still gives the last of those items.

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@SimonSapin Review ping!

@tdelacour tdelacour force-pushed the tdelacour:ISSUE-20455 branch from daf4029 to 5341a86 May 14, 2019

@tdelacour

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

merged master to keep things up to date

@jdm

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@SimonSapin Review ping!

@SimonSapin
Copy link
Member

left a comment

Sorry for the delay!

Show resolved Hide resolved components/script/textinput.rs Outdated
Show resolved Hide resolved components/script/textinput.rs Outdated
impl SubAssign for ByteOffset {
fn sub_assign(&mut self, other: ByteOffset) {
if *self > other {
*self = ByteOffset(self.0 + other.0)

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 14, 2019

Member

This should be a minus sign, right?

This comment has been minimized.

Copy link
@tdelacour

tdelacour May 15, 2019

Author Contributor

erm... oops

Show resolved Hide resolved components/script/textinput.rs Outdated
Show resolved Hide resolved components/script/textinput.rs Outdated
Show resolved Hide resolved components/script/textinput.rs Outdated
Show resolved Hide resolved components/script/textinput.rs Outdated
Show resolved Hide resolved components/script/textinput.rs Outdated
@@ -961,9 +1092,9 @@ impl<T: ClipboardProvider> TextInput<T> {
}

pub fn set_selection_range(&mut self, start: u32, end: u32, direction: SelectionDirection) {

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 14, 2019

Member

Should this method take ByteOffset parameters instead of u32?

This comment has been minimized.

Copy link
@tdelacour

tdelacour May 15, 2019

Author Contributor

I did consider this, but was unsure to what extent I should mess with upstream callers. Looks like the only immediate caller is here: https://github.com/tdelacour/servo/blob/master/components/script/dom/textcontrol.rs#L207. Would it be reasonable to just update this call to wrap start / end in UTF8Bytes? Going farther upstream is starts to get a bit messy, but I don't mind threading the type in higher up if you think that would be better in the long run!

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 15, 2019

Member

Ok. Let’s land the changes already here first, then if you’re interested you could work on another PR.

Show resolved Hide resolved components/script/textinput.rs
@SimonSapin

This comment has been minimized.

Copy link
Member

commented May 14, 2019

In case you also want to look into that, it looks like a method like this would be useful in a number of places in this PR:

trait StrExt {
    fn utf8_len(&self) -> ByteOffset;
}
impl StrExt for str {
    fn utf8_len(&self) -> ByteOffset { ByteOffset(self.len()) }
}
@tdelacour

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

ok @SimonSapin, ready for another look!

edit: gah, forgot to do test-tidy, etc. Will fix later today.

@tdelacour tdelacour force-pushed the tdelacour:ISSUE-20455 branch from 2aea36a to 14c8bbb May 16, 2019

@SimonSapin

This comment has been minimized.

Copy link
Member

commented May 17, 2019

So I should revert the fix for now and add it back in once this commit has been approved? Or maybe an entirely different PR?

No. Now that an amended commit has been pushed, as far as I know it can’t easily be undone.

What I meant is that, when a reviewer asks for changes one a large PR, it’s preferable to avoid using git rebase, git commit --amend, or any other command that changes the hash of commits that were already pushed (and cause --force to be required on the next push). Instead, edit the files and create new commits on top of the existing ones. That way, the reviewer can look at the new changes in isolation from those that have already been reviewed. After the review is approved, rebase or squash as desired.

But don’t worry about it for this PR, I’ll manage :)

@tdelacour

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@SimonSapin ohhhhh, got it! Still learning about the workflow, clearly. Making a big mental note for next time - sorry about that!

@SimonSapin
Copy link
Member

left a comment

I took the liberty of pushing to your branch directly for the last couple of changes.


pub fn saturating_sub_assign(&mut self, other: UTF8Bytes) {
if *self > other {
*self = UTF8Bytes(self.0 + other.0)

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 22, 2019

Member

This is still a plus sign instead of minus. But since there’s only one caller, maybe this method can be removed and the caller use index = index.saturating_sub(…).

@@ -137,28 +232,30 @@ pub const CMD_OR_CONTROL: Modifiers = Modifiers::META;
#[cfg(not(target_os = "macos"))]
pub const CMD_OR_CONTROL: Modifiers = Modifiers::CONTROL;

// FIXME this function does not behave as described (if given string has fewer than n

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 22, 2019

Member

On closer look your FIXME comment is wrong. This function did and does behave as documented, it returns text.len() if there are fewer than n chars. If iter yields fewer than n items, iter.take(n) acts the same as iter and .last() still gives the last of those items.

@SimonSapin

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Thanks!

@bors-servo r+

@tdelacour

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

OK thanks for the help!

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

📌 Commit 5d3fc37 has been approved by SimonSapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

⌛️ Testing commit 5d3fc37 with merge 256c2bf...

@CYBAI

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

💣 Failed to start rebuilding: Unknown error

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

⌛️ Testing commit 5d3fc37 with merge 3345737...

bors-servo added a commit that referenced this pull request May 23, 2019

Auto merge of #23272 - tdelacour:ISSUE-20455, r=SimonSapin
ISSUE-20455: introduce stronger types for textinput indexing

<!-- Please describe your changes on the following line: -->
Added two new types:
- ByteOffset
- UTF16CodeUnitOffset

I've replaced any instance of `usize` that would be better represented by one or the other of these. I also updated any downstream code, including the unit tests for `textinput.rs`. Along the way, I tried to add or edit comments to better reflect my understanding of this file - happy to revisit if I have misrepresented anything.

I did not end up finding any places where types were very obviously being mixed, as the issue description suggested I should do... LMK if I should re-audit the file for that (might need a bit of guidance)!

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20455 (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23272)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

💔 Test failed - status-taskcluster

@jdm

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

💣 Failed to start rebuilding: Unknown error

bors-servo added a commit that referenced this pull request May 23, 2019

Auto merge of #23272 - tdelacour:ISSUE-20455, r=SimonSapin
ISSUE-20455: introduce stronger types for textinput indexing

<!-- Please describe your changes on the following line: -->
Added two new types:
- ByteOffset
- UTF16CodeUnitOffset

I've replaced any instance of `usize` that would be better represented by one or the other of these. I also updated any downstream code, including the unit tests for `textinput.rs`. Along the way, I tried to add or edit comments to better reflect my understanding of this file - happy to revisit if I have misrepresented anything.

I did not end up finding any places where types were very obviously being mixed, as the issue description suggested I should do... LMK if I should re-audit the file for that (might need a bit of guidance)!

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20455 (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23272)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

⌛️ Testing commit 5d3fc37 with merge c4d8693...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

💔 Test failed - status-taskcluster

@jdm

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

💣 Failed to start rebuilding: Unknown error

bors-servo added a commit that referenced this pull request May 23, 2019

Auto merge of #23272 - tdelacour:ISSUE-20455, r=SimonSapin
ISSUE-20455: introduce stronger types for textinput indexing

<!-- Please describe your changes on the following line: -->
Added two new types:
- ByteOffset
- UTF16CodeUnitOffset

I've replaced any instance of `usize` that would be better represented by one or the other of these. I also updated any downstream code, including the unit tests for `textinput.rs`. Along the way, I tried to add or edit comments to better reflect my understanding of this file - happy to revisit if I have misrepresented anything.

I did not end up finding any places where types were very obviously being mixed, as the issue description suggested I should do... LMK if I should re-audit the file for that (might need a bit of guidance)!

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20455 (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23272)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

⌛️ Testing commit 5d3fc37 with merge f24f517...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

☀️ Test successful - arm64, linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: SimonSapin
Pushing f24f517 to master...

@bors-servo bors-servo merged commit 5d3fc37 into servo:master May 23, 2019

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@tdelacour tdelacour deleted the tdelacour:ISSUE-20455 branch May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.