[toolbox] Land the developer tools window

RESOLVED FIXED in Firefox 20

Status

defect
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

(Depends on 2 bugs, {dev-doc-complete})

unspecified
Firefox 20
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

To get the ball rolling
Posted patch v0 (obsolete) — Splinter Review
Just a test
Posted patch v0.0001 (obsolete) — Splinter Review
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.
Assignee: nobody → jwalker
Attachment #658849 - Attachment is obsolete: true
Attachment #669239 - Flags: feedback?(mihai.sucan)
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 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.
Attachment #669239 - Flags: feedback?(mihai.sucan) → feedback+
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.
(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.
(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.
Posted patch v0.0002 (obsolete) — Splinter Review
Mihai - this addresses your feedback, and seems to me mostly working.
Attachment #669239 - Attachment is obsolete: true
Attachment #671433 - Flags: review?(mihai.sucan)
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?
Attachment #671433 - Flags: review?(mihai.sucan) → review+
Posted patch Part 1: Changes to Browser (obsolete) — Splinter Review
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.
Attachment #671433 - Attachment is obsolete: true
Attachment #680708 - Flags: review?(dolske)
Comment on attachment 680708 [details] [diff] [review]
Part 1: Changes to Browser

I like this patch!
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 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.
> 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.
(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.
Tools will dynamically add menuitems and commands.
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.
Summary: [toolbox] Land a skeleton DevTools.jsm → [toolbox] Land the developer tools window
Attachment #680708 - Attachment is obsolete: true
Attachment #680708 - Flags: review?(dolske)
Attachment #680708 - Flags: review?(dao)
Attachment #681132 - Flags: review?(dolske)
Attachment #681132 - Flags: review?(dao)
Posted patch Part 2: Changes to Devtools (obsolete) — Splinter Review
Unfinished. Not for review. For information only.

We will probably be splitting this out further before we land for sign-off.
Is there anything else we can do to help the reviewer(s)?
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.
(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 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.
Attachment #681132 - Flags: review?(dao) → review+
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).
Attachment #681132 - Attachment is obsolete: true
Attachment #681132 - Flags: review?(dolske)
Attachment #683374 - Flags: review?(dolske)
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.
Attachment #683374 - Flags: review?(dolske)
Attachment #681133 - Attachment is obsolete: true
Attachment #683374 - Attachment is obsolete: true
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.
Attachment #684425 - Flags: review+
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
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 20

Updated

7 years ago
Depends on: 813031

Updated

7 years ago
Depends on: 814638

Updated

7 years ago
Depends on: 814641

Updated

7 years ago
Depends on: 814592

Updated

7 years ago
No longer blocks: 814655
Depends on: 814655

Comment 30

7 years ago
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 <jwalker@mozilla.com> - 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
dev-tree-management@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-tree-management
Backout has conflicts, leaving to Joe.
(Please backout and reland rather than fixing in-place, to make talos results clearer)
Blocks: 795268
(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,
Thanks Joe :-)

Backout:
https://hg.mozilla.org/integration/fx-team/rev/07b710216328
Whiteboard: [fixed-in-fx-team]

Updated

7 years ago
Depends on: 807274

Updated

7 years ago
No longer depends on: 807274

Updated

7 years ago
Depends on: 796006
Landing changesets:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=c77b869c2025
https://tbpl.mozilla.org/?rev=d2fbc67f69f5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 817261

Updated

7 years ago
Depends on: 817304

Updated

7 years ago
Depends on: 816990

Updated

7 years ago
Depends on: 817294

Updated

7 years ago
Depends on: 816953

Updated

7 years ago
Depends on: 814844

Updated

7 years ago
Depends on: 817539

Updated

7 years ago
No longer depends on: 817539

Updated

7 years ago
Depends on: 817546

Updated

7 years ago
Depends on: 817565

Updated

7 years ago
Duplicate of this bug: devtools.jsm

Updated

7 years ago
Duplicate of this bug: 707906

Updated

7 years ago
Depends on: 817695

Updated

7 years ago
No longer depends on: 796006

Updated

7 years ago
No longer depends on: 813031

Updated

7 years ago
No longer depends on: 814638

Updated

7 years ago
No longer depends on: 814641

Updated

7 years ago
No longer depends on: 814889

Updated

7 years ago
No longer depends on: 814890

Updated

7 years ago
No longer depends on: 815239

Updated

7 years ago
No longer depends on: 816990

Updated

7 years ago
No longer depends on: 817304
Depends on: 818988
Gavin, why did you remove the preprocessing for toolbox.css?
… we need preprocessing, but we did it wrong (we didn't include shared.inc).
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!

Updated

7 years ago
Depends on: 820447
Depends on: 820502
Depends on: 820818
Depends on: 820853

Updated

7 years ago
Depends on: 820524
Depends on: 822269
Depends on: 822290
Depends on: 823938

Updated

7 years ago
Depends on: 837010

Updated

6 years ago
Depends on: 880098

Updated

6 years ago
Depends on: 894343

Updated

6 years ago
Depends on: 894344

Updated

6 years ago
No longer depends on: 894343

Updated

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

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.