[webkit-reviews] review denied: [Bug 70137] [MutationObservers] Modifications to the style property don't dispatch "attributes" Mutation Records : [Attachment 116383] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Rafael Weinstein
<rafaelw at chromium.org>'s request for review:
Bug 70137: [MutationObservers] Modifications to the style property don't
dispatch "attributes" Mutation Records
https://bugs.webkit.org/show_bug.cgi?id=70137

Attachment 116383: Patch
https://bugs.webkit.org/attachment.cgi?id=116383&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116383&action=review


It's great that this patch is small :) Just need some polishing.

> Source/WebCore/ChangeLog:9
> +	   the RAIA pattern similar to the public ChildListMutationScope). This
manages the (sometimes conditional)

s/RAIA/RAII/

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:47
> +namespace {

I really don't like these anonymous namespaces.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:49
> +class AttributesMutationScope {

This should be named StyleAttributeMutationScope unless you're planning to use
it for other attribute changes as well.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:68
> +	   AtomicString styleAtom("style");
> +	   s_mutationRecipients =
MutationObserverInterestGroup::createForAttributesMutation(s_currentDecl->node(
), styleAtom);

Why doesn't createForAttributesMutation just take qualified name? If we can't
make that change, we should still be able to pass styleAttr.localName().

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:74
> +	   Element* element = static_cast<Element*>(s_currentDecl->node());

We don't have toElement? I swear I've used it before...

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:76
> +	   s_mutation = MutationRecord::createAttributes(element,
QualifiedName(nullAtom, styleAtom, nullAtom), oldValue);

Why are we creating a qualified name here? We should just use styleAttr. r-
because of this.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:102
> +unsigned AttributesMutationScope::s_scopeCount = 0;

This is an integral type so we should be able to initialize it in the class
declaration, no?