Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[WIP] Implement the end, script execution, and everything. #9335
Conversation
highfive
commented
Jan 15, 2016
|
|
|
|
Ms2ger
force-pushed the
Ms2ger:the-end
branch
2 times, most recently
from
5590002
to
4d68dfc
|
|
Ms2ger
force-pushed the
Ms2ger:the-end
branch
3 times, most recently
from
e2f0f96
to
e1e6d41
|
|
|
@Ms2ger What's the status of this pull request? |
|
Needs more work, but I don't have time to finish it right now. |
|
Can we close it for now, for triaging? |
|
I guess. |
|
I started looking through it the other day. Closing it just because the reviewer hasn't reviewed it yet is a misuse of the triage process :) |
|
|
|
Is there any value in keeping this open? |
|
Yes. It's perpetually in the list of things I'm responsible for. |
|
Status? |
|
No change since the previous comments. |
|
Not many comments here. I reviewed this PR focusing on the concepts involved, rather than spending a lot of time thinking about the implications of each line. I really like how this clarifies the fuzziness around loading scripts, and tying that into a readable implementation of "the end" from the spec. I think this is worth rebasing and spending more time to implement completely! |
| } | ||
|
|
||
| // Step 4. | ||
| self.dispatch_dom_content_loaded(); |
This comment has been minimized.
This comment has been minimized.
| self.dispatch_dom_content_loaded(); | ||
| } | ||
|
|
||
| if self.end_state.get() == EndState::FiredDOMContentLoaded { |
This comment has been minimized.
This comment has been minimized.
jdm
Nov 3, 2016
Member
According to the spec we should execute this block after queuing the task in the previous step, so maybe we should actually set end_state above rather than in dispatch_dom_content_loaded?
| // TODO: Step 13: completely loaded. | ||
|
|
||
| let ConstellationChan(ref sender) = window.constellation_chan(); | ||
| sender.send(ConstellationMsg::LoadComplete(window.pipeline())).unwrap(); |
This comment has been minimized.
This comment has been minimized.
jdm
Nov 3, 2016
Member
I wonder if this should be triggered as a result of running DocumentProgressHandler instead? Seems like it would be easy to cause races here.
| let document = document_from_node(elem.r()); | ||
| document.continue_the_end(); | ||
|
|
||
| document.finish_load(LoadType::Script(self.url.clone())); |
This comment has been minimized.
This comment has been minimized.
jdm
Nov 3, 2016
Member
Should we invoke finish_load before continue_the_end? The script counts as something blocking the document load event, so it has an effect on what that method does.
|
I think I can look at this once #14361 lands. |
|
I don't see anything to salvage from this PR anymore. Closing. |
Ms2ger commentedJan 15, 2016
@jdm: I started to review #6677, and ended up here somehow. Thoughts?