[webkit-reviews] review denied: [Bug 20092] Spelling markers positioned incorrectly in RTL text : [Attachment 23719] Updated patch w/testcase

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Jeremy Moskovich <playmobil at google.com>'s request
for review:
Bug 20092: Spelling markers positioned incorrectly in RTL text
https://bugs.webkit.org/show_bug.cgi?id=20092

Attachment 23719: Updated patch w/testcase
https://bugs.webkit.org/attachment.cgi?id=23719&action=edit

------- Additional Comments from mitz at webkit.org
The WebCore/ChangeLog entry should include a link to the bug and its title, and
preferably a description of the code changes in the function.

I am not sure the proliferation of local variables in this function is helpful.
One boolean saying whether the marker spans the entire box seems to be enough.
"paintStart" and "paintEnd" are now used only once each, so I would consider
getting rid of them. "Recalculate" is not a good verb to use because this is
not replacing the result of an earlier calculation.

+	     IntPoint grammarStartPoint = IntPoint(m_x + tx, y + ty);

I know this is code you are just moving, but it is buggy. Adding m_x again here
is wrong. You can see it in this example:

data:text/html,%3Cdiv%20contenteditable%3EThis%20is%20%20is%20wrong.&nbsp;%20%3
C/div%3E

(note the two spaces between "is" and "is"). If you enable English grammar
checking and click in the sentence and then after the sentence, the "tool tip"
for the second "is" will appear when you hover the letter g instead of when you
hover the underlined word. You do not have to fix this bug in this patch. You
could just add a FIXME. Is it important not to apply truncation for the grammar
marker? If not, you could use the same selection rectangle for both spelling
(when it is needed) and grammar.

+	     IntRect grammerMarkerRect =
enclosingIntRect(f->selectionRectForText(run, grammarStartPoint,
selectionHeight(), startPosition, endPosition));

Typo: grammer.

The LayoutTests/ChangeLog entry should also include a link to the bug and the
bug title. Your patch should include the expected test results (use
run-webkit-tests --pixel to generate pixel results), and the change log should
reflect that.

The test contains tabs so it cannot be landed. Please use spaces.

+	// Remove focus from the element, since the word under the curser won't
have a misspelling marker.

Typo: curser

+This tests the correct placement of inline spelling and grammer markers in
text.<br>

Typo: grammer.

Name: svn:executable
   + *

I do not think the above is necessary.