Closed Bug 1326514 Opened 8 years ago Closed 7 years ago

Implement the CSSOM bits for CSSNamespaceRule

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bzbarsky, Assigned: xidorn)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

We don't implement the "namespaceURI" or "prefix" attrs from https://drafts.csswg.org/cssom/#cssnamespacerule Should check whether other browsers do.
Priority: -- → P3
Seems to be easy.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
Comment on attachment 8938214 [details] Bug 1326514 - Implement the CSSOM bits for CSSNamespaceRule. https://reviewboard.mozilla.org/r/208962/#review214728 ::: layout/style/CSSNamespaceRule.h:39 (Diff revision 2) > > // WebIDL interfaces > uint16_t Type() const final { > return nsIDOMCSSRule::NAMESPACE_RULE; > } > + void GetNamespaceURI(nsAString& aNamespaceURI) { Justd make the arg `nsString& aNamespaceURI` and then you can call through to GetURLSpec() directly without the "spec" temporary. ::: layout/style/CSSNamespaceRule.h:44 (Diff revision 2) > + void GetNamespaceURI(nsAString& aNamespaceURI) { > + nsString spec; > + GetURLSpec(spec); > + aNamespaceURI.Assign(spec); > + } > + void GetPrefix(nsAString& aPrefix) { This works, or you could make it a DOMString& arg and then call SetOwnedAtom(GetPrefix(), DOMString::eNullNotExpected) and avoid some copies.
Attachment #8938214 - Flags: review?(bzbarsky) → review+
Thanks for the advice! It seems there are lots of things I need to learn with our WebIDL binding stuff.
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cd981f26866c Implement the CSSOM bits for CSSNamespaceRule. r=bz
Hmm. Actually, eNullNotExpected is not right, and neither was the original nsDependentAtomString thing, because the prefix can be null if this is a default-namespace rule, no? You presumably want eTreatNullAsEmpty and we should have tests for this if there aren't any so far.
Flags: needinfo?(xidorn+moz)
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c17dc856bea6 followup - Handle null prefix in namespace rule gracefully for Gecko backend.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10) > Hmm. Actually, eNullNotExpected is not right, and neither was the original > nsDependentAtomString thing, because the prefix can be null if this is a > default-namespace rule, no? This is not an issue for Servo backend, because it returns atom!("") when the prefix is null. [1] But it is an issue for Gecko backend, since that returns a nullable pointer. There is a wpt checking the behavior, and it crashes when stylo is disabled. I didn't notice that in the try push because I thought all oranges on the wpt task are from unexpected pass of the relevant tests. > You presumably want eTreatNullAsEmpty and we should have tests for this if > there aren't any so far. Anyway, I've pushed a followup to fix it. We may want to change it back to eNullNotExpected after we remove Gecko backend, if that matters. [1] https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/servo/ports/geckolib/glue.rs#1916-1921
Flags: needinfo?(xidorn+moz)
> We may want to change it back to eNullNotExpected after we remove Gecko backend, if that matters. It's a bit more efficient to have eNullNotExpected; the compiler can optimize out the null-check then. So yeah, we may want to do that.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
I've documented this: * The reference pages already existed, so I made sure the Fx support info was up to date: https://developer.mozilla.org/en-US/docs/Web/API/CSSNamespaceRule https://developer.mozilla.org/en-US/docs/Web/API/CSSNamespaceRule/namespaceURI https://developer.mozilla.org/en-US/docs/Web/API/CSSNamespaceRule/prefix * I added a note to the Fx59 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/59#CSSOM * I added this interface to the browser compat data project: https://github.com/mdn/browser-compat-data/pull/896 Please let me know if this looks OK. Thanks!
Flags: needinfo?(xidorn+moz)
Looks good, thanks.
Flags: needinfo?(xidorn+moz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: