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

Use DOMRefCell in script crate #3737

Merged
merged 14 commits into from Oct 22, 2014
Merged

Use DOMRefCell in script crate #3737

merged 14 commits into from Oct 22, 2014

Conversation

@tetsuharuohzeki
Copy link
Member

tetsuharuohzeki commented Oct 20, 2014

#3050

Altough LayoutDataRef is touched from layout, we don't use DOMRefCell in it becasuse
it's expected to manipulate in layout task.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Oct 20, 2014

Critic review: https://critic.hoppipolla.co.uk/r/2914

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@tetsuharuohzeki
Copy link
Member Author

tetsuharuohzeki commented Oct 20, 2014

I have not converted yet XMLHttpRequest, URLSearchParams, ServoHTMLParser, Page, ScriptTask, and TimerManager. I thought that it's might be very rare cases that they're touched from layout task, thus it's should be checked in code review process.

Should we use DOMRefCell in them for checking unsafe borrowing at all points?

@jdm
Copy link
Member

jdm commented Oct 20, 2014

@saneyuki I think we should. In the future I'd like to add a lint to disallow RefCell in the script crate.

@tetsuharuohzeki
Copy link
Member Author

tetsuharuohzeki commented Oct 20, 2014

@jdm ok. I'll push their patches.

@tetsuharuohzeki
Copy link
Member Author

tetsuharuohzeki commented Oct 20, 2014

After merge this, I'll open the pull request which introduce dynamic checking about borrowing.

@jdm
Copy link
Member

jdm commented Oct 20, 2014

Thanks!

bors-servo pushed a commit that referenced this pull request Oct 21, 2014
#3050

Altough LayoutDataRef is touched from layout, we don't use DOMRefCell in it becasuse
it's expected to manipulate in layout task.
bors-servo pushed a commit that referenced this pull request Oct 21, 2014
#3050

Altough LayoutDataRef is touched from layout, we don't use DOMRefCell in it becasuse
it's expected to manipulate in layout task.
@tetsuharuohzeki tetsuharuohzeki force-pushed the tetsuharuohzeki:cell branch from f288373 to f1c840d Oct 22, 2014
@tetsuharuohzeki
Copy link
Member Author

tetsuharuohzeki commented Oct 22, 2014

I feel the last bors build error is similar to the one which we encounter in #3742.

Perhaps the previous base commit might have some latent issues, so I rebased this changeset on a latest master now.

@jdm

This comment has been minimized.

Copy link

jdm commented on f1c840d Oct 22, 2014

r+

This comment has been minimized.

Copy link

jdm replied Oct 22, 2014

@bors: retry

This comment has been minimized.

Copy link

jdm replied Oct 22, 2014

@bors: retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented on f1c840d Oct 22, 2014

saw approval from jdm
at tetsuharuohzeki@f1c840d

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 22, 2014

merging saneyuki/servo/cell = f1c840d into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 22, 2014

saneyuki/servo/cell = f1c840d merged ok, testing candidate = 8cbc40a

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 22, 2014

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 22, 2014

saw approval from jdm
at tetsuharuohzeki@f1c840d

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 22, 2014

saw approval from jdm
at tetsuharuohzeki@f1c840d

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 22, 2014

merging saneyuki/servo/cell = f1c840d into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 22, 2014

saneyuki/servo/cell = f1c840d merged ok, testing candidate = f5e8df9

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 22, 2014

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 22, 2014

fast-forwarding master to auto = f5e8df9

bors-servo pushed a commit that referenced this pull request Oct 22, 2014
#3050

Altough LayoutDataRef is touched from layout, we don't use DOMRefCell in it becasuse
it's expected to manipulate in layout task.
@tetsuharuohzeki
Copy link
Member Author

tetsuharuohzeki commented Oct 22, 2014

In this pull request, I replaced almost RefCell<T> to DOMRefCell<T>, but I didn't add to use DOMRefCell<T>.borrow_for_layout(). So it will use only parts of RefCell<T> embed in the new type, I think it will not cause any side effects...

@jdm
Copy link
Member

jdm commented Oct 22, 2014

Yes, I don't believe any of the members being changed should be in use by the layout task.

bors-servo pushed a commit that referenced this pull request Oct 22, 2014
#3050

Altough LayoutDataRef is touched from layout, we don't use DOMRefCell in it becasuse
it's expected to manipulate in layout task.
@tetsuharuohzeki
Copy link
Member Author

tetsuharuohzeki commented Oct 22, 2014

Can only reviewers command to build to bors?

@jdm
Copy link
Member

jdm commented Oct 22, 2014

Yeah :/

@tetsuharuohzeki
Copy link
Member Author

tetsuharuohzeki commented Oct 22, 2014

😢

bors-servo pushed a commit that referenced this pull request Oct 22, 2014
#3050

Altough LayoutDataRef is touched from layout, we don't use DOMRefCell in it becasuse
it's expected to manipulate in layout task.
@bors-servo bors-servo closed this Oct 22, 2014
@bors-servo bors-servo merged commit f1c840d into servo:master Oct 22, 2014
1 check passed
1 check passed
default all tests passed
@tetsuharuohzeki tetsuharuohzeki deleted the tetsuharuohzeki:cell branch Oct 22, 2014
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

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