[Webkit-unassigned] [Bug 223675] Make MutationObserver's flags into enum class

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=223675

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #427497|review?                     |review-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210503/1c1fee9b/attachment-0001.htm>