GCLI needs fixes for the minor issues created by bug 692742

RESOLVED FIXED in Firefox 11

Status

()

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

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

unspecified
Firefox 11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

There's lots of work that's been done on top of this bug, and rather than go on a mad rebasingfest, I'd like to fix some issues in a separate bug.

The issues are:
- Remove the magic numbers in Popup.resizer()
- Move dynamic styles to dynamic classes in Popup.resizer()
- JavascriptField() should use a class rather than style.marginBottom
- field.js adds class names in old format
- The font assigned to .gcliterm-input-node, .gcliterm-complete-node needs to be theme specific
- Check that .gcli-af-required { padding-left } is RTL friendly
These are fixed in https://github.com/campd/gcli/pull/2
Will create official patch soon.
Blocks: 689605
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
Created attachment 575857 [details] [diff] [review]
upload 1

These are the changes that you asked for in bug 692742 - should be a simple review.
Attachment #575857 - Flags: feedback?(paul)
Attachment #575857 - Flags: review?(dcamp)

Updated

6 years ago
Attachment #575857 - Flags: review?(dcamp) → review+
https://tbpl.mozilla.org/?tree=Try&rev=28134b5df88a
Failures in previous try were not down to this patch, but just in case:
https://tbpl.mozilla.org/?tree=Try&rev=587bd6d86427

Updated

6 years ago
Attachment #575857 - Flags: feedback?(paul) → feedback+
Attachment #575857 - Flags: review+ → review?(dao)
(In reply to Joe Walker from comment #0)
> - Remove the magic numbers in Popup.resizer()

You didn't actually remove them.
(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to Joe Walker from comment #0)
> > - Remove the magic numbers in Popup.resizer()
> 
> You didn't actually remove them.

That's true, but I did explain them. I don't believe that it's the best thing to be spending time on.
It's bogus code, it doesn't help to document it. Those values are imaginary and may or may not match what they're supposed to match.
(In reply to Dão Gottwald [:dao] from comment #8)
> It's bogus code, it doesn't help to document it. Those values are imaginary
> and may or may not match what they're supposed to match.

Bug 705109
Comment on attachment 575857 [details] [diff] [review]
upload 1

> Display.prototype.resizer = function() {
>+  // Note on magic numbers: There are several magic numbers in this function.
>+  // We have 2 options - lots of complex dom math to derive the numbers or hard
>+  // code them. The former is *slightly* more resilient to refactoring (but
>+  // still breaks with dom structure changes), the latter is simpler, faster
>+  // and easier. For now we're hard coding, but will probably accept patches to
>+  // the latter technically better way of doing things.

replace with:

// FIXME bug 705109: There are several numbers hard-coded in this function.
// This is simpler than calculating them, but error-prone when the UI setup,
// the styling or display settings change.

>   this.element = dom.createElement(this.document, 'input');
>   this.element.type = 'text';
>-  this.element.className = 'gcli-field';
>+  this.element.classList.add('gcli-field');
> 
>   this.onInputChange = this.onInputChange.bind(this);
>   this.element.addEventListener('keyup', this.onInputChange, false);
>@@ -6686,7 +6689,7 @@ function SelectionField(type, options) {
>   this.items = [];
> 
>   this.element = dom.createElement(this.document, 'select');
>-  this.element.className = 'gcli-field';
>+  this.element.classList.add('gcli-field');
[...]

These changes are unnecessary and don't seem like improvements.

> /* From: $GCLI/mozilla/gcli/ui/gcliterm-gnomestripe.css */
> 
>+.gcliterm-input-node,
>+.gcliterm-complete-node {
>+  font: 12px "DejaVu Sans Mono", monospace;
>+}

Aren't we automatically using DejaVu Sans Mono when that's the monospace font the OS specifies? I.e. why hard-code it here?
(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 575857 [details] [diff] [review] [diff] [details] [review]
> upload 1
> > /* From: $GCLI/mozilla/gcli/ui/gcliterm-gnomestripe.css */
> > 
> >+.gcliterm-input-node,
> >+.gcliterm-complete-node {
> >+  font: 12px "DejaVu Sans Mono", monospace;
> >+}
> 
> Aren't we automatically using DejaVu Sans Mono when that's the monospace
> font the OS specifies? I.e. why hard-code it here?

gcliterm-complete-node - because that's not what the OS specifies. It's a div.
gcliterm-input-node is here symmetry with how it works on jsterm. I'm expecting that we will fix this in bug 704181.
Created attachment 577218 [details] [diff] [review]
upload 2
Attachment #575857 - Attachment is obsolete: true
Attachment #575857 - Flags: review?(dao)
Attachment #577218 - Flags: review?(dao)
(In reply to Joe Walker from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > Comment on attachment 575857 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > upload 1
> > > /* From: $GCLI/mozilla/gcli/ui/gcliterm-gnomestripe.css */
> > > 
> > >+.gcliterm-input-node,
> > >+.gcliterm-complete-node {
> > >+  font: 12px "DejaVu Sans Mono", monospace;
> > >+}
> > 
> > Aren't we automatically using DejaVu Sans Mono when that's the monospace
> > font the OS specifies? I.e. why hard-code it here?
> 
> gcliterm-complete-node - because that's not what the OS specifies. It's a
> div.

I think you misunderstood my question, as I don't see how this response answers it.
Let me try this again: Why did you write '"DejaVu Sans Mono", monospace' instead of just 'monospace'?
(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to Joe Walker from comment #11)
> > (In reply to Dão Gottwald [:dao] from comment #10)
...
> > > >+.gcliterm-input-node,
> > > >+.gcliterm-complete-node {
> > > >+  font: 12px "DejaVu Sans Mono", monospace;
> > > >+}
> > > 
> > > Aren't we automatically using DejaVu Sans Mono when that's the monospace
> > > font the OS specifies? I.e. why hard-code it here?
> > 
> > gcliterm-complete-node - because that's not what the OS specifies. It's a
> > div.
> 
> I think you misunderstood my question, as I don't see how this response
> answers it.
> Let me try this again: Why did you write '"DejaVu Sans Mono", monospace'
> instead of just 'monospace'?

Ah - I see. Specifically because that's what jsterm does.

I think this is the right thing to do. If the user has selected a different font for monospace, then I think we want to do the same as jsterm.

I'm adding Mihai to the CC list in case he has insight into the settings for jsterm.
If I recall correctly the DejaVu Sans Mono font was added because on some systems the default monospace font in Firefox was not a reasonable choice - Courier New and the likes.

This is not good solution, as Dão points out. We should make sure that monospace defaults are sensible on supported platforms.

Joe: Scratchpad uses monospace by default - afaik nobody complained about it. We should probably no longer bother to have DejaVu Sans Mono hard coded in the Web Console.

Thoughts?
(In reply to Mihai Sucan [:msucan] from comment #15)
> If I recall correctly the DejaVu Sans Mono font was added because on some
> systems the default monospace font in Firefox was not a reasonable choice -
> Courier New and the likes.
> 
> This is not good solution, as Dão points out. We should make sure that
> monospace defaults are sensible on supported platforms.
> 
> Joe: Scratchpad uses monospace by default - afaik nobody complained about
> it. We should probably no longer bother to have DejaVu Sans Mono hard coded
> in the Web Console.
> 
> Thoughts?

I raised bug 706047.
Comment on attachment 577218 [details] [diff] [review]
upload 2

> Display.prototype.resizer = function() {
>+  // Bug 705109: There are several numbers hard-coded in this function.

Add "FIXME" here, as suggested earlier. This is a standard annotation in Mozilla code.
Attachment #577218 - Flags: review?(dao) → review+
(In reply to Dão Gottwald [:dao] from comment #17)
> Comment on attachment 577218 [details] [diff] [review] [diff] [details] [review]
> upload 2
> 
> > Display.prototype.resizer = function() {
> >+  // Bug 705109: There are several numbers hard-coded in this function.
> 
> Add "FIXME" here, as suggested earlier. This is a standard annotation in
> Mozilla code.

GCLI has used Bug for this since before it's inclusion in Firefox. It's documented here: https://github.com/mozilla/gcli/blob/master/docs/index.md

Thanks.
Any reason why GCLI shouldn't adopt the Mozilla standard?
https://tbpl.mozilla.org/?tree=Fx-Team&rev=cf3ed4316481
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/71054aef1a3a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 11
You need to log in before you can comment on or make changes to this bug.