1220907 - Ensure browser_parsable_css.js test really tests all CSS files in the application

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

It is discovered that, browser_parsable_css.js does not really cover all css files we are shipping. Part of it is because after we switch to loading styles from chrome scheme, some stylesheets which are already loaded by the browser before the test starts are not parsed again, and thus no error is thrown. Also it seems some of the stylesheets are not covered from the beginning.

Both of them should be fixed.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Ah... okay, we cannot make it cover all shipping css files, because there are a lot of UA-only things including properties and pseudo-elements/classes.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1220907 - Add suffix to chrome css uri in browser_parsable_css test so that the file is always reparsed.

Bug 1220907 - Add suffix to chrome css uri in browser_parsable_css test so that the file is always reparsed.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 4

[:Gijs (he/him)]

Comment on attachment 8682335 [details]
MozReview Request: Bug 1220907 - Add suffix to chrome css uri in browser_parsable_css test so that the file is always reparsed.

https://reviewboard.mozilla.org/r/24067/#review21473

Thanks for filing this! I'm afraid there are some missing bits in this patch, so this isn't quite r+ yet... please let me know if I misunderstood some of the patch. I'm also still curious what's currently causing global.css to not be loaded (if it isn't just the caching).

::: browser/base/content/test/general/browser_parsable_css.js:143
(Diff revision 1)
> -function messageIsCSSError(msg, innerWindowID, outerWindowID) {
> +function messageIsCSSError(msg) {

Why did you remove the filtering by window/iframe? I also see no concrete fix to make sure global.css is included, which makes me suspect that the removal of the filtering means we now get the error because global.css is loaded via other means. That's not the right solution in that we should figure out *why* global.css isn't currently being loaded here...

::: browser/base/content/test/general/browser_parsable_css.js:147
(Diff revision 1)
> -      msg.innerWindowID === innerWindowID && msg.outerWindowID === outerWindowID) {
> +      msg.sourceName.endsWith(kPathSuffix)) {

Some of the filtering uses expressions like:

/\.css$/

which will no longer match the sourceName on the msg object here because they now end in the path suffix. It seems we should fix either ignoredError to remove that bit before comparing specifically for the sourceName property, or we should fix messageIsCSSError to remove the suffix before passing the object to ignoredError.

::: browser/base/content/test/general/browser_parsable_css.js:148
(Diff revision 1)
> +    var sourceName = msg.sourceName.slice(0, -kPathSuffix.length);

Nit: let

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

(In reply to :Gijs Kruitbosch from comment #4)
> Comment on attachment 8682335 [details]
> MozReview Request: Bug 1220907 - Add suffix to chrome css uri in
> browser_parsable_css test so that the file is always reparsed.
> 
> https://reviewboard.mozilla.org/r/24067/#review21473
> 
> Thanks for filing this! I'm afraid there are some missing bits in this
> patch, so this isn't quite r+ yet... please let me know if I misunderstood
> some of the patch. I'm also still curious what's currently causing
> global.css to not be loaded (if it isn't just the caching).

Because we only check CSS files inside the browser/ directory, while global.css is in chrome/toolkit/. I tried to include all CSS files inside the whole package, but I failed to do so because many of those files include UA-only CSS styles which we cannot parse inside chrome. (But... wait... probably we can, via the trick for highlighters.css)

> ::: browser/base/content/test/general/browser_parsable_css.js:143
> (Diff revision 1)
> > -function messageIsCSSError(msg, innerWindowID, outerWindowID) {
> > +function messageIsCSSError(msg) {
> 
> Why did you remove the filtering by window/iframe? I also see no concrete
> fix to make sure global.css is included, which makes me suspect that the
> removal of the filtering means we now get the error because global.css is
> loaded via other means. That's not the right solution in that we should
> figure out *why* global.css isn't currently being loaded here...

Well, actually I'm not expecting it catching any additional errors, but it looks more promising anyway.

Comment 6

[:Gijs (he/him)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > Comment on attachment 8682335 [details]
> > MozReview Request: Bug 1220907 - Add suffix to chrome css uri in
> > browser_parsable_css test so that the file is always reparsed.
> > 
> > https://reviewboard.mozilla.org/r/24067/#review21473
> > 
> > Thanks for filing this! I'm afraid there are some missing bits in this
> > patch, so this isn't quite r+ yet... please let me know if I misunderstood
> > some of the patch. I'm also still curious what's currently causing
> > global.css to not be loaded (if it isn't just the caching).
> 
> Because we only check CSS files inside the browser/ directory, while
> global.css is in chrome/toolkit/. I tried to include all CSS files inside
> the whole package, but I failed to do so because many of those files include
> UA-only CSS styles which we cannot parse inside chrome. (But... wait...
> probably we can, via the trick for highlighters.css)

I'm confused. On infra, doesn't this run against omni.ja, which should have everything ? It should do... (you can simulate this on your local machine by passing the right flag: ./mach mochitest --appname dist ---- you'd need to have done a ./mach package beforehand, though)

> > ::: browser/base/content/test/general/browser_parsable_css.js:143
> > (Diff revision 1)
> > > -function messageIsCSSError(msg, innerWindowID, outerWindowID) {
> > > +function messageIsCSSError(msg) {
> > 
> > Why did you remove the filtering by window/iframe? I also see no concrete
> > fix to make sure global.css is included, which makes me suspect that the
> > removal of the filtering means we now get the error because global.css is
> > loaded via other means. That's not the right solution in that we should
> > figure out *why* global.css isn't currently being loaded here...
> 
> Well, actually I'm not expecting it catching any additional errors, but it
> looks more promising anyway.

I'm not sure what you mean here. In any case, I would expect the getMessagesArray() to return all the messages accumulated so far, which seems like it should actually also include the currently-manifesting -moz-window-shadow ones that we generate just by displaying the main browser window... right? And I tried to avoid duplicating these, or showing errors for e.g. test html files that were loaded during tests, that we do not actually ship as part of the product. But the test has been rewritten a few times now, maybe it's no longer necessary?

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #5)
> > (In reply to :Gijs Kruitbosch from comment #4)
> > > Comment on attachment 8682335 [details]
> > > MozReview Request: Bug 1220907 - Add suffix to chrome css uri in
> > > browser_parsable_css test so that the file is always reparsed.
> > > 
> > > https://reviewboard.mozilla.org/r/24067/#review21473
> > > 
> > > Thanks for filing this! I'm afraid there are some missing bits in this
> > > patch, so this isn't quite r+ yet... please let me know if I misunderstood
> > > some of the patch. I'm also still curious what's currently causing
> > > global.css to not be loaded (if it isn't just the caching).
> > 
> > Because we only check CSS files inside the browser/ directory, while
> > global.css is in chrome/toolkit/. I tried to include all CSS files inside
> > the whole package, but I failed to do so because many of those files include
> > UA-only CSS styles which we cannot parse inside chrome. (But... wait...
> > probably we can, via the trick for highlighters.css)
> 
> I'm confused. On infra, doesn't this run against omni.ja, which should have
> everything ? It should do... (you can simulate this on your local machine by
> passing the right flag: ./mach mochitest --appname dist ---- you'd need to
> have done a ./mach package beforehand, though)

I almost always use --appname dist, because it's much faster in this test than without it.

It seems we have multiple omni.ja files, one in the root of the firefox directory, one in browser/, one in webapprt/. (The last one, though, is not registered by default. And to work around this, I added some code to register all chrome.manifest when I tried to include all CSS files.)

> > > ::: browser/base/content/test/general/browser_parsable_css.js:143
> > > (Diff revision 1)
> > > > -function messageIsCSSError(msg, innerWindowID, outerWindowID) {
> > > > +function messageIsCSSError(msg) {
> > > 
> > > Why did you remove the filtering by window/iframe? I also see no concrete
> > > fix to make sure global.css is included, which makes me suspect that the
> > > removal of the filtering means we now get the error because global.css is
> > > loaded via other means. That's not the right solution in that we should
> > > figure out *why* global.css isn't currently being loaded here...
> > 
> > Well, actually I'm not expecting it catching any additional errors, but it
> > looks more promising anyway.
> 
> I'm not sure what you mean here. In any case, I would expect the
> getMessagesArray() to return all the messages accumulated so far, which
> seems like it should actually also include the currently-manifesting
> -moz-window-shadow ones that we generate just by displaying the main browser
> window... right?

I guess so. But I'd always prefer we only catch the errors generated in this test, because I suppose if that includes the errors from the main browser, it may also include errors generated from other tests before.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8682335 [details]
MozReview Request: Bug 1220907 - Add suffix to chrome css uri in browser_parsable_css test so that the file is always reparsed.

Bug 1220907 - Add suffix to chrome css uri in browser_parsable_css test so that the file is always reparsed.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

So global.css is not covered simply because it is not in the correct directory.

Extending this test to cover the whole range of files needs much more work then fixing what it was doing. I've filed bug 1221383 for that with some notes there.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8682335 [details]
MozReview Request: Bug 1220907 - Add suffix to chrome css uri in browser_parsable_css test so that the file is always reparsed.

https://reviewboard.mozilla.org/r/24067/#review21645

::: browser/base/content/test/general/browser_parsable_css.js:150
(Diff revision 2)
> +      sourceName: sourceName,

Nit: just sourceName,

works.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac86f7cbd322753d6dbeaccc438c066392ab0b97
Bug 1220907 - Add suffix to chrome css uri in browser_parsable_css test so that the file is always reparsed. r=Gijs

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac86f7cbd322

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63359150f5c0

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/63359150f5c0