Skip to content

/ servo

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

Properly propagate changes when range or trees are mutated #8506

Merged
merged 6 commits into from Dec 25, 2015
Merged

Properly propagate changes when range or trees are mutated #8506

merged 6 commits into from Dec 25, 2015

Conversation

@nox
Copy link
Member

nox commented Nov 13, 2015

Does the same thing as #6817, but storing Range instances directly in their start and end containers.

Cc @dzbarsky

Review on Reviewable
@nox nox added the A-content/dom label Nov 13, 2015
@highfive
Copy link

highfive commented Nov 13, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@nox nox force-pushed the nox:finish-ranges branch from c0ce5d4 to 3f157a4 Nov 13, 2015
@dzbarsky
Copy link
Member

dzbarsky commented Nov 13, 2015

Cool, seems reasonable.
@eefriedman
Copy link
Contributor

eefriedman commented Nov 13, 2015

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Nov 13, 2015

⌛ Trying commit 3f157a4 with merge 8325787...
bors-servo added a commit that referenced this pull request Nov 13, 2015
@bors-servo
Auto merge of #8506 - nox:finish-ranges, r=<try>
8325787 
Properly propagate changes when range or trees are mutated

Does the same thing as #6817, but storing Range instances directly in their start and end containers.

Cc @dzbarsky

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

bors-servo commented Nov 13, 2015

💔 Test failed - mac-dev-ref-unit
@eefriedman
Copy link
Contributor

eefriedman commented Nov 13, 2015 

test size_of::size_characterdata ... FAILED
test size_of::size_div ... FAILED
test size_of::size_element ... FAILED
test size_of::size_htmlelement ... FAILED
test size_of::size_node ... FAILED
test size_of::size_span ... FAILED
test size_of::size_text ... FAILED

(Still waiting on the wpt results) Other results look okay.
@nox
Copy link
Member Author

nox commented Nov 14, 2015

Oh right forgot about those, thanks @eefriedman.
@nox nox force-pushed the nox:finish-ranges branch from 3f157a4 to 8e312d4 Nov 15, 2015
@nox nox removed the S-tests-failed label Nov 15, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2015

☔ The latest upstream changes (presumably #8569) made this pull request unmergeable. Please resolve the merge conflicts.
@nox nox force-pushed the nox:finish-ranges branch 2 times, most recently from 9977797 to 52699c4 Nov 26, 2015
@nox
Copy link
Member Author

nox commented Nov 26, 2015

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Nov 26, 2015

⌛ Trying commit 52699c4 with merge 4a264f3...
bors-servo added a commit that referenced this pull request Nov 26, 2015
@bors-servo
Auto merge of #8506 - nox:finish-ranges, r=<try>
4a264f3 
Properly propagate changes when range or trees are mutated

Does the same thing as #6817, but storing Range instances directly in their start and end containers.

Cc @dzbarsky

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

bors-servo commented Nov 26, 2015

💔 Test failed - mac-dev-ref-unit
@nox nox force-pushed the nox:finish-ranges branch from 52699c4 to 066c665 Nov 26, 2015
@nox
Copy link
Member Author

nox commented Nov 26, 2015

@bors-servo try
@dzbarsky
Copy link
Member

dzbarsky commented Dec 23, 2015

(I'm also not a peer so you'll need someone else to take a look at this too)
@nox
Copy link
Member Author

nox commented Dec 23, 2015

Thanks for the review! Replied to your comments. Will fix the typo.

Review status: all files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/bindings/weakref.rs, line 214 [r5] (raw file):
It's not a map if it also clears the vector out of weak references.

components/script/dom/bindings/weakref.rs, line 254 [r5] (raw file):
Hah, thanks. :)

components/script/dom/node.rs, line 1610 [r5] (raw file):
Why wouldn't there be multiple ranges? Let the users be fancy if they want, IMO.

components/script/dom/node.rs, line 2401 [r5] (raw file):
I called it drain because after the call, self's list of ranges is empty. I think "reparent" would be wrong, it's not children that you move, but ranges. Ranges don't have a parent.

components/script/dom/range.rs, line 872 [r5] (raw file):
I would like to avoid that, I don't like mixing signed and unsigned values.

components/script/dom/range.rs, line 913 [r5] (raw file):
There is nothing to reparent here.

components/script/dom/range.rs, line 997 [r5] (raw file):
Because we move only ranges' boundary points (point_node, point_offset) where point_node == self and point_offset > offset.

components/script/dom/range.rs, line 1043 [r5] (raw file):
What is a "split index"? A "split offset"?

components/script/dom/range.rs, line 1059 [r5] (raw file):
set_offset_above would be fine I guess. What's wrong with clamp?

components/script/dom/range.rs, line 1063 [r5] (raw file):
As explained above, it only moves points which are above the given one.

Comments from the review on Reviewable.io
@dzbarsky
Copy link
Member

dzbarsky commented Dec 24, 2015

Review status: all files reviewed at latest revision, 4 unresolved discussions.

components/script/dom/node.rs, line 2401 [r5] (raw file):
Ah, that's fair. I guess drain is a rust thing.

components/script/dom/range.rs, line 1043 [r5] (raw file):
yeah, a split offset. if we could capture why we need to increment in the name that would be nice

components/script/dom/range.rs, line 1059 [r5] (raw file):
I guess clamp is fine, that above part confused me.

components/script/dom/range.rs, line 1063 [r5] (raw file):
I think my discomfort with all these names stems from the fact that the direction isn't really "above", it's "later in tree order". I understand that "above" comes from "index is more than __", so maybe the name can incorporate "exceeding_offset" or "max_offset" or "after_offset" or even just "after"? Perhaps someone else will have a great suggestion, I don't want to block this if neither of us can think of good names.

Comments from the review on Reviewable.io
@nox
Copy link
Member Author

nox commented Dec 24, 2015

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

components/script/dom/range.rs, line 1043 [r5] (raw file):
What does it mean to split an offset?

Comments from the review on Reviewable.io
@dzbarsky
Copy link
Member

dzbarsky commented Dec 24, 2015

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

components/script/dom/range.rs, line 1043 [r5] (raw file):
You're moving the offset because you split the text node at that offset, right? maybe this should be "increment_offset_for_split_text"?

I was actually just thinking, if we inline this function we don't even need to name it and all the steps are together. Can we just call update from Text#split and pass in the closure from there?

Comments from the review on Reviewable.io
@dzbarsky
Copy link
Member

dzbarsky commented Dec 24, 2015

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

components/script/dom/range.rs, line 1043 [r5] (raw file):
Or even a closure that takes in a range and have a wrapper around update that can deal?

Comments from the review on Reviewable.io
@nox
Copy link
Member Author

nox commented Dec 24, 2015

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

components/script/dom/range.rs, line 1043 [r5] (raw file):
Ranges are complex, my intent was to leak as little as possible about their implementations in other modules, that's why I made all these methods on this type.

Comments from the review on Reviewable.io
@dzbarsky
Copy link
Member

dzbarsky commented Dec 24, 2015

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

components/script/dom/range.rs, line 1043 [r5] (raw file):
I'd buy that for move_to_text_child_at and any functions that change the start/end node (Because then we have to change the range arrays, etc._ but this function and functions that only affect the offsets are very straightforward manipulations and follow the spec directly, so they're not really leaking anything.

Comments from the review on Reviewable.io
@nox
Copy link
Member Author

nox commented Dec 24, 2015

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

components/script/dom/range.rs, line 1043 [r5] (raw file):
Well, if the closure takes a range, that means callers can do whatever they want, like putting them in a different vector, so you would have to trust callers more than with these methods.

Comments from the review on Reviewable.io
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 24, 2015

@bors-servo delegate=dzbarsky
@bors-servo
Copy link
Contributor

bors-servo commented Dec 24, 2015

✌️ @dzbarsky can now approve this pull request
@dzbarsky
Copy link
Member

dzbarsky commented Dec 24, 2015

I mean callers can already do whatever they want with the ranges...but whatever. It was just a suggestion. You can land it like this if you really its better.

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Dec 24, 2015

📌 Commit bc697f1 has been approved by dzbarsky
@dzbarsky
Copy link
Member

dzbarsky commented Dec 24, 2015

@bors-servo r- force

will wait for you to say it's ready, looks like you haven't pushed yet.
@nox
Properly propagate changes when range or trees are mutated
Loading status checks…
3c76835
@nox nox force-pushed the nox:finish-ranges branch from bc697f1 to 3c76835 Dec 25, 2015
@nox
Copy link
Member Author

nox commented Dec 25, 2015

I fixed the typo in "underlying".

@bors-servo r=dzbarsky
@bors-servo
Copy link
Contributor

bors-servo commented Dec 25, 2015

📌 Commit 3c76835 has been approved by dzbarsky
@bors-servo
Copy link
Contributor

bors-servo commented Dec 25, 2015

⌛ Testing commit 3c76835 with merge 89ab368...
bors-servo added a commit that referenced this pull request Dec 25, 2015
@bors-servo
Auto merge of #8506 - nox:finish-ranges, r=dzbarsky
Loading status checks…
89ab368 
Properly propagate changes when range or trees are mutated

Does the same thing as #6817, but storing Range instances directly in their start and end containers.

Cc @dzbarsky

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

bors-servo commented Dec 25, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 3c76835 into servo:master Dec 25, 2015
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable Review in progress: 19 of 20 files reviewed, 1 unresolved discussion
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Dec 25, 2015
Merged
@nox nox deleted the nox:finish-ranges branch Jan 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dzbarsky dzbarsky

Labels
A-content/dom S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@nox @highfive @dzbarsky @eefriedman @bors-servo @jdm @Ms2ger
You can’t perform that action at this time.