Closed Bug 934225 Opened 7 years ago Closed 7 years ago

GLSL code should be syntax highlighted

Categories

(DevTools Graveyard :: WebGL Shader Editor, defect, P2)

defect

Tracking

(firefox26 unaffected, firefox27 fixed, firefox28 fixed)

RESOLVED FIXED
Firefox 28
Tracking Status
firefox26 --- unaffected
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: vporof, Assigned: vporof)

Details

(Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

We're using text mode for shader code. In 2013.
Attached patch webgl-syntax.patch (obsolete) — Splinter Review
This patch adds the "c-like" mode file from the CodeMirror repo, which has designated constructs for vertex and fragment shaders.

Using it alone wasn't enough though, because all the glsl code was now blue :) All the tokens/variables/things fell under the same rules in our css files, so I had to make a few small tweaks to our light/dark theme files).
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #826431 - Flags: review?(anton)
Priority: -- → P2
Made a few more small changes to the theme files, to avoid too much color abundance. Sorry about the double r?.
Attachment #826431 - Attachment is obsolete: true
Attachment #826431 - Flags: review?(anton)
Attachment #826441 - Flags: review?(anton)
Comment on attachment 826441 [details] [diff] [review]
webgl-syntax.patch

Review of attachment 826441 [details] [diff] [review]:
-----------------------------------------------------------------

You'll need to add clike.js to the codemirror/README file and our licensing files prior to landing.

::: browser/themes/shared/devtools/dark-theme.css
@@ +157,5 @@
>  .cm-s-mozilla.CodeMirror-focused .CodeMirror-selected { /* selected text (focused) */
>    background: rgb(185, 215, 253);
>  }
>  
> +.cm-s-mozilla.CodeMirror-selected { /* selected text (unfocused) */

Was the switch from cm-s-mozilla OR CodeMirror-selected to cm-s-mozilla AND CodeMirror-selected intentional?

::: browser/themes/shared/devtools/light-theme.css
@@ +157,5 @@
>  .cm-s-mozilla.CodeMirror-focused .CodeMirror-selected { /* selected text (focused) */
>    background: rgb(185, 215, 253);
>  }
>  
> +.cm-s-mozilla.CodeMirror-selected { /* selected text (unfocused) */

Same question as in dark-theme.css
Attachment #826441 - Flags: review?(anton) → review+
(In reply to Anton Kovalyov (:anton) from comment #3)
> Comment on attachment 826441 [details] [diff] [review]
> webgl-syntax.patch
> 
> Review of attachment 826441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You'll need to add clike.js to the codemirror/README file and our licensing
> files prior to landing.
> 
> ::: browser/themes/shared/devtools/dark-theme.css
> @@ +157,5 @@
> >  .cm-s-mozilla.CodeMirror-focused .CodeMirror-selected { /* selected text (focused) */
> >    background: rgb(185, 215, 253);
> >  }
> >  
> > +.cm-s-mozilla.CodeMirror-selected { /* selected text (unfocused) */
> 
> Was the switch from cm-s-mozilla OR CodeMirror-selected to cm-s-mozilla AND
> CodeMirror-selected intentional?
> 

Yes.

It wasn't "OR", that used to be a generic descendant selector, and it didn't work at all, because both classes were applied to the same element.
(In reply to Victor Porof [:vp] from comment #4)
> 
> It wasn't "OR", that used to be a generic descendant selector, and it didn't
> work at all, because both classes were applied to the same element.

Wait, no, I was wrong :/ I misread "selected" to be "focused". I'll revert this, thanks for the catch!
(In reply to Anton Kovalyov (:anton) from comment #3)
> Comment on attachment 826441 [details] [diff] [review]
> webgl-syntax.patch
> 
> Review of attachment 826441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You'll need to add clike.js to the codemirror/README file and our licensing
> files prior to landing.
> 

I don't know where exactly I should add clike.js apart from browser/devtools/sourceeditor/codemirror/README. Also, in the codemirror/README file, only javascript.js is listed, not xml.js or css.js. Since clike.js is the same kind of thing coming from the same place, what's to be done here?

PS: "To confirm the functionality run mochitests for the following" doesn't list the netmonitor.
Flags: needinfo?(anton)
I just looked at the about:license and we don't have other files there so that's good. I think you need to add clike.js to README and we need to add other files (xml, etc.) to the README. I missed that in previous reviews. :(
Flags: needinfo?(anton)
Added stuff to readme.
Attachment #827905 - Flags: review+
Comment on attachment 827905 [details] [diff] [review]
webgl-syntax.patch + readme

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature.
User impact if declined: Code in the Shader Editor won't be syntax highlighted. Everything is black. That's sad even for users that only like dark colors.
Testing completed (on m-c, etc.): locally, fx-team
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None
Attachment #827905 - Flags: approval-mozilla-aurora?
Comment on attachment 827905 [details] [diff] [review]
webgl-syntax.patch + readme

Approving the low risk, polish fix given we still have a few weeks on aurora.
Attachment #827905 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/7c3f6ee5b59d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Whiteboard: [qa-]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.