720641 - Integrate GCLI into developer tools global toolbar

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Comment 4

[Joe Walker [:jwalker]]

shorlander - please could you attach your global-toolbar mock-ups here when they're done. Thanks.

Comment 5

[Joe Walker [:jwalker]]

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

Comment 6

[Joe Walker [:jwalker]]

Attachment: Browser CSS patch - Upload 1

Comment 7

[Joe Walker [:jwalker]]

Attachment: Devtools CSS patch - Upload 1

Comment 8

[Joe Walker [:jwalker]]

Attachment: GCLI update - Upload 1

Comment 9

[Joe Walker [:jwalker]]

Attachment: Global Toolbar - Update 1

Comment 10

[Joe Walker [:jwalker]]

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.

Comment 11

[Joe Walker [:jwalker]]

Attachment: GCLI update - Upload 2

Comment 12

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 2

Comment 13

[Stephen Horlander [:shorlander]]

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

Comment 14

[Joe Walker [:jwalker]]

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?

Comment 15

[Joe Walker [:jwalker]]

(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

[Stephen Horlander [:shorlander]]

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

Comment 17

[Stephen Horlander [:shorlander]]

Attachment: GCLI UI Mockups - i01

Comment 18

[Joe Walker [:jwalker]]

(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

[Dave Camp (:dcamp)]

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.

Comment 20

[Joe Walker [:jwalker]]

Attachment: Devtools CSS patch - Upload 2

Comment 21

[Joe Walker [:jwalker]]

Attachment: GCLI update - Upload 3

Comment 22

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 3

Comment 23

[Joe Walker [:jwalker]]

Attachment: Test pref changes - Upload 1

Trivial, but lengthy patch.
The webconsole tests no-longer need to check that GCLI is preffed-off

Comment 24

[Joe Walker [:jwalker]]

Attachment: Devtools CSS patch - Upload 3

Comment 25

[Joe Walker [:jwalker]]

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)

Comment 26

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=b80f78894bd4

Comment 27

[Joe Walker [:jwalker]]

Attachment: Browser CSS patch - Upload 2

Comment 28

[Joe Walker [:jwalker]]

Attachment: Devtools CSS patch - Upload 4

Comment 29

[Joe Walker [:jwalker]]

Attachment: GCLI update - Upload 4

Comment 30

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 4

Comment 31

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=d33df0614155

Comment 32

[Stephen Horlander [:shorlander]]

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

Comment 33

[Joe Walker [:jwalker]]

Attachment: GCLI update - Upload 5

Comment 34

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 5

Comment 35

[Dão Gottwald [::dao]]

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

[Rob Campbell [:rc] (:robcee)]

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.

Comment 37

[Rob Campbell [:rc] (:robcee)]

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

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 614462 [details] [diff] [review]
Developer Toolbar - Update 5

this patch needs to be rebased after the landing of bug 741322.

Comment 39

[Rob Campbell [:rc] (:robcee)]

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; ?

Comment 40

[Joe Walker [:jwalker]]

(In reply to Stephen Horlander from comment #32)
> ...

Thanks, I replied in bug 744906 comment 4.

Comment 41

[Dave Camp (:dcamp)]

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.

Comment 42

[Rob Campbell [:rc] (:robcee)]

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.

Comment 43

[Joe Walker [:jwalker]]

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.

Comment 44

[Joe Walker [:jwalker]]

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

Comment 45

[Joe Walker [:jwalker]]

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

Comment 46

[Joe Walker [:jwalker]]

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

Comment 47

[Joe Walker [:jwalker]]

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

Comment 48

[Joe Walker [:jwalker]]

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

Comment 49

[Joe Walker [:jwalker]]

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

Comment 50

[Joe Walker [:jwalker]]

Attachment: Browser CSS patch - Upload 3

Comment 51

[Joe Walker [:jwalker]]

Attachment: Devtools CSS patch - Upload 5

Comment 52

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 6

Comment 53

[Victor Porof [:vporof][:vp]]

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

Comment 54

[Joe Walker [:jwalker]]

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

Comment 55

[Joe Walker [:jwalker]]

Attachment: Browser CSS patch - Upload 4

The font changes in the last caused a misalignment on mac. Fixed.

Comment 56

[Joe Walker [:jwalker]]

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

Comment 57

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 7

Includes:
- victor's changes
- changes to fix a11y test for dbolter/surkov/marco review
- robcee's changes

Comment 58

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 59

[Rob Campbell [:rc] (:robcee)]

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

[Rob Campbell [:rc] (:robcee)]

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.

Comment 61

[Joe Walker [:jwalker]]

(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

[Dave Camp (:dcamp)]

(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

Comment 63

[Joe Walker [:jwalker]]

Attachment: 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?

Comment 64

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=d81d9d45bd99

Comment 65

[alexander :surkov (:asurkov)]

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,
   "");

Comment 66

[Joe Walker [:jwalker]]

(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

[Rob Campbell [:rc] (:robcee)]

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

Comment 68

[Joe Walker [:jwalker]]

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

[Rob Campbell [:rc] (:robcee)]

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!

Comment 70

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 615032 [details] [diff] [review]
Developer Toolbar - Update 8

yup.

this'll still need review from a browser peer for browser bits.

Comment 71

[Joe Walker [:jwalker]]

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.

Comment 72

[Joe Walker [:jwalker]]

(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

[Dão Gottwald [::dao]]

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.

Comment 74

[Joe Walker [:jwalker]]

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

Comment 75

[Joe Walker [:jwalker]]

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

Comment 76

[Joe Walker [:jwalker]]

Attachment: Test pref changes - Upload 2

Rebase

Comment 77

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 9

Comment 78

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=b52ac5a8364b

Comment 79

[Joe Walker [:jwalker]]

Dão, I'm hoping that you can now r+ the Browser CSS patch (attachment 614822 [details] [diff] [review])

Comment 80

[Dão Gottwald [::dao]]

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

Comment 81

[Joe Walker [:jwalker]]

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

Comment 82

[Dão Gottwald [::dao]]

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

Comment 83

[Dão Gottwald [::dao]]

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.

Comment 84

[Joe Walker [:jwalker]]

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

Comment 85

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 10

Comment 86

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=caa9fcce0cb9

Comment 87

[Dão Gottwald [::dao]]

(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

[Dão Gottwald [::dao]]

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.

Comment 89

[Joe Walker [:jwalker]]

(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

[Dão Gottwald [::dao]]

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.

Comment 91

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 11

Comment 92

[Dão Gottwald [::dao]]

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.

Comment 93

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 12

Comment 94

[Dão Gottwald [::dao]]

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

Comment 95

[Joe Walker [:jwalker]]

Attachment: Developer Toolbar - Update 13

Comment 96

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=26812464a766

Comment 97

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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.

Comment 98

[Joe Walker [:jwalker]]

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.

Comment 99

[Joe Walker [:jwalker]]

Attachment: Combined patch

About to check this in.

Comment 100

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=9d827d67bfbb

Comment 101

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/86623a74f82a

Comment 102

[Francesco Lodolo [:flod]]

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

[:Gavin Sharp [email: [e-mail]]]

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

Comment 104

[(no longer active)]

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.

Comment 105

[Joe Walker [:jwalker]]

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

Comment 106

[Rob Campbell [:rc] (:robcee)]

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?

Comment 107

[Joe Walker [:jwalker]]

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

Comment 108

[Joe Walker [:jwalker]]

Attachment: Combined patch - Upload 2

Comment 109

[Joe Walker [:jwalker]]

Attachment: Combined patch - Upload 3

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

https://tbpl.mozilla.org/?tree=Try&rev=75fd0429c01e

Comment 110

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/2e5b9ba8358b

Comment 111

[Paul Rouget [:paul]]

Is there a bug that tracks the bug to fix before preffing on this feature?

Comment 112

[Paul Rouget [:paul]]

(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

[Will Bamberg [:wbamberg]]

I think this is covered by: https://developer.mozilla.org/en-US/docs/Tools/GCLI, so marking dev-doc-complete.