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 |
| 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.
Let's do this? Get it from the iframe's document.
components/script/script_thread.rs
Outdated
| 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.
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.
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.
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.
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")) { |
| 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.
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.
| }, 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.
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.
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. |
|
⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt, mac-rel-wpt1... |
|
💔 Test failed - mac-rel-wpt1 |
Based on initial work by jdm in <#8600>.
|
@bors-servo r=Ms2ger,jdm,asajeffrey I disabled the test, because it apparently doesn't crash reliably in automation (though it does locally). |
|
📌 Commit b86965f has been approved by |
|
💔 Test failed - linux-rel-wpt |
|
@bors-servo r=Ms2ger,jdm,asajeffrey,nox |
|
📌 Commit 6075d0e has been approved by |
|
💔 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 |
|
☀️ 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