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)
:
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 Dão Gottwald [:dao] 2011-11-29 09:02:43 PST
Any reason why GCLI shouldn't adopt the Mozilla standard?
Comment 20 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 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 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.