Optional pipeline in WorkerScriptLoadOrigin should not be optional

[jdm]

All code that creates a WorkerScriptLoadOrigin passes a Some value for the pipeline, so we should remove the Option from the struct member field.

Code:

• components/script_traits/lib.rs
• components/script/dom/serviceworkerregistration.rs
• components/script/dom/worker.rs

No need to run tests - if it builds, it's good enough for a pull request.

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

[ghost]

@highfive: assign me

This looks like something I could do.

[highfive]

Hey @phil3112! Thanks for your interest in working on this issue. It's now assigned to you!

[LittleSouken]

i help but dont give back

[warren-fisher]

@highfive : assign me

It seems like no work has been completed on this so I would like to give it a try.

[highfive]

It looks like this has already been assigned to someone. I'll leave the decision to a core contributor.

[warren-fisher]

I have looked into this and this is my understanding:
components/script/dom/serviceworkerregistration.rs and components/script/dom/worker.rs create new instances of the WorkerScriptLoadOrigin struct with pipeline_id as a Some(pipeline_id). Therefore the Option type of the WorkerScriptLoadOrigin is not necessary since we can always expect a Some(pipeline_id) and never a None. So I tried changing it so that the WorkerScriptLoadOrigin pipeline_id field accepts a PipelineId type, as well as changing it so that in components/script/dom/serviceworkerregistration.rs and components/script/dom/worker.rs when creating this struct it gives it a PipelineId instead of a Some.

My problem with this is that both components/script/dom/serviceworkerglobalscope.rs and components/script/dom/dedicatedworkerglobalscope.rs expect an Option type in a few places so the compiler fails with mismatched types. Perhaps I am completely off on my ideas about this (very new to Rust) or perhaps these other files need updating as well. I will not proceed any further for the moment.

[jdm]

@warren-fisher I'm not sure I understand the issue - I believe any code in those files that expects an Option can be passed Some(pipline_id) and satisfy the compiler.

[warren-fisher]

@jdm My bad, I definitely explained it poorly. If I change the WorkerScriptLoadOrigin struct to have a PipelineId and not an Option<PipelineId> then that means I have to change the instances where Some(pipeline_id) occurs to pipeline_id or else there is a type mismatch with the Rust compiler. Perhaps I misunderstood this issue and it is not correct to change Option<PipelineId> into PipelineId in the WorkerScriptLoadOrigin struct in components/script/dom/serviceworkerregistration.rs.

This then has cascading affects in a few of the other files I mentioned where they were expecting an Optional but not are being passed a PipelineId struct instead. It is very possible that I misunderstood the goal of this issue.

I looked a little deeper and found this example of an unsafe block testing to make sure that Some(pipeline_id) = pipeline_id. I am not sure if this is necessary for making this block safe, I have not had the chance to look that far. If I were to guess I would say that this check is not necessary if the only places where the WorkerScriptLoadOrigin is initialized are in components/script/dom/worker.rs and components/script/dom/serviceworkerregistraction.rs. I am not sure of the larger picture here so I could be far off.

[jdm]

It certainly sounds like you're on the right track. We want to change the Option<PipelineId> to a PipelineId in WorkerScriptLoadOrigin; this means that any code that handles a None case when reading that value can be removed entirely. I think it might help if you linked to particular lines of code that are giving errors that you aren't sure how to address.