[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
bugzilla-daemon at webkit.org
Sat Jan 26 00:15:20 PST 2019
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()`?
More information about the webkit-reviews
mailing list