1207443 - Change browser_parsable_css.js to load stylesheets in chrome privilege

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

Currently, browser_parsable_css.js loads stylesheets with "file:" or "jar:file:" scheme. It worked fine. But as we are going to make some properties chrome-only, we need to change this test to make those files load with chrome privilege to check the identical parsing behavior.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1207443 - Change browser_parsable_css.js to load stylesheets with chrome privilege.

Bug 1207443 - Change browser_parsable_css.js to load stylesheets with chrome privilege.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8664735 [details]
MozReview Request: Bug 1207443 - Change browser_parsable_css.js to load stylesheets with chrome privilege.

https://reviewboard.mozilla.org/r/20023/#review18037

::: browser/base/content/test/general/browser_parsable_css.js:85
(Diff revision 1)
> +      reject();

Doing this will throw in the yield, and that will break the entire test. I don't think you need to reject anything if the manifest couldn't be loaded or parsed - ok(false,...) it as you're doing, and then resolve().

::: browser/base/content/test/general/browser_parsable_css.js:108
(Diff revision 1)
> +      let baseUri = `${dir}${baseDir}`;

This seems prone to breakage, especially with relative paths, as well as "override" directives.

Is there a particular reason not to use the chrome registry to resolve the package URIs you get (and kill off the filename that it will automagically add on for you)? That seems much less fragile.

::: browser/base/content/test/general/browser_parsable_css.js:153
(Diff revision 1)
> -  let uris = yield generateURIsFromDirTree(appDir, ".css");
> +  let uris = yield generateURIsFromDirTree(appDir, [".css", ".manifest"]);

Does this work locally both with and without --appname dist ?

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

(In reply to :Gijs Kruitbosch from comment #3)
> ::: browser/base/content/test/general/browser_parsable_css.js:108
> (Diff revision 1)
> > +      let baseUri = `${dir}${baseDir}`;
> 
> This seems prone to breakage, especially with relative paths, as well as
> "override" directives.
> 
> Is there a particular reason not to use the chrome registry to resolve the
> package URIs you get (and kill off the filename that it will automagically
> add on for you)? That seems much less fragile.

Because it doesn't seem to me chrome registry provides the conversion from file path to chrome path, and as you may have noticed, writing a perfect reverse conversion is non-trivial, and thus it could be an overkill to have that implemented in chrome registry just for this test.

If you're concerned that the result here may not match chrome registry, I can add an additional check to ensure the generated chrome path converts to the original file path. Does that sound good to you?

> ::: browser/base/content/test/general/browser_parsable_css.js:153
> (Diff revision 1)
> > -  let uris = yield generateURIsFromDirTree(appDir, ".css");
> > +  let uris = yield generateURIsFromDirTree(appDir, [".css", ".manifest"]);
> 
> Does this work locally both with and without --appname dist ?

Yes. I tested both.

Comment 5

[:Gijs (he/him)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > ::: browser/base/content/test/general/browser_parsable_css.js:108
> > (Diff revision 1)
> > > +      let baseUri = `${dir}${baseDir}`;
> > 
> > This seems prone to breakage, especially with relative paths, as well as
> > "override" directives.
> > 
> > Is there a particular reason not to use the chrome registry to resolve the
> > package URIs you get (and kill off the filename that it will automagically
> > add on for you)? That seems much less fragile.
> 
> Because it doesn't seem to me chrome registry provides the conversion from
> file path to chrome path, and as you may have noticed, writing a perfect
> reverse conversion is non-trivial, and thus it could be an overkill to have
> that implemented in chrome registry just for this test.
> 
> If you're concerned that the result here may not match chrome registry, I
> can add an additional check to ensure the generated chrome path converts to
> the original file path. Does that sound good to you?

I must have been unclear: this mapping you're creating manually constructs baseUri from the contents of the manifest.

Instead, you could pass `chrome://${package}/${contentorskin}/gobbledygooknonexistentfile.reallynothere` to chromeRegistry.resolveURI, and strip the nonsense filename from the result

... rather than trying to resolve the URI listed in the manifest yourself.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

(In reply to :Gijs Kruitbosch from comment #5)
> I must have been unclear: this mapping you're creating manually constructs
> baseUri from the contents of the manifest.
> 
> Instead, you could pass
> `chrome://${package}/${contentorskin}/gobbledygooknonexistentfile.
> reallynothere` to chromeRegistry.resolveURI, and strip the nonsense filename
> from the result
> 
> ... rather than trying to resolve the URI listed in the manifest yourself.

Oh, okay, that sounds good. Thanks for explaining.

Comment 7

[:Gijs (he/him)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > I must have been unclear: this mapping you're creating manually constructs
> > baseUri from the contents of the manifest.
> > 
> > Instead, you could pass
> > `chrome://${package}/${contentorskin}/gobbledygooknonexistentfile.
> > reallynothere` to chromeRegistry.resolveURI, and strip the nonsense filename
> > from the result
> > 
> > ... rather than trying to resolve the URI listed in the manifest yourself.
> 
> Oh, okay, that sounds good. Thanks for explaining.

No worries, sorry for not being clear from the get-go.

To be 100% explicit - I'm suggesting the bogus filename because normally, if you ask the chrome registry for "chrome://browser/content/", it will automatically get you the file/path for chrome://browser/content/browser.xul (and chrome://browser/skin/browser.css for skin packages). Those files might be overridden, in which case the path won't be the content/skin package's base path.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8664735 [details]
MozReview Request: Bug 1207443 - Change browser_parsable_css.js to load stylesheets with chrome privilege.

Bug 1207443 - Change browser_parsable_css.js to load stylesheets with chrome privilege.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8664735 [details]
MozReview Request: Bug 1207443 - Change browser_parsable_css.js to load stylesheets with chrome privilege.

https://reviewboard.mozilla.org/r/20023/#review18049

LGTM, see two nits below. Thanks for doing this!

First nit: can you please revert my change to https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/floating-scrollbars-light.css and return it to using a relative path to @import the floating-scrollbars.css file?

::: browser/base/content/test/general/browser_parsable_css.js:104
(Diff revision 2)
> +    let dir = manifestUri.resolve(".");

This is now unused, I think?

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

(In reply to :Gijs Kruitbosch from comment #9)
> Comment on attachment 8664735 [details]
> MozReview Request: Bug 1207443 - Change browser_parsable_css.js to load
> stylesheets with chrome privilege.
> 
> https://reviewboard.mozilla.org/r/20023/#review18049
> 
> LGTM, see two nits below. Thanks for doing this!
> 
> First nit: can you please revert my change to
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/
> floating-scrollbars-light.css and return it to using a relative path to
> @import the floating-scrollbars.css file?

OK

> ::: browser/base/content/test/general/browser_parsable_css.js:104
> (Diff revision 2)
> > +    let dir = manifestUri.resolve(".");
> 
> This is now unused, I think?

Yeah, you're right. Thanks.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/fx-team/rev/8db7294a94a3ae952f0586e820cf1c6056f259f3
Bug 1207443 - Change browser_parsable_css.js to load stylesheets with chrome privilege. r=Gijs

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8db7294a94a3