Split ScriptMsg into common and unique messages for main-thread and worker event loops

[jdm]

Enums like CommonScriptMsg, WorkerScriptMsg, and MainThreadScriptMsg, where the latter two have a variant for CommonScriptMsg, would make a lot of sense. Conflating messages that both event loops need to handle with ones that should never be dispatched to one or the other is unnecessary and confusing.

[catchmrbharath]

@jdm What do you mean by "latter two have a variant for CommonScriptMessage" ?

If that is cleared up, I would like to have at go at this issue.

[catchmrbharath]

My idea was to have three enums CommonScriptMsg, WorkerScriptMsg and MainThreadScriptMsg as separate enums. Things that are dispatched to both dedicatedworkerglobalscope and ScriptTask should are present in the CommonScriptMsg.

[jdm]

@catchmrbharath I mean that we'll have something like enum WorkerScriptMsg { A(whatever), Common(CommonScriptMsg) }.

[jdm]

Still working on this, @catchmrbharath?

[catchmrbharath]

@jdm Sorry about that. I am presently not working on this. Can you remove the C-assigned tag? I should have updated the issue long back. Sorry again :).

[jdm]

No problem!

[wldcordeiro]

I'll take a stab at this issue @jdm, could you just give me some guidance as to what attributes of the ScriptMsg enum should be part of each of the others?

[jdm]

Thanks @wldcordeiro! Any event that is processed only by the worker event loop in dedicatedworkerglobal.rs (handle_event) belongs in one enum, while any event that is only processed by the event loop in script_task.rs (handle_msgs) belongs in another. Any event that is processed by both of them belongs in a common enum. Does that make sense?

[wldcordeiro]

@jdm that does make sense, but would it also make sense to move the enums to different modules? Or would it be fine if they all were in script_task.rs?

[jdm]

Moving the worker-related enum to dedicatedworkerglobalscope.rs would make sense. Splitting up parts of script_task.rs is another worthy goal, but probably best done elsewhere.

[wldcordeiro]

Good to know @jdm. So the end result is that there will be three enums CommonScriptMsg, WorkerScriptMsg, and MainThreadScriptMsg. With WorkerScriptMsg in dedicatedworkerglobalscope.rs and the others being housed in script_task.rs? Splitting up that file would be good but maybe as its own issue?

[jdm]

Yep!

[wldcordeiro]

So I've been working on this and wanted to get a little input so I understand better.

The current ScriptMsg enum currently has these variants.

ScriptMsg

• TriggerFragment
• TriggerLoad
• FireTimer
• ExitWindow
• DOMMessage
• WorkerDispatchErrorEvent
• RunnableMessage
• RefcountCleanup
• PageFetchComplete

Now I've been trying to figure out which variants would fit in each of the three new enums, so far I've figured it to this.

CommonScriptMsg

• RefcountCleanup
• FireTimer
• RunnableMessage

MainThreadScriptMsg

• TriggerLoad
• TriggerFragment
• DOMMessage
• ExitWindow

WorkerScriptMsg

• WorkerDispatchErrorEvent

Would you say that's on the right path? Also how would you go about using the common variants within the other enums? I saw your example of enum WorkerScriptMsg { A(whatever), Common(CommonScriptMsg) } but how would that look as implemented? Like this?

pub enum WorkerScriptMsg {
    CommonRunnableMessage(CommonScriptMsg::RunnableMessage),
    CommonRefcountCleanup(CommonScriptMsg::RefcountCleanup)
}

Sorry if I'm asking too many questions @jdm, I'm learning Rust's ways and thought this would be a great project to learn from.

[jdm]

Never be sorry for asking questions! WorkerDispatchErrorEvent can actually be dispatched to both the worker and main thread event loops, so it belongs in the common enum. DOMMessage, on the other hand, only is sent to worker theads, and is actually documented as such,

With respect to the enum, you don't need to duplicate common variants. You can just declare that the enum contains a variant named Common (which contains a value of type CommonScriptMsg). Then you can use WorkerScriptMsg::Common(CommonScriptMsg::RunnableMsg) to actually create such values when necessary.

[wldcordeiro]

Okay thanks for the response @jdm so if I'm not mistaken then in the Common variant I can have a comma separated list of variants? WorkerScriptMsg::Common(CommonScriptMsg::Runable, CommonScriptMsg::WorkerDispatchErrorEvent) for example? Also I'm curious about how to handle the method handle_msg_from_script. Should I split it into more methods that work with the other enums?

[wldcordeiro]

So this is what I was thinking for the three ScriptMsg enums, I just wanted to confirm which are common and which are local to the MainScriptMsg and WorkerScriptMsg.

/// Messages used to control script event loops, such as ScriptTask and
/// DedicatedWorkerGlobalScope.
pub enum CommonScriptMsg {
    /// Fires a JavaScript timeout
    /// TimerSource must be FromWindow when dispatched to ScriptTask and
    /// must be FromWorker when dispatched to a DedicatedGlobalWorkerScope
    FireTimer(TimerSource, TimerId),
    /// Sends a message to the Worker object (dispatched to all tasks) regarding error.
    WorkerDispatchErrorEvent(TrustedWorkerAddress, DOMString, DOMString, u32, u32),
    /// Generic message that encapsulates event handling.
    RunnableMsg(Box<Runnable+Send>),
    /// A DOM object's last pinned reference was removed (dispatched to all tasks).
    RefcountCleanup(TrustedReference),
    /// The final network response for a page has arrived.
    PageFetchComplete(PipelineId, Option<SubpageId>, LoadResponse),
}

pub enum MainThreadScriptMsg {
    /// Acts on a fragment URL load on the specified pipeline (only dispatched
    /// to ScriptTask).
    TriggerFragment(PipelineId, String),
    /// Begins a content-initiated load on the specified pipeline (only
    /// dispatched to ScriptTask).
    TriggerLoad(PipelineId, LoadData),
    /// Notifies the script that a window associated with a particular pipeline
    /// should be closed (only dispatched to ScriptTask).
    ExitWindow(PipelineId),
    /// Common Variants to all ScriptMsg types
    Common(
        CommonScriptMsg::FireTimer,
        CommonScriptMsg::WorkerDispatchErrorEvent,
        CommonScriptMsg::RunnableMsg,
        CommonScriptMsg::RefcountCleanup,
        CommonScriptMsg::PageFetchComplete
    ),
}

pub enum WorkerScriptMsg {
    /// Message sent through Worker.postMessage (only dispatched to
    /// DedicatedWorkerGlobalScope).
    DOMMessage(StructuredCloneData),
    /// Common Variants to all ScriptMsg types
    Common(
        CommonScriptMsg::FireTimer,
        CommonScriptMsg::WorkerDispatchErrorEvent,
        CommonScriptMsg::RunnableMsg,
        CommonScriptMsg::RefcountCleanup,
        CommonScriptMsg::PageFetchComplete
    ),
}

[wldcordeiro]

I've pushed a commit to my fork, but I'm getting one build error currently. Could I get some help with debugging and/or guidance on what to do.

[jdm]

Common(
        CommonScriptMsg::FireTimer,
        CommonScriptMsg::WorkerDispatchErrorEvent,
        CommonScriptMsg::RunnableMsg,
        CommonScriptMsg::RefcountCleanup,
        CommonScriptMsg::PageFetchComplete
    ),

This isn't valid, since the arguments to the Common variant are not types, they are values. Enum variants are only allowed to contain types (such as WorkerScriptMsg, or CommonScriptMsg). Does that make more sense?

[jdm]

Oh, I guess you sorted that out on your own. I guess the PR builds now? :)

[wldcordeiro]

@jdm PR is building! Looks like it's working. 😄

[wldcordeiro]

@jdm You can remove the assigned tag. I've been too busy lately to work on this.

[wafflespeanut]

Now, this has become a total nightmare! (not even close to E-easy) :P

[wldcordeiro]

@wafflespeanut That's kind of what I was running into. It was definitely more complex than the description suggests.

[jdm]

Yep, my mistake. I'll make sure to think through the ramifications of such changes more thoroughly in the future before proposing them to eager new contributors.

[wafflespeanut]

Well, on the brighter side, it's gonna be fixed soon :)

[waddlesplash]

PR was merged, this should be closed now.