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

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Firefox
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
status-firefox44: --- → affected
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.
Summary: Ensure browser_parsable_css.js test really tests all css files we are shipping → Ensure browser_parsable_css.js test really tests all CSS files in the application
Depends on: 1220903
Created 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.
Attachment #8682335 - Flags: review?(gijskruitbosch+bugs)
Assignee: nobody → quanxunzhen
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bec3b8f5c301

Updated

2 years ago
Attachment #8682335 - Flags: review?(gijskruitbosch+bugs)

Comment 4

2 years ago
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
(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

2 years ago
(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?
(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 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.
Attachment #8682335 - Flags: review?(gijskruitbosch+bugs)
Blocks: 1221383
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.

Updated

2 years ago
Attachment #8682335 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 10

2 years ago
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.
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ac86f7cbd322
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/63359150f5c0
status-firefox44: affected → fixed

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/63359150f5c0
status-b2g-v2.5: --- → fixed
You need to log in before you can comment on or make changes to this bug.