[webkit-reviews] review denied: [Bug 223675] Make MutationObserver's flags into enum class : [Attachment 427497] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun May 2 17:51:54 PDT 2021


Darin Adler <darin at apple.com> has denied Tetsuharu Ohzeki [UTC+9]
<tetsuharu.ohzeki at gmail.com>'s request for review:
Bug 223675: Make MutationObserver's flags into enum class
https://bugs.webkit.org/show_bug.cgi?id=223675

Attachment 427497: Patch

https://bugs.webkit.org/attachment.cgi?id=427497&action=review




--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 427497
  --> https://bugs.webkit.org/attachment.cgi?id=427497
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427497&action=review

> Source/WebCore/dom/MutationObserver.cpp:74
> +    constexpr MutationObserverOptions validFlagPatterns = {

I don’t think "valid flag patterns" is quite the right name for this set of
options. What’s required for validity is that at least *one* of these three
options is set. It’s also valid to have more than one. So these aren’t three
distinct flag patterns.

I think the correct name for this set might be something more like
"requiredOptions". I am having trouble finding the right words, but those seem
better than "valid flag patterns".

> Source/WebCore/dom/MutationObserver.cpp:106
> +    constexpr MutationObserverOptions shouldObserveAttribute = {

Maybe the name for this set is optionsRequringAttributeObserver?

> Source/WebCore/dom/MutationObserver.h:52
> +enum class MutationObserverOption: uint8_t {

WebKit coding style puts a space before the ":".

> Source/WebCore/dom/MutationObserver.h:68
> +using MutationObserverOptions = OptionSet<MutationObserverOption>;
> +using MutationRecordDeliveryOptions = OptionSet<MutationObserverOption>;

This is messy, although no worse than before. Two different type names, but no
enforcement that we won’t use the wrong options in the wrong type. Sort of
"fake types". Not thrilled with how this turned out. Can we use inheritance or
something to avoid this?

> Source/WebCore/dom/MutationObserver.h:74
> +public:
>  
>      static Ref<MutationObserver> create(Ref<MutationCallback>&&);

No reason to include this blank line.

> Source/WebCore/dom/MutationObserverInterestGroup.cpp:92
> +    if (!m_oldValueFlag)
> +	   return options.isEmpty();
> +
> +    auto oldValueFlag = m_oldValueFlag.value();
> +    return options.contains(oldValueFlag);

Better to not use a local variable. In fact I would write the whole function
like this:

    return m_oldValueFlag ? options.contains(*oldValueFlag) :
options.isEmpty();

But I do not think this is the correct semantic. The old code implemented this:

    return m_oldValueFlag && options.contains(*oldValueFlag);

That’s not the same rule!

I’m going to say review- because of this change. And I wonder why the tests
can’t tell we got this wrong.

Also unclear to me why we aren‘t inlining this any more. I think this code
should go in the header.

> Source/WebCore/dom/MutationObserverRegistration.h:75
> +    MutationRecordDeliveryOptions deliveryOptions() const
> +    {
> +	   constexpr MutationObserverOptions hasDeliveryFlags = {
> +	       MutationObserverOption::AttributeOldValue,
> +	       MutationObserverOption::CharacterDataOldValue,
> +	   };
> +	   return m_options & hasDeliveryFlags;
> +    }
> +    MutationObserverOptions mutationTypes() const
> +    {
> +	   constexpr MutationObserverOptions allMutationTypes = {
> +	       MutationObserverOption::ChildList,
> +	       MutationObserverOption::Attributes,
> +	       MutationObserverOption::CharacterData,
> +	   };
> +	   return m_options & allMutationTypes;
> +    }

When function bodies get longer like this, I prefer that the inline function
definitions be outside the class definition, right after it. Inside the class
definition we can just declare the functions without defining them.


More information about the webkit-reviews mailing list