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 same-origin navigations use the same script task #5239

Closed
jdm opened this issue Mar 17, 2015 · 11 comments
Closed

Make same-origin navigations use the same script task #5239

jdm opened this issue Mar 17, 2015 · 11 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Mar 17, 2015

This yields better resource conservation, and is important for iframe navigation spec-compliance in particular.

Blocks #5236.

@jdm
Copy link
Member Author

@jdm jdm commented Mar 17, 2015

Relevant code: Constellation::handle_load_url_msg.
Currently we always create a new script task (see the None argument to new_pipeline). We want to use the same heuristic as handle_script_loaded_url_in_iframe.

@jdm jdm added E-less easy and removed E-easy labels Mar 17, 2015
@jdm
Copy link
Member Author

@jdm jdm commented Mar 17, 2015

Oh, this isn't as easy as I suspected at first. This task basically involves adapting the AttachLayout message that the constellation sends to script - currently it's only used for a new iframe being added to a page. We want the constellation/script task to support the following cases:

  • adding a new iframe
  • replacing an existing iframe's contents
  • replacing a top-level document
@benschulz
Copy link
Contributor

@benschulz benschulz commented Aug 30, 2015

I'd like to work on this.

@jdm
Copy link
Member Author

@jdm jdm commented Aug 30, 2015

Please do :)

@jdm jdm added the C-assigned label Aug 30, 2015
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Dec 15, 2015

@benschulz Ping

@benschulz
Copy link
Contributor

@benschulz benschulz commented Dec 15, 2015

I haven't gone past analysis on this one. So if you want to "steal" it, feel free. ;)

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Dec 15, 2015

I was simply curious of the status of this issue. As of now though, it might be better to instead wait for #8658 to land before attempting this issue.

Would you still like to pursue tackling this issue once #8658 lands?

@benschulz
Copy link
Contributor

@benschulz benschulz commented Dec 15, 2015

Sure. I'll give a ping when I start work on it, in case someone else is looking for an issue to work on before I get to it. ;)

@jdm jdm removed the C-assigned label Dec 15, 2015
@KiChjang KiChjang self-assigned this May 1, 2016
@KiChjang KiChjang added the C-assigned label May 1, 2016
KiChjang added a commit to KiChjang/servo that referenced this issue Jul 31, 2016
bors-servo added a commit that referenced this issue Jul 31, 2016
(Do not merge) Use the same script thread if new location is same origin

Fixes #5239.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12666)
<!-- Reviewable:end -->
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Aug 1, 2016

I suspect that the heavy-lifting is going to be done in script_thread::handle_new_layout when adapting AttachLayout to this new design. I'm guessing that handle_new_layout would behave a bit differently depending on the nature of the navigation. What should this function's behaviour look like when 1) a new, same-origin iframe is being added 2) an existing iframe is being replaced with same-origin content 3) navigating towards a same-origin destination in a top-level browsing context?

KiChjang added a commit to KiChjang/servo that referenced this issue Aug 18, 2016
bors-servo added a commit that referenced this issue Aug 18, 2016
(Do not merge) Use the same script thread if new location is same origin

Fixes #5239.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12666)
<!-- Reviewable:end -->
KiChjang added a commit to KiChjang/servo that referenced this issue Aug 28, 2016
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Sep 21, 2016

See #633.

KiChjang added a commit to KiChjang/servo that referenced this issue Sep 26, 2016
@KiChjang KiChjang removed the C-assigned label Oct 1, 2016
@KiChjang KiChjang removed their assignment Oct 1, 2016
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Dec 1, 2016

Clsoed by #14211.

@asajeffrey asajeffrey closed this Dec 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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