Closed Bug 1326514 Opened 3 years ago Closed 2 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.
https://hg.mozilla.org/mozilla-central/rev/cd981f26866c
https://hg.mozilla.org/mozilla-central/rev/c17dc856bea6
Status: NEW → RESOLVED
Closed: 2 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.