DevTools: added "overlayContents" mode, where DevTools content is placed around and underneath inse…

DevTools: added "overlayContents" mode, where DevTools content is placed around and underneath insepcted contents.

BUG=318751, 304730
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163695

[dgozman, 2013-11-13 15:09:26]

Take a preliminary look please.

[pfeldman, 2013-11-18 15:07:42]

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Dra...
File Source/devtools/front_end/Drawer.js (right):

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Dra...
Source/devtools/front_end/Drawer.js:35: WebInspector.Drawer =
function(inspectorView)
Drawer does not need to know this much about inspector view. All it needs it for
is for calling doResize. Converting Drawer container into SplitView would fix
his. Maybe a follow up (or preliminary) patch?

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

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Ins...
Source/devtools/front_end/InspectorView.js:47: this._overlay =
this._splitView.mainElement;
_overlayElement

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Ins...
Source/devtools/front_end/InspectorView.js:48: this._container =
this._splitView.sidebarElement;
_devtoolsElement

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Ins...
Source/devtools/front_end/InspectorView.js:110: container: function()
devtoolsElement

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Ins...
Source/devtools/front_end/InspectorView.js:371: // FIXME: make drawer a view.
Yes!

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Ins...
Source/devtools/front_end/InspectorView.js:383:
WebInspector.InspectorView.SplitView = function()
Not sure you need it, InspectorView is not that huge, just inline it.

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Sou...
File Source/devtools/front_end/SourcesPanel.js (right):

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Sou...
Source/devtools/front_end/SourcesPanel.js:928:
WebInspector.inspectorView.container().removeStyleClass("breakpoints-deactivated");
Usage of inspectorView from within panels is discouraged. In this case, we could
just add/remove this class to the this._editorContainer

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Spl...
File Source/devtools/front_end/SplitView.js (right):

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Spl...
Source/devtools/front_end/SplitView.js:395: onLayoutUpdated: function()
There is an event in SidebarView for that, all you need is to pull it into
SplitView. (WebInspector.SidebarView.EventTypes.Resized)

[dgozman, 2013-11-21 16:38:51]

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Dra...
File Source/devtools/front_end/Drawer.js (right):

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Dra...
Source/devtools/front_end/Drawer.js:35: WebInspector.Drawer =
function(inspectorView)
On 2013/11/18 15:07:42, pfeldman wrote:
> Drawer does not need to know this much about inspector view. All it needs it
for
> is for calling doResize. Converting Drawer container into SplitView would fix
> his. Maybe a follow up (or preliminary) patch?

I will look into this later (as a follow-up).

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

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Ins...
Source/devtools/front_end/InspectorView.js:47: this._overlay =
this._splitView.mainElement;
On 2013/11/18 15:07:42, pfeldman wrote:
> _overlayElement

Done.

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Ins...
Source/devtools/front_end/InspectorView.js:48: this._container =
this._splitView.sidebarElement;
On 2013/11/18 15:07:42, pfeldman wrote:
> _devtoolsElement

Done.

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Ins...
Source/devtools/front_end/InspectorView.js:110: container: function()
On 2013/11/18 15:07:42, pfeldman wrote:
> devtoolsElement

Done.

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Ins...
Source/devtools/front_end/InspectorView.js:383:
WebInspector.InspectorView.SplitView = function()
On 2013/11/18 15:07:42, pfeldman wrote:
> Not sure you need it, InspectorView is not that huge, just inline it.

Done.

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Sou...
File Source/devtools/front_end/SourcesPanel.js (right):

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Sou...
Source/devtools/front_end/SourcesPanel.js:928:
WebInspector.inspectorView.container().removeStyleClass("breakpoints-deactivated");
On 2013/11/18 15:07:42, pfeldman wrote:
> Usage of inspectorView from within panels is discouraged. In this case, we
could
> just add/remove this class to the this._editorContainer

Done.

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Spl...
File Source/devtools/front_end/SplitView.js (right):

https://codereview.chromium.org/71633003/diff/1/Source/devtools/front_end/Spl...
Source/devtools/front_end/SplitView.js:395: onLayoutUpdated: function()
On 2013/11/18 15:07:42, pfeldman wrote:
> There is an event in SidebarView for that, all you need is to pull it into
> SplitView. (WebInspector.SidebarView.EventTypes.Resized)

Looks like that event is only fired on the whole element resize, but on on
splitter drag, which is what I need.

[pfeldman, 2013-12-05 15:44:06]

A bunch of nits, otherwise looks good.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/DockController.js (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/DockController.js:43:
this.setDockSide(WebInspector.WorkerManager.isWorkerFrontend() ? "undocked" :
(WebInspector.queryParamsObject["dockSide"] || "bottom"));
There is a number of cases where dock size is not supported (workers, remote
debugging, extensions, etc.). That all should be solved in a unified manner.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/InspectorFrontendHostStub.js (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/InspectorFrontendHostStub.js:86: if (callback)
Please annotate.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/InspectorView.js (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/InspectorView.js:42: this._splitView = new
WebInspector.SplitView(false, "InspectorViewContentsSize", 300, 300);
"InspectorView.splitView"

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/InspectorView.js:47:
this._splitView.addEventListener(WebInspector.SplitView.Events.LayoutChanged,
this._onSplitViewLayout.bind(this));
var overlayView = new
WebInspector.ViewWithResizeCallback(this._onOverlayResized.bind(this)));
overlayView.show(this._overlayElement);

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/InspectorView.js:54:
this._devtoolsElement.classList.add("vbox");
we typically use addStyleClass there.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/SplitView.js (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/SplitView.js:68: WebInspector.SplitView.Events = {
rollback

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/inspector.html (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/inspector.html:71: <script type="text/javascript"
src="SplitView.js"></script>
Rollback

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/inspector.js (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/inspector.js:70: if
(!WebInspector.queryParamsObject["remoteFrontend"] &&
!WebInspector.WorkerManager.isWorkerFrontend())
We need to handle even more cases, not sure you need it here.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/inspector.js:95: return docked &&
!!WebInspector.queryParamsObject["overlayContents"];
I think you only want this bit here:

return !!WebInspector.queryParamsObject["overlayContents"];

In fact, you could check it in-place.

[dgozman, 2013-12-06 16:17:13]

PTAL

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/DockController.js (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/DockController.js:43:
this.setDockSide(WebInspector.WorkerManager.isWorkerFrontend() ? "undocked" :
(WebInspector.queryParamsObject["dockSide"] || "bottom"));
On 2013/12/05 15:44:06, pfeldman wrote:
> There is a number of cases where dock size is not supported (workers, remote
> debugging, extensions, etc.). That all should be solved in a unified manner.

Solved separately.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/InspectorFrontendHostStub.js (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/InspectorFrontendHostStub.js:86: if (callback)
On 2013/12/05 15:44:06, pfeldman wrote:
> Please annotate.

Done.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/InspectorView.js (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/InspectorView.js:42: this._splitView = new
WebInspector.SplitView(false, "InspectorViewContentsSize", 300, 300);
On 2013/12/05 15:44:06, pfeldman wrote:
> "InspectorView.splitView"

Done.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/InspectorView.js:47:
this._splitView.addEventListener(WebInspector.SplitView.Events.LayoutChanged,
this._onSplitViewLayout.bind(this));
On 2013/12/05 15:44:06, pfeldman wrote:
> var overlayView = new
> WebInspector.ViewWithResizeCallback(this._onOverlayResized.bind(this)));
> overlayView.show(this._overlayElement);

Done.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/SplitView.js (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/SplitView.js:68: WebInspector.SplitView.Events = {
On 2013/12/05 15:44:06, pfeldman wrote:
> rollback

Done.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/inspector.html (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/inspector.html:71: <script type="text/javascript"
src="SplitView.js"></script>
On 2013/12/05 15:44:06, pfeldman wrote:
> Rollback

Done.

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
File Source/devtools/front_end/inspector.js (right):

https://codereview.chromium.org/71633003/diff/200001/Source/devtools/front_en...
Source/devtools/front_end/inspector.js:95: return docked &&
!!WebInspector.queryParamsObject["overlayContents"];
On 2013/12/05 15:44:06, pfeldman wrote:
> I think you only want this bit here:
> 
> return !!WebInspector.queryParamsObject["overlayContents"];
> 
> In fact, you could check it in-place.

Done.

[pfeldman, 2013-12-06 16:44:05]

lgtm

[commit-bot: I haz the power, 2013-12-10 12:48:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/71633003/280001

[commit-bot: I haz the power, 2013-12-10 16:09:19]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-12-10 16:28:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/71633003/300001

[commit-bot: I haz the power, 2013-12-10 17:35:44]

Failed to apply patch for Source/devtools/front_end/ResourceTreeModel.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/devtools/front_end/ResourceTreeModel.js
  Hunk #1 FAILED at 177.
  Hunk #2 FAILED at 462.
  2 out of 2 hunks FAILED -- saving rejects to file
Source/devtools/front_end/ResourceTreeModel.js.rej

Patch:       Source/devtools/front_end/ResourceTreeModel.js
Index: Source/devtools/front_end/ResourceTreeModel.js
diff --git a/Source/devtools/front_end/ResourceTreeModel.js
b/Source/devtools/front_end/ResourceTreeModel.js
index
311cb4378993df3276f8842a6480e8d42884e1c8..1e6fecad68306ffdcc4dcee271219439b4786a6e
100644
--- a/Source/devtools/front_end/ResourceTreeModel.js
+++ b/Source/devtools/front_end/ResourceTreeModel.js
@@ -177,8 +177,8 @@ WebInspector.ResourceTreeModel.prototype = {
     },
 
     /**
-     * @param {PageAgent.FrameId} frameId
-     * @param {PageAgent.FrameId} parentFrameId
+     * @param {!PageAgent.FrameId} frameId
+     * @param {?PageAgent.FrameId} parentFrameId
      * @return {?WebInspector.ResourceTreeFrame}
      */
     _frameAttached: function(frameId, parentFrameId)
@@ -462,7 +462,7 @@ WebInspector.ResourceTreeModel.prototype = {
  * @param {!WebInspector.ResourceTreeModel} model
  * @param {?WebInspector.ResourceTreeFrame} parentFrame
  * @param {!PageAgent.FrameId} frameId
- * @param {PageAgent.Frame=} payload
+ * @param {?PageAgent.Frame=} payload
  */
 WebInspector.ResourceTreeFrame = function(model, parentFrame, frameId, payload)
 {

[commit-bot: I haz the power, 2013-12-11 09:10:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/71633003/320001

[commit-bot: I haz the power, 2013-12-11 10:17:02]

Change committed as 163695