Bug 118715

Summary: Web Inspector: InspectorFrontendHost is undefined
Product: WebKit Reporter: Roland Takacs <rtakacs>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, joepeck, seokju, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 118676    
Bug Blocks:    
Attachments:
Description Flags
initial patch
none
Patch none

Description Roland Takacs 2013-07-15 23:59:31 PDT
It's needed to copy the InspectorFrontendHostStub.js from the old inspector (WebCore/inspector/front-end) and paste into the Safari inspector UI.
Comment 1 Radar WebKit Bug Importer 2013-07-15 23:59:47 PDT
<rdar://problem/14452747>
Comment 2 Roland Takacs 2013-07-16 00:10:25 PDT
Created attachment 206732 [details]
initial patch

Added InspectorFrontendHostStub.js
Added HelpScreen.js / css (from the old inspector) because there are some references in the InspectorFrontendHostStub.js.
Added View.js (from the old inspectror) because HelpScreen uses it.
Added a Preferences variable, because in InspectorFrontendHost uses it in the InspectorFrontendHostStub.js.
Comment 3 Seokju Kwon 2013-07-16 01:49:44 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=206732&action=review

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:151
> +        setTimeout(WebInspector.fileManager.appendedToURL.bind(WebInspector.fileManager, url), 0);

Is there WebInspector.fileManager in New Inspector?

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:230
> +Preferences.localizeUI = false;

Unused value?

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:275
> +    __proto__: WebInspector.HelpScreen.prototype

I guess that HelpScreen does not exist in New Inspector UI Concept.
Comment 4 Timothy Hatcher 2013-07-16 06:56:07 PDT
(In reply to comment #3)
> View in context: https://bugs.webkit.org/attachment.cgi?id=206732&action=review
> 
> > Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:151
> > +        setTimeout(WebInspector.fileManager.appendedToURL.bind(WebInspector.fileManager, url), 0);
> 
> Is there WebInspector.fileManager in New Inspector?

There is not. So you can remove this.

> > Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:230
> > +Preferences.localizeUI = false;
> 
> Unused value?

Yes, remove. We always localize when we can.

> > Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:275
> > +    __proto__: WebInspector.HelpScreen.prototype
> 
> I guess that HelpScreen does not exist in New Inspector UI Concept.

It does not exist, and we don't want it.
Comment 5 Timothy Hatcher 2013-07-16 07:15:09 PDT
Comment on attachment 206732 [details]
initial patch

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

Thanks for doing this!

> Source/WebInspectorUI/ChangeLog:12
> +        There are some referenced files that also copied:
> +            * View.js
> +            * HelpScreen.js / css
> +        Added a 'Preferences' variable to the Setting.js because the InspectorFrontendHostStub.js uses it.

We want to be very selective when copying code from the deprecated Web Inspector. These things are items we don't want to bring over. This change should just have the bare minimum needed to add InspectorFrontendHostStub.js. I've commented on the patch where you can remove these dependencies and still should get a working Inspector with the minimum required.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:31
> +if (!window.InspectorFrontendHost) {

You should indent the body of the block.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:36
> +/**
> + * @constructor
> + * @implements {InspectorFrontendHostAPI}
> + */

We don't use. Remove.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:40
> +    this.isStub = true;

We don't use. Remove.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:42
> +    WebInspector.documentCopyEventFired = this.documentCopy.bind(this);

Remove.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:91
> +    setInjectedScriptForOrigin: function(origin, script)
> +    {
> +    },

We don't use. Remove.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:104
> +        document.title = WebInspector.UIString(Preferences.applicationTitle, url);

This should not use Preferences. In our UI url is really title. So rename url to title and just set document.title = title;

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:107
> +    documentCopy: function(event)

I don't think we use this.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:121
> +            var screen = new WebInspector.ClipboardAccessDeniedScreen();
> +            screen.showModal();

Remove. Just add a console.error.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:141
> +        setTimeout(WebInspector.fileManager.savedURL.bind(WebInspector.fileManager, url), 0);

We don't have fileManager. You can remove this.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:151
> +        setTimeout(WebInspector.fileManager.appendedToURL.bind(WebInspector.fileManager, url), 0);

Ditto.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:163
> +        var fileNameSuffix = (lastSlashIndex === -1) ? url : url.substring(lastSlashIndex + 1);

Remove () around (lastSlashIndex === -1).

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:165
> +        var blob = new Blob(content, { type: "application/octet-stream" });

Remove spaces inside {} except after :.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:178
> +    sendMessageToBackend: function(message)
> +    {
> +    },

I don't like how sendMessageToBackend is replaced dynamically later by the WebSocket code. This function should just know how to use the WebSocket if it exists.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:190
> +    recordActionTaken: function(actionCode)
> +    {
> +    },
> +
> +    recordPanelShown: function(panelCode)
> +    {
> +    },
> +
> +    recordSettingChanged: function(settingCode)
> +    {
> +    },

We don't use these user tracking functions. Remove.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:194
> +        return loadXHR(url);

We don't have a loadXHR function. Just inline it here.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:226
> +    supportsFileSystems: function()
> +    {
> +        return false;
> +    },
> +
> +    requestFileSystems: function()
> +    {
> +    },
> +
> +    addFileSystem: function()
> +    {
> +    },
> +
> +    removeFileSystem: function(fileSystemPath)
> +    {
> +    },
> +
> +    isolatedFileSystem: function(fileSystemId, registeredName)
> +    {
> +        return null;
> +    },
> +
> +    setZoomFactor: function(zoom)
> +    {
> +    },
> +
> +    isUnderTest: function()
> +    {
> +        return false;
> +    }

We don't use, remove.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:230
> +Preferences.localizeUI = false;

Remove.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:276
> +// Default implementation; platforms will override.
> +WebInspector.clipboardAccessDeniedMessage = function()
> +{
> +    return "";
> +}
> +
> +/**
> + * @constructor
> + * @extends {WebInspector.HelpScreen}
> + */
> +WebInspector.ClipboardAccessDeniedScreen = function()
> +{
> +    WebInspector.HelpScreen.call(this, WebInspector.UIString("Clipboard access is denied"));
> +    var platformMessage = WebInspector.clipboardAccessDeniedMessage();
> +    if (platformMessage) {
> +        var p = this.contentElement.createChild("p");
> +        p.addStyleClass("help-section");
> +        p.textContent = platformMessage;
> +    }
> +}
> +
> +WebInspector.ClipboardAccessDeniedScreen.prototype = {
> +    __proto__: WebInspector.HelpScreen.prototype
> +}
> +
> +}
> +
> +/**
> + * @constructor
> + * @extends {WebInspector.HelpScreen}
> + */
> +WebInspector.RemoteDebuggingTerminatedScreen = function(reason)
> +{
> +    WebInspector.HelpScreen.call(this, WebInspector.UIString("Detached from the target"));
> +    var p = this.contentElement.createChild("p");
> +    p.addStyleClass("help-section");
> +    p.createChild("span").textContent = "Remote debugging has been terminated with reason: ";
> +    p.createChild("span", "error-message").textContent = reason;
> +    p.createChild("br");
> +    p.createChild("span").textContent = "Please re-attach to the new target.";
> +}
> +
> +WebInspector.RemoteDebuggingTerminatedScreen.prototype = {
> +    __proto__: WebInspector.HelpScreen.prototype
> +}

Remove.

> Source/WebInspectorUI/UserInterface/Main.html:119
> +    <link rel="stylesheet" href="HelpScreen.css">

Remove.

> Source/WebInspectorUI/UserInterface/Main.html:147
> +    <script src="View.js"></script>
> +    <script src="HelpScreen.js"></script>

Remove.

> Source/WebInspectorUI/UserInterface/Setting.js:36
> +var Preferences = {
> +    localizeUI: true,
> +    applicationTitle: "Web Inspector - %s",
> +}
> +

Remove.
Comment 6 Seokju Kwon 2013-07-16 16:35:30 PDT
Created attachment 206825 [details]
Patch
Comment 7 WebKit Commit Bot 2013-07-16 17:30:58 PDT
Comment on attachment 206825 [details]
Patch

Clearing flags on attachment: 206825

Committed r152749: <http://trac.webkit.org/changeset/152749>
Comment 8 WebKit Commit Bot 2013-07-16 17:31:00 PDT
All reviewed patches have been landed.  Closing bug.