Last Comment Bug 702621 - GCLI needs fixes for the minor issues created by bug 692742
: GCLI needs fixes for the minor issues created by bug 692742
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 11
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
: Brian Grinstead [:bgrins]
Mentors:
Depends on: 692742
Blocks: GCLI-SHIP
  Show dependency treegraph
 
Reported: 2011-11-15 07:26 PST by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2011-12-08 21:23 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
upload 1 (10.63 KB, patch)
2011-11-21 07:21 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
paul: feedback+
Details | Diff | Splinter Review
upload 2 (10.38 KB, patch)
2011-11-28 04:26 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
dao+bmo: review+
Details | Diff | Splinter Review

Description User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-15 07:26:54 PST
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
Comment 1 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-15 09:20:08 PST
These are fixed in https://github.com/campd/gcli/pull/2
Will create official patch soon.
Comment 2 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 10:01:03 PST
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Comment 3 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-21 07:21:10 PST
Created attachment 575857 [details] [diff] [review]
upload 1

These are the changes that you asked for in bug 692742 - should be a simple review.
Comment 4 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-21 08:33:55 PST
https://tbpl.mozilla.org/?tree=Try&rev=28134b5df88a
Comment 5 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-22 02:42:42 PST
Failures in previous try were not down to this patch, but just in case:
https://tbpl.mozilla.org/?tree=Try&rev=587bd6d86427
Comment 6 User image Dão Gottwald [:dao] 2011-11-24 02:13:34 PST
(In reply to Joe Walker from comment #0)
> - Remove the magic numbers in Popup.resizer()

You didn't actually remove them.
Comment 7 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-24 02:23:58 PST
(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.
Comment 8 User image Dão Gottwald [:dao] 2011-11-24 03:37:50 PST
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.
Comment 9 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-24 04:59:09 PST
(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 10 User image Dão Gottwald [:dao] 2011-11-26 04:35:43 PST
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?
Comment 11 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-28 02:14:14 PST
(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.
Comment 12 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-28 04:26:20 PST
Created attachment 577218 [details] [diff] [review]
upload 2
Comment 13 User image Dão Gottwald [:dao] 2011-11-28 07:39:28 PST
(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'?
Comment 14 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-28 08:17:27 PST
(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.
Comment 15 User image Mihai Sucan [:msucan] 2011-11-29 02:37:18 PST
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?
Comment 16 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-29 02:56:31 PST
(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 17 User image Dão Gottwald [:dao] 2011-11-29 08:36:44 PST
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.
Comment 18 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-29 09:00:35 PST
(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.
Comment 19 User image Dão Gottwald [:dao] 2011-11-29 09:02:43 PST
Any reason why GCLI shouldn't adopt the Mozilla standard?
Comment 20 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-08 06:06:29 PST
https://tbpl.mozilla.org/?tree=Fx-Team&rev=cf3ed4316481
Comment 21 User image Tim Taubert [:ttaubert] 2011-12-08 21:23:36 PST
https://hg.mozilla.org/mozilla-central/rev/71054aef1a3a

Note You need to log in before you can comment on or make changes to this bug.