core(service-workers): sub-target message passing enabled. WIP #3555
Conversation
| this._parent = parentConnection; | ||
|
|
||
| this._parent.on('notification', this._onEvent.bind(this)); | ||
| // now we repeat and do this again. we need to autoattach and grab a type: `worker` within |
There was a problem hiding this comment.
I thought I understood what's going on until I read this comment, isn't the attaching within happening in the onTargetAttached over in manager where you create manager in a manager?
| this._sessionId = sessionId; | ||
| this._parent = parentConnection; | ||
|
|
||
| this._parent.on('notification', this._onEvent.bind(this)); |
There was a problem hiding this comment.
my only gripe with the overall setup is this notification listener bit since it feels like a hacky way to override a method, can it not be done with regular override and super call?
| let manager; | ||
| if (this._requiredType === 'service_worker') { | ||
| manager = new SWManager(session, {requiredType: 'worker'}); | ||
| manager.listen(); |
There was a problem hiding this comment.
my main question is if we could flatten this out and lose the recursive step.
If I'm reading/understanding this correctly (big if), it seems like it's always service_worker -> worker, and then the only thing the service_worker target is kept around (on the LH side) is for dispatching to the worker. Can we ditch a reference to the service_worker manager and instead listen for the follow up worker, and then only keep that around?
| if (manager) return manager.sendCommandToSubTargets(method, params); | ||
| else return session.sendCommand(method, params); | ||
| }); | ||
| return Promise.all(p).then(arr => _flatten(arr)); |
There was a problem hiding this comment.
it seems like this is set up to return an array of results...but what are all the results going to be coming from? Can we only attach to an active SW (so there should only be one)?
… domain. renamings.
| console.warn(`No subtargets yet for ${method}`); | ||
| } | ||
| // Some protocol domains are not supported by the dedicated worker but by the SW page | ||
| // Note: Log and Network are technically supported in both, but not completely, |
There was a problem hiding this comment.
any documentation about this somewhere? Or just something you know?
|
Shelving this until the new target selection support is landed in Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=775132 |
For early review. I have a bit more to do, but in case you're interested in big changes.. speak now. :)
fixes #709