788977 - [toolbox] Land the developer tools window

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Joe Walker [:jwalker]]

To get the ball rolling

Comment 1

[Joe Walker [:jwalker]]

Attachment: v0

Just a test

Comment 2

[Joe Walker [:jwalker]]

Attachment: v0.0001

Mihai - this contains some of Paul's work on the debugger, which is not part of this feedback request, please ignore it. This is just about the changes to the web console.

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 669239 [details] [diff] [review]
v0.0001

Review of attachment 669239 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/webconsole/HUDService.jsm
@@ +543,5 @@
> +  /**
> +   * The current tab location.
> +   * @type string
> +   */
> +  contentLocation: "",

Merge error: fixed already

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 669239 [details] [diff] [review]
v0.0001

Review of attachment 669239 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good. I like the trimming of the Web Console code.

General comments:

- You can certainly remove the web console tests: _close_button.ja and _window_zombie.js.
- I'd like to see the _panel_title.js test morph into something that still works. You can avoid the UI bothering, just check that hud.ui.contentLocation is updated after navigation.
- Please remove the code that does animations in HUDService.jsm and the code that plays with the height - resetHeight(), storeHeight() and friends.

More comments below. Thank you!

::: browser/app/profile/firefox.js
@@ +1079,5 @@
>  
>  // The last Web Console height. This is initially 0 which means that the Web
>  // Console will use the default height next time it shows.
>  // Change to -1 if you do not want the Web Console to remember its last height.
>  pref("devtools.hud.height", 0);

We should remove devtools.hud.height as well.

::: browser/devtools/framework/ToolDefinitions.jsm
@@ +5,5 @@
> +"use strict";
> +
> +const EXPORTED_SYMBOLS = [ "defaultTools" ];
> +
> +Components.utils.import("resource:///modules/WebConsolePanel.jsm");

Can you please put the new jsm in modules/devtools? like the other newer jsms.

::: browser/devtools/webconsole/HUDService.jsm
@@ +115,5 @@
>     *        True if you want to animate the opening of the Web console.
>     * @return object
>     *         The new HeadsUpDisplay instance.
>     */
> +  activateHUDForContext: function HS_activateHUDForContext(aTab, aAnimated, aIframe)

Please update the comment.

@@ +485,5 @@
>   *
>   * @param nsIDOMElement aTab
>   *        The xul:tab for which you want the WebConsole object.
> + * @param nsIDOMElement aIframe
> + *        Optional iframe into which we should create the WebConsole UI.

By the looks of the code iframe is not optional. Am I mistaken?

@@ +576,2 @@
>      this.iframe.setAttribute("tooltip", "aHTMLTooltip");
>      this.iframe.style.height = 0;

Are these three lines needed anymore? The devtools API should handle class names and making tooltips work in iframes.

@@ -564,4 @@
>      this.iframe.setAttribute("tooltip", "aHTMLTooltip");
>      this.iframe.style.height = 0;
>      this.iframe.addEventListener("load", this._onIframeLoad, true);
> -    this.iframe.setAttribute("src", UI_IFRAME_URL);

You can also remove the UI_IFRAME_URL constant.

@@ +617,5 @@
>     *
>     * @param string aPosition
>     *        The desired Web Console UI location: above, below or window.
>     */
> +  positionConsole: function WC_positionConsole()

Please update the comment.

@@ +697,5 @@
>     * @param string aTitle
>     *        New page title.
>     */
>    onLocationChange: function WC_onLocationChange(aURI, aTitle)
>    {

Please remove the method. Update webconsole.js to call owner.onLocationChange() only if it exists.

@@ +797,5 @@
>  
>      if (hudRef && hud) {
> +      HUDService.storeHeight(hudId);
> +
> +      HUDService.animate(hudId, ANIMATE_OUT, function() {

Don't bother with storeHeight() and animate() here.

::: browser/devtools/webconsole/WebConsolePanel.jsm
@@ +45,5 @@
> +  }
> +
> +  let tab = this._target.value;
> +  let parentDoc = iframeWindow.document.defaultView.parent.document;
> +  let iframe = parentDoc.querySelector('#toolbox-panel-iframe-webconsole');

nit: single quotes versus double quotes?

@@ +52,5 @@
> +
> +WebConsolePanel.prototype = {
> +  get target() this._target,
> +
> +  destroy: function WCTI_destroy()

WCP_destroy?

::: browser/devtools/webconsole/webconsole.js
@@ +533,5 @@
>     * @private
>     * @param nsIDOMEvent aEvent
>     */
>    _onPositionConsoleCommand: function WCF__onPositionConsoleCommand(aEvent)
>    {

Please remove this.

@@ +568,5 @@
> +    // this.positionMenuitems[aPosition].setAttribute("checked", true);
> +    // if (this.positionMenuitems.last) {
> +    //   this.positionMenuitems.last.setAttribute("checked", false);
> +    // }
> +    // this.positionMenuitems.last = this.positionMenuitems[aPosition];

This is no longer needed, we can just remove the code.

Comment 5

[Joe Walker [:jwalker]]

Thanks for the feedback.

browser_webconsole_bug_663443_panel_title.js uses HUD.consolePanel in several places. Do you have any advice as to what you'd like it replaced with?

> > this.iframe.setAttribute("tooltip", "aHTMLTooltip");
> Are these three lines needed anymore?

I'm not even sure what that setAttribute does. Can you help?

Will refresh patch soon.

Comment 6

[Mihai Sucan [:msucan]]

(In reply to Joe Walker [:jwalker] from comment #5)
> Thanks for the feedback.
> 
> browser_webconsole_bug_663443_panel_title.js uses HUD.consolePanel in
> several places. Do you have any advice as to what you'd like it replaced
> with?

HUD.ui.contentLocation

Effectively the test would no longer check the panel title - it would check the location, to see if it is updated on page navigation.

> > > this.iframe.setAttribute("tooltip", "aHTMLTooltip");
> > Are these three lines needed anymore?
> 
> I'm not even sure what that setAttribute does. Can you help?

This is a workaround that makes tooltips in iframes show up - otherwise they don't. aHTMLTooltip is a tooltip in browser.xul that has some js event handler that does nice magic.

For the devtools window you may or may not need this attribute. Please test.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Mihai Sucan [:msucan] from comment #6)
> (In reply to Joe Walker [:jwalker] from comment #5)
> For the devtools window you may or may not need this attribute. Please test.

It will need the attribute.

Comment 8

[Joe Walker [:jwalker]]

Attachment: v0.0002

Mihai - this addresses your feedback, and seems to me mostly working.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 671433 [details] [diff] [review]
v0.0002

Review of attachment 671433 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update. Looks good. A couple of nits below.

General thing: in a future patch I'd like the positioning methods from HUDService.jsm and webconsole.js should be eliminated / merged into other methods. That's not something we need to do here - I'll probably do it for the Global Console patch or somewhere else.

Also the web console tests have many failures here, but as I understand it's too early to play with tests.

::: browser/devtools/webconsole/HUDService.jsm
@@ +337,5 @@
>   *
>   * @param nsIDOMElement aTab
>   *        The xul:tab for which you want the WebConsole object.
> + * @param nsIDOMElement aIframe
> + *        iframe into which we should create the WebConsole UI.

nit: upper case comment start.

@@ +344,4 @@
>  {
>    this.tab = aTab;
> +  if (this.tab == null) {
> +    throw new Error('Missing tab');

nit: double quotes.

@@ +348,5 @@
> +  }
> +
> +  this.iframe = aIframe;
> +  if (this.iframe == null) {
> +    console.trace();

Is |console| available in the global scope?

Comment 10

[Joe Walker [:jwalker]]

Attachment: Part 1: Changes to Browser

This is the list of changes to browser that are part of the landing of the developer tools window.

The main body of changes is at https://github.com/joewalker/devtools-window, where it is being reviewed by the developer tools team. I will be creating a patch for the remaining changes probably sometime next week, early during the FF20 cycle.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 680708 [details] [diff] [review]
Part 1: Changes to Browser

I like this patch!

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Does this work fix bug 760409?

Comment 13

[Paul Rouget [:paul]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #11)
> I like this patch!

Yeah, most of tools will now live in an iframe (we call it the Toolbox) and build dynamically their controls.

This will avoid the tools to contaminate browser code :) 

> Does this work fix bug 760409?

Part of it (only for 4 tools), but more will come in the future.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 680708 [details] [diff] [review]
Part 1: Changes to Browser

>+// Toolbox preferences
>+pref("devtools.toolbox.footer.height", 250);
>+pref("devtools.toolbox.sidebar.width", 500);
>+pref("devtools.toolbox.host", "bottom");
>+pref("devtools.toolbox.selectedTool", "webconsole");
>+pref("devtools.toolbox.toolbarspec", '["tilt toggle","scratchpad open","screenshot"]');

toolbarSpec

> // The default Debugger UI settings
> pref("devtools.debugger.ui.height", 250);
>-pref("devtools.debugger.ui.remote-win.width", 900);
>-pref("devtools.debugger.ui.remote-win.height", 400);
>+pref("devtools.debugger.ui.win-x", 0);
>+pref("devtools.debugger.ui.win-y", 0);
>+pref("devtools.debugger.ui.win-width", 900);
>+pref("devtools.debugger.ui.win-height", 400);

I don't see these prefs being used in this patch. Window dimensions and positions are normally stored via XUL persistence. Is there any reason for doing it differently here?

>+# LOCALIZATION NOTE (open.commandkey): The key used to open the debugger in
>+# combination to e.g. ctrl + shift
>+open.commandkey=S
>+
>+# LOCALIZATION NOTE (debuggerMenu.accesskey): The access key used to open the
>+# debugger.
>+debuggerMenu.accesskey=D
>+
>+# LOCALIZATION NOTE (chromeDebuggerWindowTitle): The title displayed for the
>+# chrome (browser) debugger window.
>+chromeDebuggerWindowTitle=Browser Debugger

>+# LOCALIZATION NOTE (ToolboxDebugger.label):
>+# This string is displayed in the title of the tab when the debugger is
>+# displayed inside the developer tools window and in the Developer Tools Menu.
>+ToolboxDebugger.label=Debugger

>+<!ENTITY inspectorHTMLCopyInner.label       "Copy Inner HTML">
>+<!ENTITY inspectorHTMLCopyInner.accesskey   "I">
>+
>+<!ENTITY inspectorHTMLCopyOuter.label       "Copy Outer HTML">
>+<!ENTITY inspectorHTMLCopyOuter.accesskey   "O">
>+
>+<!ENTITY inspectorHTMLDelete.label          "Delete Node">
>+<!ENTITY inspectorHTMLDelete.accesskey      "D">
>+
>+<!ENTITY inspector.selectButton.tooltip     "Select element with mouse">

>+inspector.label=Inspector
>+inspector.commandkey=I
>+inspector.accesskey=I

>+# LOCALIZATION NOTE  (open.commandkey): This the key to use in
>+# conjunction with shift to open the style editor
>+open.commandkey=VK_F7
>+
>+# LOCALIZATION NOTE (open.accesskey): The access key used to open the style
>+# editor.
>+open.accesskey=y

>+<!ENTITY computedViewTitle     "Computed">
>+<!ENTITY ruleViewTitle         "Rules">

>+cmd.commandkey=k
>+webConsoleCmd.accesskey=W

It seems that none of these string are used. Presumably another patch will fix that, but the way this is split is at least inconvenient for review.

Comment 15

[Paul Rouget [:paul]]

> It seems that none of these string are used. Presumably another patch will fix that, but the way this is split is at least inconvenient for review.

I think this patch should not include locales/devtools modifications.

Comment 16

[Dão Gottwald [:dao]]

(In reply to Paul Rouget [:paul] from comment #15)
> > It seems that none of these string are used. Presumably another patch
> > will fix that, but the way this is split is at least inconvenient for review.
> 
> I think this patch should not include locales/devtools modifications.

Then again, this patch makes a bunch of strings unused and we need to make sure to remove them. Also, I'm curious about the replacements for the commands and menu items removed in this patch. I assume there will be replacements, otherwise most of the strings cited in comment 14 wouldn't make sense.

Comment 17

[Paul Rouget [:paul]]

Tools will dynamically add menuitems and commands.

Comment 18

[Paul Rouget [:paul]]

To answer your request: yes, we need to make sure there's no useless strings. We can include the code that makes use of these strings, but this code is not ready for review. I'll let Joe handle that.

Comment 19

[Joe Walker [:jwalker]]

Attachment: Part 1: Changes to Browser (v2)

Comment 20

[Joe Walker [:jwalker]]

Attachment: Part 2: Changes to Devtools

Unfinished. Not for review. For information only.

We will probably be splitting this out further before we land for sign-off.

Comment 21

[Paul Rouget [:paul]]

Is there anything else we can do to help the reviewer(s)?

Comment 22

[Alex Vincent [:WeirdAl]]

Find me a spot where I can help, even as a feedback? reviewer, and I'll be happy to get involved.  My college classes end approx. December 19, and I'll have a full month before the next set of classes begins - including a week off from work as well.

Comment 23

[Joe Walker [:jwalker]]

(In reply to Alex Vincent [:WeirdAl] from comment #22)
> Find me a spot where I can help, even as a feedback? reviewer, and I'll be
> happy to get involved.  My college classes end approx. December 19, and I'll
> have a full month before the next set of classes begins - including a week
> off from work as well.

Thanks Alex - I hope we will have this landed in mozilla-central soon, and we'll have a few weeks to get things really polished, which means we'll need help testing and fixing. You should be able to see when we land by watching this bug.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 681132 [details] [diff] [review]
Part 1: Changes to Browser (v2)

>@@ -1545,21 +1492,21 @@ var gBrowserInit = {
>     Services.obs.notifyObservers(window, "browser-delayed-startup-finished", "");
>     TelemetryTimestamps.add("delayedStartupFinished");
>   },
> 
>   onUnload: function() {
>     // In certain scenarios it's possible for unload to be fired before onload,
>     // (e.g. if the window is being closed after browser.js loads but before the
>     // load completes). In that case, there's nothing to do here.
>-    if (!gStartupRan)
>+    if (!gStartupRan) {
>       return;
>+    }

Avoid this change.

Comment 25

[Joe Walker [:jwalker]]

Attachment: Part 1: Changes to Browser (v3)

Thanks for the r+ Dão.
I removed the change you requested.
We're still hoping to land this bug on the 21st (i.e. Wed).

Comment 26

[Joe Walker [:jwalker]]

Comment on attachment 683374 [details] [diff] [review]
Part 1: Changes to Browser (v3)

Sorry - I'd forgotten that we didn't need more than Dão's review on this.

Comment 27

[Joe Walker [:jwalker]]

Attachment: Combined patch

Comment 28

[Paul Rouget [:paul]]

Comment on attachment 684425 [details] [diff] [review]
Combined patch

There are a couple of new FIXME in this patch. We should file bugs for them.

Comment 29

[Joe Walker [:jwalker]]

Pushed to try many times. Most recent:
https://tbpl.mozilla.org/?tree=Try&rev=fbb3ef086e3a

Push to fx-team:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=132d2e88ef03

Comment 30

[(no longer active)]

This regressed a whole bunch of stuff including Ts.  Please back it out from fx-team before merging to inbound/central.  Thanks!



Regression  Ts Paint, MED Dirty Profile increase 15.8% on MacOSX 10.8 Fx-Team
-------------------------------------------------------------------------------
    Previous: avg 648.412 stddev 24.200 of 30 runs up to revision fcad43f8558f
    New     : avg 751.158 stddev 13.055 of 5 runs since revision 132d2e88ef03
    Change  : +102.746 (15.8% / z=4.246)
    Graph   : http://mzl.la/WOvJ7x

Changeset range: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=fcad43f8558f&tochange=132d2e88ef03

Changesets:
  * http://hg.mozilla.org/integration/fx-team/rev/132d2e88ef03
    : Joe Walker <[e-mail]> - Bug 788977 - [toolbox] Land the developer tools window; r=harth,jwalker,mikeratcliffe,paul,d?o
    : http://bugzilla.mozilla.org/show_bug.cgi?id=788977

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=788977 - [toolbox] Land the developer tools window
_______________________________________________
dev-tree-management mailing list
[e-mail]
https://lists.mozilla.org/listinfo/dev-tree-management

Comment 31

[Ed Morley [:emorley]]

Backout has conflicts, leaving to Joe.
(Please backout and reland rather than fixing in-place, to make talos results clearer)

Comment 32

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley, UTC, email: emorley@moco] from comment #31)
> Backout has conflicts, leaving to Joe.
> (Please backout and reland rather than fixing in-place, to make talos
> results clearer)

Joe?

09:21:45 - edmorley: !seen jwalker
09:21:47 - firebot: jwalker was last seen 72 weeks, 4 days, 1 hour, 17 minutes and a second ago,

Comment 33

[Ed Morley [:emorley]]

Thanks Joe :-)

Backout:
https://hg.mozilla.org/integration/fx-team/rev/07b710216328

Comment 34

[Victor Porof [:vporof][:vp]]

m-c changesets:
https://hg.mozilla.org/mozilla-central/rev/132d2e88ef03
https://hg.mozilla.org/mozilla-central/rev/07b710216328

Comment 35

[Joe Walker [:jwalker]]

Landing changesets:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=c77b869c2025
https://tbpl.mozilla.org/?rev=d2fbc67f69f5

Comment 38

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a27274b6616

Comment 39

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/9a27274b6616

Comment 40

[Paul Rouget [:paul]]

Gavin, why did you remove the preprocessing for toolbox.css?

Comment 41

[Paul Rouget [:paul]]

Gavin: http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/devtools/toolbox.css#80

Comment 42

[Paul Rouget [:paul]]

… we need preprocessing, but we did it wrong (we didn't include shared.inc).

Comment 43

[:Gavin Sharp [email: [e-mail]]]

I removed it because it appeared to be unnecessary (and produced a build-time warning). If it's needed, feel free to re-add it when you make it necessary!

Comment 44

[Will Bamberg [:wbamberg]]

This has a page now: https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox, so marking as dev-doc-complete.