Inspector UI Refresh

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

[Adam Roben (:aroben)]

We have plans to refresh the Inspector's UI, with the following goals:

1. Support an integrated JavaScript debugger
2. Utilize screen real estate better, especially when docked
3. Provide clearer separation between DOM/source views
4. Reduce redundancy

The plans and mockups can currently be found at < http://trac.webkit.org/projects/webkit/wiki/ProposedWebInspectorUIRefresh>, though we may want to move them here.

[Adam Roben (:aroben)]

<rdar://problem/5792412>

[Timothy Hatcher]

Created attachment 20337 [details]
Elements screenshot

[Timothy Hatcher]

Created attachment 20338 [details]
Resources screenshot

[Timothy Hatcher]

Created attachment 20339 [details]
Resources screenshot (resource selected)

[Timothy Hatcher]

Created attachment 20340 [details]
Databases screenshot (table view)

[Timothy Hatcher]

Created attachment 20341 [details]
Databases screenshot (query view)

[Timothy Hatcher]

Created attachment 20342 [details]
Elements screenshot (console visible)

[Timothy Hatcher]

Created attachment 20343 [details]
Patch implementing most of the UI refresh

[Timothy Hatcher]

Comment on attachment 20343 [details]
Patch implementing most of the UI refresh

Here is a rough ChangeLog entry. I will not be online to edit or respond until Monday.

2008-04-04  Timothy Hatcher  <[e-mail]>

        Implements the majority of the Inspector UI refresh as shown at:
        http://trac.webkit.org/projects/webkit/wiki/ProposedWebInspectorUIRefresh

        http://bugs.webkit.org/show_bug.cgi?id=17773

        A few areas that have not been reimplemented with the new UI are:
         * Search and search results.
         * Request and response headers in the Resources panel.
         * Changing the sorting, grouping or toggling small rows in the Resources panel.
         * Image and font previews in the icon of resources.

        Reviewed by NOBODY (OOPS!).

        * English.lproj/InspectorLocalizedStrings.js: 
        * WebCore.vcproj/WebCore.vcproj: Added new files.
        * page/InspectorController.cpp: 
        (WebCore::InspectorController::setWindowVisible): Call resetScriptObjects() instead of individual
        clear functions.
        (WebCore::InspectorController::populateScriptObjects): Renamed from populateScriptResources.
        (WebCore::InspectorController::addDatabaseScriptResource): Call addDatabase instead of addResource.
        (WebCore::InspectorController::removeDatabaseScriptResource): Call removeDatabase instead of removeResource.
        (WebCore::InspectorController::resetScriptObjects): Renamed from clearScriptResources.
        (WebCore::InspectorController::didCommitLoad): Call resetScriptObjects() instead of individual
        clear functions.
        * page/InspectorController.h: Rename functions.
        * page/inspector/Console.js: Changed the object name to Console from ConsolePanel. Made it a
        standalone object and not inherit the prototype from Panel. Added code to animate in and out.
        * page/inspector/Database.js: Removed title updating and Resource pseudo-subclassing. Made more
        of a Model object that just encapsulates data. Add a getter for table names.
        * page/inspector/DatabaseQueryView.js: Added. Implements the view seen when selecting a Database
        in the DatabasesPanel. Implemented as an interactive console-like area.
        * page/inspector/DatabaseTableView.js: Added. Implements the view seen when selecting a Database
        Table in the DatabasesPanel. Matches the old Browse view of Database panels.
        * page/inspector/DatabaseView.js: Added. A simple base object for Database views shown in DatabasesPanel.
        * page/inspector/DatabasesPanel.js: Changed the object name to DatabasesPanel from DatabasePanel.
        Implements a panel that shows a sidebar of Databases and Database Tables.
        * page/inspector/ElementsPanel.js: Changed the object name to ElementsPanel from DocumentPanel.
        Imeplements the DOM tree that shows the DOM rooted at the main resource.
        * page/inspector/FontView.js: Changed the object name to FontView from FontPanel.
        * page/inspector/ImageView.js: Changed the object name to ImageView from ImagePanel.
        * page/inspector/Images/clearConsoleButtons.png: Added.
        * page/inspector/Images/consoleButtons.png: Added.
        * page/inspector/Images/darkShadow.png: Flipped.
        * page/inspector/Images/database.png: Modified to be 32x32.
        * page/inspector/Images/databaseTable.png: Added.
        * page/inspector/Images/databasesIcon.png: Added.
        * page/inspector/Images/disclosureTriangleSmallDown.png: Added.
        * page/inspector/Images/disclosureTriangleSmallDownBlack.png: Added.
        * page/inspector/Images/disclosureTriangleSmallDownWhite.png: Added.
        * page/inspector/Images/disclosureTriangleSmallRight.png: Added.
        * page/inspector/Images/disclosureTriangleSmallRightBlack.png: Added.
        * page/inspector/Images/disclosureTriangleSmallRightDown.png: Added.
        * page/inspector/Images/disclosureTriangleSmallRightDownBlack.png: Added.
        * page/inspector/Images/disclosureTriangleSmallRightDownWhite.png: Added.
        * page/inspector/Images/disclosureTriangleSmallRightWhite.png: Added.
        * page/inspector/Images/dockButtons.png: Added.
        * page/inspector/Images/elementsIcon.png: Added.
        * page/inspector/Images/gradientHighlightBottom.png:
        * page/inspector/Images/resourceCSSIcon.png: Added.
        * page/inspector/Images/resourceDocumentIcon.png: Added.
        * page/inspector/Images/resourcePlainIcon.png: Added.
        * page/inspector/Images/resourcesIcon.png: Added.
        * page/inspector/Images/resourcesSizeGraphIcon.png: Added.
        * page/inspector/Images/resourcesTimeGraphIcon.png: Added.
        * page/inspector/Images/scriptsIcon.png: Added.
        * page/inspector/Images/segment.png: Modified to fit the talled status bar.
        * page/inspector/Images/segmentEnd.png: Ditto.
        * page/inspector/Images/segmentHover.png: Ditto.
        * page/inspector/Images/segmentHoverEnd.png: Ditto.
        * page/inspector/Images/segmentSelected.png: Ditto.
        * page/inspector/Images/segmentSelectedEnd.png: Ditto.
        * page/inspector/Images/sidebarSelectionBackground.png: Added.
        * page/inspector/Images/sidebarSelectionBackgroundFocused.png: Added.
        * page/inspector/Images/sidebarSelectionBackgroundInactive.png: Added.
        * page/inspector/Images/sidebarSmallSelectionBackground.png: Added.
        * page/inspector/Images/sidebarSmallSelectionBackgroundFocused.png: Added.
        * page/inspector/Images/sidebarSmallSelectionBackgroundInactive.png: Added.
        * page/inspector/Images/statusbarBackground.png: Added.
        * page/inspector/Images/statusbarBottomBackground.png: Added.
        * page/inspector/Images/statusbarButtons.png: Added.
        * page/inspector/Images/statusbarResizerVertical.png: Added.
        * page/inspector/Images/toolbarItemSelected.png: Added.
        * page/inspector/Panel.js: Added support for toolbar items and status bar items.
        Removed View support.
        * page/inspector/Resource.js: Removed title updating and the ResourceTreeElement. Made more
        of a Model object that just encapsulates data.
        * page/inspector/ResourceCategory.js: Removed the ResourceCategoryTreeElement. Made more
        of a Model object that just encapsulates data.
        * page/inspector/ResourceView.js: Added. A simple base object for Resource views shown in ResourcesPanel.
        * page/inspector/ResourcesPanel.js: Changed the object name to ResourcesPanel from NetworkPanel.
        Imeplements the timeline graph, size graph and resource viewing by using ResourceViews.
        * page/inspector/SidebarTreeElement.js: Added. Inherits from TreeElement and implements
        a section element and a regular element that has an icon, title and optional subtitle.
        * page/inspector/SourceView.js: Changed the object name to SourceView from SourcePanel.
        * page/inspector/StylesSidebarPane.js: 
        * page/inspector/WebKit.qrc: Added new files.
        * page/inspector/inspector.css: New and changed style rules to support the new UI.
        * page/inspector/inspector.html: New and changed HTML for the UI. The search field is disabled
        until search is reimplemented.
        * page/inspector/inspector.js: Removed code related to the sidebar and back-forward lists. Also removed
        code related to navigation to panels. Added code to instantiate the new panels and setup the toolbar.
        * page/inspector/utilities.js: Fix coding style.

[Adam Roben (:aroben)]

Comment on attachment 20343 [details]
Patch implementing most of the UI refresh

First set of comments:

-void InspectorController::clearDatabaseScriptResources()
-{
-#if ENABLE(DATABASE)
-    if (!m_scriptContext || !m_scriptObject)
-        return;
-
     DatabaseResourcesSet::iterator databasesEnd = m_databaseResources.end();
     for (DatabaseResourcesSet::iterator it = m_databaseResources.begin(); it != databasesEnd; ++it) {
         InspectorDatabaseResource* resource = (*it).get();
         resource->setScriptObject(0, 0);
     }
 
-    callSimpleFunction(m_scriptContext, m_scriptObject, "clearDatabaseResources");
-#endif
-}

Looks like we've lost the #if ENABLE(DATABASE) around the uses of DatabaseResourcesSet.

@@ -1629,11 +1594,7 @@ void InspectorController::didCommitLoad(DocumentLoader* loader)
 #endif
 
         if (windowVisible()) {
-            clearScriptConsoleMessages();
-#if ENABLE(DATABASE)
-            clearDatabaseScriptResources();
-#endif
-            clearNetworkTimeline();
+            resetScriptObjects();

It looks like this will now clear all the script resources, even though the old code didn't call clearScriptResources(). Is that OK?

+    this.messagesElement.addEventListener("selectstart", this._messagesSelectStart.bind(this), true);
+    this.messagesElement.addEventListener("click", this._messagesClicked.bind(this), true);

Do we really need to set capture to true on these event listeners?

+    this.messagesElement.handleKeyEvent = this._promptKeyDown.bind(this);

It's a little strange that it's the messagesElement that becomes focused/blurred instead of the promptElement.

+        this.toggleButton.addStyleClass("toggled");

"toggled" isn't a very descriptive class for a toggle button, since it could either mean "toggled on" or "toggled off".

     hide: function()
     {
-        WebInspector.Panel.prototype.hide.call(this);
-        WebInspector.consoleListItem.deselect();
-        this.clearButton.addStyleClass("hidden");
+        if (!this._visible || this._animating)
+            return;

It seems unfortunate that you can't hide the console while it's animating in, but I guess it's not that big an issue since the animation is only .25s long.

+        if (WebInspector.currentFocusElement === this.messagesElement)
+            WebInspector.currentFocusElement = this._previousFocusElement;
+        delete this._previousFocusElement;

It's a little strange that _previousFocusElement is set inside two focus functions and then used here in hide() instead of in a blur function.

+    _messagesBlurred: function(newFocusElement)
+    {
+        if (!this.prompt.isCaretInsidePrompt() || !newFocusElement)
+            return;
+
+        var selectionRange = document.createRange();
+        selectionRange.setStart(newFocusElement, 0);
+        selectionRange.setEnd(newFocusElement, 0);
+
+        var selection = window.getSelection();
+        selection.removeAllRanges();
+        selection.addRange(selectionRange);
+    },

Why is this the Console's responsibility?

+    _startStatusBarDragging: function(event) {

The brace should be on the next line.

+    _statusBarDragging: function(event) {

Ditto.

+        height = Number.constrain(height, 75, window.innerHeight - mainElement.totalOffsetTop - 75);

Can we put this "75" into the Preferences object?

+    _endStatusBarDragging: function(event) {

The brace should be on the next line.

     _evalInInspectedWindow: function(expression)
     {
-        // This with block is needed to work around http://bugs.webkit.org/show_bug.cgi?id=11399
-        with (InspectorController.inspectedWindow()) {
-            return eval(expression);
-        }
+        return InspectorController.inspectedWindow().eval(expression);
     },

Can we make this change separately?

@@ -253,11 +372,6 @@ WebInspector.ConsolePanel.prototype = {
         if (!str.length)
             return;
 
-        this.commandHistory.push(str);
-        this.commandOffset = 0;
-
-        this.promptText = "";
-

What's the purpose of this change?

It seems like a lot of code can be shared between Console and DatabaseQueryView that is currently duplicated. Functions like _focused, _blurred, _promptKeyDown, and _enterKeyPressed seem pretty much identical.

+        function moveBackIfOutside()

In many places within Console.js you use this idiom:

var temporaryFunctionName = function()
{
}

Can we choose one or the other and use it consistently? I personally prefer the idiom you've used in DatabaseQueryView.js.

[Adam Roben (:aroben)]

Comment on attachment 20343 [details]
Patch implementing most of the UI refresh

Second set of comments:

+        var databaseTreeElement = new WebInspector.SidebarDatabaseTreeElement(database);

The name DatabaseSidebarTreeElement makes more sense to me than SidebarDatabaseTreeElement. And you seem to agree, because in the SidebarDatabaseTableTreeElement constructor you choose to use the class name "database-table-sidebar-tree-item".

+        var view;
+        if (database && tableName) {

You don't need to null-check database here, since you already did that above.

+        width = Number.constrain(width, 100, window.innerWidth / 2);

Can this "100" go in the Preferences object?

+        var canidateFocusNode = inspectedRootDocument.body;
+        if (!canidateFocusNode && inspectedRootDocument.documentElement)
+            canidateFocusNode = inspectedRootDocument.documentElement;

This can just be:

var candidateFocusNode = inspectedRootDocument.body || inspectedRootDocument.documentElement;

I think we could rename the ImagePanel -> ImageView and FontPanel -> FontView renames separately.

It seems like many of the View classes could share a base class that provided set/get visible and show/hide.

[Adam Roben (:aroben)]

Comment on attachment 20343 [details]
Patch implementing most of the UI refresh

Third set of comments:

+    refreshResource: function(resource, skipBoundryUpdate, skipSort, immediate)

Typo: skipBoundryUpdate -> skipBoundaryUpdate

+    closeCurrentResource: function()

Seems like this should be named closeVisibleResource for consistency with the rest of the code.

It would be nice if the renames in ResourcesPanel.js could be landed separately from the rest of the changes.

The calculator changes also seem like they could be split out.

+        var subtitle = "";
+        if (this.resource.domain && (!WebInspector.mainResource || (WebInspector.mainResource && this.resource.domain !== WebInspector.mainResource.domain)))
+            subtitle = this.resource.domain;

It would be nice to split this logic out into a function, since both the resource and database sidebars use it.

Why do we have two CompareByTime functions?

[Adam Roben (:aroben)]

Comment on attachment 20343 [details]
Patch implementing most of the UI refresh

Final set of comments:

Why is SidebarSectionTreeElement defined in SidebarTreeElement.js? It looks like it's only used by ResourcePanel.

-                subtitle = WebInspector.linkifyURL(url, url.trimURL(WebInspector.mainResource.domain).escapeHTML());
+                subtitle = WebInspector.linkifyURL(url, url.trimURL(/* WebInspector.mainResource.domain */).escapeHTML());

What's the reason for this? Can a FIXME be added to explain it?

[Timothy Hatcher]

(In reply to comment #10)
> Looks like we've lost the #if ENABLE(DATABASE) around the uses of
> DatabaseResourcesSet.

Fixed. Good catch!

>          if (windowVisible()) {
> -            clearScriptConsoleMessages();
> -#if ENABLE(DATABASE)
> -            clearDatabaseScriptResources();
> -#endif
> -            clearNetworkTimeline();
> +            resetScriptObjects();
> 
> It looks like this will now clear all the script resources, even though the old
> code didn't call clearScriptResources(). Is that OK?

Yes, it is fine to remove all the resources at this point since the main reosurce is added after the reset.

> +    this.messagesElement.addEventListener("selectstart",
> this._messagesSelectStart.bind(this), true);
> +    this.messagesElement.addEventListener("click",
> this._messagesClicked.bind(this), true);
> 
> Do we really need to set capture to true on these event listeners?

The click needs to capture since that intercepts link clicks. The selectstart did not need to capture, so I made it false.

> +    this.messagesElement.handleKeyEvent = this._promptKeyDown.bind(this);
> 
> It's a little strange that it's the messagesElement that becomes
> focused/blurred instead of the promptElement.

The messages element is the focus element so clicking in the document, anywhere in the console area will make it focused. I will add a comment.

> +        this.toggleButton.addStyleClass("toggled");
> 
> "toggled" isn't a very descriptive class for a toggle button, since it could
> either mean "toggled on" or "toggled off".

I will rename this style class to "toggled-on".

> It's a little strange that _previousFocusElement is set inside two focus
> functions and then used here in hide() instead of in a blur function.

The reason I did that is: we need to keep track of the focused element so it can be restored when the console is hidden. We only want to restore on hide, and blur can be called when clicking outside the console. When hide is called we know the console can't stay focused and that is the correct time to restore any previous focus element.

> +    _messagesBlurred: function(newFocusElement)
> +    {
> +        if (!this.prompt.isCaretInsidePrompt() || !newFocusElement)
> +            return;
> +
> +        var selectionRange = document.createRange();
> +        selectionRange.setStart(newFocusElement, 0);
> +        selectionRange.setEnd(newFocusElement, 0);
> +
> +        var selection = window.getSelection();
> +        selection.removeAllRanges();
> +        selection.addRange(selectionRange);
> +    },
> 
> Why is this the Console's responsibility?

This should move to inspector.js and be used for all focusing. I will change that.

> +        height = Number.constrain(height, 75, window.innerHeight -
> mainElement.totalOffsetTop - 75);
> 
> Can we put this "75" into the Preferences object?

Fixed, I also did this for all the other minimum widths and heights for resizable areas.

>      _evalInInspectedWindow: function(expression)
>      {
> -        // This with block is needed to work around
> http://bugs.webkit.org/show_bug.cgi?id=11399
> -        with (InspectorController.inspectedWindow()) {
> -            return eval(expression);
> -        }
> +        return InspectorController.inspectedWindow().eval(expression);
>      },
> 
> Can we make this change separately?

Done, landed in r31683.

> @@ -253,11 +372,6 @@ WebInspector.ConsolePanel.prototype = {
>          if (!str.length)
>              return;
> 
> -        this.commandHistory.push(str);
> -        this.commandOffset = 0;
> -
> -        this.promptText = "";
> -
> 
> What's the purpose of this change?

This was supose to land with the TextPrompt change. Landed in r31684.

> It seems like a lot of code can be shared between Console and DatabaseQueryView
> that is currently duplicated. Functions like _focused, _blurred,
> _promptKeyDown, and _enterKeyPressed seem pretty much identical.

I agree, I would like to tackle that after this all lands. TextPrompt was the first part of this sharing, but I agree there is more.

> +        function moveBackIfOutside()
> 
> In many places within Console.js you use this idiom:
> 
> var temporaryFunctionName = function()
> {
> }
> 
> Can we choose one or the other and use it consistently? I personally prefer the
> idiom you've used in DatabaseQueryView.js.

Fixed. Using named functions now.

[Timothy Hatcher]

(In reply to comment #11)
> The name DatabaseSidebarTreeElement makes more sense to me than
> SidebarDatabaseTreeElement. And you seem to agree, because in the
> SidebarDatabaseTableTreeElement constructor you choose to use the class name
> "database-table-sidebar-tree-item".

I agree, will change.

> +        var view;
> +        if (database && tableName) {
> 
> You don't need to null-check database here, since you already did that above.

Fixed.

> +        width = Number.constrain(width, 100, window.innerWidth / 2);
> 
> Can this "100" go in the Preferences object?

Fixed.

> +        var canidateFocusNode = inspectedRootDocument.body;
> +        if (!canidateFocusNode && inspectedRootDocument.documentElement)
> +            canidateFocusNode = inspectedRootDocument.documentElement;
> 
> This can just be:
> 
> var candidateFocusNode = inspectedRootDocument.body ||
> inspectedRootDocument.documentElement;

Good catch. Fixed.

> I think we could rename the ImagePanel -> ImageView and FontPanel -> FontView
> renames separately.

Done. Landed as r31685.

> It seems like many of the View classes could share a base class that provided
> set/get visible and show/hide.

I will do this, and make a generic View object.

[Timothy Hatcher]

(In reply to comment #12)
+    closeCurrentResource: function()
> 
> Seems like this should be named closeVisibleResource for consistency with the
> rest of the code.

Will change.

> It would be nice if the renames in ResourcesPanel.js could be landed separately
> from the rest of the changes.

I will atempt to seperate them out.

> The calculator changes also seem like they could be split out.

I will try to do this.

> +        var subtitle = "";
> +        if (this.resource.domain && (!WebInspector.mainResource ||
> (WebInspector.mainResource && this.resource.domain !==
> WebInspector.mainResource.domain)))
> +            subtitle = this.resource.domain;
> 
> It would be nice to split this logic out into a function, since both the
> resource and database sidebars use it.

I agree, I will find a place for it.

> Why do we have two CompareByTime functions?

One it on WebInspector.Resource and the other is on WebInspector.SidebarResourceTreeElement. I will make the ones on SidebarResourceTreeElement call through to the ones on Resource. I will also add CompareByDescendingSize and CompareBySize to Resource.

[Timothy Hatcher]

(In reply to comment #13)
> Why is SidebarSectionTreeElement defined in SidebarTreeElement.js? It looks
> like it's only used by ResourcePanel.

Currently it is only used by ResourcesPanel, but SidebarSectionTreeElement is a basic object that can be used with other sidebars later and corresponds with SidebarTreeElement.

> -                subtitle = WebInspector.linkifyURL(url,
> url.trimURL(WebInspector.mainResource.domain).escapeHTML());
> +                subtitle = WebInspector.linkifyURL(url, url.trimURL(/*
> WebInspector.mainResource.domain */).escapeHTML());
> 
> What's the reason for this? Can a FIXME be added to explain it?

I meant to fix this. Will fix.

At the time the DOM tree is populated WebInspector.mainResource might still be NULL/undefined. But we refresh the DOM view when the main resource does change, so WebInspector.mainResource just needs null checked.

[Timothy Hatcher]

Created attachment 20392 [details]
Revised Patch

[Adam Roben (:aroben)]

Comment on attachment 20392 [details]
Revised Patch

+    // The messagesElement is the focusable element so clicking in the document, anywhere
+    // in the console area will make it focused.

This would be a little clearer phrased like this: The messagesElement is the focusable element so clicking anywhere in the console area will focus the prompt.

You should use the license from WebCore/html/PreloadScanner.h in all your new files.

+WebInspector.ElementsPanel.prototype = {
+    toolbarItemClass: "elements",
+
+    get toolbarItemLabel()
+    {
+        return WebInspector.UIString("Elements");
+    },

Is there a reason we're using a getter for toolbarItemLabel but not for toolbarItemClass?

WebInspector.Resource.CompareByDescendingSize can just do: return CompareBySize(a, b) * -1; You could even move that bit of logic up to ResourceSidebarTreeElement.

Should View's show() and hide() methods be private?

You could break out the WebInspector.elementDrag* changes into a separate patch if you really wanted to make me happy.

r=me!

[Timothy Hatcher]

(In reply to comment #19)
> +WebInspector.ElementsPanel.prototype = {
> +    toolbarItemClass: "elements",
> +
> +    get toolbarItemLabel()
> +    {
> +        return WebInspector.UIString("Elements");
> +    },
> 
> Is there a reason we're using a getter for toolbarItemLabel but not for
> toolbarItemClass?

I didn't use a getter in the case where code didn't need to run to generate a value.

> WebInspector.Resource.CompareByDescendingSize can just do: return
> CompareBySize(a, b) * -1; You could even move that bit of logic up to
> ResourceSidebarTreeElement.

Good point, will change.

> Should View's show() and hide() methods be private?

Calling show and hide is valid, and will work the same as setting visible. Sometimes calling show is cleaner, and sometimes setting visible is better (in the case where you toggle: this.visible = !this.visible).

> You could break out the WebInspector.elementDrag* changes into a separate patch
> if you really wanted to make me happy.

I might. :)

[Adam Roben (:aroben)]

(In reply to comment #20)
> (In reply to comment #19)
> > Should View's show() and hide() methods be private?
> 
> Calling show and hide is valid, and will work the same as setting visible.
> Sometimes calling show is cleaner, and sometimes setting visible is better (in
> the case where you toggle: this.visible = !this.visible).

Setting visible will in some cases do slightly less work that calling show()/hide(). I don't see a great benefit to having two ways to accomplish the same thing here. But it doesn't bother me that much.

[Timothy Hatcher]

(In reply to comment #19)
> You could break out the WebInspector.elementDrag* changes into a separate patch
> if you really wanted to make me happy.

Landed in r31732.

[Adam Roben (:aroben)]

Created attachment 20412 [details]
Refreshed UI on Windows

[Timothy Hatcher]

The main part of this has landed. Regression bugs have been filed.