[webkit-reviews] review granted: [Bug 193862] Web Inspector: provide a way to edit the user agent of a remote target : [Attachment 360225] 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 193862: Web Inspector: provide a way to edit the user agent of a remote
target
https://bugs.webkit.org/show_bug.cgi?id=193862

Attachment 360225: Patch

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




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

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

r=me but I think you should be able to add a test:

    function printUserAgent() {
	TestPage.addResult(`UserAgent: ${navigator.userAgent}`);
    }
    ...
    async test() {
	await InspectorTest.evaluate(`printUserAgent()`);
	InspectorTest.log("Overriding...");
	await PageAgent.overrideUserAgent("test user agent");
	await InspectorTest.reloadPage();
	await InspectorTest.evaluate(`printUserAgent()`);
	InspectorTest.log("Restoring...");
	await PageAgent.overrideUserAgent();
	await InspectorTest.reloadPage();
	await InspectorTest.evaluate(`printUserAgent()`);
    }

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:361
> +    m_userAgentOverride = value ? *value : String();

While I like `String()` some people have been doing `{ }`. I'll leave that up
to you.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1966
> +	   if (value === WI._overridenDeviceSettings)
> +	       return;

This looks wrong. Looks like it should be be comparing the user agent, not
against the Set:

    if (value === this._overriddenDeviceUserAgent)
	return;

> Source/WebInspectorUI/UserInterface/Base/Main.js:2093
> +	   ],
> +	   [

Style: You could put these on the same line to save some space.

> Source/WebInspectorUI/UserInterface/Base/Main.js:2101
> +	       { name: `Internet Explorer 8`, value: "Mozilla/4.0 (compatible;
MSIE 8.0; Windows NT 6.0; Trident/4.0)" },
> +	       { name: `Internet Explorer 7`, value: "Mozilla/4.0 (compatible;
MSIE 7.0; Windows NT 6.0)" },

I don't think we need so many of these. I realize Safari has them...

> Source/WebInspectorUI/UserInterface/Base/Main.js:2119
> +	   // FIXME: add separators between groups

If you add a <hr> as a child of a <select> it will create a separator. AFAIK
this is non-standard but WebKit has supported that forever.

So you could do:

    if (group !== userAgents[0])
	userAgentSelect.appendChild(document.createElement("hr"));

> Source/WebInspectorUI/UserInterface/Base/Main.js:2144
> +	       if (inputEvent.keyCode ===
WI.KeyboardShortcut.Key.Enter.keyCode)
> +		   applyOverriddenUserAgent(userAgentInput.value, true);

Do we really want to do this on keydown? How about onchange or oninput?

> Source/WebInspectorUI/UserInterface/Base/Main.js:2163
> +	       userAgentInput.selectionStart = userAgentInput.selectionEnd = 0;
> +	       userAgentInput.focus();

Interesting, why not just `userAgentInput.select()`?