[DevTools] Add Toolbox page to undocked DevTools frontend.

[DevTools] Add Toolbox page to undocked DevTools frontend.

Toolbox page is the same frontend launched with "toolbox=true" parameter.
It has its own app, which creates a view hierarchy and reports to the main app.

Currently, toolbox is only created for responsive design.

BUG=327641
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175571

[dgozman, 2014-05-27 13:32:27]

Take a look please.

[pfeldman, 2014-05-27 15:37:04]

https://codereview.chromium.org/298913004/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/components/DockController.js (right):

https://codereview.chromium.org/298913004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/components/DockController.js:166:
this._toolboxWindow.addEventListener("DOMContentLoaded",
this._onToolboxWindowLoadedBound, false);
DOMContentLoaded should fire just once.

https://codereview.chromium.org/298913004/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/components/Toolbox.js (right):

https://codereview.chromium.org/298913004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/components/Toolbox.js:48:
this._createToolboxChild(WebInspector.dockController.toolboxWindow());
It seems like Toolbox and DockController share the toolbox docking
responsibilities. Should we move the code from dock controller here?

https://codereview.chromium.org/298913004/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/main/Main.js (left):

https://codereview.chromium.org/298913004/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/main/Main.js:101:
_updateRootSplitViewOnDockSideChange: function()
Where did this code go?

[lushnikov, 2014-05-28 09:27:14]

I'm not sure what the change does, but given it adds something that depends on
both UI and SDK - could you share a screenshot?

[dgozman, 2014-06-03 09:36:09]

Please take another look.

Screenshot for those who are interested: http://pbrd.co/1o2S4dA.

[dgozman, 2014-06-04 07:52:25]

Take a look please. Screenshot: http://pbrd.co/1o2S4dA.

[apavlov, 2014-06-04 09:04:25]

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/ResponsiveDesignView.js (right):

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/ResponsiveDesignView.js:199:
//console.error("_drawCanvas " + cssCanvasWidth + "x" + cssCanvasHeight + (new
Error().stack));
please remove

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/ResponsiveDesignView.js:322: if (!this._enabled ||
!this.isShowing())
Is (!this.isShowing()) by chance sufficient?

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/ResponsiveDesignView.js:373: if
(!this.availableDipSize().isEqual(oldSize))
equals() for consistency (with Java)? :)

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/main/AdvancedApp.js (right):

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:135: if (this._changingDockSide) {
We prefer early returns:

if (!this._changingDocSide)
    return;

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:188: WebInspector.ToolboxApp =
function()
Shouldn't this extend WebInspector.App?

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:199: var main =
window.opener.WebInspector.app;
mainApp?

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:202:
this._inspectedPagePlaceholder.addEventListener(WebInspector.InspectedPagePlaceholder.Events.Update,
main._onSetInspectedPageBounds.bind(main, true), main);
You don't need the last argument (main), since the callback is already bound to
its receiver.

Also, is this listener ever removed when the devtools get docked?

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:206: } else
braces around the else-arm

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:210: };
we don't normally place ';' after function literals

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:212:
WebInspector.ToolboxApp.prototype = {
If there's no inheritance, this is not needed

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/Main.js:156: // This setting is added for
backwards compatibility with Devtools CodeSchool extension. DO NOT REMOVE
added -> needed

[dgozman, 2014-06-04 11:16:19]

PTAL

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/ResponsiveDesignView.js (right):

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/ResponsiveDesignView.js:199:
//console.error("_drawCanvas " + cssCanvasWidth + "x" + cssCanvasHeight + (new
Error().stack));
On 2014/06/04 09:04:24, apavlov wrote:
> please remove

Ops. Done.

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/ResponsiveDesignView.js:322: if (!this._enabled ||
!this.isShowing())
On 2014/06/04 09:04:24, apavlov wrote:
> Is (!this.isShowing()) by chance sufficient?

No, this view can be disabled but showing.

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/ResponsiveDesignView.js:373: if
(!this.availableDipSize().isEqual(oldSize))
On 2014/06/04 09:04:24, apavlov wrote:
> equals() for consistency (with Java)? :)

I have found approximately the same number of isEqual and equals methods.
Perhaps, we can unify that in a separate patch.

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/main/AdvancedApp.js (right):

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:135: if (this._changingDockSide) {
On 2014/06/04 09:04:25, apavlov wrote:
> We prefer early returns:
> 
> if (!this._changingDocSide)
>     return;

Done.

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:188: WebInspector.ToolboxApp =
function()
On 2014/06/04 09:04:25, apavlov wrote:
> Shouldn't this extend WebInspector.App?

Not really. Renamed to just WebInspector.Toolbox.

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:199: var main =
window.opener.WebInspector.app;
On 2014/06/04 09:04:25, apavlov wrote:
> mainApp?

Renamed to |advancedApp| and added typecast.

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:202:
this._inspectedPagePlaceholder.addEventListener(WebInspector.InspectedPagePlaceholder.Events.Update,
main._onSetInspectedPageBounds.bind(main, true), main);
On 2014/06/04 09:04:24, apavlov wrote:
> You don't need the last argument (main), since the callback is already bound
to
> its receiver.
Done.

> 
> Also, is this listener ever removed when the devtools get docked?
No. AdvancedApp filters out unnecessary events. We also don't get this event
when the window is not showing anywhere.

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:206: } else
On 2014/06/04 09:04:24, apavlov wrote:
> braces around the else-arm

Done.

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:210: };
On 2014/06/04 09:04:25, apavlov wrote:
> we don't normally place ';' after function literals

Done.

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/AdvancedApp.js:212:
WebInspector.ToolboxApp.prototype = {
On 2014/06/04 09:04:24, apavlov wrote:
> If there's no inheritance, this is not needed

Done.

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/298913004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/main/Main.js:156: // This setting is added for
backwards compatibility with Devtools CodeSchool extension. DO NOT REMOVE
On 2014/06/04 09:04:25, apavlov wrote:
> added -> needed

Done.

[pfeldman, 2014-06-05 12:25:48]

lgtm

[dgozman, 2014-06-05 12:31:29]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-06-05 12:32:16]

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

[commit-bot: I haz the power, 2014-06-05 13:33:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10788)

[commit-bot: I haz the power, 2014-06-05 14:16:34]

Change committed as 175571