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

Remove Rc<T> usage from Range #8091

Merged
merged 1 commit into from Oct 23, 2015
Merged

Remove Rc<T> usage from Range #8091

merged 1 commit into from Oct 23, 2015

Conversation

@nox
Copy link
Member

nox commented Oct 20, 2015

I initially used this to correctly handle ranges when their respective containers
are mutated, to get weak references of Range objects. I now realise that the weak
references should be handled at a lower level, closer to the JS-managed object.

Review on Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2015

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

@nox nox force-pushed the nox:cleanup-range branch from a4cd9fb to b5b0666 Oct 21, 2015
@nox nox removed the S-needs-rebase label Oct 21, 2015
@eefriedman
Copy link
Contributor

eefriedman commented Oct 22, 2015

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/script/dom/range.rs, line 123 [r1] (raw file):
Are you sure self.start > self.end is correct, as opposed to !(self.start <= self.end)? (I would hope that the WPT tests cover this... but please add a test if they don't.)


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Oct 22, 2015

@eefriedman is reviewing this PR.

@nox
Copy link
Member Author

nox commented Oct 22, 2015

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/script/dom/range.rs, line 123 [r1] (raw file):
Why would one be correct and not the other? I'm not sure I understand.


Comments from the review on Reviewable.io

@eefriedman
Copy link
Contributor

eefriedman commented Oct 22, 2015

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/script/dom/range.rs, line 123 [r1] (raw file):
self.start > self.end is equivalent to self.start.partial_cmp(self.end) == Some(Ordering::Greater). The original code checks for None | Some(Ordering::Greater). The difference is relevant if the two nodes aren't part of the same tree.


Comments from the review on Reviewable.io

@nox
Copy link
Member Author

nox commented Oct 22, 2015

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/script/dom/range.rs, line 123 [r1] (raw file):
Oh, good catch. I'll check that the tests fail and will fix the code accordingly.


Comments from the review on Reviewable.io

@nox nox force-pushed the nox:cleanup-range branch from b5b0666 to f3a8e74 Oct 22, 2015
@nox
Copy link
Member Author

nox commented Oct 22, 2015

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


components/script/dom/range.rs, line 123 [r1] (raw file):
Half the tests in /dom/ranges/Range-set.html were failing without this change, indeed.


Comments from the review on Reviewable.io

@eefriedman
Copy link
Contributor

eefriedman commented Oct 23, 2015

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


components/script/dom/range.rs, line 124 [r3] (raw file):
The comment is confusing. If you think the algorithm needs to be described, say a bit more, like "if the current end is before the start or in another document, set the end to the same point as the start". If you think !(a<=b) needs to be clarified, just change it to call bp_position or partial_cmp directly.


Comments from the review on Reviewable.io

I initially used this to correctly handle ranges when their respective containers
are mutated, to get weak references of Range objects. I now realise that the weak
references should be handled at a lower-level, closer to the JS-managed object.
@nox nox force-pushed the nox:cleanup-range branch from f3a8e74 to a1b15d3 Oct 23, 2015
@nox
Copy link
Member Author

nox commented Oct 23, 2015

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


components/script/dom/range.rs, line 124 [r3] (raw file):
I removed the comment, I already spent too much time on this patch. :)


Comments from the review on Reviewable.io

@eefriedman
Copy link
Contributor

eefriedman commented Oct 23, 2015

OK.

r=me.

@nox
Copy link
Member Author

nox commented Oct 23, 2015

@bors-servo r=eefriedman

Thanks for the review!

@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2015

📌 Commit a1b15d3 has been approved by eefriedman

@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2015

Testing commit a1b15d3 with merge e3bcf7b...

bors-servo added a commit that referenced this pull request Oct 23, 2015
Remove Rc<T> usage from Range

I initially used this to correctly handle ranges when their respective containers
are mutated, to get weak references of Range objects. I now realise that the weak
references should be handled at a lower level, closer to the JS-managed object.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8091)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2015

@bors-servo bors-servo merged commit a1b15d3 into servo:master Oct 23, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nox nox deleted the nox:cleanup-range branch Dec 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.