Closed
Bug 720641
Opened 12 years ago
Closed 12 years ago
Integrate GCLI into developer tools global toolbar
Categories
(DevTools :: Console, defect, P1)
DevTools
Console
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 15
People
(Reporter: jwalker, Assigned: jwalker)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [fixed-in-fx-team])
Attachments
(1 file, 36 obsolete files)
540.77 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•12 years ago
|
Target Milestone: Firefox 13 → Firefox 14
Assignee | ||
Comment 4•12 years ago
|
||
shorlander - please could you attach your global-toolbar mock-ups here when they're done. Thanks.
Whiteboard: [ux-wanted]
Assignee | ||
Comment 5•12 years ago
|
||
This bug will comprise 4 patches (probably merged just before commit): - Code update from GCLI - Browser CSS changes - Devtools CSS changes - Global Toolbar and integration code There are a number of outstanding bits of work from the patches I'm about to upload, the biggest of which are: - UX from shorlander - More integration tests
Assignee | ||
Comment 6•12 years ago
|
||
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Comment 8•12 years ago
|
||
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Comment 10•12 years ago
|
||
Shorlander - ping on the mockups of what this feature should look like - I fear that I'm running out of time to get this into 14. Thanks.
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #611530 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #611531 -
Attachment is obsolete: true
Attachment #612898 -
Flags: feedback?(dcamp)
Comment 13•12 years ago
|
||
(In reply to Joe Walker from comment #10) > Shorlander - ping on the mockups of what this feature should look like - I > fear that I'm running out of time to get this into 14. Thanks. Will have this finished today.
Assignee | ||
Comment 14•12 years ago
|
||
Status: - CSS patches: waiting for UX update - GCLI patch: reviewed on github - Developer Toolbar patch: - Tests done - Biggest outstanding task: how do we pref off a menu item? I may strip out the about:gcli changes from this patch before r? I could also separate out the test changes?
Assignee | ||
Comment 15•12 years ago
|
||
(In reply to Stephen Horlander from comment #13) > (In reply to Joe Walker from comment #10) > > Shorlander - ping on the mockups of what this feature should look like - I > > fear that I'm running out of time to get this into 14. Thanks. > > Will have this finished today. Excellent, thanks!
Comment 16•12 years ago
|
||
First pass at integrating GCLI into the overall DevTools design. - Command Prompt Indicator - We already use the double arrow(chevron) to indicate overflow in the UI. So I tweaked it a bit to look more like an outline of an arrow. There are some alternative ideas at the bottom including just using traditional command prompt indicators like # and $. I kind of like >: though :) - Help Toggle - F1 isn't that discoverable. Placing a visible toggle in the field with a tooltip with the shortcut would make it more obvious and also in line with the mouse driven UI. - Output Pane - Right now the output is floating in a block above the command line field. Attaching it to the whole width of the field will tie them together. A potential drawback to this approach is that it might feel a little heavy if the command line is very wide. - Help Pane - Initially I experimented with a block floating at the cursor position (http://cl.ly/3t1Y30002O1z2Z301F2m). Which is nice because it is contextual but it creates some alignment issues and would require moving the block as you type. Using the same full width UI as output is consistent, allows for a consistent width and in this layout you can often align the input and suggestions. I also have some mockups in the context of the global toolbar. Still need to pull out the exact color and size specs.
Comment 17•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Stephen Horlander from comment #16) > Created attachment 613025 [details] > GCLI UI Design Overview - i01 > > First pass at integrating GCLI into the overall DevTools design. > > - Command Prompt Indicator - > > We already use the double arrow(chevron) to indicate overflow in the UI. So > I tweaked it a bit to look more like an outline of an arrow. There are some > alternative ideas at the bottom including just using traditional command > prompt indicators like # and $. I kind of like >: though :) Can you give me an example of >> for overflow? My reasoning behind >> was that it was like the > we already used, but "more so", and it worked simply anywhere, allowing a change of color and background using only CSS, which unless you know of cool tricks that I don't, you can't do with graphics. (I know you can change one of color/background using transparency, but both, independently?) > - Help Toggle - > > F1 isn't that discoverable. Placing a visible toggle in the field with a > tooltip with the shortcut would make it more obvious and also in line with > the mouse driven UI. I agree that it's not massively discoverable, but on the other hand: - it is 'the standard' - we do tell people about it in the opening blurb that shows every time until they say they've read it - it's not a requirement for using GCLI. The obvious thing to do if you're not sure is to type help, which we also support (and I guess we could add F1 instruction in there too) - the help shows automatically when we think people need it. so this is just for when people say they want help, and we've not guessed On the other hand it's not a huge burden. The location of the (?) clashes somewhat with the (tick) marker that we previously discussed to show that a command worked but had no output. How do you see those interacting? > - Output Pane - > > Right now the output is floating in a block above the command line field. > Attaching it to the whole width of the field will tie them together. > > A potential drawback to this approach is that it might feel a little heavy > if the command line is very wide. Also sometimes there isn't much command line output, so this might be heavy for little content, but we can have a go. > - Help Pane - > > Initially I experimented with a block floating at the cursor position > (http://cl.ly/3t1Y30002O1z2Z301F2m). Which is nice because it is contextual > but it creates some alignment issues and would require moving the block as > you type. > > Using the same full width UI as output is consistent, allows for a > consistent width and in this layout you can often align the input and > suggestions. > > I also have some mockups in the context of the global toolbar. Still need to > pull out the exact color and size specs. I agree that moving the hints as you type would be really annoying, but the hints change depending on which parameter the cursor is in so I think it makes sense to connect the hint to the parameter in some way. One thing I would like to do is to get away from monospace fonts. I don't see any good reason for them other than implementation detail (which is/was the issue, we might be able to fix that now). Can we get away from that? It's certainly not needed in the hint/output area, and possibly not in the input any more, although that will require some experimentation. What's the 'undefined' for in the final example? Thanks.
Comment 19•12 years ago
|
||
Comment on attachment 612897 [details] [diff] [review] GCLI update - Upload 2 For anyone concerned about the 420k GCLI update, I've been reviewing those changes as they happened on github. That review should go quickly.
Attachment #612897 -
Flags: feedback+
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #611529 -
Attachment is obsolete: true
Assignee | ||
Comment 21•12 years ago
|
||
Attachment #614078 -
Flags: review?(dcamp)
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #611528 -
Attachment is obsolete: true
Attachment #612898 -
Attachment is obsolete: true
Attachment #614077 -
Attachment is obsolete: true
Attachment #614079 -
Flags: review?(rcampbell)
Attachment #614079 -
Flags: review?(dcamp)
Attachment #612898 -
Flags: feedback?(dcamp)
Assignee | ||
Comment 23•12 years ago
|
||
Trivial, but lengthy patch. The webconsole tests no-longer need to check that GCLI is preffed-off
Attachment #614082 -
Flags: review?(rcampbell)
Assignee | ||
Comment 24•12 years ago
|
||
Attachment #612897 -
Attachment is obsolete: true
Attachment #614084 -
Flags: review?(rcampbell)
Attachment #614084 -
Flags: review?(dcamp)
Assignee | ||
Comment 25•12 years ago
|
||
I think it's unlikely that we're going to get the theme update in for this release so I'm landing this preffed off, without the CSS updates. There are 5 patches: - Developer tools CSS (CSS changes that are in an iframe only) - GCLI update (reviewed by :dcamp on github, final pull request to go) - Developer Toolbar (will need some browser peer review I think) - Test pref changes (Trivial, but lengthy. Remove gcli pref from webconsole tests) - Browser CSS (Moving to another bug, for later landing)
Assignee | ||
Comment 26•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b80f78894bd4
Assignee | ||
Comment 27•12 years ago
|
||
Attachment #614406 -
Flags: review?(dao)
Assignee | ||
Comment 28•12 years ago
|
||
Attachment #614084 -
Attachment is obsolete: true
Attachment #614407 -
Flags: review?(rcampbell)
Attachment #614084 -
Flags: review?(rcampbell)
Attachment #614084 -
Flags: review?(dcamp)
Assignee | ||
Comment 29•12 years ago
|
||
Attachment #614078 -
Attachment is obsolete: true
Attachment #614408 -
Flags: review?(dcamp)
Attachment #614078 -
Flags: review?(dcamp)
Assignee | ||
Comment 30•12 years ago
|
||
Attachment #614079 -
Attachment is obsolete: true
Attachment #614409 -
Flags: review?(rcampbell)
Attachment #614079 -
Flags: review?(rcampbell)
Attachment #614079 -
Flags: review?(dcamp)
Assignee | ||
Comment 31•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d33df0614155
Comment 32•12 years ago
|
||
(In reply to Joe Walker from comment #18) > Can you give me an example of >> for overflow? > My reasoning behind >> was that it was like the > we already used, but "more > so", and it worked simply anywhere, allowing a change of color and > background using only CSS, which unless you know of cool tricks that I > don't, you can't do with graphics. (I know you can change one of > color/background using transparency, but both, independently?) We use it for toolbar overflow: http://cl.ly/1t1W312V2s172x3b1v01 Although we use a single tailless arrow for scroll left/right also so maybe it doesn't matter that much. Where would we be reusing it so that we need to change the style that often? > I agree that it's not massively discoverable, but on the other hand: > - it is 'the standard' > - we do tell people about it in the opening blurb that shows every time > until they say they've read it > - it's not a requirement for using GCLI. The obvious thing to do if you're > not sure is to type help, which we also support (and I guess we could add F1 > instruction in there too) > - the help shows automatically when we think people need it. so this is just > for when people say they want help, and we've not guessed Fair enough, just a thought :) > The location of the (?) clashes somewhat with the (tick) marker that we > previously discussed to show that a command worked but had no output. How do > you see those interacting? They could align next to each other without much issue assuming we end up needing an indicator like that. > I agree that moving the hints as you type would be really annoying, but the > hints change depending on which parameter the cursor is in so I think it > makes sense to connect the hint to the parameter in some way. Right, let me think about that some more. I have a few things I was trying, I will post those. > One thing I would like to do is to get away from monospace fonts. I don't > see any good reason for them other than implementation detail (which is/was > the issue, we might be able to fix that now). Can we get away from that? > It's certainly not needed in the hint/output area, and possibly not in the > input any more, although that will require some experimentation. This has the makings of a larger and more contentious debate ;) There is no reason we couldn't use the system font here. I think the concern is what kind of font people expect from a command line. Especially in a situation where you might be entering code into it. I do think we should use a consistent font for both the input and the output/help panels. > What's the 'undefined' for in the final example? I actually meant to ask you that ;) It is what showed up in my GCLI output with your patches.
Assignee | ||
Comment 33•12 years ago
|
||
Attachment #614408 -
Attachment is obsolete: true
Attachment #614460 -
Flags: review?(dcamp)
Attachment #614408 -
Flags: review?(dcamp)
Assignee | ||
Comment 34•12 years ago
|
||
Attachment #614409 -
Attachment is obsolete: true
Attachment #614462 -
Flags: review?(rcampbell)
Attachment #614409 -
Flags: review?(rcampbell)
Comment 35•12 years ago
|
||
Comment on attachment 614406 [details] [diff] [review] Browser CSS patch - Upload 2 >+++ b/browser/themes/gnomestripe/browser.css >+.gcli-panel-inner-arrowcontent { >+ padding: 0px; padding: 0; >+.gclitoolbar-input-node { >+ background-color: transparent; >+ -moz-appearance: none; >+ border: none; >+ padding: 0 0 0 22px; Is this correct for RTL? >+.gclitoolbar-complete-node { >+ background-color: transparent; >+ color: transparent; >+ height: 100%; >+ line-height: 100%; >+ padding: 10px 0 0 27px; RTL? >+.gclitoolbar-prompt { >+ color: hsl(25,78%,50%); >+ -moz-appearance: textfield; The text color shouldn't be hard-coded on natively styled backgrounds. >+ font-size: 150%; >+ font-weight: bold; >+ line-height: 100%; >+ padding-top: 1px; >+ padding-left: 3px; RTL? >+++ b/browser/themes/pinstripe/browser.css >+ font-family: Consolas, Inconsolata, "Courier New", monospace; Are some of these fonts Windows- or OS-X-specific?
Comment 36•12 years ago
|
||
Comment on attachment 614082 [details] [diff] [review] Test pref changes - Upload 1 const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test//test-console.html"; we should file a follow-up [good-first-bug] to cleanup those URIs.
Attachment #614082 -
Flags: review?(rcampbell) → review+
Comment 37•12 years ago
|
||
Comment on attachment 614406 [details] [diff] [review] Browser CSS patch - Upload 2 in *stripe/browser.css: +.gcli-panel-inner-arrowcontent { + padding: 0px; +} padding: 0; + padding: 0 0 0 22px; could use just: padding-left: 22px; in winstripe: +.gclitoolbar-input-node, +.gclitoolbar-complete-node, +.gclitoolbar-prompt { + font-family: Consolas, Inconsolata, "Courier New", monospace; +} to be consistent with the webconsole, you should probably use: .webconsole-timestamp { ... font: 12px Consolas, Lucida Console, monospace; } (just the family, not necessarily size)
Comment 38•12 years ago
|
||
Comment on attachment 614462 [details] [diff] [review] Developer Toolbar - Update 5 this patch needs to be rebased after the landing of bug 741322.
Updated•12 years ago
|
Attachment #614462 -
Flags: review?(rcampbell)
Assignee | ||
Updated•12 years ago
|
Attachment #613025 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #613026 -
Attachment is obsolete: true
Comment 39•12 years ago
|
||
Comment on attachment 614407 [details] [diff] [review] Devtools CSS patch - Upload 4 diff --git a/browser/devtools/webconsole/gcli.css b/browser/devtools/webconsole/gcli.css new file mode 100644 --- /dev/null +++ b/browser/devtools/webconsole/gcli.css @@ -0,0 +1,74 @@ why are we putting this in webconsole? We should probably have a globaltoolbar or gcli directory for this stuff. in gclichrome.css: + left: 0; + bottom: 0; + font-family: Segoe UI, Helvetica Neue, Verdana, Arial, sans-serif; should probably fallback to sans-serif if Segoe UI is absent, i.e., on XP. We usually have "quotes" around multi-word font-family names. I think these are going to have to live in the themes directory anyway where you'll be able to specify fonts per platform. +.gcli-tt-error { + font-size: 80%; + color: #900; + padding: 0 10px; padding-right: 10px; ?
Attachment #614407 -
Flags: review?(rcampbell) → review-
Assignee | ||
Comment 40•12 years ago
|
||
(In reply to Stephen Horlander from comment #32) > ... Thanks, I replied in bug 744906 comment 4.
Comment 41•12 years ago
|
||
Comment on attachment 614460 [details] [diff] [review] GCLI update - Upload 5 Review of attachment 614460 [details] [diff] [review]: ----------------------------------------------------------------- This patch is all stuff I've reviewed as it was being written elsewhere.
Attachment #614460 -
Flags: review?(dcamp) → review+
Comment 42•12 years ago
|
||
in firefox.js +// How eager are we to show help: never=1, somtimes=2, always=3 +pref("devtools.gcli.eagerHelper", 2); nit: typo- sometimes browser.xul: + <panel id="gcli-tooltip" + type="arrow" + noautofocus="true" + noautohide="true" + class="gcli-panel"> + <iframe id="gcli-tooltip-frame" src="chrome://browser/content/devtools/gcliblank.xhtml" flex="1"></iframe> should probably break those lines up along the attributes. + <iframe id="gcli-output-frame" src="chrome://browser/content/devtools/gcliblank.xhtml" flex="1"></iframe> and here + <toolbarbutton id="developer-toolbar-webconsole" + label="Web Console" these should have proper entities defined. in DebuggerUI.jsm: -function DebuggerPane(aTab) { - this._tab = aTab; +function DebuggerPane(aDebuggerUI, aTab) { + this._globalUI = aDebuggerUI; this._tab = aTab; missed a line-break! @@ -133,6 +133,8 @@ DebuggerPane.prototype = { this.frame.addEventListener("DebuggerClose", this._close, true); this.frame.setAttribute("src", "chrome://browser/content/debugger.xul"); + + this._globalUI.refreshCommand(); these are liable to break soon when we land the remote UI bits. bug number is failing me... in inspector.jsm: get isInspectorOpen() { - return this.toolbar && !this.toolbar.hidden && this.highlighter; + return !!(this.toolbar && !this.toolbar.hidden && this.highlighter); }, was isInspectorOpen failing for you without the not-not? in DeveloperToolbar.jsm: if you're lazy, like me, you could add a const Cu = Components.utils; to the top of this file. + +/** + * Load the various Command JSMs. + * Should be called when the developer toolbar first opens. + * + * @return an object containing the EXPORTED_SYMBOLS from all the command + * modules. In general there is no reason when JSMs need to export symbols + * except when they need the host environment to inform them of things like the + * current window/document/etc. + */ +function loadCommands() +{ + Components.utils.import("resource:///modules/GcliCommands.jsm", {}); + Components.utils.import("resource:///modules/GcliTiltCommands.jsm", {}); +} this doesn't explicitly return anything does it? Can probably lose the "@return" in the comment. is the object literal even needed in the import() args? I am confuse. Stopping here for now, will pick this up again in the morning.
Assignee | ||
Comment 43•12 years ago
|
||
Rob and Dão both commented on ltr issues. I tried this out in rtl mode, and I came to the conclusion that I wasn't qualified to decide how things should be marked. Bug 744982 will be resolved before we pref this on.
Assignee | ||
Comment 44•12 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #39) > why are we putting this in webconsole? We should probably have a > globaltoolbar or gcli directory for this stuff. Agreed, that's bug 742672.
Assignee | ||
Comment 45•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #35) > Comment on attachment 614406 [details] [diff] [review] > Browser CSS patch - Upload 2 > > >+++ b/browser/themes/gnomestripe/browser.css > > >+.gcli-panel-inner-arrowcontent { > >+ padding: 0px; > > padding: 0; Fixed. > >+.gclitoolbar-input-node { > >+ background-color: transparent; > >+ -moz-appearance: none; > >+ border: none; > >+ padding: 0 0 0 22px; > > Is this correct for RTL? As noted elsewhere, I think so, but I'm not qualified to really know, so bug 744982. > >+.gclitoolbar-prompt { > >+ color: hsl(25,78%,50%); > >+ -moz-appearance: textfield; > > The text color shouldn't be hard-coded on natively styled backgrounds. This isn't really text - it's the prompt. It's likely that this will be changed for an image (like in the web console) before we pref it on, and it's certainly the topic of review in bug 744906. > >+++ b/browser/themes/pinstripe/browser.css > > >+ font-family: Consolas, Inconsolata, "Courier New", monospace; > > Are some of these fonts Windows- or OS-X-specific? Yes, Fixed. Thanks.
Assignee | ||
Comment 46•12 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #36) > Comment on attachment 614082 [details] [diff] [review] > Test pref changes - Upload 1 > > const TEST_URI = > "http://example.com/browser/browser/devtools/webconsole/test//test-console. > html"; > > we should file a follow-up [good-first-bug] to cleanup those URIs. Yes we should. And I did. Bug 745119.
Assignee | ||
Comment 47•12 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #37) > Comment on attachment 614406 [details] [diff] [review] > Browser CSS patch - Upload 2 > > in *stripe/browser.css: > > +.gcli-panel-inner-arrowcontent { > + padding: 0px; > +} > > padding: 0; Fixed > + padding: 0 0 0 22px; > > could use just: > padding-left: 22px; I'm fairly sure the default 0 is significant here. > in winstripe: > > +.gclitoolbar-input-node, > +.gclitoolbar-complete-node, > +.gclitoolbar-prompt { > + font-family: Consolas, Inconsolata, "Courier New", monospace; > +} > > to be consistent with the webconsole, you should probably use: > > .webconsole-timestamp { > ... > font: 12px Consolas, Lucida Console, monospace; > } > > (just the family, not necessarily size) Done.
Assignee | ||
Comment 48•12 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #39) > Comment on attachment 614407 [details] [diff] [review] > Devtools CSS patch - Upload 4 > > diff --git a/browser/devtools/webconsole/gcli.css > b/browser/devtools/webconsole/gcli.css > new file mode 100644 > --- /dev/null > +++ b/browser/devtools/webconsole/gcli.css > @@ -0,0 +1,74 @@ > > in gclichrome.css: > > + left: 0; > + bottom: 0; > + font-family: Segoe UI, Helvetica Neue, Verdana, Arial, sans-serif; > > should probably fallback to sans-serif if Segoe UI is absent, i.e., on XP. > We usually have "quotes" around multi-word font-family names. > > I think these are going to have to live in the themes directory anyway where > you'll be able to specify fonts per platform. > > +.gcli-tt-error { > + font-size: 80%; > + color: #900; > + padding: 0 10px; > > padding-right: 10px; ? As agreed on IRC we're going to land this separately in bug 704916.
Assignee | ||
Comment 49•12 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #42) > in firefox.js > > +// How eager are we to show help: never=1, somtimes=2, always=3 > +pref("devtools.gcli.eagerHelper", 2); > > nit: typo- sometimes Fixed > browser.xul: > + <panel id="gcli-tooltip" > + type="arrow" > + noautofocus="true" > + noautohide="true" > + class="gcli-panel"> > + <iframe id="gcli-tooltip-frame" > src="chrome://browser/content/devtools/gcliblank.xhtml" flex="1"></iframe> > > should probably break those lines up along the attributes. > > + <iframe id="gcli-output-frame" > src="chrome://browser/content/devtools/gcliblank.xhtml" flex="1"></iframe> > > and here Fixed > + <toolbarbutton id="developer-toolbar-webconsole" > + label="Web Console" > > these should have proper entities defined. Fixed (x3) > in DebuggerUI.jsm: > > -function DebuggerPane(aTab) { > - this._tab = aTab; > +function DebuggerPane(aDebuggerUI, aTab) { > + this._globalUI = aDebuggerUI; this._tab = aTab; > > missed a line-break! Fixed. > @@ -133,6 +133,8 @@ DebuggerPane.prototype = { > this.frame.addEventListener("DebuggerClose", this._close, true); > > this.frame.setAttribute("src", "chrome://browser/content/debugger.xul"); > + > + this._globalUI.refreshCommand(); > > these are liable to break soon when we land the remote UI bits. bug number > is failing me... Bug 741322? I hope so, because that broke me. I hope there isn't more! > in inspector.jsm: > > get isInspectorOpen() > { > - return this.toolbar && !this.toolbar.hidden && this.highlighter; > + return !!(this.toolbar && !this.toolbar.hidden && this.highlighter); > }, > > was isInspectorOpen failing for you without the not-not? Pass, that's Dave. Technically it's right. {a:1} && {b:2} -> {b:2} and not 'true' I suspect that it's in here in a bit of overzealous hot mq action. The next change, while correct, hasn't got anything much to do with developer toolbar. So, two correct, but irrelevant changes - in / out? I vote in. The code is better with them there, and I value correct code over historical purity. > in DeveloperToolbar.jsm: > > if you're lazy, like me, you could add a const Cu = Components.utils; to the > top of this file. > > + > +/** > + * Load the various Command JSMs. > + * Should be called when the developer toolbar first opens. > + * > + * @return an object containing the EXPORTED_SYMBOLS from all the command > + * modules. In general there is no reason when JSMs need to export symbols > + * except when they need the host environment to inform them of things like > the > + * current window/document/etc. > + */ > +function loadCommands() > +{ > + Components.utils.import("resource:///modules/GcliCommands.jsm", {}); > + Components.utils.import("resource:///modules/GcliTiltCommands.jsm", {}); > +} > > this doesn't explicitly return anything does it? Can probably lose the > "@return" in the comment. > > is the object literal even needed in the import() args? > > I am confuse. Sorry. Historical artefact. Previously these things did return something. I removed the @return, but left in the object literal because I don't want them messing up the local namespace. Thanks.
Assignee | ||
Comment 50•12 years ago
|
||
Attachment #614406 -
Attachment is obsolete: true
Attachment #614744 -
Flags: review?(dao)
Attachment #614406 -
Flags: review?(dao)
Assignee | ||
Comment 51•12 years ago
|
||
Attachment #614407 -
Attachment is obsolete: true
Attachment #614745 -
Flags: review?(rcampbell)
Assignee | ||
Comment 52•12 years ago
|
||
Attachment #614462 -
Attachment is obsolete: true
Attachment #614746 -
Flags: review?(rcampbell)
Comment 53•12 years ago
|
||
(In reply to Joe Walker from comment #52) > Created attachment 614746 [details] [diff] [review] > Developer Toolbar - Update 6 Everything looks fine. DebuggerUI.prototype = { + refreshCommand: function UIC_refreshCommand() { + let tab = this.aWindow.getBrowser().selectedTab; + let command = this.aWindow.document.getElementById("Tools:Debugger"); Maybe a comment describing what triggers this. We're using win.gBrowser instead of getBrowser() everywhere in the debugger code. UIC_ -> DUI_
Assignee | ||
Comment 54•12 years ago
|
||
(In reply to Victor Porof from comment #53) > (In reply to Joe Walker from comment #52) > > Created attachment 614746 [details] [diff] [review] > > Developer Toolbar - Update 6 > > Everything looks fine. > > DebuggerUI.prototype = { > + refreshCommand: function UIC_refreshCommand() { > + let tab = this.aWindow.getBrowser().selectedTab; > + let command = this.aWindow.document.getElementById("Tools:Debugger"); > > Maybe a comment describing what triggers this. > We're using win.gBrowser instead of getBrowser() everywhere in the debugger > code. > UIC_ -> DUI_ Fixed. Thanks.
Assignee | ||
Comment 55•12 years ago
|
||
The font changes in the last caused a misalignment on mac. Fixed.
Attachment #614744 -
Attachment is obsolete: true
Attachment #614822 -
Flags: review?(dao)
Attachment #614744 -
Flags: review?(dao)
Assignee | ||
Comment 56•12 years ago
|
||
Dave, See #31 - Review fixes. - I double checked the 'early delete' problem, and found another place that needed a fix. - Firefox was intermittently closing the intro box from a 'stray' keyUp event. Fixed.
Attachment #614460 -
Attachment is obsolete: true
Attachment #614825 -
Flags: review?(dcamp)
Assignee | ||
Comment 57•12 years ago
|
||
Includes: - victor's changes - changes to fix a11y test for dbolter/surkov/marco review - robcee's changes
Attachment #614746 -
Attachment is obsolete: true
Attachment #614827 -
Flags: review?(rcampbell)
Attachment #614827 -
Flags: review?(dbolter)
Attachment #614746 -
Flags: review?(rcampbell)
Comment 58•12 years ago
|
||
Comment on attachment 614827 [details] [diff] [review] Developer Toolbar - Update 7 I think this was Alexander's test so might as well move review to him.
Attachment #614827 -
Flags: review?(dbolter) → review?(surkov.alexander)
Comment 59•12 years ago
|
||
first observations, the Web Console button is still enabled by default on first launch. http://cl.ly/FoRx (dismissed the first run dialog) Close button isn't visible. Closing the toolbar via the invisible close button doesn't uncheck the menu in the Web Developer Menu.
Comment 60•12 years ago
|
||
More behavior weirdness found during testing: Opening the Developer Toolbar via the keyboard command then opening the Web Developer menu causes the Web Console button to become activated. --- One solution to setting the state of checkboxes on menu items and button checked state is to use a <broadcaster> for each tool we want to have on the global toolbar. Using these, the tool itself would toggle the state of the broadcaster and the toolbar (and any other interested objects) could simply watch for changes to the tool's broadcaster. Just throwing that out there as an alternative to manually tweaking these things. We'd probably still use something like refreshCommand though it would become refreshBroadcaster and the observer would take care of setting checked state on the buttons. in inspector.jsm get isInspectorOpen() { - return this.toolbar && !this.toolbar.hidden && this.highlighter; + return !!(this.toolbar && !this.toolbar.hidden && this.highlighter); }, ok, it can stay. in DeveloperToolbar.jsm: +/** + * A component to manage the global developer toolbar, which contains a GCLI + * and buttons for various developer tools. + * @param aBrowser The browser window to which this toolbar is attached + * @param aToolbarElement See browser.xul:<toolbar id="developer-toolbar"> + */ +function DeveloperToolbar(aBrowser, aToolbarElement) +{ + if (!commandsLoaded) { + loadCommands(); + commandsLoaded = true; + } the aBrowser parameter is somewhat misleading as it's a browser *window*. We typically use something like _chromeWindow for these references. My mind recoils when it reads things like | this._browser.getBrowser() | +/** + * Show the developer toolbar + */ +DeveloperToolbar.prototype.show = function DT_show() ... + this._browser.setTimeout(function() { + if (!DeveloperToolbar.introShownThisSession) { + this.display.maybeShowIntro(); + DeveloperToolbar.introShownThisSession = true; + } + }.bind(this), 0); oh that's not going to be popular. Why do we need the setTimeout? + + // We could "delete this.display" etc if we have hard-to-track-down memory + // leaks as a belt-and-braces approach, however this prevents our DOM node + // hunter from looking in all the nooks and crannies, so it's better if we + // can be leak-free without + well that's a scary comment. Do we ever delete this.display explicitly? +/** + * Utility for sending notifications + * @param aTopic a NOTIFICATION constant + */ +DeveloperToolbar.prototype._notify = function DT_notify(aTopic) +{ + let data = { toolbar: this }; + data.wrappedJSObject = data; why is this here? I don't think you should ever explicitly add a wrappedJSObject property to an object. +function getVerticalSpacing(aNode, aRoot) { + let win = aNode.ownerDocument.defaultView; + function pxToNum(styles, property) { + return parseInt(styles.getPropertyValue(property).replace(/px$/, ''), 10); + } + let vertSpacing = 0; some spacing around that function definition might improve readability. + do { + let styles = win.getComputedStyle(aNode); + vertSpacing += pxToNum(styles, "padding-top"); + vertSpacing += pxToNum(styles, "padding-bottom"); + vertSpacing += pxToNum(styles, "margin-top"); + vertSpacing += pxToNum(styles, "margin-bottom"); + vertSpacing += pxToNum(styles, "border-top-width"); + vertSpacing += pxToNum(styles, "border-bottom-width"); + + let prev = aNode.previousSibling; + while (prev != null) { + vertSpacing += prev.clientHeight; + prev = prev.previousSibling; + } + + let next = aNode.nextSibling; + while (next != null) { + vertSpacing += next.clientHeight; + next = next.nextSibling; + } + + aNode = aNode.parentNode; + } + while (aNode !== aRoot) I would probably put the while on the same line as the brace though since this may be the only example of do... while() in devtools code, I'll allow you to set the precedent. I will insist on a semi-colon though. ;) + return vertSpacing + 9; magic number detected! please explain via comment or add a constant. Better still, calculate this value explicitly. (is it the scrollbar? Please don't let it be the scrollbar...) +/** + * Display the OutputPanel. + */ +OutputPanel.prototype.show = function OP_show() +{ + this._panel.ownerDocument.defaultView.setTimeout(function() { + this._resize(); + }.bind(this), 0); + noooo! +/** + * Called by GCLI when an command is executed. + */ +OutputPanel.prototype._outputChanged = function OP_outputChanged(aEvent) power-nit: an -> a in the comment +/** + * Hide the OutputPanel. + */ +OutputPanel.prototype.hide = function CLP_hide() +{ + if (this._panel.state == "open" || this._panel.state == "showing") { + this._panel.hidePopup(); + } +}; do you even need the if checks? I think panel.hidePopup() is pretty much a no-op if it's not onscreen. still to be reviewed: tests.
Assignee | ||
Comment 61•12 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #60) > More behavior weirdness found during testing: > > Opening the Developer Toolbar via the keyboard command then opening the Web > Developer menu causes the Web Console button to become activated. > > --- > > One solution to setting the state of checkboxes on menu items and button > checked state is to use a <broadcaster> for each tool we want to have on the > global toolbar. > > Using these, the tool itself would toggle the state of the broadcaster and > the toolbar (and any other interested objects) could simply watch for > changes to the tool's broadcaster. > > Just throwing that out there as an alternative to manually tweaking these > things. > > We'd probably still use something like refreshCommand though it would become > refreshBroadcaster and the observer would take care of setting checked state > on the buttons. This is less apparent on Windows for some reason, but I've managed to repro it now. I'm working on it. > in DeveloperToolbar.jsm: > ... > > the aBrowser parameter is somewhat misleading as it's a browser *window*. We > typically use something like _chromeWindow for these references. My mind > recoils when it reads things like | this._browser.getBrowser() | Fixed. > +/** > + * Show the developer toolbar > + */ > +DeveloperToolbar.prototype.show = function DT_show() > ... > + this._browser.setTimeout(function() { > + if (!DeveloperToolbar.introShownThisSession) { > + this.display.maybeShowIntro(); > + DeveloperToolbar.introShownThisSession = true; > + } > + }.bind(this), 0); > > oh that's not going to be popular. Why do we need the setTimeout? Hmm - I don't think we do there. Removed. > + // We could "delete this.display" etc if we have hard-to-track-down memory > + // leaks as a belt-and-braces approach, however this prevents our DOM node > + // hunter from looking in all the nooks and crannies, so it's better if we > + // can be leak-free without > > well that's a scary comment. Do we ever delete this.display explicitly? No, and don't need to - we're leak free without it. > +/** > + * Utility for sending notifications > + * @param aTopic a NOTIFICATION constant > + */ > +DeveloperToolbar.prototype._notify = function DT_notify(aTopic) > +{ > + let data = { toolbar: this }; > + data.wrappedJSObject = data; > > why is this here? I don't think you should ever explicitly add a > wrappedJSObject property to an object. Dave? > +function getVerticalSpacing(aNode, aRoot) { > + let win = aNode.ownerDocument.defaultView; > + function pxToNum(styles, property) { > + return parseInt(styles.getPropertyValue(property).replace(/px$/, ''), > 10); > + } > + let vertSpacing = 0; > > some spacing around that function definition might improve readability. Fixed. > + do { > + let styles = win.getComputedStyle(aNode); > + vertSpacing += pxToNum(styles, "padding-top"); > + vertSpacing += pxToNum(styles, "padding-bottom"); > + vertSpacing += pxToNum(styles, "margin-top"); > + vertSpacing += pxToNum(styles, "margin-bottom"); > + vertSpacing += pxToNum(styles, "border-top-width"); > + vertSpacing += pxToNum(styles, "border-bottom-width"); > + > + let prev = aNode.previousSibling; > + while (prev != null) { > + vertSpacing += prev.clientHeight; > + prev = prev.previousSibling; > + } > + > + let next = aNode.nextSibling; > + while (next != null) { > + vertSpacing += next.clientHeight; > + next = next.nextSibling; > + } > + > + aNode = aNode.parentNode; > + } > + while (aNode !== aRoot) > > I would probably put the while on the same line as the brace though since > this may be the only example of do... while() in devtools code, I'll allow > you to set the precedent. Much as I dislike our if (...) { // ... } else { // ... } Way of doing things. There is probably some related prior art there, so I changed it to do { // ... } while (...); > I will insist on a semi-colon though. ;) Fixed. > + return vertSpacing + 9; > > magic number detected! > > please explain via comment or add a constant. Better still, calculate this > value explicitly. > > (is it the scrollbar? Please don't let it be the scrollbar...) It's not the scrollbar. Sometime I'll rant about this, but it's late and I need to sleep tonight. > +/** > + * Display the OutputPanel. > + */ > +OutputPanel.prototype.show = function OP_show() > +{ > + this._panel.ownerDocument.defaultView.setTimeout(function() { > + this._resize(); > + }.bind(this), 0); > + > > noooo! This one is needed. Either I'm missing something, or panel sizing is insane. I'd like to be able to tell a panel to size itself, but we have to do it manually. To make matters worse we have to do all sorts of size calculations, which turn out to be wrong by a constant amount (i.e. 9) and then we need to do set the size twice because otherwise is comes out wrong, sometimes, and worst of all, you can't know what size you need until it's actually rendered. The design that shorlander is working on has a different layout. I'm hoping that we can just live with this mess and revisit it when we have a design that looks better. > +/** > + * Called by GCLI when an command is executed. > + */ > +OutputPanel.prototype._outputChanged = function OP_outputChanged(aEvent) > > power-nit: an -> a in the comment Fixed. > +/** > + * Hide the OutputPanel. > + */ > +OutputPanel.prototype.hide = function CLP_hide() > +{ > + if (this._panel.state == "open" || this._panel.state == "showing") { > + this._panel.hidePopup(); > + } > +}; > > do you even need the if checks? I think panel.hidePopup() is pretty much a > no-op if it's not onscreen. Removed. > still to be reviewed: tests.
Comment 62•12 years ago
|
||
(In reply to Joe Walker from comment #61) > (In reply to Rob Campbell [:rc] (:robcee) from comment #60) > > +/** > > + * Utility for sending notifications > > + * @param aTopic a NOTIFICATION constant > > + */ > > +DeveloperToolbar.prototype._notify = function DT_notify(aTopic) > > +{ > > + let data = { toolbar: this }; > > + data.wrappedJSObject = data; > > > > why is this here? I don't think you should ever explicitly add a > > wrappedJSObject property to an object. > > Dave? This lets you pass data to the observer service as an nsISupports. It's not an uncommon pattern in mozilla-central, and we suggest it here: https://developer.mozilla.org/en/Working_with_windows_in_chrome_code#Example_5:_Using_nsIWindowWatcher_for_passing_an_arbritrary_JavaScript_object
Assignee | ||
Comment 63•12 years ago
|
||
Robcee: This patch includes the changes from comment 61, and fixes the toggle button bizarreness. There were 2 fixes: - Add type="checkbox" autocheck="false" to the devToolbar menu items and manually set the state from DeveloperToolbar.toggle() This seemed simpler than the broadcaster stuff to me - Remove onWebDeveloperMenuShowing() in browser.js (and the calls to it from the inc files) Is there a reason for this function - the tests all pass without it?
Attachment #614827 -
Attachment is obsolete: true
Attachment #615032 -
Flags: review?(surkov.alexander)
Attachment #615032 -
Flags: review?(rcampbell)
Attachment #614827 -
Flags: review?(surkov.alexander)
Attachment #614827 -
Flags: review?(rcampbell)
Assignee | ||
Comment 64•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d81d9d45bd99
Comment 65•12 years ago
|
||
Comment on attachment 615032 [details] [diff] [review] Developer Toolbar - Update 8 Review of attachment 615032 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/tests/mochitest/tree/test_dochierarchy.html @@ +31,4 @@ > > is(root.parentDocument, null, > "Wrong parent document of root accessible"); > + is(root.childDocumentCount, 3, I assume you just add two new documents into UI so that should be ok from a11y perspective. But if this is Firefox specific then you should 'if' it for SEAMONKEY like is(root.childDocumentCount, SEAMONKEY ? 1 : 3, "");
Attachment #615032 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 66•12 years ago
|
||
(In reply to alexander :surkov from comment #65) > Comment on attachment 615032 [details] [diff] [review] > Developer Toolbar - Update 8 > > Review of attachment 615032 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/tests/mochitest/tree/test_dochierarchy.html > @@ +31,4 @@ > > > > is(root.parentDocument, null, > > "Wrong parent document of root accessible"); > > + is(root.childDocumentCount, 3, > > I assume you just add two new documents into UI so that should be ok from > a11y perspective. But if this is Firefox specific then you should 'if' it > for SEAMONKEY like > > is(root.childDocumentCount, SEAMONKEY ? 1 : 3, > ""); Thanks Alexander. Done. Patch/Try soon.
Comment 67•12 years ago
|
||
(In reply to Dave Camp (:dcamp) from comment #62) > (In reply to Joe Walker from comment #61) > > (In reply to Rob Campbell [:rc] (:robcee) from comment #60) > > > +/** > > > + * Utility for sending notifications > > > + * @param aTopic a NOTIFICATION constant > > > + */ > > > +DeveloperToolbar.prototype._notify = function DT_notify(aTopic) > > > +{ > > > + let data = { toolbar: this }; > > > + data.wrappedJSObject = data; > > > > > > why is this here? I don't think you should ever explicitly add a > > > wrappedJSObject property to an object. > > > > Dave? > > This lets you pass data to the observer service as an nsISupports. It's not > an uncommon pattern in mozilla-central, and we suggest it here: > > https://developer.mozilla.org/en/ > Working_with_windows_in_chrome_code#Example_5: > _Using_nsIWindowWatcher_for_passing_an_arbritrary_JavaScript_object oh of course. I should've recognized the "data" variable name there. Brain cramp. Nice fixes, Joe! Thanks. More review after coffee.
Assignee | ||
Comment 68•12 years ago
|
||
Rob, Dão, Dave - I think we're fairly close to being able to land this bad boy, are there any hold-ups?
Comment 69•12 years ago
|
||
Comment on attachment 614745 [details] [diff] [review] Devtools CSS patch - Upload 5 in pinstripe +.gcli-row-out { + padding: 0 5px; + line-height: 1.2em; + border-top: none; + border-bottom: none; + + color: ... extra line. looks good!
Attachment #614745 -
Flags: review?(rcampbell) → review+
Comment 70•12 years ago
|
||
Comment on attachment 615032 [details] [diff] [review] Developer Toolbar - Update 8 yup. this'll still need review from a browser peer for browser bits.
Attachment #615032 -
Flags: review?(rcampbell) → review+
Assignee | ||
Comment 71•12 years ago
|
||
Comment on attachment 615032 [details] [diff] [review] Developer Toolbar - Update 8 Paul - this is preffed off while we fix some remaining issues (tracked in bug 745773). I would like to land it so more people can give feedback and see the new way commands work.
Attachment #615032 -
Flags: review?(paul)
Assignee | ||
Comment 72•12 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #69) > Comment on attachment 614745 [details] [diff] [review] > Devtools CSS patch - Upload 5 > > in pinstripe > > +.gcli-row-out { > + padding: 0 5px; > + line-height: 1.2em; > + border-top: none; > + border-bottom: none; > + > + color: ... > > extra line. > > looks good! Thanks Rob. I've removed the extra line(s). I don't propose re-creating the patch just for that, but the change will land with the next change to that file.
Comment 73•12 years ago
|
||
Comment on attachment 615032 [details] [diff] [review] Developer Toolbar - Update 8 >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >@@ -179,6 +179,14 @@ XPCOMUtils.defineLazyGetter(this, "Popup > } > }); > >+var commandExports; Don't pollute the global scope with this. >--- a/browser/base/content/browser.xul >+++ b/browser/base/content/browser.xul >+ <toolbarbutton id="developer-toolbar-closebutton" >+ class="highlighter-closebutton" This class name doesn't seem to make sense... >+ <toolbaritem id="gclitoolbar" flex="1"> >+ <stack class="gclitoolbar-stack-node" flex="1"> >+ <description class="gclitoolbar-prompt">»</description> >+ <description class="gclitoolbar-complete-node"/> >+ <textbox class="gclitoolbar-input-node" rows="1"/> >+ </stack> >+ </toolbaritem> Why did you wrap this in a toolbaritem node? I don't see you use the "gclitoolbar" id anywhere.
Attachment #615032 -
Flags: review?(paul) → review-
Assignee | ||
Comment 74•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #73) > Comment on attachment 615032 [details] [diff] [review] > Developer Toolbar - Update 8 > > >--- a/browser/base/content/browser.js > >+++ b/browser/base/content/browser.js > >@@ -179,6 +179,14 @@ XPCOMUtils.defineLazyGetter(this, "Popup > > } > > }); > > > >+var commandExports; > > Don't pollute the global scope with this. As far as I can see it's not actually used anywhere. I will remove it. > >--- a/browser/base/content/browser.xul > >+++ b/browser/base/content/browser.xul > > >+ <toolbarbutton id="developer-toolbar-closebutton" > >+ class="highlighter-closebutton" > > This class name doesn't seem to make sense... We noticed that we wanted to use the highlighter-closebutton for all developer toolbars. A separate patch (part of the debugger) is changing this. We'll probably fall into line in the UI review. > >+ <toolbaritem id="gclitoolbar" flex="1"> > >+ <stack class="gclitoolbar-stack-node" flex="1"> > >+ <description class="gclitoolbar-prompt">»</description> > >+ <description class="gclitoolbar-complete-node"/> > >+ <textbox class="gclitoolbar-input-node" rows="1"/> > >+ </stack> > >+ </toolbaritem> > > Why did you wrap this in a toolbaritem node? From: https://developer.mozilla.org/en/XUL/toolbaritem - "An item that appears on a toolbar. This element should wrap all items that are not buttons" > I don't see you use the "gclitoolbar" id anywhere. I expect that we will need this for direction:ltr.
Assignee | ||
Comment 75•12 years ago
|
||
I said I wasn't going to do an update to remove a blank line, but since I'm updating anyway ...
Attachment #614745 -
Attachment is obsolete: true
Assignee | ||
Comment 76•12 years ago
|
||
Rebase
Assignee | ||
Updated•12 years ago
|
Attachment #614082 -
Attachment is obsolete: true
Assignee | ||
Comment 77•12 years ago
|
||
Attachment #615032 -
Attachment is obsolete: true
Attachment #616062 -
Flags: review?(paul)
Assignee | ||
Comment 78•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b52ac5a8364b
Assignee | ||
Comment 79•12 years ago
|
||
Dão, I'm hoping that you can now r+ the Browser CSS patch (attachment 614822 [details] [diff] [review])
Comment 80•12 years ago
|
||
(In reply to Joe Walker from comment #74) > > >--- a/browser/base/content/browser.xul > > >+++ b/browser/base/content/browser.xul > > > > >+ <toolbarbutton id="developer-toolbar-closebutton" > > >+ class="highlighter-closebutton" > > > > This class name doesn't seem to make sense... > > We noticed that we wanted to use the highlighter-closebutton for all > developer toolbars. A separate patch (part of the debugger) is changing > this. We'll probably fall into line in the UI review. The class *name* still doesn't make sense. > > >+ <toolbaritem id="gclitoolbar" flex="1"> > > >+ <stack class="gclitoolbar-stack-node" flex="1"> > > >+ <description class="gclitoolbar-prompt">»</description> > > >+ <description class="gclitoolbar-complete-node"/> > > >+ <textbox class="gclitoolbar-input-node" rows="1"/> > > >+ </stack> > > >+ </toolbaritem> > > > > Why did you wrap this in a toolbaritem node? > > From: https://developer.mozilla.org/en/XUL/toolbaritem - "An item that > appears on a toolbar. This element should wrap all items that are not > buttons" Only matters for customizable toolbars. > > I don't see you use the "gclitoolbar" id anywhere. > > I expect that we will need this for direction:ltr. Presumably you could use the stack element for this.
Assignee | ||
Comment 81•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #80) > (In reply to Joe Walker from comment #74) > > > >--- a/browser/base/content/browser.xul > > > >+++ b/browser/base/content/browser.xul > > > > > > >+ <toolbarbutton id="developer-toolbar-closebutton" > > > >+ class="highlighter-closebutton" > > > > > > This class name doesn't seem to make sense... > > > > We noticed that we wanted to use the highlighter-closebutton for all > > developer toolbars. A separate patch (part of the debugger) is changing > > this. We'll probably fall into line in the UI review. > > The class *name* still doesn't make sense. The name is what the debugger patch changes. > > > >+ <toolbaritem id="gclitoolbar" flex="1"> > > > >+ <stack class="gclitoolbar-stack-node" flex="1"> > > > >+ <description class="gclitoolbar-prompt">»</description> > > > >+ <description class="gclitoolbar-complete-node"/> > > > >+ <textbox class="gclitoolbar-input-node" rows="1"/> > > > >+ </stack> > > > >+ </toolbaritem> > > > > > > Why did you wrap this in a toolbaritem node? > > > > From: https://developer.mozilla.org/en/XUL/toolbaritem - "An item that > > appears on a toolbar. This element should wrap all items that are not > > buttons" > > Only matters for customizable toolbars. > > > > I don't see you use the "gclitoolbar" id anywhere. > > > > I expect that we will need this for direction:ltr. > > Presumably you could use the stack element for this. Since we're following the documentation here, I think you need to make a case for changing this, and why it's worth delaying progress to do it. Thanks.
Comment 82•12 years ago
|
||
(In reply to Joe Walker from comment #81) > The name is what the debugger patch changes. What debugger patch, where, when? Why are you adding an unused nonsensical class only to rename it later, rather than /adding/ it later or using a proper name right now? > Since we're following the documentation here, As I just told you, the documentation refers to customizable toolbars.
Updated•12 years ago
|
Attachment #614822 -
Flags: review?(dao) → review+
Comment 83•12 years ago
|
||
Comment on attachment 616062 [details] [diff] [review] Developer Toolbar - Update 9 >+ <key id="key_devToolbar" key="v" command="Tools:DevToolbar" The key needs to be localized. >+#ifdef XP_MACOSX >+ modifiers="accel,alt" >+#else >+ modifiers="accel,shift" >+#endif Fix indentation. Also, previous comments still need to be addressed.
Attachment #616062 -
Flags: review?(paul) → review-
Updated•12 years ago
|
Attachment #614825 -
Flags: review?(dcamp) → review+
Assignee | ||
Comment 84•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #82) > (In reply to Joe Walker from comment #81) > > The name is what the debugger patch changes. > > What debugger patch, where, when? Why are you adding an unused nonsensical > class only to rename it later, rather than /adding/ it later or using a > proper name right now? Bug 692409.
Assignee | ||
Comment 85•12 years ago
|
||
Attachment #616062 -
Attachment is obsolete: true
Attachment #616498 -
Flags: review?(paul)
Attachment #616498 -
Flags: review?(dao)
Assignee | ||
Comment 86•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=caa9fcce0cb9
Comment 87•12 years ago
|
||
(In reply to Joe Walker from comment #84) > (In reply to Dão Gottwald [:dao] from comment #82) > > (In reply to Joe Walker from comment #81) > > > The name is what the debugger patch changes. > > > > What debugger patch, where, when? Why are you adding an unused nonsensical > > class only to rename it later, rather than /adding/ it later or using a > > proper name right now? > > Bug 692409. The patch in that bug doesn't rename the highlighter-closebutton class. It doesn't even assume it exists. So, no reason for you to introduce it here.
Comment 88•12 years ago
|
||
Comment on attachment 616498 [details] [diff] [review] Developer Toolbar - Update 10 Please overcome the resistance to address review comments. Either don't introduce the class here or give it a reasonable name now.
Attachment #616498 -
Flags: review?(dao) → review-
Assignee | ||
Comment 89•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #88) > Comment on attachment 616498 [details] [diff] [review] > Developer Toolbar - Update 10 > > Please overcome the resistance to address review comments. Either don't > introduce the class here or give it a reasonable name now. It's explained in the bug I linked to. The class is not unused or nonsensical. We plan to rename it to something more sensible, but this is not the bug to do it in.
Comment 90•12 years ago
|
||
I'm not sure why this wouldn't be the bug to introduce the class with a sensible name, but so be it. In this case I left you with another simple option: Don't add the class. There's just no reason to introduce the class with a bogus name.
Assignee | ||
Comment 91•12 years ago
|
||
Attachment #616498 -
Attachment is obsolete: true
Attachment #616589 -
Flags: review?(paul)
Attachment #616589 -
Flags: review?(dao)
Attachment #616498 -
Flags: review?(paul)
Comment 92•12 years ago
|
||
Comment on attachment 616589 [details] [diff] [review] Developer Toolbar - Update 11 >+ <key id="key_devToolbar" key="&devToolbarMenu.accesskey;" command="Tools:DevToolbar" (Didn't spot this in the previous iteration.) You can't reuse the access key here. You need to add a separate command key, even if it happens to be the same letter for en-US. >--- a/browser/base/content/browser.xul >+++ b/browser/base/content/browser.xul >@@ -1000,6 +1000,7 @@ > hidden="true"> > #ifdef XP_MACOSX > <toolbarbutton id="highlighter-closebutton" >+ class="highlighter-closebutton" > oncommand="InspectorUI.closeInspectorUI(false);" > tooltiptext="&inspectCloseButton.tooltiptext;"/> > #endif >@@ -1031,10 +1032,65 @@ > </hbox> > #ifndef XP_MACOSX > <toolbarbutton id="highlighter-closebutton" >+ class="highlighter-closebutton" > oncommand="InspectorUI.closeInspectorUI(false);" > tooltiptext="&inspectCloseButton.tooltiptext;"/> > #endif Drop these changes. Looks good otherwise.
Attachment #616589 -
Flags: review?(dao) → review-
Assignee | ||
Comment 93•12 years ago
|
||
Attachment #616589 -
Attachment is obsolete: true
Attachment #616621 -
Flags: review?(paul)
Attachment #616621 -
Flags: review?(dao)
Attachment #616589 -
Flags: review?(paul)
Comment 94•12 years ago
|
||
Comment on attachment 616621 [details] [diff] [review] Developer Toolbar - Update 12 rename devToolbarMenu.commandkey to devToolbar.commandkey or something like this. r=me with that
Attachment #616621 -
Flags: review?(dao) → review+
Assignee | ||
Comment 95•12 years ago
|
||
Attachment #616621 -
Attachment is obsolete: true
Attachment #616895 -
Flags: review?(paul)
Attachment #616621 -
Flags: review?(paul)
Assignee | ||
Comment 96•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=26812464a766
Comment 97•12 years ago
|
||
Comment on attachment 616895 [details] [diff] [review] Developer Toolbar - Update 13 Review of attachment 616895 [details] [diff] [review]: ----------------------------------------------------------------- I just gave it a quick scan since it's had some many eyeballs on it already. ::: browser/devtools/webconsole/gcliblank.xhtml @@ +23,5 @@ > + - The Original Code is GCLI. > + - > + - The Initial Developer of the Original Code is > + - Mozilla Foundation. > + - Portions created by the Initial Developer are Copyright (C) 2010 2010? also, I think we're starting to do MPL2 for new code, though everything will get updated eventually anyway so it doesn't matter much. ::: browser/locales/en-US/chrome/browser/browser.dtd @@ +217,5 @@ > > +<!ENTITY devToolbarCloseButton.tooltiptext "Close Developer Toolbar"> > +<!ENTITY devToolbarMenu.label "Developer Toolbar"> > +<!ENTITY devToolbarMenu.accesskey "v"> > +<!ENTITY devToolbar.commandkey "v"> Most of browser.dtd lines up the values to some degree, at least within blocks (though it's mostly all over the place). It doesn't really matter to me so leave it like this or add space - your call.
Attachment #616895 -
Flags: review?(paul) → review+
Assignee | ||
Comment 98•12 years ago
|
||
Thanks Paul. I think both fixes are worth doing, but not worth re-spinning for, so since you've made it my call I've fixed both in my tree just now, and they'll land with my next change to the dev-toolbar. Thanks again.
Assignee | ||
Comment 99•12 years ago
|
||
About to check this in.
Attachment #614822 -
Attachment is obsolete: true
Attachment #614825 -
Attachment is obsolete: true
Attachment #616060 -
Attachment is obsolete: true
Attachment #616061 -
Attachment is obsolete: true
Attachment #616895 -
Attachment is obsolete: true
Assignee | ||
Comment 100•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9d827d67bfbb
Assignee | ||
Updated•12 years ago
|
Whiteboard: [ux-wanted] → [ux-wanted][fixed-in-fx-team]
Comment 101•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/86623a74f82a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [ux-wanted][fixed-in-fx-team] → [ux-wanted]
Updated•12 years ago
|
Keywords: dev-doc-needed
Target Milestone: Firefox 14 → Firefox 15
Comment 102•12 years ago
|
||
Wrong idea changing this string and leaving the old ID -helpSearchManual=A search string [...] +helpSearchManual=<strong>search string</strong> to use in narrowing down the displayed commands. Regular expressions not supported. And to be honest, I'm not even sure what the new string means...
Comment 103•12 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #102) > Wrong idea changing this string and leaving the old ID Good catch! I filed bug 750318 for this - Francesco, in the future you should feel free to file a bug yourself rather than just commenting in a FIXED bug :)
Assignee | ||
Updated•12 years ago
|
Comment 104•12 years ago
|
||
This patch was in a range which caused a Ts regression, so I backed out the whole range: https://hg.mozilla.org/mozilla-central/rev/24a6a53c714a Please reland after investigating and fixing the regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•12 years ago
|
Blocks: async-webconsole
Assignee | ||
Comment 105•12 years ago
|
||
Rob - these are the changes to the main patch which should fix the talos regression. Summary: Remove the iframes from browser.xul and dynamically generate them. This has the nasty side effect of making the startup asynchronous. So: - show() now takes a callback (called when show is done) - hide may need to be delayed if show is in progress - test execution needs to be delayed (using show callback) - for symmetry with show doing creation work, hide does destroy work Thanks.
Attachment #621955 -
Flags: review?(rcampbell)
Comment 106•12 years ago
|
||
Comment on attachment 621955 [details] [diff] [review] changes to main patch const NOTIFICATIONS = { - /** DeveloperToolbar.show() has been called */ + /** DeveloperToolbar.show() has been called, and we're working on it */ + SHOWING: "developer-toolbar-showing", + + /** DeveloperToolbar.show() has completed */ SHOW: "developer-toolbar-show", these are somewhat inconsistent now. I think SHOWN would make more sense for the completed notification. Don't really care enough to make you respin this though. +DeveloperToolbar.prototype.show = function DT_show(aCallback) ... + let checkLoad = function() { + if (this.tooltipPanel && this.tooltipPanel.loaded && + this.outputPanel && this.outputPanel.loaded) { + this._onload(); + } + }.bind(this); this._input = this._doc.querySelector(".gclitoolbar-input-node"); + this.tooltipPanel = new TooltipPanel(this._doc, this._input, checkLoad); + this.outputPanel = new OutputPanel(this._doc, this._input, checkLoad); ok. nifty. +OutputPanel.prototype.destroy = function OP_destroy() ... + + console.log("OutputPanel.destroy()"); ... +TooltipPanel.prototype.destroy = function TP_destroy() ... + console.log("TooltipPanel.destroy()"); still intended?
Attachment #621955 -
Flags: review?(rcampbell) → review+
Assignee | ||
Comment 107•12 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #106) > Comment on attachment 621955 [details] [diff] [review] > changes to main patch > > const NOTIFICATIONS = { > - /** DeveloperToolbar.show() has been called */ > + /** DeveloperToolbar.show() has been called, and we're working on it */ > + SHOWING: "developer-toolbar-showing", > + > + /** DeveloperToolbar.show() has completed */ > SHOW: "developer-toolbar-show", > > these are somewhat inconsistent now. I think SHOWN would make more sense for > the completed notification. Don't really care enough to make you respin this > though. Good point. Something else that's wrong with SHOWING is that it can be misinterpreted. 'The toolbar is currently showing' is not what is meant - we mean 'The toolbar is in the process of showing itself'. So I propose LOAD/SHOW/HIDE > +OutputPanel.prototype.destroy = function OP_destroy() > ... > + > + console.log("OutputPanel.destroy()"); > ... > +TooltipPanel.prototype.destroy = function TP_destroy() > ... > + console.log("TooltipPanel.destroy()"); > > still intended? Can you tell I was fighting memleaks? Gone now, and I'm going to merge the 2 patches for try/landing.
Assignee | ||
Comment 108•12 years ago
|
||
Attachment #618224 -
Attachment is obsolete: true
Attachment #621955 -
Attachment is obsolete: true
Assignee | ||
Comment 109•12 years ago
|
||
Remove A11Y test update, it's back as before now. https://tbpl.mozilla.org/?tree=Try&rev=75fd0429c01e
Attachment #622321 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Whiteboard: [ux-wanted] → [fixed-in-fx-team]
Comment 110•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2e5b9ba8358b
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 111•12 years ago
|
||
Is there a bug that tracks the bug to fix before preffing on this feature?
Updated•12 years ago
|
Comment 112•12 years ago
|
||
(In reply to Paul Rouget [:paul] from comment #111) > Is there a bug that tracks the bug to fix before preffing on this feature? bug 745773
Comment 113•9 years ago
|
||
I think this is covered by: https://developer.mozilla.org/en-US/docs/Tools/GCLI, so marking 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
•