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

Make Document store Ranges, and update ranges in Node subclasses #6817

Closed
wants to merge 1 commit into from

Conversation

@dzbarsky
Copy link
Member

dzbarsky commented Jul 29, 2015

Review on Reviewable

@dzbarsky
Copy link
Member Author

dzbarsky commented Jul 29, 2015

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


components/script/dom/range.rs, line 861 [r1] (raw file):
I think I had to disable these because the node algorithms like to set ranges to values that are currently invalid, but will become valid after the end of the algorithm. Thoughts on just removing these lines?


Comments from the review on Reviewable.io

@dzbarsky dzbarsky force-pushed the dzbarsky:attempted_range_merge branch from 48a75fe to ba99c97 Aug 18, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2015

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

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 12, 2015

r? @nox

@highfive highfive assigned nox and unassigned Ms2ger Nov 12, 2015
@nox
Copy link
Member

nox commented Nov 12, 2015

I'm doing this differently already, with weak references and a vector per Node. Which way should we go?

@dzbarsky
Copy link
Member Author

dzbarsky commented Nov 12, 2015

This is what we talked about before, right? It's still not clear to me what we gain by storing ranged on a per-node basis.

For the Range updating steps in this PR, we need to be able to look up Ranges that start or end with a given node, as well as Ranges whose start or end node's descendant is a given node. This seems pretty complex to implement when Ranges are stored on various nodes. But if you have ideas for how to do this, let me know!

If we can't figure this out, we should do weak refs and vector per Document, so we can actually follow the spec and do the right thing

@nox
Copy link
Member

nox commented Nov 13, 2015

@dzbarsky I introduce an unbinding context which contains the necessary information about the inclusive ancestor that was removed (its index, more or less). Still, I'm going to read through the code in this PR.

bors-servo added a commit that referenced this pull request Nov 13, 2015
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 -->
@nox
Copy link
Member

nox commented Nov 17, 2015

@dzbarsky Did you have the time to look at my other branch? Which way do you think we should go?

@dzbarsky
Copy link
Member Author

dzbarsky commented Nov 17, 2015

@nox yeah I looked briefly, let's do it your way. It ended up being nicer than I expected. Not sure if I'll have time to review it in detail in the near future though.

@dzbarsky dzbarsky closed this Nov 17, 2015
bors-servo added a commit that referenced this pull request Nov 26, 2015
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 added a commit that referenced this pull request Nov 26, 2015
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 added a commit that referenced this pull request Nov 26, 2015
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 added a commit that referenced this pull request Nov 26, 2015
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 added a commit that referenced this pull request Nov 26, 2015
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 added a commit that referenced this pull request Nov 28, 2015
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 added a commit that referenced this pull request Dec 25, 2015
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 -->
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.