981440 - Add a CSS linter to the build system

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Review time spent on CSS files could be reduced by simply using a CSS linter. It will also makes sure that no properties are mispelled and spew the console.

As a first step I would like to land the linter as a |make csslint| target (which is the goal of this bug). Then in a followup bug it would be nice to make it a pre-commit hook so people will just stop to land more css issues.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug981440.patch

This patch add a |make csslint| target to the build system, and it updates the xulrunner version from 26 to 30 in order to supports the latest css enhancements.

csslinter.js is a third party library from https://github.com/stubbornella/csslint

csslint.js is the file that is call by |make csslint|. This file will get all the CSS files for the apps/ folder and the shared/ folder and will parse them.

The parsing is done in 2 steps. The first step involves Gecko and load the CSS inside a dummy document in order to check for any parsing errors. It is used in order to not fails on the latest addition like will-change, and also in order to ensure that a rule is really working on Gaia.

The second step is actually more linting related tasks as it uses the third party linter to check for duplicate rules, empty selectors, ... and some good practices that I used to say during reviews, like no units after 0, or don't land css code with important, ...

I have tweaked the third party linter ruleset in order to reflect a bit the codebase while trying to push a few more good practices, and I added a description of all the possible rules (description stolen from the source code of the linter project), and some rationale to why a rule has been disabled.

I don't feel strong on many rules and those can be changed in the future, but a few more good practices won't be a bad thing.

So for the review I suggest to look mostly the Makefile changes and csslint.js.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

For the record with this configuration the |make csslint| call returns: 68 errors and 834 warnings

It could have been worse :)

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #2)
> For the record with this configuration the |make csslint| call returns: 68
> errors and 834 warnings
> 
> It could have been worse :)

And just because it's fun, enabling all the options returns: 200 errors and 6439 warnings !

Comment 4

[[:fabrice] Fabrice Desré]

How do people land css changes without checking first that their properties have no typos in the console?

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> How do people land css changes without checking first that their properties
> have no typos in the console?

I first want to land that as a static command so it will be easier for multiple people to fix their css and then I want to turn it into a pre-commit hook.

So the pre-commit hook will prevent them to land anything, and will dump the errors in the console before committing.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8388246 [details] [diff] [review]
bug981440.patch

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

I do not have much to say about the CSS checks, it would be great to get some feedback about all these choices.
It sounds like something that should be experienced and be discussed when enabling these checks by default.

I'm bit scared to bump the xulrunner version. It has been a while since we did that.
And we got a bunch of new stuff in the build system depending on it and got more travis and tbpl scripts being crafted.
Could you spawn a pull request to ensure that travis is fine with this?
Also I heard we can push to try with custom gaia... that would be really cool to give it a try!

Otherwise, the selected xulrunner crashes... I have no idea why, but it prevents me from building the profile:
  run-js-command  settings
  make: *** [profile/settings.json] Segmentation fault (core dumped)
The patch itself looks good to me regarding the build system, we just need to fix that crash!

::: Makefile
@@ +537,1 @@
>  XULRUNNER_DIRECTORY?=$(XULRUNNER_BASE_DIRECTORY)/xulrunner-sdk

XULRUNNER_DIRECTORY is duplicated

::: build/csslint.js
@@ +33,5 @@
> +  let gaia = utils.getGaia(config);
> +
> +  let errorsCount = 0;
> +  let warningsCount = 0;
> +  

nit: useless space

@@ +90,5 @@
> +
> +  let [errors, warnings] = checkForGoodPractices(content);
> +
> +  function printMessage(msg) {
> +    dump('\t' + msg.message + " line " + msg.line + ", col " + msg.col + " \n");

I thought I would never see you use double quotes for string, but it looks like I was wrong :)

@@ +126,5 @@
> +  // Ignore @import rules.
> +  content = content.replace(/@import url('?.*'?);/g, '');
> +
> +  // Ignore -moz-osx-font-smoothing rules.
> +  content = content.replace(/-moz-osx-font-smoothing: grayscale;/g, '');

what?! Is gaia running on mac, with some macos specific tweaks!?

@@ +134,5 @@
> +  domUtils.parseStyleSheet(sheet, content);
> +
> +  // Get messages from the console and reset it.
> +  let messages = Services.console.getMessageArray();
> +  Services.console.reset();

Shouldn't you call reset() before loading the css to ensure we ignore any build script message/error?

@@ +186,5 @@
> +    errors.push(message);
> +  }
> +
> +  rules =  {
> +    /*** Errors ***/

s/Errors/Warnings/

@@ +310,5 @@
> +    // This one is not enabled since its not clear that it has any real impact
> +    // on locally installed app, which is the first target of Gaia. Also it
> +    // seems like there is some valid use cases to reuse an url when there
> +    // is multiple background images on an element, that varies based on some
> +    // classes. So maybe there is better way of doing it, or maybe this is 

nit: useless space at end of line

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alexandre Poirot (:ochameau) from comment #6)
> Comment on attachment 8388246 [details] [diff] [review]
> bug981440.patch
> 
> Review of attachment 8388246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I do not have much to say about the CSS checks, it would be great to get
> some feedback about all these choices.
> It sounds like something that should be experienced and be discussed when
> enabling these checks by default.
> 
> I'm bit scared to bump the xulrunner version. It has been a while since we
> did that.
> And we got a bunch of new stuff in the build system depending on it and got
> more travis and tbpl scripts being crafted.
> Could you spawn a pull request to ensure that travis is fine with this?
> Also I heard we can push to try with custom gaia... that would be really
> cool to give it a try!
> 



> Otherwise, the selected xulrunner crashes... I have no idea why, but it
> prevents me from building the profile:
>   run-js-command  settings
>   make: *** [profile/settings.json] Segmentation fault (core dumped)
> The patch itself looks good to me regarding the build system, we just need
> to fix that crash!
>


I pushed it on Travis yesterday (https://travis-ci.org/mozilla-b2g/gaia/builds/20419029) and it seems like Travis complains about the same issue than you. Which I can reproduce locally. It seems like a relative path is gave to nsIFile, and instead of failing with an error, it generates a segfault. I have a dirty local fix and I will open a followup for that.
 
> 
> @@ +90,5 @@
> > +
> > +  let [errors, warnings] = checkForGoodPractices(content);
> > +
> > +  function printMessage(msg) {
> > +    dump('\t' + msg.message + " line " + msg.line + ", col " + msg.col + " \n");
> 
> I thought I would never see you use double quotes for string, but it looks
> like I was wrong :)

Oh noes! I did a last minute change to this function!
 
> @@ +126,5 @@
> > +  // Ignore @import rules.
> > +  content = content.replace(/@import url('?.*'?);/g, '');
> > +
> > +  // Ignore -moz-osx-font-smoothing rules.
> > +  content = content.replace(/-moz-osx-font-smoothing: grayscale;/g, '');
> 
> what?! Is gaia running on mac, with some macos specific tweaks!?
> 

It seems like people hacking on mac needs the font to be smaller to better reflect what it will looks like on the device. Sadly this rule generate a warning on other platforms.

> @@ +186,5 @@
> > +    errors.push(message);
> > +  }
> > +
> > +  rules =  {
> > +    /*** Errors ***/
> 
> s/Errors/Warnings/
>

I kept Errors mostly because this is how they are considered by the linter. But I moved it to the warnings section because it seems like a warning to me as they are just saying that it may be an error if the developer is not understanding the box model correctly.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

For what it worth I agree that the various prefs needs to be discussed, but I really don't want to block on landing the linter just because we don't agree on some rules. Those can be changed later, likely after an huge thread of discussions....

I launched a new Travis build to see if this is building this time :) https://travis-ci.org/mozilla-b2g/gaia/builds/20453868

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Travis is green with the local hack to avoid the segfault. I'm rebuilding Gecko, hoping to have some debugs symbols for xpcshell in order to obtain a useful stack.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug981440.patch

This one should not break anything. It's running on Travis right now to make sure that this is true.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Here is the interdiff:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8388246&action=interdiff&newid=8388539&headers=1

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug981440.patch

Bugzilla is back! So let's review this version instead, as the workaround for the underlying platform issue is much more less dirty.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #12)
> Created attachment 8388894 [details] [diff] [review]
> bug981440.patch
> 
> Bugzilla is back! So let's review this version instead, as the workaround
> for the underlying platform issue is much more less dirty.

Please note that the local change in utils-xpc.js does not exist in my local branch. I forgot to |git add| the file after removing the hack, but it's done now.

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8388894 [details] [diff] [review]
bug981440.patch

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #12)
> Bugzilla is back! So let's review this version instead, as the workaround
> for the underlying platform issue is much more less dirty.

Much more r+able!

::: build/csslint.js
@@ +38,5 @@
> +  // Starts by parsing the CSS files from apps/
> +  gaia.webapps.forEach(function getAllFilesFor(webapp) {
> +    if (webapp.sourceDirectoryFile.parent.leafName != 'apps') {
> +      return;
> +    }

It would make your new feature more usable for app-specific developers,
to respect the APP variable and only process the given app:
https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-zip.js#L275-L279
  if (config.BUILD_APP_NAME != '*' &&
    webapp.sourceDirectoryName != config.BUILD_APP_NAME) {
    return;
  }

::: build/utils-xpc.js
@@ -335,4 @@
>    let file = getFile.apply(null, abs_path_chunks);
>    if (!file.exists()) {
>      try {
> -      // Then check absolute path

Hum, this comment is still relevant, isn't?

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alexandre Poirot (:ochameau) from comment #14)
> Comment on attachment 8388894 [details] [diff] [review]
> bug981440.patch
> 
> Review of attachment 8388894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #12)
> > Bugzilla is back! So let's review this version instead, as the workaround
> > for the underlying platform issue is much more less dirty.
> 
> Much more r+able!
> 
> ::: build/csslint.js
> @@ +38,5 @@
> > +  // Starts by parsing the CSS files from apps/
> > +  gaia.webapps.forEach(function getAllFilesFor(webapp) {
> > +    if (webapp.sourceDirectoryFile.parent.leafName != 'apps') {
> > +      return;
> > +    }
> 
> It would make your new feature more usable for app-specific developers,
> to respect the APP variable and only process the given app:
> https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-zip.js#L275-L279
>   if (config.BUILD_APP_NAME != '*' &&
>     webapp.sourceDirectoryName != config.BUILD_APP_NAME) {
>     return;
>   }
> 
> ::: build/utils-xpc.js
> @@ -335,4 @@
> >    let file = getFile.apply(null, abs_path_chunks);
> >    if (!file.exists()) {
> >      try {
> > -      // Then check absolute path
> 
> Hum, this comment is still relevant, isn't?

As I said previously it is still in my PR. I just removed it by mistake but I added it back already :)

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Landed: https://github.com/mozilla-b2g/gaia/commit/c2e5a0ac615ec64fb4b57f12c5e41681b4e8ae4c

I fixed the comment about respecting the APP variable before landing.