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

Dirty parent when removing a child node. #8139

Closed

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Oct 21, 2015

In Node::remove_child, dirty both the child and the parent. Fixes #8135

Review on Reviewable

@highfive
Copy link

highfive commented Oct 21, 2015

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

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 22, 2015

@Manishearth
Copy link
Member

Manishearth commented Oct 22, 2015

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


components/script/dom/node.rs, line 337 [r1] (raw file):
Minor nit: The owner_docs of the two will be the same, and self.owner_doc() roots, so we should use the local variable to avoid rooting twice


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Manishearth commented Oct 22, 2015

r=me with that nit

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 22, 2015

r? @pcwalton
I have no idea how the dirtying stuff works.

@highfive highfive assigned pcwalton and unassigned Ms2ger Oct 22, 2015
@asajeffrey
Copy link
Member Author

asajeffrey commented Oct 22, 2015

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


components/script/dom/node.rs, line 337 [r1] (raw file):
I rooted twice in case we're in the middle of a DOM mutation that's caused the parent and child to be in different document. AFAICT there's no such case at the moment, so this may be over-cautious future-proofing.


Comments from the review on Reviewable.io

@pcwalton
Copy link
Contributor

pcwalton commented Nov 2, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 2, 2015

📌 Commit c1ab8a0 has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Nov 2, 2015

Testing commit c1ab8a0 with merge 23a0f6c...

bors-servo added a commit that referenced this pull request Nov 2, 2015
Dirty parent when removing a child node.

In Node::remove_child, dirty both the child and the parent. Fixes #8135

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8139)
<!-- Reviewable:end -->
@frewsxcv frewsxcv closed this Nov 3, 2015
@frewsxcv frewsxcv reopened this Nov 3, 2015
@frewsxcv frewsxcv closed this Nov 3, 2015
@frewsxcv frewsxcv reopened this Nov 3, 2015
@frewsxcv
Copy link
Member

frewsxcv commented Nov 3, 2015

Sorry for the reopen noise. For whatever the reason, the builders keep getting stuck on this PR and for the sake of keeping them working on other things, reopening this allows them to do so

@frewsxcv frewsxcv closed this Nov 3, 2015
@frewsxcv frewsxcv reopened this Nov 3, 2015
@frewsxcv frewsxcv closed this Nov 3, 2015
@frewsxcv frewsxcv reopened this Nov 3, 2015
@eefriedman
Copy link
Contributor

eefriedman commented Nov 3, 2015

@bors-servo r-

If there's something wrong with this PR, we should just take it out of the queue.

@frewsxcv
Copy link
Member

frewsxcv commented Nov 3, 2015

Easier said than done :-/

http://build.servo.org/homu/queue/servo

@frewsxcv
Copy link
Member

frewsxcv commented Nov 3, 2015

@bors-servo r- force

@frewsxcv frewsxcv closed this Nov 3, 2015
@frewsxcv frewsxcv reopened this Nov 3, 2015
@Ms2ger Ms2ger closed this Nov 3, 2015
@Ms2ger Ms2ger reopened this Nov 3, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 3, 2015

@bors-servo r- force

@Ms2ger Ms2ger closed this Nov 3, 2015
@Manishearth
Copy link
Member

Manishearth commented Nov 3, 2015

@barosl is this a homu bug?

@barosl
Copy link

barosl commented Nov 3, 2015

Uh, I'm not sure. Did Homu refuse to remove this PR from the queue? That's quite odd. Were there any exceptions raised?

@Manishearth
Copy link
Member

Manishearth commented Nov 3, 2015

Yes, and no exceptions. This isn't the first time it's happened either. Not sure what the reason is though.

@barosl
Copy link

barosl commented Nov 3, 2015

It seems that the build results for the linux-rel, mac-rel-css, mac-rel-wpt builders are somehow still regarded as "running", even though they are not shown in the builder status. I guess this confused Homu? Considering all the other builders were OK at the time, there might have been some temporal error on the release mode builders.

But it is still strange that r- didn't remove the PR from the queue. Could we try that again? Close #8315, reopen #8139, and r- this. Let's see if the error happens again. (Sorry for the noise!)

@Manishearth
Copy link
Member

Manishearth commented Nov 3, 2015

I think this only occurs when there's a running build and we r- it

@barosl
Copy link

barosl commented Nov 3, 2015

By "running build", do you mean the situation when r-ing a PR whose status is pending?

@Manishearth
Copy link
Member

Manishearth commented Nov 3, 2015

Yes.

Actually, I think in this case the build wasn't running. Not sure.

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

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