49982 2010-11-23 10:23:19 -0800 Change EventHandler::dispatchMouseEvent code to use DOM traversal instead of render tree traversal 2010-12-10 04:15:10 -0800 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 49977 48698 1 dglazkov webkit-unassigned commit-queue eae webkit.review.bot oldest_to_newest 312910 0 dglazkov 2010-11-23 10:23:19 -0800 Per my comment introduced in bug 49977, we should use DOM traversal in discovering first mouse-focusable node, rather than render tree. 319721 1 76122 eae 2010-12-09 14:29:48 -0800 Created attachment 76122 Patch 319725 2 76122 abarth 2010-12-09 14:32:51 -0800 Comment on attachment 76122 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76122&action=review > WebCore/ChangeLog:12 > + Walk up DOM/hosted tree rather than render tree. We can't land files with tabs in them. > WebCore/page/EventHandler.cpp:1869 > && m_frame->selection()->toNormalizedRange()->compareNode(n, ec) == Range::NODE_INSIDE Can this run JavaScript? > WebCore/page/EventHandler.cpp:1875 > + node = node->parentOrHostNode(); Are we holding references during this walk? 319726 3 76122 darin 2010-12-09 14:34:04 -0800 Comment on attachment 76122 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76122&action=review > WebCore/ChangeLog:8 > + No new tests. (OOPS!) Need to remove this or replace it with an explanation of why there are no new tests. The tools won’t allow this to be checked in with OOPS in it, and unlike the Reviewed by line, the tools won’t fill in the value for us. > WebCore/ChangeLog:12 > + Walk up DOM/hosted tree rather than render tree. Tab here. Won’t be able to land a patch with a tab in it. > WebCore/page/EventHandler.cpp:1861 > + while (node) { > if (node && node->isMouseFocusable()) { No need to check node for null twice in a row here. 319728 4 webkit.review.bot 2010-12-09 14:35:43 -0800 Attachment 76122 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/page/EventHandler.cpp']" exit_code: 1 WebCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 319730 5 76122 darin 2010-12-09 14:36:27 -0800 Comment on attachment 76122 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76122&action=review >> WebCore/page/EventHandler.cpp:1875 >> + node = node->parentOrHostNode(); > > Are we holding references during this walk? Adam’s point is a good one. It’s incorrect to use the node pointer after calling JavaScript if we didn’t hold a reference to it. But that bug is not new to the new code. I can’t tell from here what is done with the node after we call break. It would be easier to read if we separated the loop searching for a mouse-focusable node from the work we do on that node. 319733 6 76125 eae 2010-12-09 14:42:11 -0800 Created attachment 76125 Patch 319735 7 eae 2010-12-09 14:51:12 -0800 Adam, > > WebCore/ChangeLog:12 > > + Walk up DOM/hosted tree rather than render tree. > > We can't land files with tabs in them. My bad, fixed. > > WebCore/page/EventHandler.cpp:1869 > > && m_frame->selection()->toNormalizedRange()-> > >compareNode(n, ec) == Range::NODE_INSIDE > > Can this run JavaScript? As far as I can tell it can not. It does update the render tree though. > > WebCore/page/EventHandler.cpp:1875 > > + node = node->parentOrHostNode(); > > Are we holding references during this walk? No, I don't see how they could become invalidated during the execution though as no dom or js interactions take place. 319978 8 76125 eric 2010-12-10 00:02:04 -0800 Comment on attachment 76125 Patch Seems sane. 320092 9 76125 commit-queue 2010-12-10 04:15:04 -0800 Comment on attachment 76125 Patch Clearing flags on attachment: 76125 Committed r73717: <http://trac.webkit.org/changeset/73717> 320093 10 commit-queue 2010-12-10 04:15:10 -0800 All reviewed patches have been landed. Closing bug. 76122 2010-12-09 14:29:48 -0800 2010-12-09 14:42:08 -0800 Patch bug-49982-20101209142947.patch text/plain 2866 eae SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3MzY0NykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTAtMTItMDkgIEVtaWwgRWtsdW5kICA8ZWFlQGNocm9taXVtLm9y Zz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBDaGFu Z2UgRXZlbnRIYW5kbGVyOjpkaXNwYXRjaE1vdXNlRXZlbnQgY29kZSB0byB1c2UgRE9NIHRyYXZl cnNhbCBpbnN0ZWFkIG9mIHJlbmRlciB0cmVlIHRyYXZlcnNhbAorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDk5ODIKKworICAgICAgICBObyBuZXcgdGVz dHMuIChPT1BTISkKKworICAgICAgICAqIHBhZ2UvRXZlbnRIYW5kbGVyLmNwcDoKKyAgICAgICAg KFdlYkNvcmU6OkV2ZW50SGFuZGxlcjo6ZGlzcGF0Y2hNb3VzZUV2ZW50KToKKwlXYWxrIHVwIERP TS9ob3N0ZWQgdHJlZSByYXRoZXIgdGhhbiByZW5kZXIgdHJlZS4KKwogMjAxMC0xMi0wOCAgRGFy aW4gQWRsZXIgIDxkYXJpbkBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgR2VvZmZy ZXkgR2FyZW4uCkluZGV4OiBXZWJDb3JlL3BhZ2UvRXZlbnRIYW5kbGVyLmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBXZWJDb3JlL3BhZ2UvRXZlbnRIYW5kbGVyLmNwcAkocmV2aXNpb24gNzM2MzApCisrKyBX ZWJDb3JlL3BhZ2UvRXZlbnRIYW5kbGVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTg1NSwxNiAr MTg1NSwxMiBAQCBib29sIEV2ZW50SGFuZGxlcjo6ZGlzcGF0Y2hNb3VzZUV2ZW50KGNvCiAgICAg ICAgIC8vIGlzIGV4cGVjdGVkIGJ5IHNvbWUgc2l0ZXMgdGhhdCByZWx5IG9uIG9uQ2hhbmdlIGhh bmRsZXJzIHJ1bm5pbmcKICAgICAgICAgLy8gZnJvbSBmb3JtIGZpZWxkcyBiZWZvcmUgdGhlIGJ1 dHRvbiBjbGljayBpcyBwcm9jZXNzZWQuCiAgICAgICAgIE5vZGUqIG5vZGUgPSBtX25vZGVVbmRl ck1vdXNlLmdldCgpOwotICAgICAgICBSZW5kZXJPYmplY3QqIHJlbmRlcmVyID0gbm9kZSA/IG5v ZGUtPnJlbmRlcmVyKCkgOiAwOwogCi0gICAgICAgIC8vIFdhbGsgdXAgdGhlIHJlbmRlciB0cmVl IHRvIHNlYXJjaCBmb3IgYSBub2RlIHRvIGZvY3VzLgotICAgICAgICAvLyBXYWxraW5nIHVwIHRo ZSBET00gdHJlZSB3b3VsZG4ndCB3b3JrIGZvciBzaGFkb3cgdHJlZXMsIGxpa2UgdGhvc2UgYmVo aW5kIHRoZSBlbmdpbmUtYmFzZWQgdGV4dCBmaWVsZHMuCi0gICAgICAgIC8vIEZJWE1FOiBSZXdv cmsgdG8gdXNlIHNoYWRvd1BhcmVudC4gTm8gbmVlZCB0byB0cmF2ZXJzZSB3aXRoIHRoZSByZW5k ZXIgdHJlZS4KLSAgICAgICAgd2hpbGUgKHJlbmRlcmVyKSB7Ci0gICAgICAgICAgICBub2RlID0g cmVuZGVyZXItPm5vZGUoKTsKKyAgICAgICAgLy8gV2FsayB1cCB0aGUgRE9NIHRyZWUgdG8gc2Vh cmNoIGZvciBhIG5vZGUgdG8gZm9jdXMuCisgICAgICAgIHdoaWxlIChub2RlKSB7CiAgICAgICAg ICAgICBpZiAobm9kZSAmJiBub2RlLT5pc01vdXNlRm9jdXNhYmxlKCkpIHsKLSAgICAgICAgICAg ICAgICAvLyBUbyBmaXggPHJkYXI6Ly9wcm9ibGVtLzQ4OTU0Mjg+IENhbid0IGRyYWcgc2VsZWN0 ZWQgVG9Ebywgd2UgZG9uJ3QgZm9jdXMgYSAKLSAgICAgICAgICAgICAgICAvLyBub2RlIG9uIG1v dXNlIGRvd24gaWYgaXQncyBzZWxlY3RlZCBhbmQgaW5zaWRlIGEgZm9jdXNlZCBub2RlLiBJdCB3 aWxsIGJlIAorICAgICAgICAgICAgICAgIC8vIFRvIGZpeCA8cmRhcjovL3Byb2JsZW0vNDg5NTQy OD4gQ2FuJ3QgZHJhZyBzZWxlY3RlZCBUb0RvLCB3ZSBkb24ndCBmb2N1cyBhCisgICAgICAgICAg ICAgICAgLy8gbm9kZSBvbiBtb3VzZSBkb3duIGlmIGl0J3Mgc2VsZWN0ZWQgYW5kIGluc2lkZSBh IGZvY3VzZWQgbm9kZS4gSXQgd2lsbCBiZQogICAgICAgICAgICAgICAgIC8vIGZvY3VzZWQgaWYg dGhlIHVzZXIgZG9lcyBhIG1vdXNldXAgb3ZlciBpdCwgaG93ZXZlciwgYmVjYXVzZSB0aGUgbW91 c2V1cAogICAgICAgICAgICAgICAgIC8vIHdpbGwgc2V0IGEgc2VsZWN0aW9uIGluc2lkZSBpdCwg d2hpY2ggd2lsbCBjYWxsIHNldEZvY3VzZU5vZGVJZk5lZWRlZC4KICAgICAgICAgICAgICAgICBF eGNlcHRpb25Db2RlIGVjID0gMDsKQEAgLTE4NzMsMTEgKzE4NjksMTAgQEAgYm9vbCBFdmVudEhh bmRsZXI6OmRpc3BhdGNoTW91c2VFdmVudChjbwogICAgICAgICAgICAgICAgICAgICAmJiBtX2Zy YW1lLT5zZWxlY3Rpb24oKS0+dG9Ob3JtYWxpemVkUmFuZ2UoKS0+Y29tcGFyZU5vZGUobiwgZWMp ID09IFJhbmdlOjpOT0RFX0lOU0lERQogICAgICAgICAgICAgICAgICAgICAmJiBuLT5pc0Rlc2Nl bmRhbnRPZihtX2ZyYW1lLT5kb2N1bWVudCgpLT5mb2N1c2VkTm9kZSgpKSkKICAgICAgICAgICAg ICAgICAgICAgcmV0dXJuIGZhbHNlOwotICAgICAgICAgICAgICAgICAgICAKKwogICAgICAgICAg ICAgICAgIGJyZWFrOwogICAgICAgICAgICAgfQotICAgICAgICAgICAgCi0gICAgICAgICAgICBy ZW5kZXJlciA9IHJlbmRlcmVyLT5wYXJlbnQoKTsKKyAgICAgICAgICAgIG5vZGUgPSBub2RlLT5w YXJlbnRPckhvc3ROb2RlKCk7CiAgICAgICAgIH0KIAogICAgICAgICAvLyBJZiBmb2N1cyBzaGlm dCBpcyBibG9ja2VkLCB3ZSBlYXQgdGhlIGV2ZW50LiAgTm90ZSB3ZSBzaG91bGQgbmV2ZXIgY2xl YXIgc3dhbGxvd0V2ZW50Cg== 76125 2010-12-09 14:42:11 -0800 2010-12-10 04:15:04 -0800 Patch bug-49982-20101209144210.patch text/plain 2659 eae SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3MzY0NykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTQgQEAKKzIwMTAtMTItMDkgIEVtaWwgRWtsdW5kICA8ZWFlQGNocm9taXVtLm9y Zz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBDaGFu Z2UgRXZlbnRIYW5kbGVyOjpkaXNwYXRjaE1vdXNlRXZlbnQgY29kZSB0byB1c2UgRE9NIHRyYXZl cnNhbCBpbnN0ZWFkIG9mIHJlbmRlciB0cmVlIHRyYXZlcnNhbAorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDk5ODIKKworICAgICAgICAqIHBhZ2UvRXZl bnRIYW5kbGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkV2ZW50SGFuZGxlcjo6ZGlzcGF0Y2hN b3VzZUV2ZW50KToKKyAgICAgICAgV2FsayB1cCBET00vaG9zdGVkIHRyZWUgcmF0aGVyIHRoYW4g cmVuZGVyIHRyZWUuCisKIDIwMTAtMTItMDggIERhcmluIEFkbGVyICA8ZGFyaW5AYXBwbGUuY29t PgogCiAgICAgICAgIFJldmlld2VkIGJ5IEdlb2ZmcmV5IEdhcmVuLgpJbmRleDogV2ViQ29yZS9w YWdlL0V2ZW50SGFuZGxlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wYWdlL0V2ZW50SGFu ZGxlci5jcHAJKHJldmlzaW9uIDczNjMwKQorKysgV2ViQ29yZS9wYWdlL0V2ZW50SGFuZGxlci5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTE4NTUsMTYgKzE4NTUsMTIgQEAgYm9vbCBFdmVudEhhbmRs ZXI6OmRpc3BhdGNoTW91c2VFdmVudChjbwogICAgICAgICAvLyBpcyBleHBlY3RlZCBieSBzb21l IHNpdGVzIHRoYXQgcmVseSBvbiBvbkNoYW5nZSBoYW5kbGVycyBydW5uaW5nCiAgICAgICAgIC8v IGZyb20gZm9ybSBmaWVsZHMgYmVmb3JlIHRoZSBidXR0b24gY2xpY2sgaXMgcHJvY2Vzc2VkLgog ICAgICAgICBOb2RlKiBub2RlID0gbV9ub2RlVW5kZXJNb3VzZS5nZXQoKTsKLSAgICAgICAgUmVu ZGVyT2JqZWN0KiByZW5kZXJlciA9IG5vZGUgPyBub2RlLT5yZW5kZXJlcigpIDogMDsKIAotICAg ICAgICAvLyBXYWxrIHVwIHRoZSByZW5kZXIgdHJlZSB0byBzZWFyY2ggZm9yIGEgbm9kZSB0byBm b2N1cy4KLSAgICAgICAgLy8gV2Fsa2luZyB1cCB0aGUgRE9NIHRyZWUgd291bGRuJ3Qgd29yayBm b3Igc2hhZG93IHRyZWVzLCBsaWtlIHRob3NlIGJlaGluZCB0aGUgZW5naW5lLWJhc2VkIHRleHQg ZmllbGRzLgotICAgICAgICAvLyBGSVhNRTogUmV3b3JrIHRvIHVzZSBzaGFkb3dQYXJlbnQuIE5v IG5lZWQgdG8gdHJhdmVyc2Ugd2l0aCB0aGUgcmVuZGVyIHRyZWUuCi0gICAgICAgIHdoaWxlIChy ZW5kZXJlcikgewotICAgICAgICAgICAgbm9kZSA9IHJlbmRlcmVyLT5ub2RlKCk7Ci0gICAgICAg ICAgICBpZiAobm9kZSAmJiBub2RlLT5pc01vdXNlRm9jdXNhYmxlKCkpIHsKLSAgICAgICAgICAg ICAgICAvLyBUbyBmaXggPHJkYXI6Ly9wcm9ibGVtLzQ4OTU0Mjg+IENhbid0IGRyYWcgc2VsZWN0 ZWQgVG9Ebywgd2UgZG9uJ3QgZm9jdXMgYSAKLSAgICAgICAgICAgICAgICAvLyBub2RlIG9uIG1v dXNlIGRvd24gaWYgaXQncyBzZWxlY3RlZCBhbmQgaW5zaWRlIGEgZm9jdXNlZCBub2RlLiBJdCB3 aWxsIGJlIAorICAgICAgICAvLyBXYWxrIHVwIHRoZSBET00gdHJlZSB0byBzZWFyY2ggZm9yIGEg bm9kZSB0byBmb2N1cy4KKyAgICAgICAgd2hpbGUgKG5vZGUpIHsKKyAgICAgICAgICAgIGlmIChu b2RlLT5pc01vdXNlRm9jdXNhYmxlKCkpIHsKKyAgICAgICAgICAgICAgICAvLyBUbyBmaXggPHJk YXI6Ly9wcm9ibGVtLzQ4OTU0Mjg+IENhbid0IGRyYWcgc2VsZWN0ZWQgVG9Ebywgd2UgZG9uJ3Qg Zm9jdXMgYQorICAgICAgICAgICAgICAgIC8vIG5vZGUgb24gbW91c2UgZG93biBpZiBpdCdzIHNl bGVjdGVkIGFuZCBpbnNpZGUgYSBmb2N1c2VkIG5vZGUuIEl0IHdpbGwgYmUKICAgICAgICAgICAg ICAgICAvLyBmb2N1c2VkIGlmIHRoZSB1c2VyIGRvZXMgYSBtb3VzZXVwIG92ZXIgaXQsIGhvd2V2 ZXIsIGJlY2F1c2UgdGhlIG1vdXNldXAKICAgICAgICAgICAgICAgICAvLyB3aWxsIHNldCBhIHNl bGVjdGlvbiBpbnNpZGUgaXQsIHdoaWNoIHdpbGwgY2FsbCBzZXRGb2N1c2VOb2RlSWZOZWVkZWQu CiAgICAgICAgICAgICAgICAgRXhjZXB0aW9uQ29kZSBlYyA9IDA7CkBAIC0xODc2LDggKzE4NzIs NyBAQCBib29sIEV2ZW50SGFuZGxlcjo6ZGlzcGF0Y2hNb3VzZUV2ZW50KGNvCiAgICAgICAgICAg ICAgICAgICAgIAogICAgICAgICAgICAgICAgIGJyZWFrOwogICAgICAgICAgICAgfQotICAgICAg ICAgICAgCi0gICAgICAgICAgICByZW5kZXJlciA9IHJlbmRlcmVyLT5wYXJlbnQoKTsKKyAgICAg ICAgICAgIG5vZGUgPSBub2RlLT5wYXJlbnRPckhvc3ROb2RlKCk7CiAgICAgICAgIH0KIAogICAg ICAgICAvLyBJZiBmb2N1cyBzaGlmdCBpcyBibG9ja2VkLCB3ZSBlYXQgdGhlIGV2ZW50LiAgTm90 ZSB3ZSBzaG91bGQgbmV2ZXIgY2xlYXIgc3dhbGxvd0V2ZW50Cg==