Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Integrate GCLI into developer tools global toolbar

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Console
P1
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

({dev-doc-complete})

unspecified
Firefox 15
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 36 obsolete attachments)

540.77 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
Blocks: 717757
Duplicate of this bug: 718729
Duplicate of this bug: 719044
Duplicate of this bug: 671311
Target Milestone: Firefox 13 → Firefox 14
shorlander - please could you attach your global-toolbar mock-ups here when they're done. Thanks.
Whiteboard: [ux-wanted]
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
Created attachment 611528 [details] [diff] [review]
Browser CSS patch - Upload 1
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
Created attachment 611529 [details] [diff] [review]
Devtools CSS patch - Upload 1
Created attachment 611530 [details] [diff] [review]
GCLI update - Upload 1
Created attachment 611531 [details] [diff] [review]
Global Toolbar - Update 1
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.
Created attachment 612897 [details] [diff] [review]
GCLI update - Upload 2
Attachment #611530 - Attachment is obsolete: true
Created attachment 612898 [details] [diff] [review]
Developer Toolbar - Update 2
Attachment #611531 - Attachment is obsolete: true
Attachment #612898 - Flags: feedback?(dcamp)
(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.
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?
(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!
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 :)


- 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.
Created attachment 613026 [details]
GCLI UI Mockups - i01
(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.
Blocks: 683511

Comment 19

5 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+
Created attachment 614077 [details] [diff] [review]
Devtools CSS patch - Upload 2
Attachment #611529 - Attachment is obsolete: true
Created attachment 614078 [details] [diff] [review]
GCLI update - Upload 3
Attachment #614078 - Flags: review?(dcamp)
Created attachment 614079 [details] [diff] [review]
Developer Toolbar - Update 3
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)
Created attachment 614082 [details] [diff] [review]
Test pref changes - Upload 1

Trivial, but lengthy patch.
The webconsole tests no-longer need to check that GCLI is preffed-off
Attachment #614082 - Flags: review?(rcampbell)
Created attachment 614084 [details] [diff] [review]
Devtools CSS patch - Upload 3
Attachment #612897 - Attachment is obsolete: true
Attachment #614084 - Flags: review?(rcampbell)
Attachment #614084 - Flags: review?(dcamp)
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)
https://tbpl.mozilla.org/?tree=Try&rev=b80f78894bd4
Created attachment 614406 [details] [diff] [review]
Browser CSS patch - Upload 2
Attachment #614406 - Flags: review?(dao)
Created attachment 614407 [details] [diff] [review]
Devtools CSS patch - Upload 4
Attachment #614084 - Attachment is obsolete: true
Attachment #614407 - Flags: review?(rcampbell)
Attachment #614084 - Flags: review?(rcampbell)
Attachment #614084 - Flags: review?(dcamp)
Created attachment 614408 [details] [diff] [review]
GCLI update - Upload 4
Attachment #614078 - Attachment is obsolete: true
Attachment #614408 - Flags: review?(dcamp)
Attachment #614078 - Flags: review?(dcamp)
Created attachment 614409 [details] [diff] [review]
Developer Toolbar - Update 4
Attachment #614079 - Attachment is obsolete: true
Attachment #614409 - Flags: review?(rcampbell)
Attachment #614079 - Flags: review?(rcampbell)
Attachment #614079 - Flags: review?(dcamp)
https://tbpl.mozilla.org/?tree=Try&rev=d33df0614155
(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.
Created attachment 614460 [details] [diff] [review]
GCLI update - Upload 5
Attachment #614408 - Attachment is obsolete: true
Attachment #614460 - Flags: review?(dcamp)
Attachment #614408 - Flags: review?(dcamp)
Created attachment 614462 [details] [diff] [review]
Developer Toolbar - Update 5
Attachment #614409 - Attachment is obsolete: true
Attachment #614462 - Flags: review?(rcampbell)
Attachment #614409 - Flags: review?(rcampbell)
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 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 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 on attachment 614462 [details] [diff] [review]
Developer Toolbar - Update 5

this patch needs to be rebased after the landing of bug 741322.
Attachment #614462 - Flags: review?(rcampbell)
Blocks: 744906
Attachment #613025 - Attachment is obsolete: true
Attachment #613026 - Attachment is obsolete: true
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-
(In reply to Stephen Horlander from comment #32)
> ...

Thanks, I replied in bug 744906 comment 4.

Comment 41

5 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+
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.
Blocks: 744982
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.
(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.
(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.
(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.
(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.
(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.
(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.
Created attachment 614744 [details] [diff] [review]
Browser CSS patch - Upload 3
Attachment #614406 - Attachment is obsolete: true
Attachment #614744 - Flags: review?(dao)
Attachment #614406 - Flags: review?(dao)
Created attachment 614745 [details] [diff] [review]
Devtools CSS patch - Upload 5
Attachment #614407 - Attachment is obsolete: true
Attachment #614745 - Flags: review?(rcampbell)
Created attachment 614746 [details] [diff] [review]
Developer Toolbar - Update 6
Attachment #614462 - Attachment is obsolete: true
Attachment #614746 - Flags: review?(rcampbell)
(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_
(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.
Created attachment 614822 [details] [diff] [review]
Browser CSS patch - Upload 4

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)
Created attachment 614825 [details] [diff] [review]
GCLI update - Upload 6

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)
Created attachment 614827 [details] [diff] [review]
Developer Toolbar - Update 7

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

5 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
Created attachment 615032 [details] [diff] [review]
Developer Toolbar - Update 8

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)
https://tbpl.mozilla.org/?tree=Try&rev=d81d9d45bd99

Comment 65

5 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+
(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.
(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.
Rob, Dão, Dave - I think we're fairly close to being able to land this bad boy, are there any hold-ups?
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 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+
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)
(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 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">&#187;</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-
(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">&#187;</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.
Created attachment 616060 [details] [diff] [review]
Devtools CSS patch - Upload 6

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
Created attachment 616061 [details] [diff] [review]
Test pref changes - Upload 2

Rebase
Attachment #614082 - Attachment is obsolete: true
Created attachment 616062 [details] [diff] [review]
Developer Toolbar - Update 9
Attachment #615032 - Attachment is obsolete: true
Attachment #616062 - Flags: review?(paul)
https://tbpl.mozilla.org/?tree=Try&rev=b52ac5a8364b
Dão, I'm hoping that you can now r+ the Browser CSS patch (attachment 614822 [details] [diff] [review])
(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">&#187;</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.
(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">&#187;</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.
(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

5 years ago
Attachment #614822 - Flags: review?(dao) → review+
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

5 years ago
Attachment #614825 - Flags: review?(dcamp) → review+
(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.
Created attachment 616498 [details] [diff] [review]
Developer Toolbar - Update 10
Attachment #616062 - Attachment is obsolete: true
Attachment #616498 - Flags: review?(paul)
Attachment #616498 - Flags: review?(dao)
https://tbpl.mozilla.org/?tree=Try&rev=caa9fcce0cb9
(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 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-
(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.
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.
Created attachment 616589 [details] [diff] [review]
Developer Toolbar - Update 11
Attachment #616498 - Attachment is obsolete: true
Attachment #616589 - Flags: review?(paul)
Attachment #616589 - Flags: review?(dao)
Attachment #616498 - Flags: review?(paul)
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-
Created attachment 616621 [details] [diff] [review]
Developer Toolbar - Update 12
Attachment #616589 - Attachment is obsolete: true
Attachment #616621 - Flags: review?(paul)
Attachment #616621 - Flags: review?(dao)
Attachment #616589 - Flags: review?(paul)
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+
Created attachment 616895 [details] [diff] [review]
Developer Toolbar - Update 13
Attachment #616621 - Attachment is obsolete: true
Attachment #616895 - Flags: review?(paul)
Attachment #616621 - Flags: review?(paul)
https://tbpl.mozilla.org/?tree=Try&rev=26812464a766
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+
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.
Created attachment 618224 [details] [diff] [review]
Combined patch

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
https://tbpl.mozilla.org/?tree=Try&rev=9d827d67bfbb
Whiteboard: [ux-wanted] → [ux-wanted][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/86623a74f82a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [ux-wanted][fixed-in-fx-team] → [ux-wanted]
Keywords: dev-doc-needed
Target Milestone: Firefox 14 → Firefox 15
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...
Depends on: 750318
(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 :)
Blocks: 750318
No longer depends on: 750318
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

5 years ago
Blocks: 673148
Created attachment 621955 [details] [diff] [review]
changes to main patch

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 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+
(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.
Created attachment 622321 [details] [diff] [review]
Combined patch - Upload 2
Attachment #618224 - Attachment is obsolete: true
Attachment #621955 - Attachment is obsolete: true
Created attachment 622352 [details] [diff] [review]
Combined patch - Upload 3

Remove A11Y test update, it's back as before now.

https://tbpl.mozilla.org/?tree=Try&rev=75fd0429c01e
Attachment #622321 - Attachment is obsolete: true
Whiteboard: [ux-wanted] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2e5b9ba8358b
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Is there a bug that tracks the bug to fix before preffing on this feature?
No longer blocks: 673148, 683511, 744906, 744982, 750318, 717757

Updated

5 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

Updated

5 years ago
Depends on: 764625

Updated

5 years ago
Depends on: 780735
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
You need to log in before you can comment on or make changes to this bug.