27154 2009-07-10 12:44:57 -0700 useless null-check statement in visible_units.cpp@logicalStartOfLine 2009-07-15 11:07:46 -0700 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 0 tonikitoo webkit-unassigned bfulgham eric jparent justin.garcia ojan rniwa xji oldest_to_newest 130922 0 tonikitoo 2009-07-10 12:44:57 -0700 quoted code below and bug summary are self explanatory. (...) VisiblePosition logicalStartOfLine(const VisiblePosition& c) { VisiblePosition visPos = logicalStartPositionForLine(c); if (visPos.isNull()) return c.honorEditableBoundaryAtOrAfter(visPos); return c.honorEditableBoundaryAtOrAfter(visPos); } (...) honorEditableBoundaryAtOrAfter is called regardless the null check and possibly (wrongly) twice 131463 1 rniwa 2009-07-13 18:44:33 -0700 It seems like the only difference between logicalStartOfLine and logicalStartPositionForLine is that we call honorEditableBoundaryAtOrAfter for logicalStartOfLine. But because there is no mentioning of editable boundaries in the function, it's utterly confusing. We should fix that. 131464 2 justin.garcia 2009-07-13 18:47:26 -0700 > if (visPos.isNull()) > return c.honorEditableBoundaryAtOrAfter(visPos); > > return c.honorEditableBoundaryAtOrAfter(visPos); Hmm strange. Does logicalEndOfLine have this problem as well? Justin 131467 3 32693 tonikitoo 2009-07-13 18:54:22 -0700 Created attachment 32693 simple fix: statement removed 131475 4 rniwa 2009-07-13 19:31:45 -0700 logicalStartPositionForLine is only used in logicalStartOfLine. I don't understand what was the reason for making two functions, one of which is only used in the other function. It seems like this function is added in https://trac.webkit.org/changeset/43032. Maybe that the person intended to remove logicalStartOfLine later? 131476 5 rniwa 2009-07-13 19:35:25 -0700 (In reply to comment #2) > Hmm strange. Does logicalEndOfLine have this problem as well? > > Justin logicalEndOfLine is slightly better: VisiblePosition visPos = logicalEndPositionForLine(c); // Make sure the end of line is at the same line as the given input position. For a wrapping line, the logical end // position for the not-last-2-lines might incorrectly hand back the logical beginning of the next line. // For example, <div contenteditable dir="rtl" style="line-break:before-white-space">abcdefg abcdefg abcdefg // a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg </div> // In this case, use the previous position of the computed logical end position. if (!inSameLogicalLine(c, visPos)) visPos = visPos.previous(); return c.honorEditableBoundaryAtOrBefore(visPos); But logicalEndPositionForLine isn't used anywhere else either. 131477 6 rniwa 2009-07-13 19:42:05 -0700 (In reply to comment #3) > Created an attachment (id=32693) [details] > simple fix: statement removed Thank you for posting a patch, but I'd like to investigate more about these functions. It seems like we don't even need logicalStartPositionForLine (we can put everything inside logicalStartOfLine). I have added the author of the changeset 43032. Hopefully he can give us some feedback on this. 131604 7 xji 2009-07-14 10:26:03 -0700 Thanks for identifying and working on this bug! logicalStartPositionForLine/logicalStartOfLine and logicalEndPositionForLine/logicalEndOfLine are introduced when fixing issue 24168 (RTL: Home/End key does not behave correctly in mixed bidi text in RTL document ). Since Home/End key are logical keys, those logical operations are introduced, while the original startPositionForLine/startOfLine/endPositionForLine/endOfLine are preserved for visual operations. I think it is my bug having the following code in logicalStartOfLine (although honorEditableBoundaryAtOrAfter wont be called twice). if (visPos.isNull()) return c.honorEditableBoundaryAtOrAfter(visPos); return c.honorEditableBoundaryAtOrAfter(visPos); Antonio Gomes's patch should fix it. The structure of logicalStartOfLine was similar to that of startOfLine() which have "if (visPos.isNotNull())". But during the testing, I did not find any cases where the above condition is true, so, I removed it (see the last 4th review feedback in comments #20 of issue 24168). I guess the above mistake was introduced when I removed it (should remove the if block completely). The structure of logicalEndOfLine is slightly different because I did find cases where the returning position from logicalEndPositionForLine() need to be adjusted (see comments in the code). I am leaving the logicalStartPositionForLine and logicalStartOfLine (same for end of line) as separate functions to follow the pattern of startPositionForLine and startOfLine. I prefer to leave it as is (although logicalStartOfLine() is pretty simple right now), but I do not have strong opinion about it. Maybe mitz has better suggestions. I did not replace any of the startPositionForLine/startOfLine/endPositionForLine/endofLine simply because they are visual operations and are used in several other places. I do not have enough knowledge of whether those functionalities are correct or not and whether their usages are correct or not. 131958 8 bfulgham 2009-07-15 11:07:46 -0700 Landed in http://trac.webkit.org/changeset/45926. 32693 2009-07-13 18:54:22 -0700 2009-07-14 11:53:28 -0700 simple fix: statement removed BUG_27154_v1.diff text/plain 1714 tonikitoo Y29tbWl0IDdkZTE1YjEyMzZhODBhNDEwMTdjZWFmODM4M2VmYWUwMDliODc2NzAKQXV0aG9yOiBB bnRvbmlvIEdvbWVzICh0b25pa2l0b28pIDxhbnRvbmlvLmdvbWVzQG9wZW5ib3NzYS5vcmc+CkRh dGU6ICAgTW9uIEp1bCAxMyAyMTo1MToyNCAyMDA5IC0wNDAwCgogICAgMjAwOS0wNy0xMyAgQW50 b25pbyBHb21lcyAgIDxhbnRvbmlvLmdvbWVzQG9wZW5ib3NzYS5vcmc+CiAgICAKICAgICAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCiAgICAKICAgICAgICAgICAgdXNlbGVzcyBu dWxsLWNoZWNrIHN0YXRlbWVudCBpbiB2aXNpYmxlX3VuaXRzLmNwcEBsb2dpY2FsU3RhcnRPZkxp bmUKICAgICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI3 MTU0CiAgICAKICAgICAgICAgICAgU2ltcGxlIGZpeC4KICAgIAogICAgICAgICAgICAqIGVkaXRp bmcvdmlzaWJsZV91bml0cy5jcHA6CiAgICAgICAgICAgIChXZWJDb3JlOjpsb2dpY2FsU3RhcnRP ZkxpbmUpOiBEb3VibGVkIGhvbm9yRWRpdGFibGVCb3VuZGFyeUF0T3JBZnRlcigpIGNhbGwgcmVt b3ZlZC4KCmRpZmYgLS1naXQgYS9XZWJDb3JlL0NoYW5nZUxvZyBiL1dlYkNvcmUvQ2hhbmdlTG9n CmluZGV4IDlmNzIxMmQuLmRiYWRhNDMgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvQ2hhbmdlTG9nCisr KyBiL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMDktMDctMTMgIEFudG9u aW8gR29tZXMgICA8YW50b25pby5nb21lc0BvcGVuYm9zc2Eub3JnPgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIHVzZWxlc3MgbnVsbC1jaGVjayBzdGF0 ZW1lbnQgaW4gdmlzaWJsZV91bml0cy5jcHBAbG9naWNhbFN0YXJ0T2ZMaW5lCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNzE1NAorCisgICAgICAgIFNp bXBsZSBmaXguCisKKyAgICAgICAgKiBlZGl0aW5nL3Zpc2libGVfdW5pdHMuY3BwOgorICAgICAg ICAoV2ViQ29yZTo6bG9naWNhbFN0YXJ0T2ZMaW5lKTogRG91YmxlZCBob25vckVkaXRhYmxlQm91 bmRhcnlBdE9yQWZ0ZXIoKSBjYWxsIHJlbW92ZWQuCisKIDIwMDktMDctMTMgIEVyaWsgQXJ2aWRz c29uICA8YXJ2QGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBEYXJpbiBBZGxl ciBhbmQgTWFjaWVqIFN0YWNob3dpYWsuCmRpZmYgLS1naXQgYS9XZWJDb3JlL2VkaXRpbmcvdmlz aWJsZV91bml0cy5jcHAgYi9XZWJDb3JlL2VkaXRpbmcvdmlzaWJsZV91bml0cy5jcHAKaW5kZXgg MDJlOWZiOC4uODY5Yzg5MyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9lZGl0aW5nL3Zpc2libGVfdW5p dHMuY3BwCisrKyBiL1dlYkNvcmUvZWRpdGluZy92aXNpYmxlX3VuaXRzLmNwcApAQCAtMTE3OCw5 ICsxMTc4LDYgQEAgVmlzaWJsZVBvc2l0aW9uIGxvZ2ljYWxTdGFydE9mTGluZShjb25zdCBWaXNp YmxlUG9zaXRpb24mIGMpCiB7CiAgICAgVmlzaWJsZVBvc2l0aW9uIHZpc1BvcyA9IGxvZ2ljYWxT dGFydFBvc2l0aW9uRm9yTGluZShjKTsKICAgICAKLSAgICBpZiAodmlzUG9zLmlzTnVsbCgpKQot ICAgICAgICByZXR1cm4gYy5ob25vckVkaXRhYmxlQm91bmRhcnlBdE9yQWZ0ZXIodmlzUG9zKTsK LQogICAgIHJldHVybiBjLmhvbm9yRWRpdGFibGVCb3VuZGFyeUF0T3JBZnRlcih2aXNQb3MpOwog fQogCg==