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)
Core
CSS Parsing and Computation
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.
Updated•8 years ago
|
Priority: -- → P3
Updated•8 years ago
|
Keywords: dev-doc-needed
![]() |
Reporter | |
Comment 1•7 years ago
|
||
According to https://web-confluence.appspot.com/#!/catalog?releaseKeys=%5B%22Firefox_56.0_Windows_10.0%22,%22Safari_11.0_OSX_10.12.6%22,%22Chrome_61.0.3163.79_Windows_10.0%22,%22Edge_15.15063_Windows_10.0%22%5D&releaseOptions=null&numAvailable=null¤tPage=3&gap=5&itemPerPage=15&searchKey=%22%22 this is implemented by Safari, Chrome, and Edge.
Xidorn, want to take a shot at this?
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 2•7 years ago
|
||
Seems to be easy.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
![]() |
Reporter | |
Comment 6•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
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
![]() |
Reporter | |
Comment 10•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c17dc856bea6
followup - Handle null prefix in namespace rule gracefully for Gecko backend.
Assignee | ||
Comment 12•7 years ago
|
||
(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)
![]() |
Reporter | |
Comment 13•7 years ago
|
||
> 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.
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cd981f26866c
https://hg.mozilla.org/mozilla-central/rev/c17dc856bea6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 15•7 years ago
|
||
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)
Keywords: dev-doc-needed → dev-doc-complete
Updated•7 years ago
|
status-firefox53:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•