feat(oop iframes): integrate OOP iframes with the frame manager#7556
Conversation
|
@OrKoN Let me know what you think about this pr and if we should incrementally add OOP iframe support or have everything in one big batch? |
|
Could you describe on a high-level what was missing for OOPIF support and how we are adding it (and what are the difficulties)? |
Sure, I updated the description. |
|
What would be the effects of landing partial support? I guess it'd break some clients so maybe it's better to have all changes in one batch? |
|
It looks good to me but I am a bit fuzzy about implications for other functionality. Is it easily possible to implement OOPIF support behind a flag? Not suggesting, but just curious if it'd be safer to allow users to opt-in and test at first. |
As we currently don't support OOP iframes at all this isn't changing any existing behaviour but only adding new things. As long as we don't regress on existing behaviour, I think we should be fine without a flag. |
Yeah but I mean new frames might show up unexpectedly for the users and things like this. E.g., they expected frames.length === 1 and suddenly an OOPIF shows up making frames.length === 2. Do you think that won't be common? |
|
@OrKoN Could you take another look at this? |
OrKoN
left a comment
There was a problem hiding this comment.
Still LGTM. Please make sure that is marked as a breaking change in the release notes.
|
Landed and marked as breaking change 🎉 |
|
I'll need to do more work to find a shareable repro, but in case someone else runs into this, I sometimes get this error after upgrading to v11:
Update: no repro so far, but this is some of the target info: I think _onAttachedToTarget is firing 1ms before _onFrameAttached. |
|
@mattzeuner I am getting that error. How to fix it? |
| }) | ||
| .catch(debugError); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is there a reason why the logic under line#512 is performed only for worker targets? Is this preventing the iframe targets to execute that logic? If so, what is the purpose of it or why don't we need that for iframe targets?
|
I am still having issues with the iframe. I have a simple automation that runs Puppeteer and a seems the iframe is oop. I am using latest version. Should this help? https://pptr.dev/api/puppeteer.page.evaluateonnewdocument |
This pull request tries to add better support for OOP iframes (see #2548)
The current problem with OOP iframes is that they are moved to a different target. Because of this, the current implementation of Puppeteer pretty much ignores them.
This change extends the FrameManager to already take OOP iframes into account and hides the fact that those frames are actually in different targets.
Further work needs to be done to also make the NetworkManager aware of these and to make sure that settings like emulations etc. are also properly passed down to the new targets.