1221383 - Make browser_parsable_css.js test cover all CSS files we are shipping

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Currently, browser_parsable_css.js test covers only the CSS files inside browser/ directory. We may want to extend the range to all CSS files we are shipping, including those in the chrome/ directory and webapprt/ directory.

If we are going to do so, there are several things we need to do:

* Handle UA-only things including some properties and pseudo-elements/classes. This probably can be done via the trick used for injecting devtools/server/actors/highlighters.css.

* Properly register all necessary manifests. The manifests inside webapprt/ are not registered by default. We would need to register them before testing. To make this work with packaged build, we may need to add a method to nsIComponentRegistrar which calls XRE_AddJarManifestLocation instead of XRE_AddManifestLocation, so that we can register files inside omni.ja.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Also, the appDir should get 'GreD' instead of 'XCurProcD'.

Comment 2

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d025d5a2092c

Comment 3

[Florian Quèze [:florian]]

Here is the output from the try run:

All platforms

error message for chrome://mozapps/skin/aboutNetworking.css: Error in parsing value for ‘border’. Declaration dropped.
error message for chrome://mozapps/skin/extensions/newaddon.css: Error in parsing value for ‘-moz-box-pack’. Declaration dropped. - 
missing chrome://global/skin/columnselect.gif referenced from chrome://global/skin/menu.css


Linux

missing chrome://global/skin/progressmeter/progressmeter-busy.gif referenced from chrome://global/skin/tree.css
missing chrome://global/skin/scale/scale-tray-horiz.gif referenced from chrome://global/skin/scale.css
missing chrome://global/skin/scale/scale-tray-vert.gif referenced from chrome://global/skin/scale.css
missing chrome://mozapps/skin/update/update.png referenced from chrome://mozapps/skin/extensions/update.css
missing chrome://mozapps/skin/extensions/rating-unrated.png referenced from chrome://mozapps/skin/extensions/extensions.css


Mac

missing chrome://global/skin/filepicker/dir-closed.gif referenced from chrome://global/skin/filepicker.css
missing chrome://global/skin/filepicker/blank.gif referenced from chrome://global/skin/filepicker.css
missing chrome://global/skin/filepicker/folder-up.gif referenced from chrome://global/skin/filepicker.css
missing chrome://global/skin/filepicker/folder-home.gif referenced from chrome://global/skin/filepicker.css
missing chrome://mozapps/skin/update/warning.gif referenced from chrome://mozapps/skin/extensions/update.css


Windows

missing chrome://global/skin/progressmeter/progressmeter-busy.gif referenced from chrome://global/skin/tree.css
missing chrome://global/skin/icons/notfound.png referenced from chrome://mozapps/skin/update/updates.css
missing chrome://global/skin/scale/scale-tray-horiz.gif referenced from chrome://global/skin/scale.css
missing chrome://global/skin/scale/scale-tray-vert.gif referenced from chrome://global/skin/scale.css
missing chrome://mozapps/skin/update/update.png referenced from chrome://mozapps/skin/extensions/update.css

Comment 4

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c70dc41bb686

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch

Only bug 1303355 is left in the dependencies, and someone has started working on it, so I think it's time to request review here :-).

Comment 6

[:Gijs (he/him)]

Comment on attachment 8798380 [details] [diff] [review]
Patch

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

r=me

::: browser/base/content/test/general/browser_parsable_css.js
@@ +31,5 @@
>    {sourceName: /responsive-ua\.css$/i,
>     errorMessage: /Unknown pseudo-class.*moz-dropdown-list/i,
>     isFromDevTools: true},
> +
> +  {sourceName: /(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\.css$/i,

This should probably have a \b or \/ before the group to avoid substring matching other stylesheets. Same for the html/mathml/ua one below.

@@ +32,5 @@
>     errorMessage: /Unknown pseudo-class.*moz-dropdown-list/i,
>     isFromDevTools: true},
> +
> +  {sourceName: /(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\.css$/i,
> +   errorMessage: /Unknown pseudo-class.*-moz-/i,

This is quite broad. Why is this necessary, specifically? Why are we declaring all these styles that we don't understand - is it just because these are UA sheets and some of their styles don't apply when used as a content sheet? Can we fix this by parsing them as UA sheets somehow (maybe as a followup bug) ?

Comment 7

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #6)

> ::: browser/base/content/test/general/browser_parsable_css.js
> @@ +31,5 @@
> >    {sourceName: /responsive-ua\.css$/i,
> >     errorMessage: /Unknown pseudo-class.*moz-dropdown-list/i,
> >     isFromDevTools: true},
> > +
> > +  {sourceName: /(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\.css$/i,
> 
> This should probably have a \b or \/ before the group to avoid substring
> matching other stylesheets. Same for the html/mathml/ua one below.

Good idea, will do.

> @@ +32,5 @@
> >     errorMessage: /Unknown pseudo-class.*moz-dropdown-list/i,
> >     isFromDevTools: true},
> > +
> > +  {sourceName: /(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\.css$/i,
> > +   errorMessage: /Unknown pseudo-class.*-moz-/i,
> 
> This is quite broad. Why is this necessary, specifically? Why are we
> declaring all these styles that we don't understand - is it just because
> these are UA sheets and some of their styles don't apply when used as a
> content sheet?

Yes, it's stuff that only makes sense in UA sheets, here is the list of errors ignored by this exception:

chrome/toolkit/res/forms.css: -moz-button-content
chrome/toolkit/res/forms.css: -moz-display-comboboxcontrol-frame
chrome/toolkit/res/forms.css: -moz-dropdown-list
chrome/toolkit/res/forms.css: -moz-fieldset-content
chrome/toolkit/res/forms.css: -moz-number-spin-box
chrome/toolkit/res/forms.css: -moz-number-spin-down
chrome/toolkit/res/forms.css: -moz-number-spin-up
chrome/toolkit/res/forms.css: -moz-number-text
chrome/toolkit/res/forms.css: -moz-number-wrapper
chrome/toolkit/res/html.css: -moz-html-canvas-content
chrome/toolkit/res/html.css: -moz-native-anonymous
chrome/toolkit/res/html.css: -moz-suppressed
chrome/toolkit/res/html.css: -moz-user-disabled
chrome/toolkit/res/ua.css: -moz-anonymous-block
chrome/toolkit/res/ua.css: -moz-anonymous-flex-item
chrome/toolkit/res/ua.css: -moz-anonymous-positioned-block
chrome/toolkit/res/ua.css: -moz-browser-frame
chrome/toolkit/res/ua.css: -moz-column-content
chrome/toolkit/res/ua.css: -moz-inline-table
chrome/toolkit/res/ua.css: -moz-native-anonymous
chrome/toolkit/res/ua.css: -moz-page
chrome/toolkit/res/ua.css: -moz-page-sequence
chrome/toolkit/res/ua.css: -moz-pagebreak
chrome/toolkit/res/ua.css: -moz-pagecontent
chrome/toolkit/res/ua.css: -moz-ruby
chrome/toolkit/res/ua.css: -moz-ruby-base
chrome/toolkit/res/ua.css: -moz-ruby-base-container
chrome/toolkit/res/ua.css: -moz-ruby-text
chrome/toolkit/res/ua.css: -moz-ruby-text-container
chrome/toolkit/res/ua.css: -moz-scrolled-content
chrome/toolkit/res/ua.css: -moz-table
chrome/toolkit/res/ua.css: -moz-table-cell
chrome/toolkit/res/ua.css: -moz-table-column
chrome/toolkit/res/ua.css: -moz-table-column-group
chrome/toolkit/res/ua.css: -moz-table-row
chrome/toolkit/res/ua.css: -moz-table-row-group
chrome/toolkit/res/ua.css: -moz-table-wrapper
chrome/toolkit/res/ua.css: -moz-viewport
chrome/toolkit/res/ua.css: -moz-viewport-scroll
chrome/toolkit/res/ua.css: -moz-xul-anonymous-block
editor/composer/res/EditorOverride.css: -moz-canvas
editor/composer/res/EditorOverride.css: -moz-display-comboboxcontrol-frame
layout/mathml/mathml.css: -moz-mathml-anonymous-block
layout/style/contenteditable.css: -moz-canvas
layout/style/contenteditable.css: -moz-display-comboboxcontrol-frame
layout/svg/svg.css: -moz-svg-foreign-content
layout/svg/svg.css: -moz-svg-marker-anon-child
layout/svg/svg.css: -moz-svg-text
layout/svg/svg.css: -moz-viewport

> Can we fix this by parsing them as UA sheets somehow (maybe
> as a followup bug) ?

I can file a bug if you like, but I don't see right now how we would handle parsing as UA sheets.

Comment 8

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9c0e2390ea1

Comment 9

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6720d4bfdb12

Comment 10

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/12a9cca092f816fabb67cc8a139e0cbd5b1cea37
Bug 1221383 - Make browser_parsable_css.js test cover all CSS files we are shipping, r=Gijs.

Comment 11

[Carsten Book [:Tomcat]]

sorry had to back this out for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11953796&repo=fx-team

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c63097845cfb
Backed out changeset 12a9cca092f8 for frequent failures in browser_parsable_css.js

Comment 13

[Florian Quèze [:florian]]

(In reply to Carsten Book [:Tomcat] from comment #11)
> sorry had to back this out for frequent failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=11953796&repo=fx-team

I attached the fix in bug 1307445.

Comment 14

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/0342df5d06e1f718155d6d754c25aae5f8676764
Bug 1221383 - Make browser_parsable_css.js test cover all CSS files we are shipping, r=Gijs.

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0342df5d06e1