Bug 70137

It looks to me like adding a check in CSSMutableStyleDeclaration::setNeedsStyleRecalc which asks whether there are any observers interested in the style attribute is the way to do this. If any observer cares about the value, then we synchronously update the style attribute, otherwise we can just dispatch a record inline and leave the style attribute invalid. Does this seem like a reasonable approach?

Created attachment 116383 [details] Patch

Patch ready for review. Note that this patch takes a slightly different patch that the previous comment suggests. In particular, detecting that observers exist during setNeedsStyleRecalc will be too late if any observer requests oldValue, because at that point the change has already taken place and the last value may not have been serialized to the attribute.

Comment on attachment 116383 [details] Patch 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?

Comment on attachment 116383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116383&action=review >> Source/WebCore/ChangeLog:9 >> + the RAIA pattern similar to the public ChildListMutationScope). This manages the (sometimes conditional) > > s/RAIA/RAII/ done >> 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. done >> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:68 >> + 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(). good call. done. >> 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... done. >> 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. done. >> 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? That's what I thought as well. As adam explained to me, static declarations don't actually reserve storage, so nothing can be initialized. It has to be done as below.

Comment on attachment 116383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116383&action=review >>> 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? > > That's what I thought as well. As adam explained to me, static declarations don't actually reserve storage, so nothing can be initialized. It has to be done as below. To be precise, the issue here is the non-constness. "static const" integral types are allowed to be initialized inline (and don't even necessarily need storage), but non-const integrals both need storage and can't be initialized inline.

Created attachment 116393 [details] Patch

Comment on attachment 116393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116393&action=review > Source/WebCore/dom/WebKitMutationObserver.cpp:157 > -PassOwnPtr<MutationObserverInterestGroup> MutationObserverInterestGroup::createForAttributesMutation(Node* target, const AtomicString& attributeName) > +PassOwnPtr<MutationObserverInterestGroup> MutationObserverInterestGroup::createForAttributesMutation(Node* target, const QualifiedName& attributeName) > { > - return adoptPtr(new MutationObserverInterestGroup(target, WebKitMutationObserver::Attributes, attributeName)); > + return adoptPtr(new MutationObserverInterestGroup(target, WebKitMutationObserver::Attributes, attributeName.localName())); Nice!

Created attachment 116397 [details] Patch

Comment on attachment 116397 [details] Patch Clearing flags on attachment: 116397 Committed r101101: <http://trac.webkit.org/changeset/101101>

All reviewed patches have been landed. Closing bug.