[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

--- Comment #14 from Tetsuharu Ohzeki [UTC+9] <tetsuharu.ohzeki at gmail.com> ---
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.

-- 
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/20210526/10d6b90b/attachment.htm>