Bug 46015 - Implement shadow DOM-aware event targeting and introduce EventContext to track the context of each event dispatch.
Summary: Implement shadow DOM-aware event targeting and introduce EventContext to trac...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 49569 49868 49977 49986 50163 50477 52065 52195
Blocks: 44907 48648
  Show dependency treegraph
 
Reported: 2010-09-17 17:13 PDT by Dimitri Glazkov (Google)
Modified: 2011-02-08 09:45 PST (History)
9 users (show)

See Also:


Attachments
End of week checkpoint (31.49 KB, patch)
2010-09-17 17:14 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (31.86 KB, patch)
2010-10-01 16:23 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (32.74 KB, patch)
2010-10-04 09:06 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (35.89 KB, patch)
2010-10-04 12:18 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch for landing. (40.38 KB, patch)
2010-10-29 11:12 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch for landing for realz. (45.39 KB, patch)
2010-10-29 11:59 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch for landing 2.0 (45.62 KB, patch)
2010-10-29 15:56 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
With a stale ptr fix (46.53 KB, patch)
2010-11-01 16:01 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (56.91 KB, patch)
2010-11-05 15:54 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (57.85 KB, patch)
2010-11-05 16:17 PDT, Dimitri Glazkov (Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2010-09-17 17:13:28 PDT
Implement shadow DOM-aware event targeting and introduce EventContext to track the context of each event dispatch.
Comment 1 Dimitri Glazkov (Google) 2010-09-17 17:14:17 PDT
Created attachment 67976 [details]
End of week checkpoint
Comment 2 Dimitri Glazkov (Google) 2010-09-20 09:43:08 PDT
Notes to self:

- use templates to combine EventContext and WindowEventContext
- study SVG event firing and simplify
- add project changes for all platforms
Comment 3 Dimitri Glazkov (Google) 2010-10-01 16:23:36 PDT
Created attachment 69537 [details]
Patch
Comment 4 Dimitri Glazkov (Google) 2010-10-01 16:25:21 PDT
(In reply to comment #2)
> Notes to self:
> 
> - use templates to combine EventContext and WindowEventContext

This seemed like a good idea, but turned cumbersome very quickly.

> - study SVG event firing and simplify

I chickened out here and did only a very limited part, leaving out a FIXME. SVG is very, very hairy.

> - add project changes for all platforms

Done.
Comment 5 WebKit Review Bot 2010-10-01 23:00:16 PDT
Attachment 69537 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4230038
Comment 6 Dimitri Glazkov (Google) 2010-10-02 06:37:54 PDT
Comment on attachment 69537 [details]
Patch

Will check on Chromium borkage...
Comment 7 Dimitri Glazkov (Google) 2010-10-04 09:06:04 PDT
Created attachment 69640 [details]
Patch
Comment 8 Dimitri Glazkov (Google) 2010-10-04 12:18:02 PDT
Created attachment 69665 [details]
Patch
Comment 9 Darin Adler 2010-10-13 17:29:08 PDT
Comment on attachment 69665 [details]
Patch

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

Design looks good. Are you sure you have enough test coverage?

> WebCore/dom/EventContext.cpp:52
> +Node* EventContext::node() const
> +{
> +    return m_node.get();
> +}
> +
> +EventTarget* EventContext::target() const
> +{
> +    return m_target.get();
> +}

I’d like to see these inlined.

> WebCore/dom/EventContext.cpp:61
> +WindowEventContext::WindowEventContext(Event* event, Node* node, const Vector<EventContext>& ancestors)

This class should go into a separate source file.

It’s a little strange to pass in a vector simply so we can extract the last element of it.

> WebCore/dom/EventContext.cpp:89
> +DOMWindow* WindowEventContext::window() const
> +{
> +    return m_window.get();
> +}
> +
> +EventTarget* WindowEventContext::target() const
> +{
> +    return m_target.get();
> +}

I’d like to see these inlined.

> WebCore/dom/EventContext.h:35
> +class ContainerNode;

Not used in this header.

> WebCore/dom/Node.cpp:2553
> +    bool skip = false;
> +    do {

I think that shouldSkipNextXXX (whatever XXX is) is a better boolean local variable name than “skip”. Since skip is a verb it’s hard to know what it means.

do while (true) is not the usual idiom for an infinite loop. I’d prefer that you use while (true), which is the more usual one in this project at least.
Comment 10 Dimitri Glazkov (Google) 2010-10-29 10:48:36 PDT
Comment on attachment 69665 [details]
Patch

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

>> WebCore/dom/EventContext.cpp:52
>> +}
> 
> I’d like to see these inlined.

Done.

>> WebCore/dom/EventContext.cpp:61
>> +WindowEventContext::WindowEventContext(Event* event, Node* node, const Vector<EventContext>& ancestors)
> 
> This class should go into a separate source file.
> 
> It’s a little strange to pass in a vector simply so we can extract the last element of it.

Agreed. Split up the file and moved resolving of top ancestor back into Node.cpp

>> WebCore/dom/EventContext.cpp:89
>> +}
> 
> I’d like to see these inlined.

Done.

>> WebCore/dom/EventContext.h:35
>> +class ContainerNode;
> 
> Not used in this header.

Removed.

>> WebCore/dom/Node.cpp:2553
>> +    do {
> 
> I think that shouldSkipNextXXX (whatever XXX is) is a better boolean local variable name than “skip”. Since skip is a verb it’s hard to know what it means.
> 
> do while (true) is not the usual idiom for an infinite loop. I’d prefer that you use while (true), which is the more usual one in this project at least.

Done. Sorry, "do while" stuff was due to me mucking with trying to not make it infinite at first :)
Comment 11 Dimitri Glazkov (Google) 2010-10-29 10:54:39 PDT
(In reply to comment #9)
> (From update of attachment 69665 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69665&action=review
> 
> Design looks good. Are you sure you have enough test coverage?

There's pretty good coverage for events fired from shadow DOM already, but it wouldn't hurt to write more tests specifically around target/currentTarget and how they change when going in/out of shadow DOM. I will tackle this next.
Comment 12 Dimitri Glazkov (Google) 2010-10-29 11:12:15 PDT
Created attachment 72351 [details]
Patch for landing.
Comment 13 Early Warning System Bot 2010-10-29 11:41:33 PDT
Attachment 72351 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4787068
Comment 14 Dimitri Glazkov (Google) 2010-10-29 11:59:42 PDT
Created attachment 72360 [details]
Patch for landing for realz.
Comment 15 Darin Adler 2010-10-29 12:31:47 PDT
Comment on attachment 72360 [details]
Patch for landing for realz.

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

> LayoutTests/ChangeLog:8
> +        Tuned the test to better reflect its point: the even should indeed fire (it used to be swallowed),

even -> event

> LayoutTests/ChangeLog:9
> +        but it's target should be a non-shadow node.

it's -> its

> LayoutTests/fast/events/shadow-boundary-crossing.html:9
> +            success = event.target.parentNode;

If the aim here is to check that this is a non-shadow node, maybe we should check for the specific correct node, instead of allowing any node with a parent.

> WebCore/dom/EventContext.h:40
> +    EventContext(Node*, EventTarget* currentTarget, EventTarget* target);

Our usual style is to use PassRefPtr for arguments when the object will take a ref to them, even if the object is not the “sole owner”. I think you should follow that here.

Can the node be a ContainerNode* instead of Node*? Do we construct these with arbitrary nodes?

> WebCore/dom/Node.cpp:1445
> +ContainerNode* Node::parentOrHostNode()

Candidate for inlining?

> WebCore/dom/Node.cpp:2516
> -void Node::eventAncestors(Vector<RefPtr<ContainerNode> > &ancestors)
> +void Node::eventAncestors(Vector<EventContext> &ancestors, EventTarget* originalTarget)

Misplaced "&" here. It's supposed to be next to the type, not the argument name.

Functions like this one, that use an out argument, should typically have “get” in their name. Not sure why this didn’t have that already.

> WebCore/dom/Node.cpp:2544
> +    };

Stray semicolon here. It should be removed.

> WebCore/dom/Node.cpp:2575
> +    RefPtr<EventTarget> originalTarget(event->target());

I prefer the "=" style syntax to the constructor style in cases like this. What do you think?

> WebCore/dom/Node.cpp:2618
> +            goto doneDispatching;

This could just be a “break” and probably should be. The reason we use goto above is that it’s nested inside two loops.

> WebCore/dom/Node.h:216
>      // FIXME: Should be named getEventAncestors.
> -    void eventAncestors(Vector<RefPtr<ContainerNode> > &ancestors);
> +    void eventAncestors(Vector<EventContext> &ancestors, EventTarget*);

As I mentioned above, misplaced & here, and the name should be changed as the FIXME says.

> WebCore/dom/WindowEventContext.cpp:50
> +        topLevelContainer = topEventContext->node();
> +        targetForWindowEvents = topEventContext->target();
> +    }

This might read better like this:

    EventTarget* targetForWindowEvents = topEventContext ? topEventContext->node() : node;
    Node* topLevelContainer = topEventContext ? topEventContext->target() : node;

Not sure. I suppose it depends on whether you are comfortable with ?: or not.

> WebCore/dom/WindowEventContext.h:43
> +    WindowEventContext(Event*, Node*, const EventContext*);

Same comment as above about PassRefPtr.
Comment 16 Eric Seidel (no email) 2010-10-29 13:31:34 PDT
Attachment 72351 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4795092
Comment 17 Dimitri Glazkov (Google) 2010-10-29 15:54:56 PDT
Comment on attachment 72360 [details]
Patch for landing for realz.

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

>> LayoutTests/ChangeLog:8
>> +        Tuned the test to better reflect its point: the even should indeed fire (it used to be swallowed),
> 
> even -> event

DOH.

>> LayoutTests/ChangeLog:9
>> +        but it's target should be a non-shadow node.
> 
> it's -> its

Ouch. I apologize.

>> LayoutTests/fast/events/shadow-boundary-crossing.html:9
>> +            success = event.target.parentNode;
> 
> If the aim here is to check that this is a non-shadow node, maybe we should check for the specific correct node, instead of allowing any node with a parent.

Sure! Fixed.

>> WebCore/dom/EventContext.h:40
>> +    EventContext(Node*, EventTarget* currentTarget, EventTarget* target);
> 
> Our usual style is to use PassRefPtr for arguments when the object will take a ref to them, even if the object is not the “sole owner”. I think you should follow that here.
> 
> Can the node be a ContainerNode* instead of Node*? Do we construct these with arbitrary nodes?

Sure, changed to PassRefPtr.

The reason I ended up using Node* was not a good one: the loop in getEventAncestors starts with "ancestor = this" (where "this" is a Node). I tried to rework the loop on the spot just now, but it was getting inelegant. I added a FIXME instead.

>> WebCore/dom/Node.cpp:1445
>> +ContainerNode* Node::parentOrHostNode()
> 
> Candidate for inlining?

Sure. Done.

>> WebCore/dom/Node.cpp:2516
>> +void Node::eventAncestors(Vector<EventContext> &ancestors, EventTarget* originalTarget)
> 
> Misplaced "&" here. It's supposed to be next to the type, not the argument name.
> 
> Functions like this one, that use an out argument, should typically have “get” in their name. Not sure why this didn’t have that already.

Done.

>> WebCore/dom/Node.cpp:2544
>> +    };
> 
> Stray semicolon here. It should be removed.

Sorry -- removed.

>> WebCore/dom/Node.cpp:2575
>> +    RefPtr<EventTarget> originalTarget(event->target());
> 
> I prefer the "=" style syntax to the constructor style in cases like this. What do you think?

Sure. Fixed.

>> WebCore/dom/Node.cpp:2618
>> +            goto doneDispatching;
> 
> This could just be a “break” and probably should be. The reason we use goto above is that it’s nested inside two loops.

Fixed.

>> WebCore/dom/Node.h:216
>> +    void eventAncestors(Vector<EventContext> &ancestors, EventTarget*);
> 
> As I mentioned above, misplaced & here, and the name should be changed as the FIXME says.

Done.

>> WebCore/dom/WindowEventContext.cpp:50
>> +    }
> 
> This might read better like this:
> 
>     EventTarget* targetForWindowEvents = topEventContext ? topEventContext->node() : node;
>     Node* topLevelContainer = topEventContext ? topEventContext->target() : node;
> 
> Not sure. I suppose it depends on whether you are comfortable with ?: or not.

I like it. Changed.

>> WebCore/dom/WindowEventContext.h:43
>> +    WindowEventContext(Event*, Node*, const EventContext*);
> 
> Same comment as above about PassRefPtr.

Sure, done. It didn't come out looking pretty, since node may or may not be ref'd up -- but the constructor signature does indicate intent more clearly.
Comment 18 Dimitri Glazkov (Google) 2010-10-29 15:56:30 PDT
Created attachment 72410 [details]
Patch for landing 2.0
Comment 19 Dimitri Glazkov (Google) 2010-10-30 10:32:56 PDT
Committed r70984: <http://trac.webkit.org/changeset/70984>
Comment 20 Dimitri Glazkov (Google) 2010-10-30 11:48:47 PDT
Reverted r70984 for reason:

Made media/audio-delete-while-slider-thumb-clicked.html crash.

Committed r70985: <http://trac.webkit.org/changeset/70985>
Comment 21 Dimitri Glazkov (Google) 2010-10-30 17:09:58 PDT
(In reply to comment #20)
> Reverted r70984 for reason:
> 
> Made media/audio-delete-while-slider-thumb-clicked.html crash.
> 
> Committed r70985: <http://trac.webkit.org/changeset/70985>

I can't repro on platform/mac. Sounds like a port-specific misuse of something, argh...
Comment 22 Dimitri Glazkov (Google) 2010-11-01 15:26:28 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > Reverted r70984 for reason:
> > 
> > Made media/audio-delete-while-slider-thumb-clicked.html crash.
> > 
> > Committed r70985: <http://trac.webkit.org/changeset/70985>
> 
> I can't repro on platform/mac. Sounds like a port-specific misuse of something, argh...

Ah, turns out it's a difference in garbage collector. New patch/fix coming up...
Comment 23 Dimitri Glazkov (Google) 2010-11-01 16:01:49 PDT
Created attachment 72582 [details]
With a stale ptr fix
Comment 24 Dimitri Glazkov (Google) 2010-11-01 16:11:45 PDT
(In reply to comment #23)
> Created an attachment (id=72582) [details]
> With a stale ptr fix

Thanks for your patience. I found a stale pointer that's only visible with the new patch and over-active garbage collector. It's pretty obvious by inspection, but I am going to work on a layout test to make sure we capture the regression. Apparently, I am doomed to write large patches :)
Comment 25 Dimitri Glazkov (Google) 2010-11-02 20:54:14 PDT
(In reply to comment #24)
> (In reply to comment #23)
> > Created an attachment (id=72582) [details] [details]
> > With a stale ptr fix
> 
> Thanks for your patience. I found a stale pointer that's only visible with the new patch and over-active garbage collector. It's pretty obvious by inspection, but I am going to work on a layout test to make sure we capture the regression. Apparently, I am doomed to write large patches :)

Turns out this was 2 pixels short of actually reaching a thumb on platform/mac:

http://trac.webkit.org/browser/trunk/LayoutTests/media/audio-delete-while-slider-thumb-clicked.html#L57

So this test didn't actually test what it had intended. In Chromium, the scrubber thumb is more rectangular in shape and it was triggering the crash. I'll improve the test and submit with this change.
Comment 26 WebKit Review Bot 2010-11-02 20:59:41 PDT
Attachment 72360 [details] did not build on win:
Build output: http://queues.webkit.org/results/4942020
Comment 27 Dimitri Glazkov (Google) 2010-11-02 21:03:05 PDT
(In reply to comment #26)
> Attachment 72360 [details] did not build on win:
> Build output: http://queues.webkit.org/results/4942020

Yeah, I realized that today too :)
Comment 28 Dimitri Glazkov (Google) 2010-11-03 10:02:58 PDT
Committed r71244: <http://trac.webkit.org/changeset/71244>
Comment 29 WebKit Review Bot 2010-11-03 14:09:54 PDT
http://trac.webkit.org/changeset/71244 might have broken Leopard Intel Release (Tests), Leopard Intel Debug (Tests), SnowLeopard Intel Release (Tests), GTK Linux 32-bit Release, GTK Linux 32-bit Debug, and GTK Linux 64-bit Debug
The following tests are not passing:
editing/spelling/context-menu-suggestions.html
media/controls-right-click-on-timebar.html
Comment 30 Ryosuke Niwa 2010-11-03 14:13:49 PDT
(In reply to comment #29)
> http://trac.webkit.org/changeset/71244 might have broken Leopard Intel Release (Tests), Leopard Intel Debug (Tests), SnowLeopard Intel Release (Tests), GTK Linux 32-bit Release, GTK Linux 32-bit Debug, and GTK Linux 64-bit Debug
> The following tests are not passing:
> editing/spelling/context-menu-suggestions.html
> media/controls-right-click-on-timebar.html

http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r71244%20(23660)/results.html
context-menu-suggestions.html indeed seems to be failing consistently.  Could you look into this?
Comment 31 Dimitri Glazkov (Google) 2010-11-03 16:07:28 PDT
Sorry, rolled out again in http://trac.webkit.org/changeset/71278. Turns out Darin was right: there's not enough test coverage. I realized that with this change, I also need to adjust event handling in existing shadow DOM elements, because right now they expect their events to be handled by their hosts, which breaks context menu, for instance.
Comment 32 Dimitri Glazkov (Google) 2010-11-04 11:22:49 PDT
(In reply to comment #31)
> Sorry, rolled out again in http://trac.webkit.org/changeset/71278. Turns out Darin was right: there's not enough test coverage. I realized that with this change, I also need to adjust event handling in existing shadow DOM elements, because right now they expect their events to be handled by their hosts, which breaks context menu, for instance.

Found the problem. http://trac.webkit.org/changeset/60418 erroneously flipped a condition:

Before http://trac.webkit.org/browser/trunk/WebCore/rendering/TextControlInnerElements.cpp?rev=58914#L123
After http://trac.webkit.org/browser/trunk/WebCore/rendering/TextControlInnerElements.cpp?rev=60418#L141

Prior to event retargeting, it would just fall back to HTMLInputElement::defaultEventHandler, so this problem wasn't visible.
Comment 33 Darin Adler 2010-11-04 18:05:39 PDT
(In reply to comment #32)
> http://trac.webkit.org/changeset/60418 erroneously flipped a condition:

Oops, sorry!
Comment 34 Dimitri Glazkov (Google) 2010-11-04 18:22:15 PDT
(In reply to comment #33)
> (In reply to comment #32)
> > http://trac.webkit.org/changeset/60418 erroneously flipped a condition:
> 
> Oops, sorry!

No worries, it's all good. With the help of Erik Arvidsson, I made good progress on layout test coverage of the edge cases around event shadow boundary crossing and will include the first set of tests with the patch.
Comment 35 Dimitri Glazkov (Google) 2010-11-05 15:54:13 PDT
Created attachment 73131 [details]
Patch
Comment 36 Darin Adler 2010-11-05 16:00:03 PDT
Comment on attachment 73131 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        * media/audio-delete-while-slider-thumb-clicked.html:

No comment about why this test had to change?

> WebCore/rendering/ShadowElement.h:52
> -    HTMLElement* m_shadowParent;
> +    RefPtr<HTMLElement> m_shadowParent;

Are you sure this change is safe? Can’t this lead to a reference cycle?
Comment 37 Dimitri Glazkov (Google) 2010-11-05 16:12:04 PDT
(In reply to comment #36)
> (From update of attachment 73131 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73131&action=review
> 
> > LayoutTests/ChangeLog:11
> > +        * media/audio-delete-while-slider-thumb-clicked.html:
> 
> No comment about why this test had to change?

The explanation is at the top, but I should probably move it down to individual test. Will re-upload in a sec.

> > WebCore/rendering/ShadowElement.h:52
> > -    HTMLElement* m_shadowParent;
> > +    RefPtr<HTMLElement> m_shadowParent;
> 
> Are you sure this change is safe? Can’t this lead to a reference cycle?

Hmm. That's a good point. Because ShadowElement's owner (like RenderSlider) is not ref-counted, I don't believe so:

HTMLInputElement-raw->RenderSlider-RefPtr->ShadowElement->RefPtr->HTMLInputElement.
Comment 38 Dimitri Glazkov (Google) 2010-11-05 16:17:01 PDT
Created attachment 73137 [details]
Patch
Comment 39 Darin Adler 2010-11-11 14:19:53 PST
Comment on attachment 73137 [details]
Patch

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

> WebCore/dom/Node.cpp:2535
>  #if ENABLE(SVG)
> -            // Skip <use> shadow tree elements.
> -            if (ancestor->isSVGElement() && ancestor->isShadowNode())
> -                continue;
> +        // Skip SVGShadowTreeRootElement.
> +        shouldSkipNextAncestor = ancestor->isSVGElement() && ancestor->isShadowNode();
> +        if (shouldSkipNextAncestor)
> +            continue;
>  #endif

Idea probably for next iteration: If we put the isSVGElement && isShadowNode check into an inline function we could get the #if out of this function, much as you did with the eventTargetRespectingSVGTargetRules function.

> WebCore/dom/Node.cpp:2555
> +    return ancestors.isEmpty() ? 0 : &(ancestors.last());

You don’t need those parentheses. To me they make the code harder to read, although others may not feel the same way.

> WebCore/dom/Node.h:214
> +    // Node's parent or shadow tree host.
> +    ContainerNode* parentOrHostNode();

I’m not sure this function needs “Node” in its name.

> WebCore/dom/WindowEventContext.cpp:41
> +    // We don't dispatch load events to the window. That quirk was originally
> +    // added because Mozilla doesn't propagate load events to the window object.

Should we say “This quirk” rather than “That quirk”?
Comment 40 Dimitri Glazkov (Google) 2010-11-12 11:44:48 PST
(In reply to comment #39)
> (From update of attachment 73137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73137&action=review
> 
> > WebCore/dom/Node.cpp:2535
> >  #if ENABLE(SVG)
> > -            // Skip <use> shadow tree elements.
> > -            if (ancestor->isSVGElement() && ancestor->isShadowNode())
> > -                continue;
> > +        // Skip SVGShadowTreeRootElement.
> > +        shouldSkipNextAncestor = ancestor->isSVGElement() && ancestor->isShadowNode();
> > +        if (shouldSkipNextAncestor)
> > +            continue;
> >  #endif
> 
> Idea probably for next iteration: If we put the isSVGElement && isShadowNode check into an inline function we could get the #if out of this function, much as you did with the eventTargetRespectingSVGTargetRules function.

Yeah, I really need to take another shot at this mess.

> 
> > WebCore/dom/Node.cpp:2555
> > +    return ancestors.isEmpty() ? 0 : &(ancestors.last());
> 
> You don’t need those parentheses. To me they make the code harder to read, although others may not feel the same way.

Removed.

> 
> > WebCore/dom/Node.h:214
> > +    // Node's parent or shadow tree host.
> > +    ContainerNode* parentOrHostNode();
> 
> I’m not sure this function needs “Node” in its name.

It's mimicking parentNode(). I'll leave it as-is for now. I am not entirely happy with the term "host" either, but I'll paint that shed in a better color when we get closer to actual XBL2 implementation.

> > WebCore/dom/WindowEventContext.cpp:41
> > +    // We don't dispatch load events to the window. That quirk was originally
> > +    // added because Mozilla doesn't propagate load events to the window object.
> 
> Should we say “This quirk” rather than “That quirk”?

Yep!
Comment 41 Dimitri Glazkov (Google) 2010-11-12 11:49:52 PST
Committed r71934: <http://trac.webkit.org/changeset/71934>
Comment 42 WebKit Review Bot 2010-11-12 13:07:55 PST
http://trac.webkit.org/changeset/71934 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
media/controls-right-click-on-timebar.html
Comment 43 Dimitri Glazkov (Google) 2010-11-12 13:24:14 PST
(In reply to comment #42)
> http://trac.webkit.org/changeset/71934 might have broken GTK Linux 64-bit Debug
> The following tests are not passing:
> media/controls-right-click-on-timebar.html

Looking into the breakage ...
Comment 44 Dimitri Glazkov (Google) 2010-11-12 13:41:18 PST
(In reply to comment #43)
> (In reply to comment #42)
> > http://trac.webkit.org/changeset/71934 might have broken GTK Linux 64-bit Debug
> > The following tests are not passing:
> > media/controls-right-click-on-timebar.html
> 
> Looking into the breakage ...

It's due to https://bugs.webkit.org/show_bug.cgi?id=40601.