Closed
Bug 702621
Opened 13 years ago
Closed 12 years ago
GCLI needs fixes for the minor issues created by bug 692742
Categories
(DevTools :: Console, defect, P1)
DevTools
Console
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 11
People
(Reporter: jwalker, Assigned: jwalker)
References
Details
Attachments
(1 file, 1 obsolete file)
10.38 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•13 years ago
|
||
These are fixed in https://github.com/campd/gcli/pull/2 Will create official patch soon.
Assignee | ||
Comment 2•13 years ago
|
||
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
Assignee | ||
Comment 3•13 years ago
|
||
These are the changes that you asked for in bug 692742 - should be a simple review.
Attachment #575857 -
Flags: feedback?(paul)
Assignee | ||
Updated•13 years ago
|
Attachment #575857 -
Flags: review?(dcamp)
Updated•13 years ago
|
Attachment #575857 -
Flags: review?(dcamp) → review+
Assignee | ||
Comment 4•13 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=28134b5df88a
Assignee | ||
Comment 5•13 years ago
|
||
Failures in previous try were not down to this patch, but just in case: https://tbpl.mozilla.org/?tree=Try&rev=587bd6d86427
Updated•13 years ago
|
Attachment #575857 -
Flags: feedback?(paul) → feedback+
Assignee | ||
Updated•13 years ago
|
Attachment #575857 -
Flags: review+ → review?(dao)
Comment 6•13 years ago
|
||
(In reply to Joe Walker from comment #0) > - Remove the magic numbers in Popup.resizer() You didn't actually remove them.
Assignee | ||
Comment 7•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
(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•13 years ago
|
||
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?
Assignee | ||
Comment 11•13 years ago
|
||
(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.
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #575857 -
Attachment is obsolete: true
Attachment #575857 -
Flags: review?(dao)
Attachment #577218 -
Flags: review?(dao)
Comment 13•13 years ago
|
||
(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'?
Assignee | ||
Comment 14•13 years ago
|
||
(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•13 years ago
|
||
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?
Assignee | ||
Comment 16•13 years ago
|
||
(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•13 years ago
|
||
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+
Assignee | ||
Comment 18•13 years ago
|
||
(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•13 years ago
|
||
Any reason why GCLI shouldn't adopt the Mozilla standard?
Assignee | ||
Comment 20•13 years ago
|
||
https://tbpl.mozilla.org/?tree=Fx-Team&rev=cf3ed4316481
Whiteboard: [fixed-in-fx-team]
Comment 21•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/71054aef1a3a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 11
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•