core(oopif): attach to all descendants #7608

patrickhulce · 2019-03-20T20:32:08Z

Summary
OK, OOPIF fixes attempt 479 :)

The good news: this is now 100% contained in driver.js and should work in all our environments 🎉

The bad news: it's kinda ugly to look at and reason about (we have to do nested Target.sendMessageToTarget calls and unpack nested receiveMessageFromTarget messages)

Related Issues/PRs
#7517 #7566 also #6922 #6337 #6078

paulirish

this is dope. nice work sorting all this out

lighthouse-core/gather/driver.js

patrickhulce · 2019-03-21T21:06:42Z

DZL, do a barrel roll

devtools-bot · 2019-03-21T21:26:58Z

DZL is done! Go check it out http://lh-dzl-7608.surge.sh

brendankenny

very nice and clean solution! Going to be a little hard to reason about how it works a month or two from now, but that seems pretty inherent to what we have to do to make it work :/

Not-directly-this-PR-related thoughts:

I think we should consider splitting driver in the near future, like we've talked about a few times. This PR could make a nice split point, wrapping connection as it's passed into the constructor with a multiTargetConnection that hides this logic inside
I'm a little concerned about the scalability of handling each type of Target message passed up individually, so we should think about what we want to do (transparent like these Network events? And/or some kind of explicit session handling for gatherers that want to ask a particular target a question?).

Not just academic, @paulirish is already talking about injecting some code in there to do some SW shenanigans 😭

lighthouse-core/lib/network-recorder.js

lighthouse-core/gather/driver.js

brendankenny · 2019-03-21T20:35:51Z

lighthouse-core/gather/driver.js

+      this._handleTargetAttached(protocolMessage.params, sessionIdPath);
+    }
+
+    if (protocolMessage.method.startsWith('Network')) {


emit this one so it's handled as usual instead of handling it here?

well our current handling isn't actually on driver events it's off the connection event, and I thought it would be weird to forcibly emit an event from within driver on the connection. but I guess you disagree?

🤞 hopes the answer isn't let's refactor this whole thing now ;)

compromise: moved this bit into a function that gets called by both?

well our current handling isn't actually on driver events it's off the connection event

ah, I had forgotten about this. I also remembered we did it the way it is because you can't listen to Network.*, so we needed to listen to protocolevent, which is too bad.

moved this bit into a function that gets called by both?

sounds perfect

lighthouse-core/gather/driver.js

brendankenny · 2019-03-21T22:59:35Z

lighthouse-core/gather/driver.js

+
+    // We receive messages from the outermost subtarget which wraps the messages from the inner subtargets.
+    // We are recursively processing them from outside in, so build the list of parentSessionIds accordingly.
+    const sessionIdPath = parentSessionIds.concat([sessionId]);


can we just push() like normal people :P

(also, if we're only ever going to look at it in reverse order, unshift() and remove the reverse() instead?)

you think a normal person would prefer a list that's mutated through these layers on top of everything else that's going on!?! 😱

you feelin' alright @brendankenny? you might want to go to a doctor to get that checked out 😛

the reverse point is worth discussing though :)

IMO, it's easiest to keep track of what the list looks like based on how it's being built, not how it's being used. This way you do not need to be thinking about the complexities of another function just to understand what's going inside this one. The fact that it gets used to build the messages inside out is an implementation detail of its sibling function (afterall I could have written sendMessageToTarget in a recursive way in which case the list wouldn't need to be reversed). WDYT?

you think a normal person would prefer a list that's mutated through these layers on top of everything else that's going on!?! 😱

lol, I forgot about that benefit of concat :P :P

IMO, it's easiest to keep track of what the list looks like based on how it's being built, not how it's being used.

i mean, [sessionId, ... parentSessionIds] doesn't seem any more surprising to me than the other way around, honestly. I do find

The fact that it gets used to build the messages inside out is an implementation detail of its sibling function (afterall I could have written sendMessageToTarget in a recursive way in which case the list wouldn't need to be reversed).

compelling, but I think the justification works for either approach (if _handleReceivedMessageFromTarget was written differently :). The main downside of the current approach is the .slice().reverse(), which adds mental overhead to the code we actually have written

you had me at [sessionId, ...parentSessionIds] :) done

brendankenny · 2019-03-21T23:20:55Z

lighthouse-cli/test/fixtures/oopif.html

@@ -5,6 +5,6 @@
  </head>
  <body>
    <h1>Hello frames</h1>
-    <iframe name="oopif" src="https://airhorner.com"></iframe>
+    <iframe name="oopif" src="https://www.paulirish.com/2012/why-moving-elements-with-translate-is-better-than-posabs-topleft/" style="width: 100vw; height: 100vh;"></iframe>


also, should we just be making our own OOPIF here rather than relying on this page having another nested one? We could do a page on the other port to go cross origin (assuming that triggers OOP) and have that embed the youtube video, for instance

I wish, I tried like 15 different combinations but localhost never seems to be oopif'd on its own, so we need at least one publicly hosted page that then also iframes a google-y origin that will guarantee 2 separate processes beyond our localhost one

paulirish · 2019-03-21T20:28:57Z

lighthouse-core/gather/driver.js

+    // Events from subtargets will be stringified and sent back on `Target.receivedMessageFromTarget`.
+    // We want to receive information about network requests from iframes, so enable the Network domain.
+    this.sendMessageToTarget(sessionIdPath, 'Network.enable');
+    this.sendMessageToTarget(sessionIdPath, 'Runtime.runIfWaitingForDebugger');


do this after setAutoAttach (as the last thing), much like how devtools frontend does this right now.

also.. can you add a comment that explains what this does. (Basically the target was put into a suspended/paused state with waitForDebuggerOnStart: true and this resumes it.)

was too fast for ya already done! :) but yeah i'll add a comment

brendankenny

just the lint error left

LGTM!

lighthouse-core/gather/driver.js

devtools-bot · 2019-03-22T01:28:32Z

DZL is done! Go check it out http://lh-dzl-7608.surge.sh

connorjclark · 2019-08-09T03:12:10Z

For future sleuthing (blame sends you to this PR): I asked Patrick "Why didn't we use flatten?"

Answer: the extension protocol API only allows for "send message to target interface". see #7566 (comment)

core(oopif): attach to all descendants

39a03f7

patrickhulce requested review from brendankenny and paulirish as code owners March 20, 2019 20:32

patrickhulce mentioned this pull request Mar 20, 2019

core(oopif): autoattach to all descendants #7517

Closed

still resume service workers

3edfccf

paulirish approved these changes Mar 21, 2019

View reviewed changes

lighthouse-core/gather/driver.js Outdated Show resolved Hide resolved

lighthouse-core/gather/driver.js Outdated Show resolved Hide resolved

feedback

9bfe385

brendankenny reviewed Mar 21, 2019

View reviewed changes

paulirish reviewed Mar 21, 2019

View reviewed changes

patrickhulce added 2 commits March 21, 2019 18:51

feedback

1e4a35c

more comments

c8ee8aa

brendankenny approved these changes Mar 22, 2019

View reviewed changes

lighthouse-core/gather/driver.js Show resolved Hide resolved

lint

9ac2e10

patrickhulce merged commit 82e8a64 into master Mar 22, 2019

patrickhulce deleted the oopif_recursive_v2 branch March 22, 2019 02:30

patrickhulce mentioned this pull request Mar 22, 2019

core(oopifs): skip OOPIF network records in some gatherers #7640

Merged

brendankenny mentioned this pull request Sep 20, 2019

Embedding in another extension #7350

Closed

brendankenny mentioned this pull request Apr 19, 2021

[WIP] core(fr): split out shared driver utilities #12366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(oopif): attach to all descendants #7608

core(oopif): attach to all descendants #7608

patrickhulce commented Mar 20, 2019 •

edited by paulirish

Loading

paulirish left a comment

patrickhulce commented Mar 21, 2019

devtools-bot commented Mar 21, 2019

brendankenny left a comment

brendankenny Mar 21, 2019

patrickhulce Mar 21, 2019 •

edited

Loading

patrickhulce Mar 21, 2019

brendankenny Mar 21, 2019

brendankenny Mar 21, 2019

patrickhulce Mar 21, 2019

brendankenny Mar 21, 2019

patrickhulce Mar 21, 2019

brendankenny Mar 21, 2019

patrickhulce Mar 21, 2019

paulirish Mar 21, 2019

patrickhulce Mar 21, 2019

brendankenny left a comment •

edited

Loading

devtools-bot commented Mar 22, 2019

connorjclark commented Aug 9, 2019

core(oopif): attach to all descendants #7608

core(oopif): attach to all descendants #7608

Conversation

patrickhulce commented Mar 20, 2019 • edited by paulirish Loading

paulirish left a comment

Choose a reason for hiding this comment

patrickhulce commented Mar 21, 2019

devtools-bot commented Mar 21, 2019

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment • edited Loading

Choose a reason for hiding this comment

devtools-bot commented Mar 22, 2019

connorjclark commented Aug 9, 2019

patrickhulce commented Mar 20, 2019 •

edited by paulirish

Loading

patrickhulce Mar 21, 2019 •

edited

Loading

brendankenny left a comment •

edited

Loading