Bug 68956 - [MutationObservers] Implement WebKitMutationObserver.observe for attributes
Summary: [MutationObservers] Implement WebKitMutationObserver.observe for attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on:
Blocks: 68729 68955 68957
  Show dependency treegraph
 
Reported: 2011-09-27 16:36 PDT by Adam Klein
Modified: 2011-10-17 16:57 PDT (History)
11 users (show)

See Also:


Attachments
Patch (23.88 KB, patch)
2011-10-13 10:12 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (32.40 KB, patch)
2011-10-13 14:33 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (34.41 KB, patch)
2011-10-13 20:33 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (34.27 KB, patch)
2011-10-14 12:21 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (34.36 KB, patch)
2011-10-17 12:26 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (35.71 KB, patch)
2011-10-17 14:57 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (35.60 KB, patch)
2011-10-17 15:22 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-09-27 16:36:10 PDT
[MutationObservers] Implement WebKitMutationObserver.observe for attributes
Comment 1 Adam Klein 2011-10-07 14:51:47 PDT
As discussed, this patch is expected to include registration of observers and delivery at end of outermost script exit, so as to expose the new functionality to layout tests.
Comment 2 Rafael Weinstein 2011-10-13 10:12:28 PDT
Created attachment 110865 [details]
Patch
Comment 3 Rafael Weinstein 2011-10-13 10:14:02 PDT
Comment on attachment 110865 [details]
Patch

Initial work -- Not yet ready for review.
Comment 4 Rafael Weinstein 2011-10-13 14:33:57 PDT
Created attachment 110906 [details]
Patch
Comment 5 Rafael Weinstein 2011-10-13 14:35:43 PDT
Now ready for review. This is my first webkit feature work (earlier patchers were chromium plumbing) -- please do *NOT* be gentle =-).
Comment 6 Ryosuke Niwa 2011-10-13 15:02:43 PDT
Comment on attachment 110906 [details]
Patch

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?
Comment 7 Rafael Weinstein 2011-10-13 20:01:41 PDT
Comment on attachment 110906 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110906&action=review

>> LayoutTests/fast/mutation/observe-attributes.html:16
>> +var mutations2;
> 
> Do they really need to be in the global scope?

Yes. otherwise they are not visible to shouldBe() which evals in the global scope.

>> LayoutTests/fast/mutation/observe-attributes.html:31
>> +        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.

Debug output added

>> LayoutTests/fast/mutation/observe-attributes.html:237
>> +    var hit; 
> 
> It seems like only variables that need to be accessed finish are div and listener.
> The rest should be declared inside start.

hit removed. Only div2 & subDiv aren't used in finish() for cleanup. These are all scope inside this subtest. Doesn't seem to buy very much to move them into 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?

done.

>> Source/WebCore/dom/Element.cpp:627
>> +    if (observers) {
> 
> Please declare observers inside the if statement as in:
> if (OwnPtr<...> observers = ...)
> or early exit as in:
> if (!observers)
>     return;

Done. now exiting early.

>> Source/WebCore/dom/Element.cpp:657
>> +    enqueueAttributesMutationRecord(this, attributeName);
> 
> Are you certain that new attribute value is always set? I bet there are cases where setting attribute value is ignored.

I don't see how that can happen. If someone sees something I'm missing, please point it out.

>> Source/WebCore/dom/Node.cpp:558
>> +    MutationObserverData* d = mutationObserverData();
> 
> Please don't use an abbreviation like d.

done.

>> Source/WebCore/dom/Node.cpp:559
>> +    if (d) {
> 
> Please declare the variable inside if.

done

>> 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.

The point here is to retrieve the observers that *will* need to receive the given mutation type. I've tried a new name. Let me know if this works for you.

>> Source/WebCore/dom/Node.cpp:2671
>> +    MutationObserverData* d = mutationObserverData();
> 
> Please don't use abbreviations like d.

Done.

>> 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.

Done

>> Source/WebCore/dom/Node.cpp:2686
>> +    MutationObserverData* d = ensureMutationObserverData();
> 
> Ditto about one letter variable name.

Done.

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

Done

>> Source/WebCore/dom/Node.h:97
>> +#endif
> 
> Please make the suggested interface change above so that we can get rid of this typedef.

done

>> Source/WebCore/dom/Node.h:604
>> +    bool registerMutationObserver(PassRefPtr<WebKitMutationObserver>, uint8_t options);
> 
> Instead of adding a comment, can we use an enum e.g. ObserverRegistrationStatus { ObserverIsNewlyRegistered, ObserverWasAlreadyRegistered } ?

Done.

>> 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.

Done

>> Source/WebCore/dom/NodeRareData.h:83
>> +        : observer(observer),
> 
> , should be on the next line.

done

>> Source/WebCore/dom/NodeRareData.h:84
>> +          options(options) { }
> 
> I think { } needs to be on a separate line.

done

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

done.

>> Source/WebCore/dom/WebKitMutationObserver.h:50
>> +    enum Type {
> 
> Type sounds too generic. How about MutationType?

done

>> Source/WebCore/dom/WebKitMutationObserver.h:56
>> +    enum Flags {
> 
> Flags sounds too generic too. How about MutationObservationFilter?

These aren't filter values. Using more descriptive name.
Comment 8 Rafael Weinstein 2011-10-13 20:33:35 PDT
Created attachment 110952 [details]
Patch
Comment 9 Adam Klein 2011-10-14 11:50:02 PDT
Comment on attachment 110952 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110952&action=review

> Source/WebCore/ChangeLog:11
> +        delivering mutation records at the end of the outer-most script invokation and

Please add a note that the outermost script invokation stuff only works in V8.

> Source/WebCore/bindings/v8/V8Proxy.cpp:69
> +#if ENABLE(MUTATION_OBSERVERS)

This guard isn't necessary, as the include file will be empty of the flag isn't enabled.

> Source/WebCore/dom/Element.cpp:52
> +#if ENABLE(MUTATION_OBSERVERS)

Another unnecessary guard.

> Source/WebCore/dom/Node.h:35
> +#if ENABLE(MUTATION_OBSERVERS)

Another unnecessary guard.

> Source/WebCore/dom/Node.h:606
> +

nit: Remove one line of whitespace here

> Source/WebCore/dom/NodeRareData.h:31
> +#if ENABLE(MUTATION_OBSERVERS)

Another unnecessary guard.

> Source/WebCore/dom/WebKitMutationObserver.cpp:61
> +    uint8_t optionFlags = 0;

unsigned char
Comment 10 Adam Klein 2011-10-14 12:02:01 PDT
Comment on attachment 110952 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110952&action=review

> Source/WebCore/dom/Node.h:67
> +#if ENABLE(MUTATION_OBSERVERS)

This guard may not be worth the noise either.
Comment 11 Ryosuke Niwa 2011-10-14 12:13:07 PDT
Sam, Maciej, Darin, do you have any comments on this patch? If not, I'm going to r+ it by this coming Monday.
Comment 12 Rafael Weinstein 2011-10-14 12:21:13 PDT
Created attachment 111047 [details]
Patch
Comment 13 Rafael Weinstein 2011-10-14 13:18:35 PDT
Sorry -- learning the tools. Apparently I need to reply to review comments before uploading the new patch.

Adam, the new patch addresses all of your comments (i.e. ALL DONE).
Comment 14 Ryosuke Niwa 2011-10-14 13:20:20 PDT
(In reply to comment #13)
> Sorry -- learning the tools. Apparently I need to reply to review comments before uploading the new 
patch.

Comments are still visible on the old patch. You can reply there instead :)
Comment 15 Rafael Weinstein 2011-10-14 13:24:32 PDT
Comment on attachment 110952 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110952&action=review

>> Source/WebCore/ChangeLog:11
>> +        delivering mutation records at the end of the outer-most script invokation and
> 
> Please add a note that the outermost script invokation stuff only works in V8.

done

>> Source/WebCore/bindings/v8/V8Proxy.cpp:69
>> +#if ENABLE(MUTATION_OBSERVERS)
> 
> This guard isn't necessary, as the include file will be empty of the flag isn't enabled.

done

>> Source/WebCore/dom/Element.cpp:52
>> +#if ENABLE(MUTATION_OBSERVERS)
> 
> Another unnecessary guard.

done

>> Source/WebCore/dom/Node.h:35
>> +#if ENABLE(MUTATION_OBSERVERS)
> 
> Another unnecessary guard.

done

>> Source/WebCore/dom/Node.h:67
>> +#if ENABLE(MUTATION_OBSERVERS)
> 
> This guard may not be worth the noise either.

done

>> Source/WebCore/dom/Node.h:606
>> +
> 
> nit: Remove one line of whitespace here

done

>> Source/WebCore/dom/NodeRareData.h:31
>> +#if ENABLE(MUTATION_OBSERVERS)
> 
> Another unnecessary guard.

done

>> Source/WebCore/dom/WebKitMutationObserver.cpp:61
>> +    uint8_t optionFlags = 0;
> 
> unsigned char

done
Comment 16 Rafael Weinstein 2011-10-17 12:26:42 PDT
Created attachment 111301 [details]
Patch
Comment 17 WebKit Review Bot 2011-10-17 12:28:31 PDT
Attachment 111301 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒0﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒1﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒2﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Adam Klein 2011-10-17 13:29:06 PDT
Comment on attachment 111301 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111301&action=review

> Source/WebCore/dom/Element.cpp:64
> +#if ENABLE(MUTATION_OBSERVERS)

One more unnecessary #if

> Source/WebCore/dom/Node.h:34
> +#include "WebKitMutationObserver.h"

Unfortunately, this will fail on Mac, I think.  WebKitMutationObserver.h needs to be made "Private" in WebCore.xcodeproj.  This may be possible without running Xcode by adding

"settings = {ATTRIBUTES = (Private, ); };"

to the first line for WebKitMutationObserver.h in WebCore.xcodeproj/project.pbxproj (around line 5469).  But safest is to actually do that from Xcode.  Too bad the EWS didn't try to build this patch.
Comment 19 Ryosuke Niwa 2011-10-17 13:45:48 PDT
Comment on attachment 111301 [details]
Patch

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.
Comment 20 Rafael Weinstein 2011-10-17 14:56:42 PDT
Comment on attachment 111301 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111301&action=review

>> Source/WebCore/dom/Element.cpp:64
>> +#if ENABLE(MUTATION_OBSERVERS)
> 
> One more unnecessary #if

done

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

coming in adam's pending patch.

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

done

>> Source/WebCore/dom/Node.h:34
>> +#include "WebKitMutationObserver.h"
> 
> Unfortunately, this will fail on Mac, I think.  WebKitMutationObserver.h needs to be made "Private" in WebCore.xcodeproj.  This may be possible without running Xcode by adding
> 
> "settings = {ATTRIBUTES = (Private, ); };"
> 
> to the first line for WebKitMutationObserver.h in WebCore.xcodeproj/project.pbxproj (around line 5469).  But safest is to actually do that from Xcode.  Too bad the EWS didn't try to build this patch.

done

>> Source/WebCore/dom/NodeRareData.h:93
>> +    MutationObserverRegistration(PassRefPtr<WebKitMutationObserver> observer, unsigned char options)
> 
> Nit: I'd prefer calling an entry so MutationObserverEntry.

done

>> Source/WebCore/dom/NodeRareData.h:105
>> +    unsigned char options;
> 
> It'll be nice if these members were private and this class instead provided methods like matches(WebKitMutationObserver:: MutationType).

done

>> 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.

removed

>> Source/WebCore/dom/NodeRareData.h:109
>> +struct MutationObserverData {
> 
> I don't think MutationObserverData is a descriptive name. I'd call it MutationObserverRegistry or MutationObserverList.

done

>> Source/WebCore/dom/NodeRareData.h:112
>> +public:
> 
> No need. The default visibility for struct is public.

removed

>> Source/WebCore/dom/NodeRareData.h:114
>> +    ~MutationObserverData() { }
> 
> No need. Compiler-generated d'tor should do the work.

removed

>> 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;

done

>> 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.

kept. per discussion

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

done

>> Source/WebCore/dom/WebKitMutationObserver.cpp:113
>> +        activeMutationObservers().remove(iter);
> 
> ListHashSet has removeLast() and last(). We should use that here.

kept. per discussion

>> Source/WebCore/dom/WebKitMutationObserver.cpp:118
>> +void WebKitMutationObserver::deliver()
> 
> I'd put this function above deliverAllMutations since enqueueMutationRecord and deliver are both non-static.

done

>> 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.

kept for consistency.
Comment 21 Rafael Weinstein 2011-10-17 14:57:07 PDT
Created attachment 111326 [details]
Patch
Comment 22 WebKit Review Bot 2011-10-17 15:00:18 PDT
Attachment 111326 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   665fcc3..1bcef7b  master     -> origin/master
	M	LayoutTests/platform/chromium/test_expectations.txt
	M	LayoutTests/ChangeLog
r97656 = 1bcef7b0ecfb0a4e2c1f14cb2804f00466599154 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒1﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒2﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒3﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Ryosuke Niwa 2011-10-17 15:08:32 PDT
Comment on attachment 111326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111326&action=review

> Source/WebCore/dom/WebKitMutationObserver.h:80
> +    Vector<Node*> m_registrations; // NodeRareData has a RefPtr to this, so use a weak pointer to avoid a cycle.

Sorry I must have missed this in my previous review. I'd prefer giving it a more descriptive name like observedNodes.

> Source/WebCore/dom/WebKitMutationObserver.h:84
> +void ensureMutationQueued(PassRefPtr<WebKitMutationObserver> , PassRefPtr<MutationRecord>);
> +

Where is this function defined or called?
Comment 24 Rafael Weinstein 2011-10-17 15:22:50 PDT
Created attachment 111330 [details]
Patch
Comment 25 WebKit Review Bot 2011-10-17 15:24:38 PDT
Attachment 111330 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒0﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒1﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3﷒2﷓; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Adam Klein 2011-10-17 15:30:26 PDT
Comment on attachment 111330 [details]
Patch

Clearing flags on attachment: 111330

Committed r97659: <http://trac.webkit.org/changeset/97659>
Comment 27 Adam Klein 2011-10-17 15:30:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Adam Klein 2011-10-17 15:31:42 PDT
(In reply to comment #26)
> (From update of attachment 111330 [details])
> Clearing flags on attachment: 111330
> 
> Committed r97659: <http://trac.webkit.org/changeset/97659>

Landed for rafaelw to avoid bot bustage.