Created attachment 140505 [details] WIP. concept of new spec.

As we explained in 2012 WebKit contributors meeting, this is a concept of new implementation of event dispatching. The spec is not updated yet. The document we used in the meeting is here: https://docs.google.com/document/d/1PY7KkTRI-doU9Hew3Mud9JBgjj5xtVqH8sY0Ui305nw/edit

Hi Dimitri and Shadow DOM guys, Could you take a look at WIP patch? I won't land this patch as is. It's just a implementation of new algorithm. A final patch should have equivalent results of this WIP patch. I hope this patch would help Dimitri to update the Event Dispatching spec in Shadow DOM. Although I'll continue improve this patch, especially the performance, if you have any comments or find any significant fault in the approach, please let me know it.

Comment on attachment 140505 [details] WIP. concept of new spec. I think the general approach is good. I am sad to see EventContext grow, but it's not something we can avoid.

*** Bug 85854 has been marked as a duplicate of this bug. ***

Hi Dimitri, Okay. Let me proceed using the current approach. Before trying to improve the performance seriously, I'd like to have some benchmark to compare the performance. AFAIR, you've told me that you used a micro benchmark when you tried to modify event dispatching before. Could you give me the used benchmark if you still had it? I think a micro benchmark is enough for now and it is not difficult to write such a benchmark. But I'd be happy if you gave me that :) (In reply to comment #4) > (From update of attachment 140505 [details]) > I think the general approach is good. I am sad to see EventContext grow, but it's not something we can avoid.

(In reply to comment #6) > Hi Dimitri, > > Okay. Let me proceed using the current approach. Before trying to improve the performance seriously, I'd like to have some benchmark to compare the performance. > > AFAIR, you've told me that you used a micro benchmark when you tried to modify event dispatching before. Could you give me the used benchmark if you still had it? I think a micro benchmark is enough for now and it is not difficult to write such a benchmark. But I'd be happy if you gave me that :) > > (In reply to comment #4) > > (From update of attachment 140505 [details] [details]) > > I think the general approach is good. I am sad to see EventContext grow, but it's not something we can avoid. I think it's somewhere in a bug under this tree: https://bugs.webkit.org/showdependencytree.cgi?id=48698, but I couldn't locate it easily.

Thank you! Let me find it. (In reply to comment #7) > I think it's somewhere in a bug under this tree: https://bugs.webkit.org/showdependencytree.cgi?id=48698, but I couldn't locate it easily.

Created attachment 141073 [details] WIP. Fix an underlining bug. Every existing test should pass now.

Comment on attachment 141073 [details] WIP. Fix an underlining bug. Every existing test should pass now. View in context: https://bugs.webkit.org/attachment.cgi?id=141073&action=review > Source/WebCore/dom/EventContext.h:57 > + bool m_shouldDispatch; Isn't this overkill? Once we see an ancestor on which we shouldn't dispatch an event, we can just exit early. Do we really need to store an extra member on the context?

Welcome to the complex world. I thought the same thing at first. But we can create an example where we should continue to climb up ancestors even if nodes in the middle should not be dispatched. Thats' exactly the case2 in the https://docs.google.com/document/d/1PY7KkTRI-doU9Hew3Mud9JBgjj5xtVqH8sY0Ui305nw/edit. Event should not be dispatched on G, but should be dispatched on A and B. (In reply to comment #10) > (From update of attachment 141073 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141073&action=review > > > Source/WebCore/dom/EventContext.h:57 > > + bool m_shouldDispatch; > > Isn't this overkill? Once we see an ancestor on which we shouldn't dispatch an event, we can just exit early. Do we really need to store an extra member on the context?

(In reply to comment #11) > Welcome to the complex world. I thought the same thing at first. But we can create an example where we should continue to climb up ancestors even if nodes in the middle should not be dispatched. > > Thats' exactly the case2 in the https://docs.google.com/document/d/1PY7KkTRI-doU9Hew3Mud9JBgjj5xtVqH8sY0Ui305nw/edit. > > Event should not be dispatched on G, but should be dispatched on A and B. > > > (In reply to comment #10) > > (From update of attachment 141073 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=141073&action=review > > > > > Source/WebCore/dom/EventContext.h:57 > > > + bool m_shouldDispatch; > > > > Isn't this overkill? Once we see an ancestor on which we shouldn't dispatch an event, we can just exit early. Do we really need to store an extra member on the context? But that diagram is incorrect. G is never an adjusted target in that subtree. H is.

(In reply to comment #12) > But that diagram is incorrect. G is never an adjusted target in that subtree. H is. Sorry, it's K.

Can you look over stuff I wrote here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16176#c10 and see if you can poke holes in that thinking?

Yeah, I am now taking a look at that. Let me address your comment soon. In current WIP patch, insertionPoint and ShadowRoot can receive events, but I've not written tests for that. The doc might be out-of-date and doesn't reflect recent code. The doc should have insertionPoint (and ShadowRoot) in the picture and table. Let me update that too. (In reply to comment #14) > Can you look over stuff I wrote here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16176#c10 and see if you can poke holes in that thinking?

I've added a comment to https://www.w3.org/Bugs/Public/show_bug.cgi?id=16176. Now I am sure I understand your ideas. Let me reflect your ideas in a following patch. (In reply to comment #15) > Yeah, I am now taking a look at that. Let me address your comment soon. > > In current WIP patch, insertionPoint and ShadowRoot can receive events, but I've not written tests for that. > The doc might be out-of-date and doesn't reflect recent code. The doc should have insertionPoint (and ShadowRoot) in the picture and table. Let me update that too. > > (In reply to comment #14) > > Can you look over stuff I wrote here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16176#c10 and see if you can poke holes in that thinking?

Let me note what should be modified: - Update adjusting target algorithm so that it can handle InsertionPoint correctly. - We don't need lowest common boundary any more. - For each scope of target ancestors, calculate a related target. - If target == relatedTarget, stop event dispatching. - Remove m_shouldBeDispatched from EventContext.

Created attachment 141109 [details] WIP. Fix the calculation of adjusted target. Now it considers InsertionPoints correctly.

Created attachment 141123 [details] WIP. New ajusting relatedTarget algorithm.

Created attachment 141138 [details] WIP. Now event dispatching is stopped where target == relatedTarget.

Most of them are done. We should define what should be set as relatedTarget if there is no ancestor of relatedTarget in the current scope. (In reply to comment #17) > Let me note what should be modified: > > - Update adjusting target algorithm so that it can handle InsertionPoint correctly. > - We don't need lowest common boundary any more. > - For each scope of target ancestors, calculate a related target. > - If target == relatedTarget, stop event dispatching. > - Remove m_shouldBeDispatched from EventContext.

Created attachment 141146 [details] WIP. Find an available relatedTarget from ancestor scope.

Now event dispatcher can find a relatedTarget from parent tree scope, recursively. (In reply to comment #21) > Most of them are done. > > We should define what should be set as relatedTarget if there is no ancestor of relatedTarget in the current scope. > > (In reply to comment #17) > > Let me note what should be modified: > > > > - Update adjusting target algorithm so that it can handle InsertionPoint correctly. > > - We don't need lowest common boundary any more. > > - For each scope of target ancestors, calculate a related target. > > - If target == relatedTarget, stop event dispatching. > > - Remove m_shouldBeDispatched from EventContext.

Comment on attachment 141146 [details] WIP. Find an available relatedTarget from ancestor scope. View in context: https://bugs.webkit.org/attachment.cgi?id=141146&action=review This looks very encouraging. Does it pass all tests? :) We should probably approach this in pieces, to minimize regressions. Chunk ideas: 1) update ensureEventAncestors logic. I can easily see that the stack + logic structure could be a nice self-contained class. 2) land the logic for calculating relative relatedTarget -- maybe even not use it for the moment 3) switch implementation to use the new relatedTarget logic. WDYT? > Source/WebCore/dom/EventDispatcher.cpp:139 > + EventTarget* relatedTarget = findRelatedTarget(scope->parentTreeScope(), relatedTargetMap); This is neat, I haven't thought of this! Though I wold rather avoid recursion here. This logic + data + transient state seem like a good candidate for a helper class.

(In reply to comment #24) > (From update of attachment 141146 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141146&action=review > > This looks very encouraging. Does it pass all tests? :) Yes, it passed all existing tests. > > We should probably approach this in pieces, to minimize regressions. Chunk ideas: > 1) update ensureEventAncestors logic. I can easily see that the stack + logic structure could be a nice self-contained class. > 2) land the logic for calculating relative relatedTarget -- maybe even not use it for the moment > 3) switch implementation to use the new relatedTarget logic. > > WDYT? I agree. I'd like to proceed in incremental way. Let me think how we should divide this patch into smaller pieces further. > > Source/WebCore/dom/EventDispatcher.cpp:139 > > + EventTarget* relatedTarget = findRelatedTarget(scope->parentTreeScope(), relatedTargetMap); > > This is neat, I haven't thought of this! Though I wold rather avoid recursion here. This logic + data + transient state seem like a good candidate for a helper class. This, trying to find a related target in upper tree scope, is just one idea. I've not decided what should be done if there is no available related target ancestor in the current scope. At first attempt, I just set relatedTarget to 'null' if there is no related target in the current scope. That is another idea. I am not sure what is better or not. If we try to find a relatedTarget from upper scopes, I agree we should avoid recursion. Let me rethink implementation again. I am also afraid that using HashMap might be bad here from the view of performance. Since the number of TreeScope is small in general, it might be faster that using some other linear search.

(In reply to comment #25) > > I am not sure what is better or not. > I will spec this for you tomorrow.

I am afraid that if we land 1) without 2), we might leak nodes through a relatedTarget. We should proceed carefully... (In reply to comment #25) > (In reply to comment #24) > > (From update of attachment 141146 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=141146&action=review > > > > This looks very encouraging. Does it pass all tests? :) > > Yes, it passed all existing tests. > > > > > We should probably approach this in pieces, to minimize regressions. Chunk ideas: > > 1) update ensureEventAncestors logic. I can easily see that the stack + logic structure could be a nice self-contained class. > > 2) land the logic for calculating relative relatedTarget -- maybe even not use it for the moment > > 3) switch implementation to use the new relatedTarget logic. > > > > WDYT? > > I agree. I'd like to proceed in incremental way. Let me think how we should divide this patch into smaller pieces further. > > > > Source/WebCore/dom/EventDispatcher.cpp:139 > > > + EventTarget* relatedTarget = findRelatedTarget(scope->parentTreeScope(), relatedTargetMap); > > > > This is neat, I haven't thought of this! Though I wold rather avoid recursion here. This logic + data + transient state seem like a good candidate for a helper class. > > This, trying to find a related target in upper tree scope, is just one idea. I've not decided what should be done if there is no available related target ancestor in the current scope. > At first attempt, I just set relatedTarget to 'null' if there is no related target in the current scope. That is another idea. > > I am not sure what is better or not. > > If we try to find a relatedTarget from upper scopes, I agree we should avoid recursion. Let me rethink implementation again. > I am also afraid that using HashMap might be bad here from the view of performance. Since the number of TreeScope is small in general, it might be faster that using some other linear search.

(In reply to comment #27) > I am afraid that if we land 1) without 2), we might leak nodes through a relatedTarget. The reason is that current algorithm of calculating relatedTarget does not consider InsertionPoint, by which we can cross lower boundaries. I am afraid that I have to land that at once. Hmm... > We should proceed carefully... > > (In reply to comment #25) > > (In reply to comment #24) > > > (From update of attachment 141146 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=141146&action=review > > > > > > This looks very encouraging. Does it pass all tests? :) > > > > Yes, it passed all existing tests. > > > > > > > > We should probably approach this in pieces, to minimize regressions. Chunk ideas: > > > 1) update ensureEventAncestors logic. I can easily see that the stack + logic structure could be a nice self-contained class. > > > 2) land the logic for calculating relative relatedTarget -- maybe even not use it for the moment > > > 3) switch implementation to use the new relatedTarget logic. > > > > > > WDYT? > > > > I agree. I'd like to proceed in incremental way. Let me think how we should divide this patch into smaller pieces further. > > > > > > Source/WebCore/dom/EventDispatcher.cpp:139 > > > > + EventTarget* relatedTarget = findRelatedTarget(scope->parentTreeScope(), relatedTargetMap); > > > > > > This is neat, I haven't thought of this! Though I wold rather avoid recursion here. This logic + data + transient state seem like a good candidate for a helper class. > > > > This, trying to find a related target in upper tree scope, is just one idea. I've not decided what should be done if there is no available related target ancestor in the current scope. > > At first attempt, I just set relatedTarget to 'null' if there is no related target in the current scope. That is another idea. > > > > I am not sure what is better or not. > > > > If we try to find a relatedTarget from upper scopes, I agree we should avoid recursion. Let me rethink implementation again. > > I am also afraid that using HashMap might be bad here from the view of performance. Since the number of TreeScope is small in general, it might be faster that using some other linear search.

Created attachment 141355 [details] WIP. Introduce an EventAncestors class.

I've started to introduce EventAncestors class, which encapsulates all operations and data which are related to ancestors of an event target. But it turned out that it was not so worth as I expected. That causes redundant indirection at least. WDYT? To go or not to go? (In reply to comment #29) > Created an attachment (id=141355) [details] > WIP. Introduce an EventAncestors class.

Comment on attachment 141355 [details] WIP. Introduce an EventAncestors class. View in context: https://bugs.webkit.org/attachment.cgi?id=141355&action=review Yes, this is getting unwieldy. I think the problem here is that you treat EventAncestors as a data+function encapsulation primitive, but it really just acts as a function encapsulation primitive. What about something like EventAncestorsBuilder class, which only exists to create eventAncestors list. Similarly, the RelatedTargetAdjuster class will only operate on the eventAncestors to adjust the list according to relatedTarget data? > Source/WebCore/dom/EventDispatcher.cpp:142 > + // Per XBL 2.0 spec, mutation events should never cross shadow DOM boundary: > + // http://dev.w3.org/2006/xbl2/#event-flow-and-targeting-across-shadow-s > + if (event->hasInterface(eventNames().interfaceForMutationEvent)) > + return StayInsideShadowDOM; We don't fire mutation events inside of the shadow DOM subtrees anymore, so this is not needed. > Source/WebCore/dom/EventDispatcher.cpp:153 > +void EventAncestors::adjustByRelatedTarget(Event* event, PassRefPtr<EventTarget> prpRelatedTarget) This looks outside of EventsAncestor helper. > Source/WebCore/dom/EventDispatcher.h:57 > + void adjustByRelatedTarget(Event*, PassRefPtr<EventTarget> prpRelatedTarget); I don't like the addition of "By" here. It doesn't clarify the name.

Thank you for the review. (In reply to comment #31) > (From update of attachment 141355 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141355&action=review > > Yes, this is getting unwieldy. I think the problem here is that you treat EventAncestors as a data+function encapsulation primitive, but it really just acts as a function encapsulation primitive. What about something like EventAncestorsBuilder class, which only exists to create eventAncestors list. Similarly, the RelatedTargetAdjuster class will only operate on the eventAncestors to adjust the list according to relatedTarget data? That sounds nice, but I am not sure whether it is worth or not to separe these functions into separate classes since these functions are not so long now (about 20 lines). But if separating makes our intention clear, we should do it. Let me try it. > > > Source/WebCore/dom/EventDispatcher.cpp:142 > > + // Per XBL 2.0 spec, mutation events should never cross shadow DOM boundary: > > + // http://dev.w3.org/2006/xbl2/#event-flow-and-targeting-across-shadow-s > > + if (event->hasInterface(eventNames().interfaceForMutationEvent)) > > + return StayInsideShadowDOM; > > We don't fire mutation events inside of the shadow DOM subtrees anymore, so this is not needed. > Thank you. Let me remove it from the next patch. > > Source/WebCore/dom/EventDispatcher.cpp:153 > > +void EventAncestors::adjustByRelatedTarget(Event* event, PassRefPtr<EventTarget> prpRelatedTarget) > > This looks outside of EventsAncestor helper. > > > Source/WebCore/dom/EventDispatcher.h:57 > > + void adjustByRelatedTarget(Event*, PassRefPtr<EventTarget> prpRelatedTarget); > > I don't like the addition of "By" here. It doesn't clarify the name. Let me remove 'By' and name it simply 'adjustRelatedTarget'.

Created attachment 141719 [details] WIP. Introduce an EventRelatedTargetAdjuster.

Created attachment 141720 [details] Benchmark for event-dispatching.

I updated the patch. The differences are: - Reverted the previous attempt which introduced EventAncestors class. - Introduce an EventRelatedTargetAdjuster - Optimize the performance of calculating adjusted related target. We now skip looking up relatedTarget if treeScope does not change. And I've just done very rough benchmark. I've attached the benchmark here: https://bugs.webkit.org/attachment.cgi?id=141720 The benchmark measures overall performance of event dispatching. I've not isolated the part of calculating event ancestors and adjusting related-target yet. That requires C++ code change. The results of benchmark looks same before/after this patch. Let me paste the results in the bottom. I am afraid that I could not isolate the effect of this patch. Let me think more sophisticated benchmark later if required. --- Before this patch ---- Benchmark for event dispatching. Shadow-Free Tree (height=10) Took 9.3 ms. (93/10) Teee with ShadowHost (height=10) Took 8.9 ms. (89/10) Shadow-Free Tree (height=20) Took 16.6 ms. (166/10) Teee with ShadowHost (height=20) Took 14.3 ms. (143/10) Shadow-Free Tree (height=30) Took 28.3 ms. (283/10) Teee with ShadowHost (height=30) Took 26.2 ms. (262/10) Shadow-Free Tree (height=50) Took 58.6 ms. (586/10) Teee with ShadowHost (height=50) Took 57.6 ms. (576/10) Shadow-Free Tree (height=100) Took 230.6 ms. (2306/10) Teee with ShadowHost (height=100) Took 212.3 ms. (2123/10) Shadow-Free Tree (height=200) Took 885.9 ms. (8859/10) Teee with ShadowHost (height=200) Took 806.8 ms. (8068/10) Shadow-Free Tree (height=300) Took 2028 ms. (20280/10) Teee with ShadowHost (height=300) Took 1721.7 ms. (17217/10) #EOF --- After this patch ---- Benchmark for event dispatching. Shadow-Free Tree (height=10) Took 9.9 ms. (99/10) Teee with ShadowHost (height=10) Took 9.7 ms. (97/10) Shadow-Free Tree (height=20) Took 17.7 ms. (177/10) Teee with ShadowHost (height=20) Took 15.6 ms. (156/10) Shadow-Free Tree (height=30) Took 29.5 ms. (295/10) Teee with ShadowHost (height=30) Took 28.1 ms. (281/10) Shadow-Free Tree (height=50) Took 61.7 ms. (617/10) Teee with ShadowHost (height=50) Took 61.9 ms. (619/10) Shadow-Free Tree (height=100) Took 248.9 ms. (2489/10) Teee with ShadowHost (height=100) Took 219.3 ms. (2193/10) Shadow-Free Tree (height=200) Took 925.6 ms. (9256/10) Teee with ShadowHost (height=200) Took 818.4 ms. (8184/10) Shadow-Free Tree (height=300) Took 2092 ms. (20920/10) Teee with ShadowHost (height=300) Took 1770.8 ms. (17708/10)

Comment on attachment 141719 [details] WIP. Introduce an EventRelatedTargetAdjuster. View in context: https://bugs.webkit.org/attachment.cgi?id=141719&action=review I really like the way this patch is heading.... However, I apologize for trying to force my design ideas upon you. This is your work and I trust your design sense to come up with the optimal solution. Please feel free to discard the notion of helper classes. I will not be upset :) > Source/WebCore/dom/EventDispatcher.h:53 > + void build(Node*, Vector<EventContext>&); Should probably contain the stack and the walker as members. > Source/WebCore/dom/EventDispatcher.h:58 > + void adjust(Vector<EventContext>&, Node* relatedTarget); Looks like relatedTarget could be a constructor argument. This way, you could initialized the walker as part of the constructor.

(In reply to comment #35) > Let me think more sophisticated benchmark later if required. Sure. FWIW, the benchmark code looks like it's doing what it's supposed to.

Created attachment 141936 [details] WIP.

Created attachment 141937 [details] modify EventDispatcher for benchmark

Thank you for the review. (In reply to comment #36) > (From update of attachment 141719 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141719&action=review > > I really like the way this patch is heading.... > > However, I apologize for trying to force my design ideas upon you. This is your work and I trust your design sense to come up with the optimal solution. Please feel free to discard the notion of helper classes. I will not be upset :) Don't worry. I won't extract EventDispatcher::ensureEventAncestors into a separate class. It's gone. > > > Source/WebCore/dom/EventDispatcher.h:53 > > + void build(Node*, Vector<EventContext>&); > > Should probably contain the stack and the walker as members. > > > Source/WebCore/dom/EventDispatcher.h:58 > > + void adjust(Vector<EventContext>&, Node* relatedTarget); > > Looks like relatedTarget could be a constructor argument. This way, you could initialized the walker as part of the constructor. Done. As for the walker, I'd like to use the walker as a local variable in EventRelatedTargetAdjuster::adjust() since walker has internal state and I don't want to make walkers' lifecycle survive beyond the function. I have one more thing. I've done another benchmark in C++. To isolate the effect of WIP patch, I've inserted code to EventDispatcher::adjustRelatedTarget(..) so that we can measure how much time this function takes. The diff is here: https://bugs.webkit.org/attachment.cgi?id=141937 I've also inserted the similar benchmark for ToT so that we compare the results. As a result, it turned out that this function occupies most time of event dispatching in both cases. Let me paste the results, including my comments to explain the results. Good news: The results looks almost same before/after applying WIP patch in every cases. I think the WIP patch has already enough competitive performance so that we can land this. Bad news: Nothing so far as long as I did a correct benchmark. We are very close to goal finally. Let me clean up the patch for the review. I'll add a detailed ChangeLog entry in next patch, including the benchmark results. -------------------------------- Before applying WIP patch. # The following are how much time EventDispatcher::adjustRelatedTarget(..) takes in each case. (Measured in C++). Took 18.821045 ms Took 17.198975 ms Took 33.204102 ms Took 28.000977 ms Took 54.199951 ms Took 52.866943 ms Took 117.178955 ms Took 114.033447 ms Took 461.781982 ms Took 420.091064 ms Took 1761.498047 ms Took 1577.511230 ms Took 3835.390137 ms Took 3359.852051 ms # The followings are how much time whole event dispatching takes in each case. (Measured from JavaScript using event-benchmark.html). Shadow-Free Tree (height=10) Took 22 ms. Teee with ShadowHost (height=10) Took 18 ms. Shadow-Free Tree (height=20) Took 34 ms. Teee with ShadowHost (height=20) Took 28 ms. Shadow-Free Tree (height=30) Took 55 ms. Teee with ShadowHost (height=30) Took 54 ms. Shadow-Free Tree (height=50) Took 119 ms. Teee with ShadowHost (height=50) Took 115 ms. Shadow-Free Tree (height=100) Took 467 ms. Teee with ShadowHost (height=100) Took 425 ms. Shadow-Free Tree (height=200) Took 1780 ms. Teee with ShadowHost (height=200) Took 1596 ms. Shadow-Free Tree (height=300) Took 3875 ms. Teee with ShadowHost (height=300) Took 3401 ms. #EOF -------------------------------- After applying WIP patch. Took 20.304199 ms Took 19.757080 ms Took 33.982910 ms Took 30.224854 ms Took 55.161865 ms Took 55.628906 ms Took 117.801758 ms Took 119.816895 ms Took 462.904053 ms Took 423.419189 ms Took 1721.790039 ms Took 1581.446777 ms Took 3957.707275 ms Took 3439.345947 ms Shadow-Free Tree (height=10) Took 23 ms. Teee with ShadowHost (height=10) Took 20 ms. Shadow-Free Tree (height=20) Took 35 ms. Teee with ShadowHost (height=20) Took 31 ms. Shadow-Free Tree (height=30) Took 56 ms. Teee with ShadowHost (height=30) Took 57 ms. Shadow-Free Tree (height=50) Took 120 ms. Teee with ShadowHost (height=50) Took 122 ms. Shadow-Free Tree (height=100) Took 468 ms. Teee with ShadowHost (height=100) Took 429 ms. Shadow-Free Tree (height=200) Took 1741 ms. Teee with ShadowHost (height=200) Took 1599 ms. Shadow-Free Tree (height=300) Took 3999 ms. Teee with ShadowHost (height=300) Took 3483 ms. #EOF

Created attachment 142203 [details] ready for reviews

Created attachment 142219 [details] ready for reviews. Refine a changelog.

Comment on attachment 142219 [details] ready for reviews. Refine a changelog. View in context: https://bugs.webkit.org/attachment.cgi?id=142219&action=review Maybe it's because I spent a lot of time on this part of the spec, but the new code reads _a lot_ better than the old one to me. Good work! A few nits -- up to you to fix before or after landing. > Source/WebCore/ChangeLog:90 > + I've also conducted a micro benchmark using both > + Shadow-Free-DOM-Tree and DOM-Tree-With-Shadow-Host. > + See https://bugs.webkit.org/show_bug.cgi?id=78586#c40 for the results. > + It seems that the new implementation has more capabilities, but > + doesn't sacrifice a performance of event dispatching in either cases. You really didn't pull any punches on this ChangeLog, did you :) > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:212 > + if (node->isShadowRoot()) { > + const ShadowRoot* shadowRoot = toShadowRoot(node); This would probably look a bit better as early return. > Source/WebCore/dom/EventContext.cpp:50 > +static MouseEvent* toMouseEvent(Event* event) > +{ > + ASSERT(event && event->isMouseEvent()); > + return static_cast<MouseEvent*>(event); > +} This seems like it belongs in MouseEvent.h?

Thank you for the review. Let me land this patch after fixing what you commented and updating Skipped files on some platforms. (In reply to comment #43) > (From update of attachment 142219 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142219&action=review > > Maybe it's because I spent a lot of time on this part of the spec, but the new code reads _a lot_ better than the old one to me. Good work! A few nits -- up to you to fix before or after landing. > > > Source/WebCore/ChangeLog:90 > > + I've also conducted a micro benchmark using both > > + Shadow-Free-DOM-Tree and DOM-Tree-With-Shadow-Host. > > + See https://bugs.webkit.org/show_bug.cgi?id=78586#c40 for the results. > > + It seems that the new implementation has more capabilities, but > > + doesn't sacrifice a performance of event dispatching in either cases. > > You really didn't pull any punches on this ChangeLog, did you :) I've learned the expression of 'pull any punches'. :) > > > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:212 > > + if (node->isShadowRoot()) { > > + const ShadowRoot* shadowRoot = toShadowRoot(node); > > This would probably look a bit better as early return. Thank you. I'll fix it. > > > Source/WebCore/dom/EventContext.cpp:50 > > +static MouseEvent* toMouseEvent(Event* event) > > +{ > > + ASSERT(event && event->isMouseEvent()); > > + return static_cast<MouseEvent*>(event); > > +} > > This seems like it belongs in MouseEvent.h? Yeah, that should be. I'll move it.

Created attachment 142403 [details] Patch for landing

Created attachment 142404 [details] Patch for landing

Comment on attachment 142404 [details] Patch for landing Clearing flags on attachment: 142404 Committed r117394: <http://trac.webkit.org/changeset/117394>

All reviewed patches have been landed. Closing bug.