Bug 46015

Created attachment 67976 [details] End of week checkpoint

Notes to self: - use templates to combine EventContext and WindowEventContext - study SVG event firing and simplify - add project changes for all platforms

Created attachment 69537 [details] Patch

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

Attachment 69537 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4230038

Comment on attachment 69537 [details] Patch Will check on Chromium borkage...

Created attachment 69640 [details] Patch

Created attachment 69665 [details] Patch

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

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

Created attachment 72351 [details] Patch for landing.

Attachment 72351 [details] did not build on qt: Build output: http://queues.webkit.org/results/4787068

Created attachment 72360 [details] Patch for landing for realz.

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.

Attachment 72351 [details] did not build on mac: Build output: http://queues.webkit.org/results/4795092

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.

Created attachment 72410 [details] Patch for landing 2.0

Committed r70984: <http://trac.webkit.org/changeset/70984>

Reverted r70984 for reason: Made media/audio-delete-while-slider-thumb-clicked.html crash. Committed r70985: <http://trac.webkit.org/changeset/70985>

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

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

Created attachment 72582 [details] With a stale ptr fix

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

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

Attachment 72360 [details] did not build on win: Build output: http://queues.webkit.org/results/4942020

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

Committed r71244: <http://trac.webkit.org/changeset/71244>

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

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

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.

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

(In reply to comment #32) > http://trac.webkit.org/changeset/60418 erroneously flipped a condition: Oops, sorry!

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

Created attachment 73131 [details] Patch

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?

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

Created attachment 73137 [details] Patch

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”?

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

Committed r71934: <http://trac.webkit.org/changeset/71934>

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

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

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