Closed Bug 1408322 Opened 8 years ago Closed 8 years ago

replace inDOMUtils.CssPropertyIsValid() with CSS.supports()

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
firefox58 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file)

Bug 1408322 - Remove inDOMUtils.cssPropertyIsValid() and use CSS.supports() instead.
59 bytes, text/x-review-board-request
jryans
: review+
Details
inDOMUtils::CssPropertyIsValid currently uses nsCSSParser to check if a string is a valid value for a given CSS property. We should call into Servo to do this.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc417a53a71e6d3b0a7783430a475d6c9f43de41 There is one caller to nsCSSParser::IsValidValueForProperty left after this, in KeyframeUtils.cpp, but it's in a Gecko-only block, so can be removed at the same as the block.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Summary: make inDOMUtils::CssPropertyIsValid use Servo → replace inDOMUtils.CssPropertyIsValid() with CSS.supports()
(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #2) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=cc417a53a71e6d3b0a7783430a475d6c9f43de41 Hmm, I guess that didn't work. Tom, is it expected that regular window globals like CSS.supports() are not available in devtools/server/actors/styles.js? If so I guess I'll just re-implement cssPropertyIsValid internally in terms of CSS.supports().
Flags: needinfo?(ttromey)
(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #3) > (In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment > #2) > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=cc417a53a71e6d3b0a7783430a475d6c9f43de41 > > Hmm, I guess that didn't work. Tom, is it expected that regular window > globals like CSS.supports() are not available in > devtools/server/actors/styles.js? If so I guess I'll just re-implement > cssPropertyIsValid internally in terms of CSS.supports(). The actors run in windowless sandboxes, but we can import certain globals for use in situations like this. `CSS` is imported already. `CSS.escape` is used in styles.js, for example: http://searchfox.org/mozilla-central/rev/1c1a5cef772214e9cff487040ac3068d56e96cc6/devtools/server/actors/styles.js#908 Perhaps `CSS.supports` requires a window in some way that fails for the actors?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4) > The actors run in windowless sandboxes, but we can import certain globals > for use in situations like this. `CSS` is imported already. `CSS.escape` > is used in styles.js, for example: > > http://searchfox.org/mozilla-central/rev/ > 1c1a5cef772214e9cff487040ac3068d56e96cc6/devtools/server/actors/styles.js#908 > > Perhaps `CSS.supports` requires a window in some way that fails for the > actors? Yeah, seems that's the issue. CSS::Supports tries to get an nsGlobalObject from the JS global (which isn't there), but CSS::Escape doesn't need one. http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/layout/style/CSS.cpp#34-37
Flags: needinfo?(ttromey)
It's possible to reach a content window from the actors, so maybe that's what we want here. In styles.js, something like `this.inspector.tabActor.window` would be the main window for the tab. That seems like it would give a CSS object useable for this purpose. Or if you need the specific window that loaded a sheet (assuming multiple frames in the tab), we can work that out too, just let me know.
Comment on attachment 8918537 [details] Bug 1408322 - Remove inDOMUtils.cssPropertyIsValid() and use CSS.supports() instead. https://reviewboard.mozilla.org/r/189396/#review194942 Thanks for working on this! :) Looks good overall to me. ::: devtools/server/actors/styles.js:1119 (Diff revision 4) > form.declarations = declarations.map(decl => { > - decl.isValid = DOMUtils.cssPropertyIsValid(decl.name, decl.value); > + // Use the 1-arg CSS.supports() call so that we also accept !important > + // in the value. (We need to grab CSS from the window, since calling > + // supports() on the one from the current global will fail due to not > + // being an HTML global.) > + let CSS = this.pageStyle.inspector.tabActor.window.CSS; Move the `let CSS = ...` line up and out of the `.map` block, since it's a constant that doesn't change for each declaration. (I guess the comment about grabbing it from the window should also move with it, but leave the 1-arg comment where it is.)
Attachment #8918537 - Flags: review?(jryans) → review+
Thanks for the tip to look for the window through the inspector actor. :-)
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8db059812121 Remove inDOMUtils.cssPropertyIsValid() and use CSS.supports() instead. r=jryans
https://hg.mozilla.org/mozilla-central/rev/8db059812121
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: