Bug 68956 – [MutationObservers] Implement WebKitMutationObserver.observe for attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Adam Klein]

[MutationObservers] Implement WebKitMutationObserver.observe for attributes

[Adam Klein]

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.

[Rafael Weinstein]

Created attachment 110865 [details]
Patch

[Rafael Weinstein]

Comment on attachment 110865 [details]
Patch

Initial work -- Not yet ready for review.

[Rafael Weinstein]

Created attachment 110906 [details]
Patch

[Rafael Weinstein]

Now ready for review. This is my first webkit feature work (earlier patchers were chromium plumbing) -- please do *NOT* be gentle =-).

[Ryosuke Niwa]

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?

[Rafael Weinstein]

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.

[Rafael Weinstein]

Created attachment 110952 [details]
Patch

[Adam Klein]

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

[Adam Klein]

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.

[Ryosuke Niwa]

Sam, Maciej, Darin, do you have any comments on this patch? If not, I'm going to r+ it by this coming Monday.

[Rafael Weinstein]

Created attachment 111047 [details]
Patch

[Rafael Weinstein]

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

[Ryosuke Niwa]

(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 :)

[Rafael Weinstein]

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

[Rafael Weinstein]

Created attachment 111301 [details]
Patch

[WebKit Review Bot]

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.6@9637; 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.6@9637; 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.6@9637; 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.

[Adam Klein]

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.

[Ryosuke Niwa]

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.

[Rafael Weinstein]

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.

[Rafael Weinstein]

Created attachment 111326 [details]
Patch

[WebKit Review Bot]

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.6@9637; 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.6@9637; 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.6@9637; 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.

[Ryosuke Niwa]

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?

[Rafael Weinstein]

Created attachment 111330 [details]
Patch

[WebKit Review Bot]

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.6@9637; 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.6@9637; 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.6@9637; 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.

[Adam Klein]

Comment on attachment 111330 [details]
Patch

Clearing flags on attachment: 111330

Committed r97659: <http://trac.webkit.org/changeset/97659>

[Adam Klein]

All reviewed patches have been landed.  Closing bug.

[Adam Klein]

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