Bug 74790

Summary: Web Inspector: PageAgent.open() dosen't belong
Product: WebKit Reporter: Timothy Hatcher <timothy>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, bweinstein, fishd, japhet, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch yurys: review+

Description Timothy Hatcher 2011-12-17 13:49:42 PST
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.
Comment 1 Pavel Feldman 2011-12-18 06:00:46 PST
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.
Comment 2 Timothy Hatcher 2011-12-18 09:14:25 PST
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.
Comment 3 Timothy Hatcher 2011-12-18 09:16:08 PST
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.
Comment 4 Pavel Feldman 2011-12-18 09:35:52 PST
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.
Comment 5 Timothy Hatcher 2011-12-18 09:44:26 PST
OK, I'm confused then. I thought PageAgent went to the backend?
Comment 6 Timothy Hatcher 2011-12-18 09:47:32 PST
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.
Comment 7 Pavel Feldman 2011-12-18 09:57:01 PST
> 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.
Comment 8 Pavel Feldman 2012-01-20 02:29:57 PST
Created attachment 123274 [details]
Patch
Comment 9 WebKit Review Bot 2012-01-20 02:32:44 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 10 Yury Semikhatsky 2012-01-20 03:41:20 PST
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.
Comment 11 Joseph Pecoraro 2012-01-20 11:12:51 PST
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?
Comment 12 Joseph Pecoraro 2012-01-20 11:13:45 PST
(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.
Comment 13 Pavel Feldman 2012-01-23 02:04:31 PST
Committed r105600: <http://trac.webkit.org/changeset/105600>