Closed Bug 1370802 Opened 7 years ago Closed 7 years ago

stylo: consider storing xml:lang="" and lang="" attributes as atoms

Categories

(Core :: CSS Parsing and Computation, enhancement, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: heycam, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Bug 1370802: Fix style issues in GenericSpecifiedValue subclasses.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1370802: Parse lang attributes as atoms.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1370802: Clean up copy-pasta in GenericSpecifiedValues code.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1370802: Cleanup style and indentation in GenericSpecifiedValues.h and related files.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1370802: Make the lang rule operate on atoms, not strings.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1370802: There's actually no need for lang to be lowercased.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
The patches in bug 1365162 will always atomize the values of xml:lang="" and lang="" that we get from LangValue() in ServoBindings.cpp.  (It's also done in the existing Gecko_GetXMLLangValue.)  It might be worth just always storing these as atoms so that we don't have to repeatedly atomize when returning them here.

Although LangValue() should only be called when handling changes to lang="" values, as :lang() pseudo-class matching when there is no lang="" change involved is done all on the C++ side, so there's no passing of lang values back to rust.
The simplest thing is probably to parse all attrs with localname "lang", regardless of namespace, as atoms in Element::ParseAttribute.

We'll need to fix MapLangAttributeInto over in nsGenericHTMLElement.cpp, but I think everything else will just work.
Blocks: stylo-perf
Priority: -- → P4
See Also: → 1373357
I'll give this a shot, should be easy and bug 1373357 indicates that matching :lang a little faster could help with all the revalidation stuff.
Assignee: nobody → emilio+bugs
Comment on attachment 8878759 [details]
Bug 1370802: Fix style issues in GenericSpecifiedValue subclasses.

https://reviewboard.mozilla.org/r/150034/#review154816
Attachment #8878759 - Flags: review?(cam) → review+
Comment on attachment 8878760 [details]
Bug 1370802: Parse lang attributes as atoms.

https://reviewboard.mozilla.org/r/150036/#review154818

::: dom/base/nsIContent.h:939
(Diff revision 1)
>     */
> +  nsIAtom* GetLang() const;
> +
>    bool GetLang(nsAString& aResult) const {
> -    for (const nsIContent* content = this; content; content = content->GetParent()) {
> -      if (content->GetAttrCount() > 0) {
> +    if (auto* lang = GetLang()) {
> +      aResult.Assign(lang->GetUTF16String(), lang->GetLength());

Maybe:

  aResult.Assign(nsDependentAtomString(lang));

::: dom/base/nsIContent.h
(Diff revision 1)
> -        }
> +    }
> -      }
> +
> -    }
>      return false;
>    }
> -

Nit: don't drop this blank line.

::: dom/html/nsGenericHTMLElement.cpp:1217
(Diff revision 1)
> -                                      langValue->GetStringValue());
> +
> +    const nsDependentAtomString atomString(atom);
> +    Maybe<nsCOMPtr<nsIAtom>> lowerAtom;
> +    if (nsContentUtils::StringContainsASCIIUpper(atomString)) {
> +      nsAutoString dest;
> +      dest.SetCapacity(atomString.Length());

Nit: this line isn't necessary, since ASCIIToLower will do a SetLength on the destination string anyway.  (Unlikely we'll have attribute values larger than 64 characters anyway.)
Attachment #8878760 - Flags: review?(cam) → review+
Comment on attachment 8878761 [details]
Bug 1370802: Clean up copy-pasta in GenericSpecifiedValues code.

https://reviewboard.mozilla.org/r/150038/#review154826
Attachment #8878761 - Flags: review?(cam) → review+
Comment on attachment 8878762 [details]
Bug 1370802: Cleanup style and indentation in GenericSpecifiedValues.h and related files.

https://reviewboard.mozilla.org/r/150040/#review154828

::: layout/style/nsRuleData.cpp:25
(Diff revision 1)
> -  static_assert(size_t(-1) > size_t(0),
> +  static_assert(size_t(-1) > size_t(0), "expect size_t to be unsigned");
> -                "expect size_t to be unsigned");

(I don't particularly mind here, but arguably the symmetry between the three static_asserts is more important for readability than keeping that third one on one line.)
Attachment #8878762 - Flags: review?(cam) → review+
Comment on attachment 8878763 [details]
Bug 1370802: Make the lang rule operate on atoms, not strings.

https://reviewboard.mozilla.org/r/150042/#review154832
Attachment #8878763 - Flags: review?(cam) → review+
Comment on attachment 8878764 [details]
Bug 1370802: There's actually no need for lang to be lowercased.

https://reviewboard.mozilla.org/r/150068/#review154838

Ah, yeah, because nsCSSRuleProcessor::LangPseudoMatches will already do an ASCII case-insensitive comparison.

::: layout/style/nsHTMLStyleSheet.cpp
(Diff revision 1)
> -  Maybe<nsCOMPtr<nsIAtom>> lowerAtom;
> -  nsDependentAtomString langString(aLanguage);
> -  if (nsContentUtils::StringContainsASCIIUpper(langString)) {
> -    nsString dest;
> -    nsContentUtils::ASCIIToLower(langString, dest);
> -    lowerAtom.emplace(NS_AtomizeMainThread(dest));
> -  }
> -
> -  const nsIAtom* key = lowerAtom ? *lowerAtom : aLanguage;
>    auto entry =
> -    static_cast<LangRuleTableEntry*>(mLangRuleTable.Add(key, fallible));
> +    static_cast<LangRuleTableEntry*>(mLangRuleTable.Add(aLanguage, fallible));

The lowercasing here does let us avoid having unique rules for lang="en-us" and lang="en-US", for example.  Probably not super important though.
Attachment #8878764 - Flags: review?(cam) → review+
Comment on attachment 8878760 [details]
Bug 1370802: Parse lang attributes as atoms.

https://reviewboard.mozilla.org/r/150036/#review154818

> Nit: this line isn't necessary, since ASCIIToLower will do a SetLength on the destination string anyway.  (Unlikely we'll have attribute values larger than 64 characters anyway.)

I didn't address this one because it goes away in the last patch.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1878879c4c1c
Fix style issues in GenericSpecifiedValue subclasses. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4e324e5d9965
Parse lang attributes as atoms. r=heycam
https://hg.mozilla.org/integration/autoland/rev/dd774d1c3569
Clean up copy-pasta in GenericSpecifiedValues code. r=heycam
https://hg.mozilla.org/integration/autoland/rev/fd9e6e55b4f4
Cleanup style and indentation in GenericSpecifiedValues.h and related files. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c1f31a96126f
Make the lang rule operate on atoms, not strings. r=heycam
https://hg.mozilla.org/integration/autoland/rev/499a91e7887f
There's actually no need for lang to be lowercased. r=heycam
Blocks: 1373357
Depends on: 1394311
Depends on: 1413111
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: