[webkit-reviews] review granted: [Bug 195847] Web Inspector: provide a way to inject "bootstrap" JavaScript into the page as the first script executed : [Attachment 381660] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 195847: Web Inspector: provide a way to inject "bootstrap" JavaScript into
the page as the first script executed
https://bugs.webkit.org/show_bug.cgi?id=195847

Attachment 381660: Patch

https://bugs.webkit.org/attachment.cgi?id=381660&action=review




--- Comment #20 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 381660
  --> https://bugs.webkit.org/attachment.cgi?id=381660
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381660&action=review

What does the Web Inspector UI look like if you pause in a bootstrap code?

---
window.doPause = function() {
    console.log("Inside bootstrap");
    debugger;
}
-----

Log line and pausing in the debugger would be interesting to see. Maybe the
name is very long in the console and might be annoying (Console Evaluation is
sometimes annoying).

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:774
> +    frame.script().evaluate(ScriptSourceCode(m_bootstrapScript, URL(URL(),
"web-inspector://bootstrap.js"_s)));

Style: I've been seeing styles like the following more and more:

    URL { { }, "web-inspector://bootstrap.js"_s }
    URL { URL(), "web-inspector://bootstrap.js"_s }

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:81
> +	       this._bootstrapScriptSourceSetting = new
WI.Setting("bootstrap-script-source", "");

I wonder if this should be in an IndexedDB (like LocalResourceOverrides)
instead of a LocalStorage setting. I could see someone dumping large libraries,
such as Underscore.js or something, into the bootstrap so it is always
available. In which case this fills the local storage quota rather quickly.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:281
> +    toggleBootstrapScript(enabled)

Hmm, I don't particularly like this. I'd rather there just be a setting `set
bootstrapScriptEnabled` and the other side set it appropriately.

> Source/WebInspectorUI/UserInterface/Models/Script.js:131
> +	       console.assert(WI.NetworkManager.supportsBootstrapScript());

The assertions in these display functions don't seem accurate. Someone could
always make a web-inspector:// request or sourceURL name. I think you could
just drop the assertions or only use the name if bootstrap script is enabled.

Actually, should we even consider _sourceURL here, or just _url?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:2182
> -	   if
(!this.target.hasCommand("Runtime.getRuntimeTypesForVariablesAtOffsets"))
> +	   if (!this.target ||
!this.target.hasCommand("Runtime.getRuntimeTypesForVariablesAtOffsets"))

Hmm, makes me wonder if there are other references to a Script/Resource having
a `target`.

Correct me if I'm wrong, these should be unreachable right now since you've
disabled the buttons for ScriptContentView? Still okay to do.