Bug 74790 – Web Inspector: PageAgent.open() dosen't belong

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Timothy Hatcher]

PageAgent.open() is used to open resources in a new window/tab. In a remote environment this will open it on the wrong client. An open function should exist on InspectorFrontendHost, not PageAgent.

There might be use for PageAgent.open(), but all the uses I see today intend for it to open on the client showing the Inspector window.

[Pavel Feldman]

That's exactly the reason behind its being hidden. My plan is to add non-hidder Page::navigate() that would perform the navigation within the same page. Before this is done, users can use RuntimeAgent::evaluate to assign to window.location.href in order to achieve this result.

[Timothy Hatcher]

I think you are not understanding (or I'm not).

All of the UI actions associated with PageAgent.open are actions that I would expect to cause a window/tab to open on the computer showing the Inspector window. If I'm remotly inspecting a page, PageAgent.open will open it on the remote client.

What you proposed is basically the same thing as PageAgent.open, which has a newWindow argument you can pass as false.

[Timothy Hatcher]

That is why I say these UI actions should use something on InspectorFrontendHost, which is local to the Inspector, to open a new window/tab. And no, not window.open, which has opener rules.

[Pavel Feldman]

To further clarify: WebKit remote debugging protocol is operating within the page context. I.e. discovery of the pages and engaging with the page for debugging is happening outside of the protocol. InspectorFrontendHost would be the one covering it, but we don't yet have UI actions that would require it.

[Timothy Hatcher]

OK, I'm confused then. I thought PageAgent went to the backend?

[Timothy Hatcher]

I am not talking about discovery. I talking about all the users of PageAgent.open in the front-end.

That goes to the backend.

I don't want the backend opening Resources that I double-click in the Resources sidebar.

I want the front-end client to open it.

[Pavel Feldman]

> I don't want the backend opening Resources that I double-click in the Resources sidebar.
> I want the front-end client to open it.

Oh, ok, gotcha, I thought we were talking about the navigate scenario. Yes, InspectorFrontendHost would be the place for it, its soft version that is used for remote debugging would be doing window.open.

[Pavel Feldman]

Created attachment 123274 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Yury Semikhatsky]

Comment on attachment 123274 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:230
> +                    { "name": "url", "type": "string", "description": "URL to navigate the page to." }

This may well be performed by evaluating location.href = <new url> in the inspected page, but as discussed offline let's leave this method for convenience.

[Joseph Pecoraro]

Comment on attachment 123274 [details]
Patch

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

I like this changes!

> Source/WebCore/ChangeLog:16
> +        * inspector/InspectorFrontendHost.h:
> +        * inspector/InspectorFrontendHost.idl:

You should have a comment in the ChangeLog (probably just above the Reviewed by line) about InspectorFrontendHost.openInNewTab and PageAgent.navigate.

I'm not sure "openInNewTab" makes much sense for a InspectorFrontendHost that might not have tabs, I think Just "openURL" would be fine.

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:193
> +    // FIXME: Why does one use mainFrame and the other frame?
> +    frame->loader()->changeLocation(mainFrame->document()->securityOrigin(), frame->document()->completeURL(url), "", false, false);

Is the FIXME necessary? Can you just use frame->document()->securityOrigin() since you're making the request in a new Window?

[Joseph Pecoraro]

(In reply to comment #11)
> (From update of attachment 123274 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123274&action=review
> 
> I like this changes!

change*. English has never been my strong suit.

[Pavel Feldman]

Committed r105600: <http://trac.webkit.org/changeset/105600>