Renaming methods to prefix them with add make it clear there is a modification of the instance

[mozfreddyb]

I am filing this on behalf of @annevk who was making this point in an earlier meeting:

The suggestion was to rename methods to addElement, addRemoveElement, addReplaceWithChildrenElement, etc. (prefix with add to make it clear there is a modification of the instance)

I suggest we do this independent of #237, so I'm splitting this out.

I'm also asking everyone to wait for @annevk to describe this further so we are not arguing against a strawman.

[annevk]

Proposals:

A

• element()
• removeElement()
• comment()

---

B

• addElement()
• addRemoveElement()
• setComment()

---

C

• appendElement()
• appendRemoveElement()
• setComment()

---

D

• addToElements()
• addToRemoveElements()
• setComment()

---

E

• allowElement()
• removeElement()
• replaceWithChildrenElement()
• setComment()

(I don't like E as it's not internally consistent.)

[benbucksch]

I'm assuming this issue is referring to this part of the spec:

dictionary SanitizerConfig {
  sequence<SanitizerElementWithAttributes> elements;
  sequence<SanitizerElement> removeElements;
  sequence<SanitizerElement> replaceWithChildrenElements;

  sequence<SanitizerAttribute> attributes;
  sequence<SanitizerAttribute> removeAttributes;

  boolean comments;
  boolean dataAttributes;
};

However, that also doesn't explain what these functions actually do. Could there please be some JavaDoc-style comment in human-readable English (no pseudo-code)? What does this actually do?

1. Does it add or remove a DOM element, immediately, once? If so, then:

• addElement()
• removeElement()

2. Does it modify the list of DOM elements to filter? It applies not immediately, but whenever somebody adds DOM elements in whatever way? If so, then:

• addElementFilter()
• removeElementFilter()

Given that the interface is called SanitizerConfig, I assume 2. is meant.

[benbucksch]

Or, for 2., even better (similar to E):

• allowElement()
• denyElement()

It's completely unclear to me what replaceWithChildrenElement() is supposed to do, esp. the Children part. If you mean to clear the filter list and start over from scratch, then maybe allowOnlyElements() (plural) would make sense to me. But I'd also expect a clear() function.

[annevk]

No, this is in relation to #237.

[benbucksch]

OK. From what I understand from #237, this is about modifying the filter lists (allow lists and deny lists), right? If so:

My suggestion would be:

• allowElement(element)
• denyElement(element)

And for clearing maybe, based on discussion in #237:

• resetToDefaults() (deletes all config entries, and sets default safe filters)
• allowAllUnsafe() (deletes all config entries, and allows everything, including unsafe HTML)
• allowOnlyElements(element[]) (deletes all config entries, sets default safe filters, adds a default-deny filter, and adds all the given elements as allowElement())

[mozfreddyb]

What is the internal consistency issue in (E)?

I would prefer (A) and (E), as I find terminologies like addRemove or appendRemove quite counter-intuitive.

[otherdaniel]

For me: (A) + (E) are preferred; then (D), then (B) and (C). I'm happy to go with the majority, though.

What is the internal consistency issue in (E)?

The dictionary has elements and removeElements (rather than allowElements. The allow forms drop the verb.); while (E) has verbs everywhere. If we do settle for (E), I think we should resolve this by making the dictionary say allowElements, too.

[annevk]

I think the dictionary experience would be worse with allowElements. And I also think the dictionary experience is more important.

My preference order: B, C, D, A, E.

Arguably Fetch's Body mixin is precedence for the naming in A, but the methods on Body don't (really) manipulate. They return data. I think it's quite confusing to have methods whose primary purpose is manipulation to not indicate that in any way.

Looking at E, while say replaceWithChildrenElement has a verb in it, it doesn't actually describe what it does. It manipulates the internal replaceWithChildrenElements list (plus possibly some adjacent bookkeeping), but that is not what that name indicates.