934225 - GLSL code should be syntax highlighted

Status: RESOLVED FIXED

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

Description

[Victor Porof [:vporof][:vp]]

We're using text mode for shader code. In 2013.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: webgl-syntax.patch

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).

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: webgl-syntax.patch

Made a few more small changes to the theme files, to avoid too much color abundance. Sorry about the double r?.

Comment 3

[Anton Kovalyov (: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

Comment 4

[Victor Porof [:vporof][:vp]]

(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.

Comment 5

[Victor Porof [:vporof][:vp]]

(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!

Comment 6

[Victor Porof [:vporof][:vp]]

(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.

Comment 7

[Anton Kovalyov (: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. :(

Comment 8

[Victor Porof [:vporof][:vp]]

Attachment: webgl-syntax.patch + readme

Added stuff to readme.

Comment 9

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/7c3f6ee5b59d

Comment 10

[Victor Porof [:vporof][:vp]]

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

Comment 11

[bhavana bajaj [:bajaj]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c3f6ee5b59d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/72614b4ccdfa