An update to blink::Modulator

[Kouhei Ueno]

Hi module-dev,

FYI: haraken, masonfreed

hiroshige@ and I chatted and we agreed that we should remove most of the methods from Modulator.

I had always regretted its design, and we are now confident that it is no longer providing the values that it originally intended to provide.

blink::Modulator [cs] today serves multiple purposes. It serves

- as a holder for various module specific states bound to a document's lifetime

- as an entrypoint to fetch algorithms, and

- as an entrypoint to bindings/v8

We believe that there is not much value of it being the latter two. There were some original intents when I designed the class, but they no longer make sense:

- [fetch] custom steps may be inserted for some worker module fetches.

-- -> turns out that we use the same ModulatorImplBase implementations, and switch by ModuleScriptCustomFetchType [cs]

- [bindings] centralize callpath to bindings/v8

-- -> Indirection is hurting the readability, and there were some codepaths (on classic script ScriptController) which didn't use the "centralized" path, leading to confusing fork.

- Misuse ScriptState of worker hosting page / worker global scope.

-- -> hiroshige@ said they should be much easier now?

- Use mock implementation of some fetch algorithms in unit tests. (e.g. ModuleTreeLinkerTestModulator [cs])

-- we should be introducing a test-only overridable constructor hook (global variable or something), the common pattern seen in content/ code. This enough doesn't seem to justify Modulator being an entrypoint to fetch algorithms.

kouhei@ plans to start making the changes as his P2-3-ish work. Let me know if you have any concerns.

Thanks,

kouhei

[Hiroki Nakagawa]

Overall, rearchitecting blink::Modulator sounds good to me.

Do you already have a rough plan on the end-state of class structures? Specifically I'm curious how to switch implementations based the execution context types and fetch algorithms. Currently it's done by Modulator sub-classes (DocumentModulatorImpl, WorkerModulatorImpl etc) and ModuleScriptCustomFetchType.

--
You received this message because you are subscribed to the Google Groups "module-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to module-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/module-dev/CAPVAxLXTQ%3DpaR_ycn5R4BPi2%2Be1Wt_Dp%3DMXn9Qr6b0QNvytTOg%40mail.gmail.com.

[Kouhei Ueno]

On Thu, May 27, 2021 at 1:22 PM Hiroki Nakagawa <nhi...@chromium.org> wrote:

Overall, rearchitecting blink::Modulator sounds good to me.

Do you already have a rough plan on the end-state of class structures? Specifically I'm curious how to switch implementations based the execution context types and fetch algorithms. Currently it's done by Modulator sub-classes (DocumentModulatorImpl, WorkerModulatorImpl etc) and ModuleScriptCustomFetchType.

I actually want your input here since you have the most expertise in a worker module fetch code path. What do you believe is the best end state here?

I'm planning to do this incrementally and start from the bindings onion-souping. For those methods I plan to simply remove the method one-by-one on Modulator/DummyModulator/ModulatorImplBase, and inline the ModulatorImplBase implementation on their callers.

--

kouhei

[Hiroki Nakagawa]

On Thu, May 27, 2021 at 1:32 PM Kouhei Ueno <kou...@google.com> wrote:

On Thu, May 27, 2021 at 1:22 PM Hiroki Nakagawa <nhi...@chromium.org> wrote:

Overall, rearchitecting blink::Modulator sounds good to me.

Do you already have a rough plan on the end-state of class structures? Specifically I'm curious how to switch implementations based the execution context types and fetch algorithms. Currently it's done by Modulator sub-classes (DocumentModulatorImpl, WorkerModulatorImpl etc) and ModuleScriptCustomFetchType.

I actually want your input here since you have the most expertise in a worker module fetch code path. What do you believe is the best end state here?

OK, let me have time to consider it more :)

At least we could merge sub-classes into ModulatorImplBase as those are quite small.

 

I'm planning to do this incrementally and start from the bindings onion-souping. For those methods I plan to simply remove the method one-by-one on Modulator/DummyModulator/ModulatorImplBase, and inline the ModulatorImplBase implementation on their callers.

SGTM. Can you include me to CLs as a reviewer?

[Hiroshige Hayashizaki]

> how to switch implementations based the execution context types and fetch algorithms.

I think this would (partially) revisit previous discussions (among kouhei/nhiroki/yhirano/me a couple of years ago?) around ModuleScriptFetcher cleanup.

Maybe the callers of ModuleTreeLinker::Fetch() etc. passes ModuleScriptFetcher(Factory) instead of/in addition to ModuleScriptCustomFetchType?

Perhaps this matches with the spec structure (passing custom fetch steps as ModuleScriptFetcher(Factory)).

I feel I discussed similar idea before but this didn't happen at that time.