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

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


Some nits.

> Source/WebCore/dom/Node.cpp:2708
> +Node::MutationRegistrationResult
Node::registerMutationObserver(PassRefPtr<WebKitMutationObserver> observer,
unsigned char options)

We should probably typedef unsigned char.

> Source/WebCore/dom/Node.cpp:2723
> +void Node::deregisterMutationObserver(PassRefPtr<WebKitMutationObserver>
observer)

Nit: s/deregister/unregister/

> Source/WebCore/dom/NodeRareData.h:93
> +struct MutationObserverRegistration {
> +    MutationObserverRegistration(PassRefPtr<WebKitMutationObserver>
observer, unsigned char options)

Nit: I'd prefer calling an entry so MutationObserverEntry.

> Source/WebCore/dom/NodeRareData.h:105
> +    RefPtr<WebKitMutationObserver> observer;
> +    unsigned char options;

It'll be nice if these members were private and this class instead provided
methods like matches(WebKitMutationObserver:: MutationType).

> Source/WebCore/dom/NodeRareData.h:108
> +typedef Vector<MutationObserverRegistration>
MutationObserverRegistrationVector;

I'm not sure typedef-ing this vector is that helpful. It only saves to
characters.

> Source/WebCore/dom/NodeRareData.h:109
> +struct MutationObserverData {

I don't think MutationObserverData is a descriptive name. I'd call it
MutationObserverRegistry or MutationObserverList.

> Source/WebCore/dom/NodeRareData.h:112
> +public:

No need. The default visibility for struct is public.

> Source/WebCore/dom/NodeRareData.h:114
> +    ~MutationObserverData() { }

No need. Compiler-generated d'tor should do the work.

> Source/WebCore/dom/NodeRareData.h:197
> +    OwnPtr<MutationObserverData> m_mutationObserverData;

I'd declare OwnPtr to Vector<MutationObserverRegistration> if I were you as in:

OwnPtr<Vector<MutationObserverRegistration> > m_mutationObservers;

> Source/WebCore/dom/WebKitMutationObserver.cpp:78
> +    for (Vector<Node*>::iterator iter = m_registrations.begin(); iter != 
m_registrations.end(); ++iter)

Since it's a vector, you can iterate through them using a size_t index.

> Source/WebCore/dom/WebKitMutationObserver.cpp:84
> +void WebKitMutationObserver::wasDeregistered(Node* node)

Nit: unregistered.

> Source/WebCore/dom/WebKitMutationObserver.cpp:113
> +	   MutationObserverSet::iterator iter =
activeMutationObservers().begin();
> +	   RefPtr<WebKitMutationObserver> observer = *iter;
> +	   activeMutationObservers().remove(iter);

ListHashSet has removeLast() and last(). We should use that here.

> Source/WebCore/dom/WebKitMutationObserver.cpp:118
> +void WebKitMutationObserver::deliver()

I'd put this function above deliverAllMutations since enqueueMutationRecord and
deliver are both non-static.

> Source/WebCore/dom/WebKitMutationObserver.cpp:122
> +    m_callback->handleEvent(&records, this);

Really? We call it handleEvent? That's awfully misleading. We should use some
other name for this method.