core: append sourceURL comment to eval code #13754
Conversation
|
oops, this was overlooked EDIT uhhhh looks like it doesn't work for eval on new doc?
|
False alarm, I was seeing confusing results because Debugger.getScriptSource needs to be run on the target that provided the scriptParsedEvents–scriptIds are not unique across the entire run, but only within the same process :( That also means we'll need to modify how we refer to scripts internally (again...), and I'm not sure there's an easy fix here. aside, looks like we gotta ignore some stuff in issues that we cause
|
| @@ -97,7 +97,8 @@ class ExecutionContext { | |||
| .catch(${pageFunctions.wrapRuntimeEvalErrorInBrowserString}) | |||
| .then(resolve); | |||
| }); | |||
| }())`, | |||
| }()) | |||
| //# sourceURL=lighthouse-eval.js`, | |||
There was a problem hiding this comment.
Could we add a few underscores to this guy? I feel like someone is using lighthouse-eval.js as their script name somewhere out in the web.
There was a problem hiding this comment.
If that's the case, then the url property will be the full absolute URL ending with this name. But when we set it via sourceURL it's exactly this string.
There was a problem hiding this comment.
I still think //# sourceURL=lighthouse-eval.js is within the realm of possibilities and it can't hurt to add a few underscores or something.
| @@ -68,8 +84,9 @@ class Scripts extends FRGatherer { | |||
| // it also blocks scripts from the same origin but that happen to run in a different process, | |||
| // like a worker. | |||
| if (event.method === 'Debugger.scriptParsed' && !sessionId) { | |||
| // Events without an embedderName (read: a url) are for JS that we ran over the protocol. | |||
| if (event.params.embedderName) this._scriptParsedEvents.push(event.params); | |||
| if (!isLighthouseRuntimeEvaluateScript(event.params)) { | |||
There was a problem hiding this comment.
nit: now can we add to the condition above ;)
|
@adamraine LGTM? |
|
oh i shoulda been more clear about #13754 (comment) only an issue for my WIP branch where we stop ignoring OOPIFs |
| @@ -97,7 +97,8 @@ class ExecutionContext { | |||
| .catch(${pageFunctions.wrapRuntimeEvalErrorInBrowserString}) | |||
| .then(resolve); | |||
| }); | |||
| }())`, | |||
| }()) | |||
| //# sourceURL=lighthouse-eval.js`, | |||
There was a problem hiding this comment.
I still think //# sourceURL=lighthouse-eval.js is within the realm of possibilities and it can't hurt to add a few underscores or something.
This allows us to more correctly ignore scripts that we created via the Runtime.evaluate method. The current assumption (that all those scripts would have a blank embedderName) does not hold when considering iframes.