46504 2010-09-24 13:24:35 -0700 FormatBlockCommand's modifyRange and doApply should be merged 2010-09-25 13:42:59 -0700 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) All All RESOLVED FIXED P2 Enhancement --- 19795 1 rniwa rniwa darin enrica ojan rniwa tkent tony oldest_to_newest 284651 0 rniwa 2010-09-24 13:24:35 -0700 FormatBlockCommand's doApply mainly implements insertion and removal of lists in single paragraph. To handle a selection with multiple paragraphs, doApply calls modifyRange, which iterates over paragraphs in the selection and calls doApply on each paragraph. doApply calls modifyRange whenever the selection is range except when the starting and the ending of the selection is in the same paragraph in which case modifyRange bails out by returning false and doApply proceed with its list insertion & removal. However, this peculiar structure makes the code harder to read. This bug is similar to https://bugs.webkit.org/show_bug.cgi?id=41403. I propose to merge FormatBlockCommand's modifyRange with doApply and isolate the logic for single paragraph case into a separate function. 284676 1 68741 rniwa 2010-09-24 13:59:07 -0700 Created attachment 68741 Patch 284864 2 68741 tony 2010-09-24 17:20:48 -0700 Comment on attachment 68741 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68741&action=review In general, this seems fine. It looks like it reduces the amount of recursion used, which is also good. > WebCore/editing/FormatBlockCommand.cpp:71 > + if (endingSelection().isCaret() || startOfParagraph(visibleStart) == startOfLastParagraph) { > + doApplyForSingleParagraph(); > return; > + } Can you comment on why you added the isCaret() check? It seems like it's a new condition. 284869 3 rniwa 2010-09-24 17:32:57 -0700 (In reply to comment #2) > (From update of attachment 68741 [details]) > > WebCore/editing/FormatBlockCommand.cpp:71 > > + if (endingSelection().isCaret() || startOfParagraph(visibleStart) == startOfLastParagraph) { > > + doApplyForSingleParagraph(); > > return; > > + } > > Can you comment on why you added the isCaret() check? It seems like it's a new condition. So we used to call doModify when endingSelection().isRange() returned true, and proceeded with our implementation of format block for single paragraph otherwise. In my patch, I'm exchanging the order so that we early exit when we have a caret and handle the multiple paragraphs case otherwise. i.e. we used to have if range and startOfParagraph(visibleStart) == startOfLastParagraph (this condition is checked inside modifyRange): call doApply on each paragraph in the selection else: formatBlock on single paragraph I'm changing it to: if caret or startOfParagraph(visibleStart) == startOfLastParagraph: do formatBlock on single paragraph else: do formatBlock on each paragraph in the selection. Does this clarify your question? 284870 4 rniwa 2010-09-24 17:34:02 -0700 Oops, I meant > if range and startOfParagraph(visibleStart) != startOfLastParagraph for the first case. 284876 5 tony 2010-09-24 17:44:51 -0700 (In reply to comment #3) > > Does this clarify your question? I see, yes that makes sense. LGTM. 285047 6 rniwa 2010-09-25 13:42:59 -0700 Committed r68337: <http://trac.webkit.org/changeset/68337> 68741 2010-09-24 13:59:07 -0700 2010-09-24 17:20:47 -0700 Patch bug-46504-20100924135905.patch text/plain 6023 rniwa SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2ODI4NikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjAgQEAKKzIwMTAtMDktMjQgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5v cmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRm9y bWF0QmxvY2tDb21tYW5kJ3MgbW9kaWZ5UmFuZ2UgYW5kIGRvQXBwbHkgc2hvdWxkIGJlIG1lcmdl ZAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDY1MDQK KworICAgICAgICBJc29sYXRlZCB0aGUgY29kZSBpbiBkb0FwcGx5IHRvIGluc2VydCBhbmQgcmVt b3ZlIGxpc3RzIGZvciBzaW5nbGUgcGFyYWdyYXBoIGludG8gZG9BcHBseUZvclNpbmdsZVBhcmFn cmFwaC4KKyAgICAgICAgTWVyZ2VkIHRoZSBjb2RlIGluIG1vZGlmeVJhbmdlIGludG8gZG9BcHBs eSBhbmQgY2xlYW5lZCB1cC4KKworICAgICAgICBObyBuZXcgdGVzdHMgYXJlIGFkZGVkIHNpbmNl IHRoaXMgaXMgYSBjbGVhbnVwLgorCisgICAgICAgICogZWRpdGluZy9Gb3JtYXRCbG9ja0NvbW1h bmQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6Rm9ybWF0QmxvY2tDb21tYW5kOjpkb0FwcGx5KTog TWVyZ2VkIHdpdGggbW9kaWZ5UmFuZ2U7IGNhbGxzIGRvQXBwbHlGb3JTaW5nbGVQYXJhZ3JhcGgu CisgICAgICAgIChXZWJDb3JlOjpGb3JtYXRCbG9ja0NvbW1hbmQ6OmRvQXBwbHlGb3JTaW5nbGVQ YXJhZ3JhcGgpOiBBZGRlZC4KKyAgICAgICAgKiBlZGl0aW5nL0Zvcm1hdEJsb2NrQ29tbWFuZC5o OgorCiAyMDEwLTA5LTI0ICBEYXZpZCBIeWF0dCAgPGh5YXR0QGFwcGxlLmNvbT4KIAogICAgICAg ICBSZXZpZXdlZCBieSBTYW0gV2VpbmlnLgpJbmRleDogV2ViQ29yZS9lZGl0aW5nL0Zvcm1hdEJs b2NrQ29tbWFuZC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9lZGl0aW5nL0Zvcm1hdEJsb2Nr Q29tbWFuZC5jcHAJKHJldmlzaW9uIDY4MjcyKQorKysgV2ViQ29yZS9lZGl0aW5nL0Zvcm1hdEJs b2NrQ29tbWFuZC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTQxLDMzICs0MSw2IEBAIEZvcm1hdEJs b2NrQ29tbWFuZDo6Rm9ybWF0QmxvY2tDb21tYW5kKEQKIHsKIH0KIAotYm9vbCBGb3JtYXRCbG9j a0NvbW1hbmQ6Om1vZGlmeVJhbmdlKCkKLXsKLSAgICBBU1NFUlQoZW5kaW5nU2VsZWN0aW9uKCku aXNSYW5nZSgpKTsKLSAgICBWaXNpYmxlUG9zaXRpb24gdmlzaWJsZVN0YXJ0ID0gZW5kaW5nU2Vs ZWN0aW9uKCkudmlzaWJsZVN0YXJ0KCk7Ci0gICAgVmlzaWJsZVBvc2l0aW9uIHZpc2libGVFbmQg PSBlbmRpbmdTZWxlY3Rpb24oKS52aXNpYmxlRW5kKCk7Ci0gICAgVmlzaWJsZVBvc2l0aW9uIHN0 YXJ0T2ZMYXN0UGFyYWdyYXBoID0gc3RhcnRPZlBhcmFncmFwaCh2aXNpYmxlRW5kKTsKLSAgICAK LSAgICBpZiAoc3RhcnRPZlBhcmFncmFwaCh2aXNpYmxlU3RhcnQpID09IHN0YXJ0T2ZMYXN0UGFy YWdyYXBoKQotICAgICAgICByZXR1cm4gZmFsc2U7Ci0KLSAgICBzZXRFbmRpbmdTZWxlY3Rpb24o dmlzaWJsZVN0YXJ0KTsKLSAgICBkb0FwcGx5KCk7Ci0gICAgdmlzaWJsZVN0YXJ0ID0gZW5kaW5n U2VsZWN0aW9uKCkudmlzaWJsZVN0YXJ0KCk7Ci0gICAgVmlzaWJsZVBvc2l0aW9uIG5leHRQYXJh Z3JhcGggPSBlbmRPZlBhcmFncmFwaCh2aXNpYmxlU3RhcnQpLm5leHQoKTsKLSAgICB3aGlsZSAo bmV4dFBhcmFncmFwaC5pc05vdE51bGwoKSAmJiBuZXh0UGFyYWdyYXBoICE9IHN0YXJ0T2ZMYXN0 UGFyYWdyYXBoKSB7Ci0gICAgICAgIHNldEVuZGluZ1NlbGVjdGlvbihuZXh0UGFyYWdyYXBoKTsK LSAgICAgICAgZG9BcHBseSgpOwotICAgICAgICBuZXh0UGFyYWdyYXBoID0gZW5kT2ZQYXJhZ3Jh cGgoZW5kaW5nU2VsZWN0aW9uKCkudmlzaWJsZVN0YXJ0KCkpLm5leHQoKTsKLSAgICB9Ci0gICAg c2V0RW5kaW5nU2VsZWN0aW9uKHZpc2libGVFbmQpOwotICAgIGRvQXBwbHkoKTsKLSAgICB2aXNp YmxlRW5kID0gZW5kaW5nU2VsZWN0aW9uKCkudmlzaWJsZUVuZCgpOwotICAgIHNldEVuZGluZ1Nl bGVjdGlvbihWaXNpYmxlU2VsZWN0aW9uKHZpc2libGVTdGFydC5kZWVwRXF1aXZhbGVudCgpLCB2 aXNpYmxlRW5kLmRlZXBFcXVpdmFsZW50KCksIERPV05TVFJFQU0pKTsKLQotICAgIHJldHVybiB0 cnVlOwotfQotCiB2b2lkIEZvcm1hdEJsb2NrQ29tbWFuZDo6ZG9BcHBseSgpCiB7CiAgICAgaWYg KCFlbmRpbmdTZWxlY3Rpb24oKS5pc05vbk9ycGhhbmVkQ2FyZXRPclJhbmdlKCkpCkBAIC04Niwx MiArNTksMzUgQEAgdm9pZCBGb3JtYXRCbG9ja0NvbW1hbmQ6OmRvQXBwbHkoKQogICAgIC8vIEZJ WE1FOiBXZSBwYWludCB0aGUgZ2FwIGJlZm9yZSBzb21lIHBhcmFncmFwaHMgdGhhdCBhcmUgaW5k ZW50ZWQgd2l0aCBsZWZ0IAogICAgIC8vIG1hcmdpbi9wYWRkaW5nLCBidXQgbm90IG90aGVycy4g IFdlIHNob3VsZCBtYWtlIHRoZSBnYXAgcGFpbnRpbmcgbW9yZSBjb25zaXN0ZW50IGFuZCAKICAg ICAvLyB0aGVuIHVzZSBhIGxlZnQgbWFyZ2luL3BhZGRpbmcgcnVsZSBoZXJlLgotICAgIGlmICh2 aXNpYmxlRW5kICE9IHZpc2libGVTdGFydCAmJiBpc1N0YXJ0T2ZQYXJhZ3JhcGgodmlzaWJsZUVu ZCkpCisgICAgaWYgKHZpc2libGVFbmQgIT0gdmlzaWJsZVN0YXJ0ICYmIGlzU3RhcnRPZlBhcmFn cmFwaCh2aXNpYmxlRW5kKSkgewogICAgICAgICBzZXRFbmRpbmdTZWxlY3Rpb24oVmlzaWJsZVNl bGVjdGlvbih2aXNpYmxlU3RhcnQsIHZpc2libGVFbmQucHJldmlvdXModHJ1ZSkpKTsKKyAgICAg ICAgdmlzaWJsZUVuZCA9IGVuZGluZ1NlbGVjdGlvbigpLnZpc2libGVFbmQoKTsKKyAgICB9CiAK LSAgICBpZiAoZW5kaW5nU2VsZWN0aW9uKCkuaXNSYW5nZSgpICYmIG1vZGlmeVJhbmdlKCkpCisg ICAgVmlzaWJsZVBvc2l0aW9uIHN0YXJ0T2ZMYXN0UGFyYWdyYXBoID0gc3RhcnRPZlBhcmFncmFw aCh2aXNpYmxlRW5kKTsKKyAgICBpZiAoZW5kaW5nU2VsZWN0aW9uKCkuaXNDYXJldCgpIHx8IHN0 YXJ0T2ZQYXJhZ3JhcGgodmlzaWJsZVN0YXJ0KSA9PSBzdGFydE9mTGFzdFBhcmFncmFwaCkgewor ICAgICAgICBkb0FwcGx5Rm9yU2luZ2xlUGFyYWdyYXBoKCk7CiAgICAgICAgIHJldHVybjsKKyAg ICB9CisKKyAgICBzZXRFbmRpbmdTZWxlY3Rpb24odmlzaWJsZVN0YXJ0KTsKKyAgICBkb0FwcGx5 Rm9yU2luZ2xlUGFyYWdyYXBoKCk7CisgICAgdmlzaWJsZVN0YXJ0ID0gZW5kaW5nU2VsZWN0aW9u KCkudmlzaWJsZVN0YXJ0KCk7CisgICAgVmlzaWJsZVBvc2l0aW9uIG5leHRQYXJhZ3JhcGggPSBl bmRPZlBhcmFncmFwaCh2aXNpYmxlU3RhcnQpLm5leHQoKTsKKyAgICB3aGlsZSAobmV4dFBhcmFn cmFwaC5pc05vdE51bGwoKSAmJiBuZXh0UGFyYWdyYXBoICE9IHN0YXJ0T2ZMYXN0UGFyYWdyYXBo KSB7CisgICAgICAgIHNldEVuZGluZ1NlbGVjdGlvbihuZXh0UGFyYWdyYXBoKTsKKyAgICAgICAg ZG9BcHBseUZvclNpbmdsZVBhcmFncmFwaCgpOworICAgICAgICBuZXh0UGFyYWdyYXBoID0gZW5k T2ZQYXJhZ3JhcGgoZW5kaW5nU2VsZWN0aW9uKCkudmlzaWJsZVN0YXJ0KCkpLm5leHQoKTsKKyAg ICB9CisgICAgc2V0RW5kaW5nU2VsZWN0aW9uKHZpc2libGVFbmQpOworICAgIGRvQXBwbHlGb3JT aW5nbGVQYXJhZ3JhcGgoKTsKKyAgICB2aXNpYmxlRW5kID0gZW5kaW5nU2VsZWN0aW9uKCkudmlz aWJsZUVuZCgpOwogCisgICAgc2V0RW5kaW5nU2VsZWN0aW9uKFZpc2libGVTZWxlY3Rpb24odmlz aWJsZVN0YXJ0LmRlZXBFcXVpdmFsZW50KCksIHZpc2libGVFbmQuZGVlcEVxdWl2YWxlbnQoKSwg RE9XTlNUUkVBTSkpOworfQorCit2b2lkIEZvcm1hdEJsb2NrQ29tbWFuZDo6ZG9BcHBseUZvclNp bmdsZVBhcmFncmFwaCgpCit7CiAgICAgRXhjZXB0aW9uQ29kZSBlYzsKICAgICBTdHJpbmcgbG9j YWxOYW1lLCBwcmVmaXg7CiAgICAgaWYgKCFEb2N1bWVudDo6cGFyc2VRdWFsaWZpZWROYW1lKG1f dGFnTmFtZSwgcHJlZml4LCBsb2NhbE5hbWUsIGVjKSkKQEAgLTEwMiwxNCArOTgsMTQgQEAgdm9p ZCBGb3JtYXRCbG9ja0NvbW1hbmQ6OmRvQXBwbHkoKQogICAgIGlmIChyZWZOb2RlLT5oYXNUYWdO YW1lKHFUeXBlT2ZCbG9jaykpCiAgICAgICAgIC8vIFdlJ3JlIGFscmVhZHkgaW4gYSBibG9jayB3 aXRoIHRoZSBmb3JtYXQgd2Ugd2FudCwgc28gd2UgZG9uJ3QgaGF2ZSB0byBkbyBhbnl0aGluZwog ICAgICAgICByZXR1cm47Ci0gICAgCisKICAgICBWaXNpYmxlUG9zaXRpb24gcGFyYWdyYXBoU3Rh cnQgPSBzdGFydE9mUGFyYWdyYXBoKGVuZGluZ1NlbGVjdGlvbigpLnZpc2libGVTdGFydCgpKTsK ICAgICBWaXNpYmxlUG9zaXRpb24gcGFyYWdyYXBoRW5kID0gZW5kT2ZQYXJhZ3JhcGgoZW5kaW5n U2VsZWN0aW9uKCkudmlzaWJsZVN0YXJ0KCkpOwogICAgIFZpc2libGVQb3NpdGlvbiBibG9ja1N0 YXJ0ID0gc3RhcnRPZkJsb2NrKGVuZGluZ1NlbGVjdGlvbigpLnZpc2libGVTdGFydCgpKTsKICAg ICBWaXNpYmxlUG9zaXRpb24gYmxvY2tFbmQgPSBlbmRPZkJsb2NrKGVuZGluZ1NlbGVjdGlvbigp LnZpc2libGVTdGFydCgpKTsKICAgICBSZWZQdHI8RWxlbWVudD4gYmxvY2tOb2RlID0gY3JlYXRl SFRNTEVsZW1lbnQoZG9jdW1lbnQoKSwgbV90YWdOYW1lKTsKICAgICBSZWZQdHI8RWxlbWVudD4g cGxhY2Vob2xkZXIgPSBjcmVhdGVCcmVha0VsZW1lbnQoZG9jdW1lbnQoKSk7Ci0gICAgCisKICAg ICBOb2RlKiByb290ID0gZW5kaW5nU2VsZWN0aW9uKCkuc3RhcnQoKS5ub2RlKCktPnJvb3RFZGl0 YWJsZUVsZW1lbnQoKTsKICAgICBpZiAodmFsaWRCbG9ja1RhZyhyZWZOb2RlLT5ub2RlTmFtZSgp Lmxvd2VyKCkpICYmIAogICAgICAgICBwYXJhZ3JhcGhTdGFydCA9PSBibG9ja1N0YXJ0ICYmIHBh cmFncmFwaEVuZCA9PSBibG9ja0VuZCAmJiAKQEAgLTEyMiw3ICsxMTgsNyBAQCB2b2lkIEZvcm1h dEJsb2NrQ29tbWFuZDo6ZG9BcHBseSgpCiAgICAgICAgIGluc2VydE5vZGVBdChibG9ja05vZGUs IHBhcmFncmFwaFN0YXJ0LmRlZXBFcXVpdmFsZW50KCkudXBzdHJlYW0oKSk7CiAgICAgfQogICAg IGFwcGVuZE5vZGUocGxhY2Vob2xkZXIsIGJsb2NrTm9kZSk7Ci0gICAgCisKICAgICBWaXNpYmxl UG9zaXRpb24gZGVzdGluYXRpb24oUG9zaXRpb24ocGxhY2Vob2xkZXIuZ2V0KCksIDApKTsKICAg ICBpZiAocGFyYWdyYXBoU3RhcnQgPT0gcGFyYWdyYXBoRW5kICYmICFsaW5lQnJlYWtFeGlzdHNB dFZpc2libGVQb3NpdGlvbihwYXJhZ3JhcGhTdGFydCkpIHsKICAgICAgICAgc2V0RW5kaW5nU2Vs ZWN0aW9uKGRlc3RpbmF0aW9uKTsKSW5kZXg6IFdlYkNvcmUvZWRpdGluZy9Gb3JtYXRCbG9ja0Nv bW1hbmQuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2VkaXRpbmcvRm9ybWF0QmxvY2tDb21tYW5k LmgJKHJldmlzaW9uIDY4MjcyKQorKysgV2ViQ29yZS9lZGl0aW5nL0Zvcm1hdEJsb2NrQ29tbWFu ZC5oCSh3b3JraW5nIGNvcHkpCkBAIC00MSw5ICs0MSw5IEBAIHByaXZhdGU6CiAgICAgRm9ybWF0 QmxvY2tDb21tYW5kKERvY3VtZW50KiwgY29uc3QgQXRvbWljU3RyaW5nJiB0YWdOYW1lKTsKIAog ICAgIHZpcnR1YWwgdm9pZCBkb0FwcGx5KCk7CisgICAgdm9pZCBkb0FwcGx5Rm9yU2luZ2xlUGFy YWdyYXBoKCk7CiAgICAgdmlydHVhbCBFZGl0QWN0aW9uIGVkaXRpbmdBY3Rpb24oKSBjb25zdCB7 IHJldHVybiBFZGl0QWN0aW9uRm9ybWF0QmxvY2s7IH0KIAotICAgIGJvb2wgbW9kaWZ5UmFuZ2Uo KTsKICAgICBBdG9taWNTdHJpbmcgbV90YWdOYW1lOwogfTsKIAo=