Closed Bug 1410074 Opened 2 years ago Closed 2 years ago

Allow MatchMedia in chrome code to parse and match system metric media queries.

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1410074: Load input.css from a chrome uri, so windows reftests can use system metric media queries.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1410074: Restrict system metric stuff in content pages.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
That way we can unship all those from content pages.
Comment on attachment 8921006 [details]
Bug 1410074: Load input.css from a chrome uri, so windows reftests can use system metric media queries.

https://reviewboard.mozilla.org/r/191970/#review197454
Attachment #8921006 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8921007 [details]
Bug 1410074: Make the caller type from MatchMedia arrive to the CSS code.

https://reviewboard.mozilla.org/r/191972/#review197474

Looks reasonable to me, but I'm not very familiar with the CallerType stuff, so r? bz in addition. You would need a DOM peer for the WebIDL change anyway.

::: layout/style/MediaList.h:48
(Diff revision 1)
>    /**
>     * Creates a MediaList backed by the given StyleBackendType.
>     */
>    static already_AddRefed<MediaList> Create(StyleBackendType,
> -                                            const nsAString& aMedia);
> +                                            const nsAString& aMedia,
> +                                            CallerType aCallerType);

Seems to me you can just add a default parameter here instead of adding a function overload.
Attachment #8921007 - Flags: review?(xidorn+moz) → review+
Attachment #8921007 - Flags: review?(bzbarsky)
Comment on attachment 8921008 [details]
Bug 1410074: Honor CallerType for media query parsing.

https://reviewboard.mozilla.org/r/191974/#review197478

::: layout/style/nsCSSParser.cpp:359
(Diff revision 1)
> -    return mParsingMode == css::eAgentSheetFeatures;
> +    return mParsingMode == css::eAgentSheetFeatures ||
> +      mDomCallerType == dom::CallerType::System;

Do we need this?

I don't think this makes any difference here, and in general, I don't think we should enable agent features for chrome code...

::: servo/ports/geckolib/glue.rs:2743
(Diff revision 1)
> -pub extern "C" fn Servo_MediaList_SetText(list: RawServoMediaListBorrowed, text: *const nsACString) {
> -    let text = unsafe { text.as_ref().unwrap().as_str_unchecked() };
> +pub unsafe extern "C" fn Servo_MediaList_SetText(
> +    list: RawServoMediaListBorrowed,
> +    text: *const nsACString,
> +    caller_type: CallerType,
> +) {

The callsite of this function is not changed?

Also it seems to me you need to add `CallerType` to `ServoBindings.toml`, into `whitelist-types` in `[structs]` and `structs-types` in `[bindings]`.

::: servo/ports/geckolib/glue.rs:2753
(Diff revision 1)
> -    let context = ParserContext::new_for_cssom(url_data, Some(CssRuleType::Media),
> +    let origin = match caller_type {
> +        CallerType::System => Origin::UserAgent,
> +        CallerType::NonSystem => Origin::Author,
> +    };

I'm not really happy with giving it user-agent permission, although it may not matter a lot for media list...

Can we use an alternate dummy url data for that?
Attachment #8921008 - Flags: review?(xidorn+moz)
Comment on attachment 8921008 [details]
Bug 1410074: Honor CallerType for media query parsing.

https://reviewboard.mozilla.org/r/191974/#review197480

::: layout/style/nsCSSParser.cpp:363
(Diff revision 1)
>    bool AgentRulesEnabled() const {
> -    return mParsingMode == css::eAgentSheetFeatures;
> +    return mParsingMode == css::eAgentSheetFeatures ||
> +      mDomCallerType == dom::CallerType::System;
>    }
>    bool ChromeRulesEnabled() const {
> -    return mIsChrome;
> +    return mIsChrome || mDomCallerType == dom::CallerType::System;

Actually... can we simply reuse the `mIsChrome` flag for this purpose in `nsCSSParser`?
Comment on attachment 8921009 [details]
Bug 1410074: Restrict system metric stuff in content pages.

https://reviewboard.mozilla.org/r/191976/#review197484

::: layout/style/nsMediaFeatures.cpp:421
(Diff revision 2)
> -  if (ShouldResistFingerprinting(aPresContext)) {
> +  MOZ_ASSERT(aFeature->mReqFlags & nsMediaFeature::eUserAgentAndChromeOnly);
> -    return;
> -  }

I suggest you don't remove this. See bug 1408702 comment 9.

::: layout/style/test/chrome/bug418986-2.js:7
(Diff revision 2)
>  
>  /* jshint esnext:true */
>  /* jshint loopfunc:true */
>  /* global window, screen, ok, SpecialPowers, matchMedia */
>  
> +const is_chrome_window = window.location.protocol === "chrome:";

I don't see how it makes sense to have this variable here... Are you using it anywhere?

::: layout/style/test/chrome/bug418986-2.js
(Diff revision 2)
> -  "-moz-scrollbar-end-backward",
> -  "-moz-scrollbar-end-forward",
> -  "-moz-scrollbar-start-backward",
> -  "-moz-scrollbar-start-forward",
> -  "-moz-scrollbar-thumb-proportional",
> -  "-moz-touch-enabled",

Shouldn't you keep checking `-moz-touch-enabled`?

Also it might be better to check that all of them are consistently unavailable rather than simply remove them from the test.
Attachment #8921009 - Flags: review?(xidorn+moz)
status-firefox57: --- → wontfix
Priority: -- → P3
Comment on attachment 8921009 [details]
Bug 1410074: Restrict system metric stuff in content pages.

https://reviewboard.mozilla.org/r/191976/#review197538

::: layout/style/nsMediaFeatures.cpp:421
(Diff revision 2)
> -  if (ShouldResistFingerprinting(aPresContext)) {
> +  MOZ_ASSERT(aFeature->mReqFlags & nsMediaFeature::eUserAgentAndChromeOnly);
> -    return;
> -  }

Oh, right... That's obscure, but... I guess

::: layout/style/test/chrome/bug418986-2.js:7
(Diff revision 2)
>  
>  /* jshint esnext:true */
>  /* jshint loopfunc:true */
>  /* global window, screen, ok, SpecialPowers, matchMedia */
>  
> +const is_chrome_window = window.location.protocol === "chrome:";

Yup, I moved it when I was trying to rewrite the tests... but then decided they weren't worth it. But per the previous comment, they are, so will send a revised patch your way.

::: layout/style/test/chrome/bug418986-2.js
(Diff revision 2)
> -  "-moz-scrollbar-end-backward",
> -  "-moz-scrollbar-end-forward",
> -  "-moz-scrollbar-start-backward",
> -  "-moz-scrollbar-start-forward",
> -  "-moz-scrollbar-thumb-proportional",
> -  "-moz-touch-enabled",

Right, so the point is that this test runs in both a chrome and a non-chrome document, so you need to handle both cases. We already have the `unparseable` tests to test they're not usable in content, and since I was removing the "claim not to ever match when `privacy.resistFingerprinting` is on", the test looked extremely stupid... I guess if I don't remove that check as you suggest I can keep testing stuff.
Comment on attachment 8921008 [details]
Bug 1410074: Honor CallerType for media query parsing.

https://reviewboard.mozilla.org/r/191974/#review197542

::: layout/style/nsCSSParser.cpp:359
(Diff revision 1)
> -    return mParsingMode == css::eAgentSheetFeatures;
> +    return mParsingMode == css::eAgentSheetFeatures ||
> +      mDomCallerType == dom::CallerType::System;

Hm, ok, makes sense. I can also explicitly test `mDomCallerType` in media query parsing if you prefer.

::: layout/style/nsCSSParser.cpp:363
(Diff revision 1)
>    bool AgentRulesEnabled() const {
> -    return mParsingMode == css::eAgentSheetFeatures;
> +    return mParsingMode == css::eAgentSheetFeatures ||
> +      mDomCallerType == dom::CallerType::System;
>    }
>    bool ChromeRulesEnabled() const {
> -    return mIsChrome;
> +    return mIsChrome || mDomCallerType == dom::CallerType::System;

Perhaps, yeah! Will do that.

::: servo/ports/geckolib/glue.rs:2743
(Diff revision 1)
> -pub extern "C" fn Servo_MediaList_SetText(list: RawServoMediaListBorrowed, text: *const nsACString) {
> -    let text = unsafe { text.as_ref().unwrap().as_str_unchecked() };
> +pub unsafe extern "C" fn Servo_MediaList_SetText(
> +    list: RawServoMediaListBorrowed,
> +    text: *const nsACString,
> +    caller_type: CallerType,
> +) {

Hmpf, I think there was a commit in the middle that got lost it seems? huh

::: servo/ports/geckolib/glue.rs:2753
(Diff revision 1)
> -    let context = ParserContext::new_for_cssom(url_data, Some(CssRuleType::Media),
> +    let origin = match caller_type {
> +        CallerType::System => Origin::UserAgent,
> +        CallerType::NonSystem => Origin::Author,
> +    };

Yeah, will think about it.
Attachment #8921007 - Flags: review?(bzbarsky)
Attachment #8921008 - Flags: review?(xidorn+moz)
Attachment #8921009 - Flags: review?(xidorn+moz)
Comment on attachment 8921008 [details]
Bug 1410074: Honor CallerType for media query parsing.

WTF mozreview
Attachment #8921008 - Flags: review?(xidorn+moz)
Attachment #8921009 - Flags: review?(xidorn+moz)
Comment on attachment 8921008 [details]
Bug 1410074: Honor CallerType for media query parsing.

https://reviewboard.mozilla.org/r/191974/#review197590
Attachment #8921008 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8921009 [details]
Bug 1410074: Restrict system metric stuff in content pages.

https://reviewboard.mozilla.org/r/191976/#review197594

::: layout/style/test/chrome/bug418986-2.js:203
(Diff revision 3)
>  // Creates a series of lines of CSS, each of which corresponds to
>  // a different media query. If the query produces a match to the
>  // expected value, then the element will be colored green.
>  var generateCSSLines = function (resisting) {
>    let lines = ".spoof { background-color: red;}\n";
>    let is_chrome_window = window.location.protocol === "chrome:";

You can remove this line now, I guess.
Attachment #8921009 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8921007 [details]
Bug 1410074: Make the caller type from MatchMedia arrive to the CSS code.

https://reviewboard.mozilla.org/r/191972/#review197820

::: layout/style/nsCSSParser.cpp:1332
(Diff revision 2)
>    };
>    nsCSSSection  mSection;
>  
>    nsXMLNameSpaceMap *mNameSpaceMap;  // weak, mSheet owns it
>  
> +  // Whether the caller of the DOM API that invoked us is chrome or not.

s/is chrome/has the system principal/
Attachment #8921007 - Flags: review?(bzbarsky) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d026ae10f36d
Make the caller type from MatchMedia arrive to the CSS code. r=xidorn,bz
https://hg.mozilla.org/integration/autoland/rev/a1fb2b6665d9
Honor CallerType for media query parsing. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/d045057cdf68
Restrict system metric stuff in content pages. r=xidorn
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4d771e18c365
Fix browser_parsable_css to account for the restricted media queries. r=me
https://hg.mozilla.org/integration/autoland/rev/5d54875788e3
Fix windows-specific resistFingerprinting tests. r=me
Leaving need-open because I forgot about the windows editor reftests ;_;

I just requested a backout, will get those sorted out tomorrow.
Keywords: leave-open
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f029da33b7e1
Backed out 3 changesets for Windows reftest failures.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/783de43210e9
Fixup tests by relanding a test change which should've been part of the second patch. r=me
Attachment #8921007 - Attachment is obsolete: true
Attachment #8921008 - Attachment is obsolete: true
Comment on attachment 8921006 [details]
Bug 1410074: Load input.css from a chrome uri, so windows reftests can use system metric media queries.

https://reviewboard.mozilla.org/r/191970/#review198608
Attachment #8921006 - Flags: review?(bzbarsky) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1e66f5d7cbc2
Restrict system metric stuff in content pages. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/356109a93130
Load input.css from a chrome uri, so windows reftests can use system metric media queries. r=bz,xidorn
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/1e66f5d7cbc2
https://hg.mozilla.org/mozilla-central/rev/356109a93130
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Emilio, we now no longer have test coverage for these media queries being parseable in chrome stylesheets, right?  Is there an existing followup bug to add that test coverage?
Flags: needinfo?(emilio)
And we should include the new media queries from bug 1413166 in that followup test.
Hm, well, we have this:

 * http://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/layout/style/test/chrome/bug418986-2.js#113

But that only tests known-good cases, I guess, and only of a subset of them. I'll file.
Flags: needinfo?(emilio)
See Also: → 1413754
You need to log in before you can comment on or make changes to this bug.