Bug 193862 – Web Inspector: provide a way to edit the user agent of a remote target

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

This will include all the items in "Develop > User Agent" (including a custom input).

[Devin Rousso]

<rdar://problem/47359292>

[Devin Rousso]

Created attachment 360225 [details]
Patch

[Devin Rousso]

Created attachment 360226 [details]
[Image] After Patch is applied (Popover)

[Devin Rousso]

Created attachment 360227 [details]
[Image] After Patch is applied (<select>)

[Devin Rousso]

Created attachment 360228 [details]
[Image] After Patch is applied (Custom)

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Joseph Pecoraro]

Comment on attachment 360225 [details]
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()`?

[Joseph Pecoraro]

I see rniwa just landed userAgentForJavaScript. You might need to move this code a bit.

[Devin Rousso]

Comment on attachment 360225 [details]
Patch

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

(In reply to Joseph Pecoraro from comment #7)
> 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()`);
>     }
I actually tried exactly doing that (almost identical to what you wrote), but after the first reload there would be exceptions that `TestPage` wasn't defined.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:1966
>> +            return;
> 
> This looks wrong. Looks like it should be be comparing the user agent, not against the Set:
> 
>     if (value === this._overriddenDeviceUserAgent)
>         return;

Oops.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:2093
>> +        [
> 
> Style: You could put these on the same line to save some space.

I personally hate that style :|

>> 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"));

I spent so long trying to find an alternative T.T

I even almost went and added something to HTMLOptionElement to specifically support this.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:2144
>> +                applyOverriddenUserAgent(userAgentInput.value, true);
> 
> Do we really want to do this on keydown? How about onchange or oninput?

I don't think we want to apply it on every input, as we reload the page.  I only want it to happen when the user is "done".

>> Source/WebInspectorUI/UserInterface/Base/Main.js:2163
>> +            userAgentInput.focus();
> 
> Interesting, why not just `userAgentInput.select()`?

`.select()` causes the <input> to scroll its contents to the end.  I thought that looked bad.  This way, you see the beginning of the custom value, and can scroll/key over to see the rest of it.

[Devin Rousso]

Created attachment 360246 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 360246 [details]
Patch

Clearing flags on attachment: 360246

Committed r240549: <https://trac.webkit.org/changeset/240549>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.