Closed Bug 1408322 Opened 5 years ago Closed 5 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: 5 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.