Bug 223675 – Make MutationObserver's flags into enum class

• Component: DOM
• Status: RESOLVED DUPLICATE

[Tetsuharu Ohzeki [UTC+9]]

...

[Tetsuharu Ohzeki [UTC+9]]

*** Bug 223676 has been marked as a duplicate of this bug. ***

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 424122 [details]
Patch

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 424123 [details]
Patch

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 424148 [details]
Patch

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 424162 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/76040447>

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 425976 [details]
Patch

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 426014 [details]
Patch

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 426015 [details]
Patch

[Darin Adler]

Comment on attachment 426015 [details]
Patch

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

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

I think MutationObserverOption would be a fine name. Don’t need to name values in an OptionSet as "options flag".

[Tetsuharu Ohzeki [UTC+9]]

Thank you, Darin!

I'll fix to that.

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 427497 [details]
Patch

[Darin Adler]

Comment on attachment 427497 [details]
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.

[Tetsuharu Ohzeki [UTC+9]]

Darin, Thank you for your review.
And I'm sorry to late this reply.


(In reply to Darin Adler from comment #13)
> Comment on attachment 427497 [details]
> 
> > 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?

By receiving your review, I had thought to use phantom type to make them different type
but I think these type should be single `MutationObserverOptions`.

Previously, both `MutationObserverOptions` and `MutationRecordDeliveryOptions` are `unsigned char`,
and we operate `MutationObserverOptions & MutationRecordDeliveryOptions` in some place.

So I feel there is no reason to differentiate these types by these reasons,
and it would be useful to use assertion to check whether the object contains some delivery flags. 

How do you think about this?


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


This `m_oldValueFlag` always contains and is passed only single flags, not a bit flags sets, and the default value is 0.
So I guess that if `m_oldValueFlag === 0, then `hasOldValue(options)` return false always. So my patch would work.

I think this `m_oldValueFlag` should be also typed as OptionSet<MutationObserverOption>.
I'll change to that.

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 429797 [details]
Patch

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 433258 [details]
Patch

[Tetsuharu Ohzeki [UTC+9]]

Same thing has been fixed in bug 231489

*** This bug has been marked as a duplicate of bug 231489 ***