Joe Walker [:jwalker] Assignee
	Description • 12 years ago

To get the ball rolling

	Joe Walker [:jwalker] Assignee
	Comment 1 • 12 years ago

Just a test

	Joe Walker [:jwalker] Assignee
	Comment 2 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 3 • 12 years ago

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

	Mihai Sucan [:msucan]
	Comment 4 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 5 • 12 years ago

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.

	Mihai Sucan [:msucan]
	Comment 6 • 12 years ago

(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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 7 • 12 years ago

(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.

	Joe Walker [:jwalker] Assignee
	Comment 8 • 12 years ago

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

	Mihai Sucan [:msucan]
	Comment 9 • 12 years ago

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?

	Joe Walker [:jwalker] Assignee
	Comment 10 • 12 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 11 • 12 years ago

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

I like this patch!

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 12 • 12 years ago

Does this work fix bug 760409?

	Paul Rouget [:paul]
	Comment 13 • 12 years ago

(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.

	Dão Gottwald [:dao]
	Comment 14 • 12 years ago

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.

	Paul Rouget [:paul]
	Comment 15 • 12 years ago

> 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.

	Dão Gottwald [:dao]
	Comment 16 • 12 years ago

(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.

	Paul Rouget [:paul]
	Comment 17 • 12 years ago

Tools will dynamically add menuitems and commands.

	Paul Rouget [:paul]
	Comment 18 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 20 • 12 years ago

Unfinished. Not for review. For information only.

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

	Paul Rouget [:paul]
	Comment 21 • 12 years ago

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

	Alex Vincent [:WeirdAl]
	Comment 22 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 23 • 12 years ago

(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.

	Dão Gottwald [:dao]
	Comment 24 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 25 • 12 years ago

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).

	Joe Walker [:jwalker] Assignee
	Comment 26 • 12 years ago

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.

	Paul Rouget [:paul]
	Comment 28 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 29 • 12 years ago

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

	(no longer active)
	Comment 30 • 12 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

	Ed Morley [:emorley]
	Comment 31 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 32 • 12 years ago

(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,

	Ed Morley [:emorley]
	Comment 33 • 12 years ago

Thanks Joe :-)

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

	Victor Porof [:vporof][:vp]
	Comment 34 • 12 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 35 • 12 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 38 • 12 years ago

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

	(no longer active)
	Comment 39 • 12 years ago

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

	Paul Rouget [:paul]
	Comment 40 • 12 years ago

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

	Paul Rouget [:paul]
	Comment 41 • 12 years ago

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

	Paul Rouget [:paul]
	Comment 42 • 12 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 43 • 12 years ago

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!

	Will Bamberg [:wbamberg]
	Comment 44 • 10 years ago

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