Closed
Bug 788977
Opened 11 years ago
Closed 11 years ago
[toolbox] Land the developer tools window
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 20
People
(Reporter: jwalker, Assigned: jwalker)
References
(Depends on 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 7 obsolete files)
1.22 MB,
patch
|
paul
:
review+
|
Details | Diff | Splinter Review |
To get the ball rolling
Assignee | ||
Comment 1•11 years ago
|
||
Just a test
Assignee | ||
Comment 2•11 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.
Assignee: nobody → jwalker
Attachment #658849 -
Attachment is obsolete: true
Attachment #669239 -
Flags: feedback?(mihai.sucan)
Assignee | ||
Comment 3•11 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
Comment 4•11 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.
Attachment #669239 -
Flags: feedback?(mihai.sucan) → feedback+
Assignee | ||
Comment 5•11 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.
Comment 6•11 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.
Comment 7•11 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.
Assignee | ||
Comment 8•11 years ago
|
||
Mihai - this addresses your feedback, and seems to me mostly working.
Attachment #669239 -
Attachment is obsolete: true
Attachment #671433 -
Flags: review?(mihai.sucan)
Comment 9•11 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?
Attachment #671433 -
Flags: review?(mihai.sucan) → review+
Assignee | ||
Comment 10•11 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.
Attachment #671433 -
Attachment is obsolete: true
Attachment #680708 -
Flags: review?(dolske)
Assignee | ||
Updated•11 years ago
|
Attachment #680708 -
Flags: review?(dao)
Comment 11•11 years ago
|
||
Comment on attachment 680708 [details] [diff] [review] Part 1: Changes to Browser I like this patch!
Comment 12•11 years ago
|
||
Does this work fix bug 760409?
Comment 13•11 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.
Comment 14•11 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.
Comment 15•11 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.
Comment 16•11 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.
Comment 17•11 years ago
|
||
Tools will dynamically add menuitems and commands.
Comment 18•11 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.
Assignee | ||
Updated•11 years ago
|
Summary: [toolbox] Land a skeleton DevTools.jsm → [toolbox] Land the developer tools window
Assignee | ||
Comment 19•11 years ago
|
||
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)
Assignee | ||
Comment 20•11 years ago
|
||
Unfinished. Not for review. For information only. We will probably be splitting this out further before we land for sign-off.
Comment 21•11 years ago
|
||
Is there anything else we can do to help the reviewer(s)?
Comment 22•11 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.
Assignee | ||
Comment 23•11 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.
Comment 24•11 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.
Attachment #681132 -
Flags: review?(dao) → review+
Assignee | ||
Comment 25•11 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).
Attachment #681132 -
Attachment is obsolete: true
Attachment #681132 -
Flags: review?(dolske)
Attachment #683374 -
Flags: review?(dolske)
Assignee | ||
Comment 26•11 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.
Attachment #683374 -
Flags: review?(dolske)
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #681133 -
Attachment is obsolete: true
Attachment #683374 -
Attachment is obsolete: true
Comment 28•11 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.
Attachment #684425 -
Flags: review+
Assignee | ||
Comment 29•11 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
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 20
Updated•11 years ago
|
Comment 30•11 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
Comment 31•11 years ago
|
||
Backout has conflicts, leaving to Joe. (Please backout and reland rather than fixing in-place, to make talos results clearer)
Comment 32•11 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,
Comment 33•11 years ago
|
||
Thanks Joe :-) Backout: https://hg.mozilla.org/integration/fx-team/rev/07b710216328
Whiteboard: [fixed-in-fx-team]
Comment 34•11 years ago
|
||
m-c changesets: https://hg.mozilla.org/mozilla-central/rev/132d2e88ef03 https://hg.mozilla.org/mozilla-central/rev/07b710216328
Assignee | ||
Comment 35•11 years ago
|
||
Landing changesets: https://tbpl.mozilla.org/?tree=Fx-Team&rev=c77b869c2025 https://tbpl.mozilla.org/?rev=d2fbc67f69f5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Comment 40•11 years ago
|
||
Gavin, why did you remove the preprocessing for toolbox.css?
Comment 41•11 years ago
|
||
Gavin: http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/devtools/toolbox.css#80
Comment 42•11 years ago
|
||
… we need preprocessing, but we did it wrong (we didn't include shared.inc).
Comment 43•11 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!
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 44•9 years ago
|
||
This has a page now: https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox, so marking as dev-doc-complete.
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•