I think the spec here doesn't really make sense.

I thought I had raised this as an issue at some point in the repo, but I can't find it anymore so I probably didn't.

I should probably raise a new issue for this?

I think it's fine, but I will change it for you.

Arg, you got me. I really need to find a way to remove that footgun. Maybe we could disallow the bool operator for Maybe<bool>?

Do you mean two overloads, one that takes nsAtom/namespaceid and one that takes CanonicalName? Creating a CanonicalName from nsAtom/namespaceid would be a bit inefficient and I actually tried quite a bit to avoid it in the IsDefaultConfig case.

Note to self, update this.

Sanitizer::CanonicalizeConfiguration ensures that mDataAttributes always exists when mAttributes exists. dataAttributes is basically an "extension" of attributes.

For the default config we also always initialize mDataAttributes, because it conceptually has "attributes". (See Sanitizer::SetDefaultConfig)

I had opened bug 1989215 to look into an ordered hash set (and maybe a map).

Good point. Maybe it would be useful to make the type hierarchy better here instead? e.g. have CanonicalElement extend CanonicalName, CanonicalAttribute extend CanonicalName, and CanonicalElementWithAttributes extend CanonicalElement.

And I tried to keep mLocalName and mNamespace as protected.

How do we know mDataAttributes isn't Nothing() here? Since if it is, *mDataAttributes has a failing assertion.

Also this, why is this ok?

...since here we anyhow check that the value isSome

Why do we need MaybeMaterializeDefaultConfig here when the spec doesn't have such?
It is not enough to call it in constructor or something?

Step 2 in the spec is "If configuration["elements"] exists:"

Why is step 3 before step 4 ?

Er, why isn't step 3 before step 4 :)

Why is this here? Makes it hard to follow the algorithm when it doesn't follow the spec.

Why is this check ok if mComments is Nothing()?

As as I said above, this a property of the Sanitizer. We are in the config[attributes] branch, so config[dataAttributes] must exist.

https://wicg.github.io/sanitizer-api/#configuration-canonicalize

If configuration["attributes"] exists and configuration["dataAttributes"] does not exist, then set configuration["dataAttributes"] to allowCommentsAndDataAttributes.

Same argument. Part of the canonical form.

This is the config[removeAttributes] branch, which is not supposed to have config[dataAttributes]. (So it's not added by the canonicalization)

The default config is not real or "materialized", it's basically constant data. If someone wants to modify a Sanitizer with the default config, we have to actually create the lists that can be modified.

You are looking at a revision that is newer than this patch.

https://pr-preview.s3.amazonaws.com/evilpie/sanitizer-api/pull/307.html#sanitizerconfig-allow-an-element should be the state of the patch.

To make the std::move work. The order is unobservable.

This is again to make std::move work. It might actually be possible to just switch the order in the spec. (The difference here is obviously not observable)

mComments is always initialized. I could probably just change it to a boolean to be honest.

I am going to re-order the steps in the spec: https://github.com/WICG/sanitizer-api/pull/314.

The main reason for having a lazy default config is performance and memory usage. The code in SanitizeChildren is templated for the default config case and can omit some of the complexity. When creating a Sanitizer with a default config after the first time we don't need to keep creating the huge lists with elements and attributes. This sharing of the config is both faster and more memory efficient.