stop registering namespaces in @namespace rules during parsing

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(5 attachments, 4 obsolete attachments)

2.15 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.21 KB, patch
bholley
: review+
Details | Diff | Splinter Review
27.68 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
6.01 KB, patch
bholley
: review+
Details | Diff | Splinter Review
7.01 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
Assignee

Description

a year ago
This is a problem for OMT parsing because the namespace manager isn't threadsafe.

The only reason we do this is so that we can stick the namespace id in the serialization on the style struct, which then gets parsed out by the frame constructor during content generation. There are obviously better ways to do this.

I have patches.
Assignee

Comment 2

a year ago
dbaron asked for this in bug 507762, but it never happened.

MozReview-Commit-ID: 6KQKGIAHJ4c
Attachment #8965079 - Flags: review?(xidorn+moz)
Assignee

Comment 3

a year ago
MozReview-Commit-ID: 3CiyFImX8UO
Attachment #8965081 - Flags: review?(bzbarsky)
Assignee

Comment 4

a year ago
Happy to hear suggestions on how to do this better.

MozReview-Commit-ID: 55KFG1fwCTt
Attachment #8965082 - Flags: review?(hsivonen)
Assignee

Comment 5

a year ago
I didn't end up using this, but mystor reviewed it and it's useful.

MozReview-Commit-ID: 5LNzCEGpbLy
Attachment #8965083 - Flags: review+
Assignee

Comment 6

a year ago
MozReview-Commit-ID: 2532dHCGPXW
Attachment #8965084 - Flags: review?(xidorn+moz)
Comment on attachment 8965079 [details] [diff] [review]
Part 1 - Add test for case insensitivity for content: attr() identifiers in html documents. v1

Review of attachment 8965079 [details] [diff] [review]:
-----------------------------------------------------------------

This probably should be a test in wpt.
Attachment #8965079 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8965084 [details] [diff] [review]
Part 5 - Stop using Gecko namespace ids in servo. v1

Review of attachment 8965084 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1704,2 @@
>        int32_t attrNameSpace = kNameSpaceID_None;
> +      if (ns) {

nit: you can do
> if (RefPtr<nsAtom> ns = attr->mNamespaceURL) {
>   ...
> }

::: layout/style/nsComputedDOMStyle.cpp
@@ +1386,5 @@
>        case eStyleContentType_Attr: {
> +        // XXXbholley: We don't correctly serialize namespaces here. Doing so
> +        // would require either storing the prefix on the nsStyleContentAttr,
> +        // or poking at the namespaces in the stylesheet to map from the
> +        // namespace URL.

I guess it isn't a big problem to just store prefix in nsStyleContentAttr. Is there any concern for doing that?

::: layout/style/nsStyleStruct.cpp
@@ +4143,5 @@
>    } else if (mType == eStyleContentType_Counter ||
>               mType == eStyleContentType_Counters) {
>      mContent.mCounters->Release();
> +  } else if (mType == eStyleContentType_String) {
> +    MOZ_ASSERT(mContent.mString);

This assertion doesn't seem to be very useful since `free` is a no-op if the pointer is nullptr. Actually, there is no guarantee from the previous code that mContent.mString must have value when mType is eStyleContentType_String.

@@ +4151,1 @@
>    }

Please instead assert here that mContent.mString is null to ensure that we are not leaking anything.

::: servo/components/style/properties/gecko.mako.rs
@@ +5515,1 @@
>                                      },

Hmmm... I really hope we have some sugar for refptr and nsAtom... but I guess this is fine for now.

@@ +5612,5 @@
> +                            let ns = if s.mNamespaceURL.mRawPtr.is_null() {
> +                                None
> +                            } else {
> +                                // FIXME(bholley): We don't have any way to get the prefix here. :-(
> +                                let prefix = Atom::from("");

`atom!("")`.
Attachment #8965084 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8965081 [details] [diff] [review]
Part 2 - Add an atom overload for namespace registration. v1

Do you still need the empty-string check in the string overload?  Do we hit that case often?

r=me either way.
Attachment #8965081 - Flags: review?(bzbarsky) → review+
Comment on attachment 8965082 [details] [diff] [review]
Part 3 - Add a ToLowerCase utility method for nsAtom. v1

Review of attachment 8965082 [details] [diff] [review]:
-----------------------------------------------------------------

The use cases for this method seem to apply to attribute (and element?) names, but it uses per-code point Unicode case mapping. Attribute and element names should use ASCII-lowercasing (https://infra.spec.whatwg.org/#ascii-lowercase).
Attachment #8965082 - Flags: review?(hsivonen) → review-
Assignee

Comment 11

a year ago
MozReview-Commit-ID: CcGeRiaQxNW
Attachment #8965425 - Flags: review?(hsivonen)
Assignee

Updated

a year ago
Attachment #8965082 - Attachment is obsolete: true
Assignee

Comment 12

a year ago
Inverted a boolean condition in the last patch, which the tests caught.
Attachment #8965425 - Attachment is obsolete: true
Attachment #8965447 - Flags: review?(hsivonen)
Attachment #8965425 - Flags: review?(hsivonen)
Assignee

Comment 13

a year ago
The test from part 1, rewritten as wpt.

MozReview-Commit-ID: 5W8rOZSGjkk
Attachment #8965448 - Flags: review+
Assignee

Comment 14

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> Do you still need the empty-string check in the string overload?  Do we hit
> that case often?

It's unlikely, I suppose. I'll try removing it.
Assignee

Comment 15

a year ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8)
> ::: layout/base/nsCSSFrameConstructor.cpp
> @@ +1704,2 @@
> >        int32_t attrNameSpace = kNameSpaceID_None;
> > +      if (ns) {
> 
> nit: you can do
> > if (RefPtr<nsAtom> ns = attr->mNamespaceURL) {
> >   ...
> > }

Fixed.

> 
> ::: layout/style/nsComputedDOMStyle.cpp
> @@ +1386,5 @@
> >        case eStyleContentType_Attr: {
> > +        // XXXbholley: We don't correctly serialize namespaces here. Doing so
> > +        // would require either storing the prefix on the nsStyleContentAttr,
> > +        // or poking at the namespaces in the stylesheet to map from the
> > +        // namespace URL.
> 
> I guess it isn't a big problem to just store prefix in nsStyleContentAttr.
> Is there any concern for doing that?

I played around with it a bit, but couldn't figure out how to write a testcase to exercise it (suggestions?). In any case, this was already broken and not very important, so I'm just going to punt on it for now.

> 
> ::: layout/style/nsStyleStruct.cpp
> @@ +4143,5 @@
> >    } else if (mType == eStyleContentType_Counter ||
> >               mType == eStyleContentType_Counters) {
> >      mContent.mCounters->Release();
> > +  } else if (mType == eStyleContentType_String) {
> > +    MOZ_ASSERT(mContent.mString);
> 
> This assertion doesn't seem to be very useful since `free` is a no-op if the
> pointer is nullptr. Actually, there is no guarantee from the previous code
> that mContent.mString must have value when mType is eStyleContentType_String.

Ok, I'll remove the assertion.
> 
> @@ +4151,1 @@
> >    }
> 
> Please instead assert here that mContent.mString is null to ensure that we
> are not leaking anything.

I assume you mean in a new |else| branch? I can add that.

> ::: servo/components/style/properties/gecko.mako.rs
> @@ +5515,1 @@
> >                                      },
> 
> Hmmm... I really hope we have some sugar for refptr and nsAtom... but I
> guess this is fine for now.

Yeah :\

> 
> @@ +5612,5 @@
> > +                            let ns = if s.mNamespaceURL.mRawPtr.is_null() {
> > +                                None
> > +                            } else {
> > +                                // FIXME(bholley): We don't have any way to get the prefix here. :-(
> > +                                let prefix = Atom::from("");
> 
> `atom!("")`.

Fixed.
Assignee

Comment 17

a year ago
Removed duplicate (and unused) macro which caused redefinition failures.
Attachment #8965482 - Flags: review?(hsivonen)
Assignee

Updated

a year ago
Attachment #8965079 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8965447 - Attachment is obsolete: true
Attachment #8965447 - Flags: review?(hsivonen)
Comment on attachment 8965482 [details] [diff] [review]
Part 3 - Add a ToLowerCaseASCII utility method for nsAtom. v4

Review of attachment 8965482 [details] [diff] [review]:
-----------------------------------------------------------------

r+ provided that the comments are addressed.

::: intl/unicharutil/util/nsUnicharUtils.cpp
@@ +192,5 @@
> +void
> +ToLowerCaseASCII(const char16_t *aIn, char16_t *aOut, uint32_t aLen)
> +{
> +  for (uint32_t i = 0; i < aLen; i++) {
> +    char32_t ch = aIn[i];

This should be char16_t.

@@ +193,5 @@
> +ToLowerCaseASCII(const char16_t *aIn, char16_t *aOut, uint32_t aLen)
> +{
> +  for (uint32_t i = 0; i < aLen; i++) {
> +    char32_t ch = aIn[i];
> +    aOut[i] = IS_ASCII_UPPER(ch) ? ToLowerCaseASCII(ch) : ch;

Please make this line
aOut[i] = IS_ASCII_UPPER(ch) ? (ch + 0x20) : ch;

(ToLowerCaseASCII() assumes that the compiler isn't smart enough to optimize IS_ASCII_UPPER into a single branch, and optimizing it to a single branch is simple, so I'd be surprised if our compilers didn't do it.)

::: intl/unicharutil/util/nsUnicharUtils.h
@@ +33,4 @@
>  void ToUpperCase(const nsAString& aSource, nsAString& aDest);
>  
>  uint32_t ToLowerCase(uint32_t aChar);
> +uint32_t ToLowerCaseASCII(uint32_t aChar);

Then adding this isn't necessary.
Attachment #8965482 - Flags: review?(hsivonen) → review+
Comment on attachment 8965084 [details] [diff] [review]
Part 5 - Stop using Gecko namespace ids in servo. v1

Review of attachment 8965084 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the drive-by nit-reporting pass :)

::: servo/components/style/gecko/selector_parser.rs
@@ +469,5 @@
>          ))
>      }
>  
>      fn default_namespace(&self) -> Option<Namespace> {
> +        self.namespaces.default.clone().as_ref().map(|ns| ns.clone())

Why the double clone? Isn't self.namespaces.default.clone() enough (It's an Option<Namespace>?

@@ +474,4 @@
>      }
>  
>      fn namespace_for_prefix(&self, prefix: &Atom) -> Option<Namespace> {
> +        self.namespaces.prefixes.get(prefix).map(|ns| ns.clone())

nit: cloned() instead of map(|ns| ns.clone()) is a bit nicer.

Comment 22

a year ago
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2189673fb8ea
Add test for case insensitivity for content: attr() identifiers in html documents. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/447ff1af0d59
Add an atom overload for namespace registration. r=bz
https://hg.mozilla.org/integration/autoland/rev/56564b7c2fe7
Add a ToLowerCaseASCII utility method for nsAtom. r=hisvonen
https://hg.mozilla.org/integration/autoland/rev/9b93666a3de9
Stop using Gecko namespace ids in servo. r=xidorn
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10367 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.