Simplify and improve Pipeline creation. #11437

Ms2ger · 2016-05-26T09:28:29Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes do not require tests because refactoring

This change is

highfive · 2016-05-26T09:28:32Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/pipeline.rs, components/constellation/constellation.rs

nox · 2016-05-26T10:14:22Z

r? @asajeffrey

asajeffrey · 2016-05-26T17:48:00Z

Looks good, some minor nits.

Previously, nox (Anthony Ramine) wrote…

r? @asajeffrey

Reviewed 1 of 2 files at r3, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

components/constellation/constellation.rs, line 40 [r9] (raw file):

use net_traits::{self, ResourceThreads, IpcSend};
use offscreen_gl_context::{GLContextAttributes, GLLimits};
use pipeline::{ChildProcess, InitialPipelineState, Pipeline};

Are we ever actually using ChildProcess in the contellation? We are keeping a vector of them, but as far as I can tell, never actually doing anything with it.

components/constellation/constellation.rs, line 408 [r9] (raw file):

                    initial_window_size: Option<TypedSize2D<PagePx, f32>>,
                    script_channel: Option<IpcSender<ConstellationControlMsg>>,
                    load_data: LoadData) {

This function is now a lot nicer!

components/constellation/constellation.rs, line 432 [r9] (raw file):

        });

        let (pipeline, child_process) = match result {

if we don't actually use the child process we could just make this a Result<Pipeline>.

components/constellation/pipeline.rs, line 41 [r9] (raw file):

use webrender_traits;

pub enum ChildProcess {

Make this private? Or just get rid of it, as I don't see where it's being used.

components/constellation/pipeline.rs, line 186 [r9] (raw file):

        if let Some((script_port, pipeline_port)) = content_ports {
            // Route messages coming from content to devtools as appropriate.
            let script_to_devtools_chan = state.devtools_chan.as_ref().map(|devtools_chan| {

@nox would tell you off for side-effecting map :)

components/constellation/pipeline.rs, line 245 [r9] (raw file):

            };

            // Spawn the child process.

Nice that this is in pipeline rather than constellation.

components/constellation/pipeline.rs, line 392 [r9] (raw file):

#[derive(Deserialize, Serialize)]
pub struct UnprivilegedPipelineContent {

Can this be made private? Also, the name is odd now we don't have PrivilegedPipelineContent.

Comments from Reviewable

bors-servo · 2016-05-26T20:44:07Z

☔ The latest upstream changes (presumably #11430) made this pull request unmergeable. Please resolve the merge conflicts.


        Privatize Pipeline::new().


        Move spawn_multiprocess into UnprivilegedPipelineContent.


        Introduce Pipeline::spawn().


        Inline Pipeline::create() into Pipeline::spawn().


        Remove PrivilegedPipelineContent.


        Avoid some unnecessary work if we're not spawning content.


        Improve code flow in Pipeline::spawn().


        Avoid some unnecessary runtime checks.


        Move the Pipeline creation to the end of Pipeline::spawn().

Ms2ger · 2016-05-27T07:31:43Z

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

components/constellation/constellation.rs, line 40 [r9] (raw file):