-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Implement synchronous about:blank loading. #13996
Conversation
Heads up! This PR modifies the following files:
|
@bors-servo try |
Implement synchronous about:blank loading. Based on initial work by jdm in <#8600>.
💔 Test failed - mac-rel-wpt1 |
|
r? @jdm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This design is much better than the one in #8600. Great work!
assert!(self.nested_browsing_context.get().is_none()); | ||
// Synchronously create a new context and navigate it to about:blank. | ||
let url = Url::parse("about:blank").unwrap(); | ||
// TODO - loaddata here should have referrer info (not None, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this? Get it from the iframe's document.
pub fn window_for_pipeline(pipeline: PipelineId) -> Root<Window> { | ||
SCRIPT_THREAD_ROOT.with(|root| { | ||
let script_thread = unsafe { &*root.get().unwrap() }; | ||
let page = script_thread.find_child_context(pipeline).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this function return Option<Root<Window>>
instead.
impl Runnable for IframeLoadEventSteps { | ||
fn handler(self: Box<IframeLoadEventSteps>) { | ||
let this = self.frame_element.root(); | ||
this.iframe_load_event_steps(this.pipeline_id().unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the pipeline id should be stored in the runnable, to avoid problems with initiating multiple loads for an iframe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea why that method even takes a pipeline id; all it does is compare it to self.pipeline_id().unwrap()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to determine if the load event is relevant to the current iframe contents. As written, this is redundant; we should store the pipeline that was in use when the runnable was created instead.
fn process_the_iframe_attributes(&self, mode: ProcessingMode) { | ||
// TODO: srcdoc | ||
|
||
if mode == ProcessingMode::FirstTime && !self.upcast::<Element>().has_attribute(&atom!("src")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment pointing at whatwg/html#490.
@@ -73,6 +83,7 @@ pub struct HTMLIFrameElement { | |||
sandbox_allowance: Cell<Option<SandboxAllowance>>, | |||
load_blocker: DOMRefCell<Option<LoadBlocker>>, | |||
visibility: Cell<bool>, | |||
nested_browsing_context: MutNullableHeap<JS<BrowsingContext>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me what the purpose of this is, except for asserting that it doesn't exist in create_nested_browsing_context.
@@ -277,7 +277,9 @@ impl LayoutThreadFactory for LayoutThread { | |||
layout.start(); | |||
}, reporter_name, sender, Msg::CollectReports); | |||
} | |||
let _ = content_process_shutdown_chan.send(()); | |||
if let Some(content_process_shutdown_chan) = content_process_shutdown_chan { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me what the effect of this change is. Why is it ok to not have one for about:blank layout threads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only listen on the receiver if we spawned a new script thread (or process) for the new pipeline; about:blank
always lives in its parent's script thread. I could create a channel and immediately drop the receiver, but that seemed pointless.
☔ The latest upstream changes (presumably #13742) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors-servo try |
Implement synchronous about:blank loading. Based on initial work by jdm in <#8600>. <!-- 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/13996) <!-- Reviewable:end -->
💔 Test failed - linux-rel-wpt |
I'm going to delay reviewing the newest changes until the tests pass. |
💔 Test failed - linux-rel-wpt |
⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt... |
💔 Test failed - linux-rel-wpt |
⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt... |
💔 Test failed - linux-rel-wpt |
⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt... |
💔 Test failed - linux-rel-wpt |
Another collection of intermittents, sigh:
|
Let's see how intermittent @bors-servo retry |
Implement synchronous about:blank loading. Based on initial work by jdm in <#8600>. <!-- 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/13996) <!-- Reviewable:end -->
☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev |
@Ms2ger must have the magic touch. |
Based on initial work by jdm in #8600.
This change is