[webkit-reviews] review denied: [Bug 68956] [MutationObservers] Implement WebKitMutationObserver.observe for attributes : [Attachment 110906] 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 68956: [MutationObservers] Implement WebKitMutationObserver.observe for
attributes
https://bugs.webkit.org/show_bug.cgi?id=68956

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

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


I'd be interested in seeing a test case where attribute is added by HTML5
parser.

> LayoutTests/fast/mutation/observe-attributes-expected.txt:11
> +PASS mutations.length is 1
> +PASS mutations[0].type is "attributes"
> +PASS mutations[0].attributeName is "foo"
> +PASS mutations[0].attributeNamespace is null
> +PASS mutations is null
> +PASS mutations.length is 2

These results aren't informative. They don't tell us why we expect these
results. You probably need to use debug or include some function calls inside
the first argument of shouldBe to print out some description on what happened
before checking these values.

> LayoutTests/fast/mutation/observe-attributes.html:16
> +var mutations;
> +var mutations2;

Do they really need to be in the global scope?

> LayoutTests/fast/mutation/observe-attributes.html:31
> +	   observer.observe(div, { attributes: true, characterData: true });
> +	   div.setAttribute('foo', 'bar');

It'll be nice to print something here by debug so that we can see what the test
is doing in the expected result.

> LayoutTests/fast/mutation/observe-attributes.html:237
> +    var div, div2, subDiv;
> +    var observer;
> +    var listener;
> +    var hit; 

It seems like only variables that need to be accessed finish are div and
listener.
The rest should be declared inside start.

> Source/WebCore/ChangeLog:9
> +	   on nodes, delivering mutation records at the end of the outer-most

Seems like we can fit "on nodes" on the previous line to avoid awkward line
wrapping?

> Source/WebCore/dom/Element.cpp:627
> +    OwnPtr<ObserverPtrVector> observers =
element->interestedMutationObservers(WebKitMutationObserver::Attributes);
> +    if (observers) {

Please declare observers inside the if statement as in:
if (OwnPtr<...> observers = ...)
or early exit as in:
if (!observers)
    return;

> Source/WebCore/dom/Element.cpp:657
> +    // The call to attributeChanged below may dispatch DOMSubtreeModified,
so it's important to enqueue a MutationRecord now.
> +    enqueueAttributesMutationRecord(this, attributeName);

Are you certain that new attribute value is always set? I bet there are cases
where setting attribute value is ignored.

> Source/WebCore/dom/Node.cpp:558
> +    MutationObserverData* d = mutationObserverData();

Please don't use an abbreviation like d.

> Source/WebCore/dom/Node.cpp:559
> +    if (d) {

Please declare the variable inside if.

> Source/WebCore/dom/Node.cpp:2669
> +PassOwnPtr<ObserverPtrVector>
Node::interestedMutationObservers(WebKitMutationObserver::Type type)

I'm not sure if "interested" MutationObservers is a particularly good name. I
personally prefer filterMutationObservers if any.

> Source/WebCore/dom/Node.cpp:2671
> +    MutationObserverData* d = mutationObserverData();

Please don't use abbreviations like d.

> Source/WebCore/dom/Node.cpp:2675
> +    OwnPtr<ObserverPtrVector> interestedObservers = adoptPtr(new
Vector<WebKitMutationObserver*>);

More conventional approach is for this function to get a mutable reference of a
Vector and fill that up. r- because of this conventional interface.

> Source/WebCore/dom/Node.cpp:2686
> +    MutationObserverData* d = ensureMutationObserverData();

Ditto about one letter variable name.

> Source/WebCore/dom/Node.cpp:2701
> +    MutationObserverData* d = mutationObserverData();

Ditto.

> Source/WebCore/dom/Node.h:97
> +#if ENABLE(MUTATION_OBSERVERS)
> +typedef Vector<WebKitMutationObserver*> ObserverPtrVector;
> +#endif

Please make the suggested interface change above so that we can get rid of this
typedef.

> Source/WebCore/dom/Node.h:604
> +    // Returns true if the observer wasn't already registered on this node.
> +    bool registerMutationObserver(PassRefPtr<WebKitMutationObserver>,
uint8_t options);

Instead of adding a comment, can we use an enum e.g. ObserverRegistrationStatus
{ ObserverIsNewlyRegistered, ObserverWasAlreadyRegistered } ?

> Source/WebCore/dom/NodeRareData.h:82
> +    MutationObserverRegistration(PassRefPtr<WebKitMutationObserver>
observer, uint8_t options)

I don't think we normally use uint8_t. Please use unsigned or unsigned char
instead. You can even typedef something if you'd like.

> Source/WebCore/dom/NodeRareData.h:83
> +	   : observer(observer),

, should be on the next line.

> Source/WebCore/dom/NodeRareData.h:84
> +	     options(options) { }

I think { } needs to be on a separate line.

> Source/WebCore/dom/NodeRareData.h:92
> +    uint8_t options;

Ditto about uint8_t.

> Source/WebCore/dom/WebKitMutationObserver.h:50
> +    enum Type {

Type sounds too generic. How about MutationType?

> Source/WebCore/dom/WebKitMutationObserver.h:51
> +	   ChildList = (1 << 0),

We don't normally wrap these values by parenthesis.

> Source/WebCore/dom/WebKitMutationObserver.h:56
> +    enum Flags {

Flags sounds too generic too. How about MutationObservationFilter?