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

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied 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 372249: [Patch] WIP

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 372249
  --> https://bugs.webkit.org/attachment.cgi?id=372249
[Patch] WIP

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

I can't wait to see this land! Next patch lets see some screenshots.

> Source/JavaScriptCore/inspector/protocol/Page.json:254
> +	       "name": "setBootstrapScript",

I still don't particularly like the name `Bootstrap`. I still think "Injected
Script" tends closer to reality. And this could support multiple "Inspector
Injected Scripts" though then order becomes relevant. Bootstrap doesn't sound
debuggable, because technically it sounds like it would be before something is
completely initialized / setup.

Some other naming thoughts:

    incipient - "in an initial stage; beginning to happen or develop"
    inceptive - "relating to or marking the beginning of something;"

That said, I love how simple this is on the backend and "bootstrap" is a
convenient moniker for the feature, so gives a nice name.

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:904
> +    frame.script().evaluate(ScriptSourceCode(m_bootstrapScript.value(),
URL(), TextPosition(), JSC::SourceProviderSourceType::Module));

If this does not have a URL / name it doesn't seem like the user can set
breakpoints in the script.

Users could use `debugger` statements, but I think we should allow breakpoints.
For example if you put functions inside your bootstrap script and want to pause
in them.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:665
> +    set bootstrapScript(source)
> +    {
> +	   console.assert(NetworkManager.supportsBootstrapScript());
> +	   console.assert(!source || typeof source === "string");
> +
> +	   this._bootstrapScriptSetting.value = source;
> +
> +	   for (let target of WI.targets) {
> +	       if (target.PageAgent && target.PageAgent.setBootstrapScript)
> +		  
target.PageAgent.setBootstrapScript(this._bootstrapScriptSetting.value ||
undefined);
> +	   }
> +    }

It seems weird to see this on NetworkManager, but that seems fine. It raises a
few interesting points:

    • Should this be injected into the Main Frame or All Frames?
	- I think I'd vote All Frames, like Injected Scripts. It looks like
this does that right now.
    • Should this be injected into Workers / Service Worker contexts?
	- They will have a Network Agent, and a JS environment, but they won't
have frames so won't work right now with the backend.
	- I think I'd vote No right now because it seems likely someone will
put DOM code in their bootstrap script and not think about Workers.
	- We should probably rely on "Local Resource Substitution" enhancement
ideas if we ever wanted to inject code into Workers.

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1677
> +	       contextMenu.appendItem(WI.UIString("Inspector Style Sheet"), ()
=> {

It seems we are inconsistent on wording:

    WI.UIString("Author Stylesheet");
    WI.UIString("User Stylesheet");
    WI.UIString("User Agent Stylesheet");
    WI.UIString("User Agent Stylesheet");
    WI.UIString("Inspector Style Sheet")

I think we should standardize on "Stylesheet".

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:218
> +	   if (!this._textEditor.string && this._textEditor.readOnly)

Hah! That must have been tricky to discover.