Closed
Bug 1410074
Opened 3 years ago
Closed 3 years ago
Allow MatchMedia in chrome code to parse and match system metric media queries.
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(2 files, 2 obsolete files)
That way we can unship all those from content pages.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•3 years ago
|
||
mozreview-review |
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 7•3 years ago
|
||
mozreview-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+
Updated•3 years ago
|
Attachment #8921007 -
Flags: review?(bzbarsky)
Comment 8•3 years ago
|
||
mozreview-review |
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 9•3 years ago
|
||
mozreview-review |
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 10•3 years ago
|
||
mozreview-review |
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)
Updated•3 years ago
|
status-firefox57:
--- → wontfix
Priority: -- → P3
Assignee | ||
Comment 11•3 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 12•3 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•3 years ago
|
Attachment #8921007 -
Flags: review?(bzbarsky)
Attachment #8921008 -
Flags: review?(xidorn+moz)
Attachment #8921009 -
Flags: review?(xidorn+moz)
Assignee | ||
Comment 17•3 years ago
|
||
Comment on attachment 8921008 [details] Bug 1410074: Honor CallerType for media query parsing. WTF mozreview
Attachment #8921008 -
Flags: review?(xidorn+moz)
Assignee | ||
Updated•3 years ago
|
Attachment #8921009 -
Flags: review?(xidorn+moz)
Comment 18•3 years ago
|
||
mozreview-review |
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 19•3 years ago
|
||
mozreview-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 20•3 years ago
|
||
mozreview-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+
Comment 21•3 years ago
|
||
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
Comment 22•3 years ago
|
||
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
Assignee | ||
Comment 23•3 years ago
|
||
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
Comment 24•3 years ago
|
||
Backout by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f029da33b7e1 Backed out 3 changesets for Windows reftest failures.
Comment 25•3 years ago
|
||
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
Comment 26•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/783de43210e9
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•3 years ago
|
Attachment #8921007 -
Attachment is obsolete: true
Assignee | ||
Updated•3 years ago
|
Attachment #8921008 -
Attachment is obsolete: true
![]() |
||
Comment 29•3 years ago
|
||
mozreview-review |
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+
Comment 30•3 years ago
|
||
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
Assignee | ||
Updated•3 years ago
|
Keywords: leave-open
![]() |
||
Comment 31•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1e66f5d7cbc2 https://hg.mozilla.org/mozilla-central/rev/356109a93130
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
![]() |
||
Comment 32•3 years ago
|
||
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)
![]() |
||
Comment 33•3 years ago
|
||
And we should include the new media queries from bug 1413166 in that followup test.
Assignee | ||
Comment 34•3 years ago
|
||
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)
You need to log in
before you can comment on or make changes to this bug.
Description
•