Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the regular preprocess-function for the CSS files as well #14886

Merged
merged 2 commits into from
May 8, 2022

Conversation

Snuffleupagus
Copy link
Collaborator

An old shortcoming of the preprocessCSS-function is its complete lack of support for our "normal" defines, which makes it very difficult to have build-specific CSS rules. Recently we've started using specially crafted comments to remove CSS rules from the MOZCENTRAL build, but (ab)using the preprocessCSS-function in this way really doesn't feel great.
However, it turns out to be surprisingly simple to instead use the "regular" preprocess-function for the CSS files as well. The only special-handling that's still necessary is the helper-function for dealing with CSS-imports, but apart from that everything seems to just work.

One reason, as far as I can tell, for having a separate preprocessCSS-function was likely that we originally used lots of vendor-prefixed CSS rules in our CSS files. With improvements over the years, especially thanks to Autoprefixer and PostCSS, we've been able to remove almost all non-standard CSS rules and the need for special-casing the CSS parsing has mostly vanished.

Please note: As part of testing this patch I've diffed the output of gulp generic, gulp mozcentral, and gulp chromium against the master-branch to check that there was no obvious breakage.

An old shortcoming of the `preprocessCSS`-function is its complete lack of support for our "normal" defines, which makes it very difficult to have build-specific CSS rules. Recently we've started using specially crafted comments to remove CSS rules from the MOZCENTRAL build, but (ab)using the `preprocessCSS`-function in this way really doesn't feel great.
However, it turns out to be surprisingly simple to instead use the "regular" `preprocess`-function for the CSS files as well. The only special-handling that's still necessary is the helper-function for dealing with CSS-imports, but apart from that everything seems to just work.

One reason, as far as I can tell, for having a separate `preprocessCSS`-function was likely that we originally used *lots* of vendor-prefixed CSS rules in our CSS files. With improvements over the years, especially thanks to Autoprefixer and PostCSS, we've been able to remove *almost* all non-standard CSS rules and the need for special-casing the CSS parsing has mostly vanished.

*Please note:* As part of testing this patch I've diffed the output of `gulp generic`, `gulp mozcentral`, and `gulp chromium` against the `master`-branch to check that there was no obvious breakage.
This is, for all intents and purposes, equivalent to PR 14833 and slightly reduces the size of the `gulp chromium` output.
@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

pdfjsbot commented May 7, 2022

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/49ee759f31f9298/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 7, 2022

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/5310d9315caaf09/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 7, 2022

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/49ee759f31f9298/output.txt

Total script time: 4.89 mins

  • Integration Tests: FAILED

@pdfjsbot
Copy link

pdfjsbot commented May 7, 2022

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/5310d9315caaf09/output.txt

Total script time: 9.01 mins

  • Integration Tests: Passed

@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented May 7, 2022

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/83df4179b13172a/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 7, 2022

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/83df4179b13172a/output.txt

Total script time: 2.65 mins

Published

@timvandermeij timvandermeij merged commit 72943ae into mozilla:master May 8, 2022
@timvandermeij
Copy link
Contributor

timvandermeij commented May 8, 2022

Awesome work! Not only does this make the builder much simpler, it also fixes the preprocessing in a much nicer way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants