DevTools: Get rid of WebInspector.panels

DevTools: Get rid of WebInspector.panels

R=eustas, pfeldman, yurys
BUG=351662
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173643

[pfeldman, 2014-05-06 19:56:59]

I don't think this brings us into a better place. Lets work on plumbing these
instances instead. And we should also remove all the showPanel and panel() call
sites since they don't specify the concrete return type.

[pfeldman, 2014-05-06 19:57:13]

https://codereview.chromium.org/268293003/diff/1/LayoutTests/http/tests/inspe...
File LayoutTests/http/tests/inspector/appcache/appcache-test.js (right):

https://codereview.chromium.org/268293003/diff/1/LayoutTests/http/tests/inspe...
LayoutTests/http/tests/inspector/appcache/appcache-test.js:84: var
applicationCacheTreeElement =
WebInspector.inspectorView.panel("resources").applicationCacheListTreeElement;
I don't see how this is better. Lets keep it as is in tests?

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/pr...
File Source/devtools/front_end/profiler/CPUProfileView.js (right):

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/pr...
Source/devtools/front_end/profiler/CPUProfileView.js:649:
WebInspector.inspectorView.panel("profiles").showProfile(recordedProfile);
How does this compile? Getting rid of global accessors means plumbing instances
into call sites.

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/so...
File Source/devtools/front_end/sources/SourcesView.js (right):

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/so...
Source/devtools/front_end/sources/SourcesView.js:80:
WebInspector.inspectorView.panel("sources").showUISourceCode(unsavedSourceCodes[i]);
You should plumb delegates instead.

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/ui...
File Source/devtools/front_end/ui/InspectorView.js (right):

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/InspectorView.js:136: panel: function(panelName,
skipFallback)
Either change the naming scheme or make it pure accessor. skipFallback semantics
is also unclear. Should it always be "true"?

[vsevik, 2014-05-07 08:22:04]

What is the purpose of this change?
This will significantly regress devtools debugging experience (e.g. I frequently
use WebInspector.panels.sources.visibleView._uiSourceCode to access currently
active uiSourceCode and I wouldn't like to type quotes and brackets).
Can we use compiler to enforce not using such accessors in the front-end cod
einstead?

[apavlov, 2014-05-07 10:18:13]

On 2014/05/07 08:22:04, vsevik wrote:
> What is the purpose of this change?

Pavel asked me to do this.

> This will significantly regress devtools debugging experience (e.g. I
frequently
> use WebInspector.panels.sources.visibleView._uiSourceCode to access currently
> active uiSourceCode and I wouldn't like to type quotes and brackets).

You will be able to just as easily use
WebInspector.inspectorView._panels.sources.visibleView._uiSourceCode

> Can we use compiler to enforce not using such accessors in the front-end cod
> einstead?

This seems a wrong approach to me, but you should be able to access the same
panels map using a different field.

[apavlov, 2014-05-07 10:26:13]

https://codereview.chromium.org/268293003/diff/1/LayoutTests/http/tests/inspe...
File LayoutTests/http/tests/inspector/appcache/appcache-test.js (right):

https://codereview.chromium.org/268293003/diff/1/LayoutTests/http/tests/inspe...
LayoutTests/http/tests/inspector/appcache/appcache-test.js:84: var
applicationCacheTreeElement =
WebInspector.inspectorView.panel("resources").applicationCacheListTreeElement;
On 2014/05/06 19:57:14, pfeldman wrote:
> I don't see how this is better. Lets keep it as is in tests?

There doesn't seem to be any point in keeping WebInspector.panels and "just"
forbidding the access in ordinary code. Can we change this to
WebInspector.inspectorView._panels instead?

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/pr...
File Source/devtools/front_end/profiler/CPUProfileView.js (right):

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/pr...
Source/devtools/front_end/profiler/CPUProfileView.js:649:
WebInspector.inspectorView.panel("profiles").showProfile(recordedProfile);
On 2014/05/06 19:57:14, pfeldman wrote:
> How does this compile? Getting rid of global accessors means plumbing
instances
> into call sites.

It's no different from accessing fields of WebInspector.panels. This compiles
due to
https://groups.google.com/forum/#!topic/closure-compiler-discuss/Gdo8xIwXXgo

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/so...
File Source/devtools/front_end/sources/SourcesView.js (right):

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/so...
Source/devtools/front_end/sources/SourcesView.js:80:
WebInspector.inspectorView.panel("sources").showUISourceCode(unsavedSourceCodes[i]);
On 2014/05/06 19:57:14, pfeldman wrote:
> You should plumb delegates instead.

Agree. My point was to do mechanical changes first.

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/ui...
File Source/devtools/front_end/ui/InspectorView.js (right):

https://codereview.chromium.org/268293003/diff/1/Source/devtools/front_end/ui...
Source/devtools/front_end/ui/InspectorView.js:136: panel: function(panelName,
skipFallback)
On 2014/05/06 19:57:14, pfeldman wrote:
> Either change the naming scheme or make it pure accessor. skipFallback
semantics
> is also unclear. Should it always be "true"?

I don't know. AFAIU, the case behind the fallback is restoring an
extension-provided panel when the extension has been disabled. Perhaps, we
should explicitly have the "defaultPanel()" accessor or something.

[yurys, 2014-05-07 14:43:41]

https://codereview.chromium.org/268293003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/profiler/HeapSnapshotView.js (right):

https://codereview.chromium.org/268293003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/profiler/HeapSnapshotView.js:1229:
WebInspector.Revealer.reveal(profile);
We could implement this as an event on corresponding profile type, the panel
could listen to that event and reveal the profile when it is complete.

https://codereview.chromium.org/268293003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/profiler/ProfilesPanel.js (right):

https://codereview.chromium.org/268293003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/profiler/ProfilesPanel.js:433:
this._registerProfileType(types[i]);
Let's register panel as a listener for
HeapSnapshotProfileType.Events.ProfileComplete on
WebInspector.ProfileTypeRegistry.heapSnapshotProfileType and
WebInspector.ProfileTypeRegistry.trackingHeapSnapshotProfileType here. The
listener will call this.showProfile. This way we can get rid of the new
Revealer.

https://codereview.chromium.org/268293003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/profiler/ProfilesPanel.js:1209:
WebInspector.Revealer.reveal(this.profile);
We can store panel reference in a field.

[apavlov, 2014-05-07 15:54:23]

https://codereview.chromium.org/268293003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/profiler/HeapSnapshotView.js (right):

https://codereview.chromium.org/268293003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/profiler/HeapSnapshotView.js:1229:
WebInspector.Revealer.reveal(profile);
On 2014/05/07 14:43:41, yurys wrote:
> We could implement this as an event on corresponding profile type, the panel
> could listen to that event and reveal the profile when it is complete.

Done.

https://codereview.chromium.org/268293003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/profiler/ProfilesPanel.js (right):

https://codereview.chromium.org/268293003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/profiler/ProfilesPanel.js:433:
this._registerProfileType(types[i]);
On 2014/05/07 14:43:41, yurys wrote:
> Let's register panel as a listener for
> HeapSnapshotProfileType.Events.ProfileComplete on
> WebInspector.ProfileTypeRegistry.heapSnapshotProfileType and
> WebInspector.ProfileTypeRegistry.trackingHeapSnapshotProfileType here. The
> listener will call this.showProfile. This way we can get rid of the new
> Revealer.

Done.

https://codereview.chromium.org/268293003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/profiler/ProfilesPanel.js:1209:
WebInspector.Revealer.reveal(this.profile);
On 2014/05/07 14:43:41, yurys wrote:
> We can store panel reference in a field.

Done.

[pfeldman, 2014-05-07 16:04:51]

lgtm. Please show this to yurys@ prior to landing.

[yurys, 2014-05-08 08:08:42]

lgtm

https://codereview.chromium.org/268293003/diff/90001/Source/devtools/front_en...
File Source/devtools/front_end/sources/SourcesView.js (right):

https://codereview.chromium.org/268293003/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/sources/SourcesView.js:80:
WebInspector.Revealer.reveal(unsavedSourceCodes[i]);
Looks like you don't need Revealer here, sourcesPanel.showUISourceCode should
work.

[apavlov, 2014-05-08 11:00:50]

https://codereview.chromium.org/268293003/diff/90001/Source/devtools/front_en...
File Source/devtools/front_end/sources/SourcesView.js (right):

https://codereview.chromium.org/268293003/diff/90001/Source/devtools/front_en...
Source/devtools/front_end/sources/SourcesView.js:80:
WebInspector.Revealer.reveal(unsavedSourceCodes[i]);
On 2014/05/08 08:08:43, yurys wrote:
> Looks like you don't need Revealer here, sourcesPanel.showUISourceCode should
> work.

According to vsevik, sourcesPanel is here in error, it should not be present
from the design standpoint, so he advised using the revealer instead.

[apavlov, 2014-05-08 11:05:30]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-05-08 11:06:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/apavlov@chromium.org/268293003/90001

[commit-bot: I haz the power, 2014-05-08 11:06:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-08 11:06:41]

Failed to apply patch for Source/devtools/front_end/profiler/CPUProfileView.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/devtools/front_end/profiler/CPUProfileView.js
  Hunk #1 FAILED at 646.
  Hunk #2 succeeded at 753 (offset -2 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
Source/devtools/front_end/profiler/CPUProfileView.js.rej

Patch:       Source/devtools/front_end/profiler/CPUProfileView.js
Index: Source/devtools/front_end/profiler/CPUProfileView.js
diff --git a/Source/devtools/front_end/profiler/CPUProfileView.js
b/Source/devtools/front_end/profiler/CPUProfileView.js
index
237942e76572fff502d56843c82a4a01ef8c583d..ce02fff45b467e0c96a4607c229d705e9b0cc3ae
100644
--- a/Source/devtools/front_end/profiler/CPUProfileView.js
+++ b/Source/devtools/front_end/profiler/CPUProfileView.js
@@ -646,7 +646,7 @@ WebInspector.CPUProfileType.prototype = {
             this._profileBeingRecorded.updateStatus("");
             var recordedProfile = this._profileBeingRecorded;
             this._profileBeingRecorded = null;
-            WebInspector.panels.profiles.showProfile(recordedProfile);
+           
this.dispatchEventToListeners(WebInspector.ProfileType.Events.ProfileComplete,
recordedProfile);
         }
         ProfilerAgent.stop(didStopProfiling.bind(this));
     },
@@ -755,11 +755,12 @@ WebInspector.CPUProfileHeader.prototype = {
 
     /**
      * @override
+     * @param {!WebInspector.ProfilesPanel} panel
      * @return {!WebInspector.ProfileSidebarTreeElement}
      */
-    createSidebarTreeElement: function()
+    createSidebarTreeElement: function(panel)
     {
-        return new WebInspector.ProfileSidebarTreeElement(this,
"profile-sidebar-tree-item");
+        return new WebInspector.ProfileSidebarTreeElement(panel, this,
"profile-sidebar-tree-item");
     },
 
     /**

[apavlov, 2014-05-08 11:34:14]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-05-08 11:35:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/apavlov@chromium.org/268293003/110001

[commit-bot: I haz the power, 2014-05-08 12:46:56]

Change committed as 173643