[webkit-reviews] review denied: [Bug 55717] On Mac, the bounding box sent to EditorClient::showCorrectionPanel() is incorrect when the correction occurs in an iframe. : [Attachment 84642] Proposed patch (v1)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied jpu at apple.com's request for review:
Bug 55717: On Mac, the bounding box sent to EditorClient::showCorrectionPanel()
is incorrect when the correction occurs in an iframe.
https://bugs.webkit.org/show_bug.cgi?id=55717

Attachment 84642: Proposed patch (v1)
https://bugs.webkit.org/attachment.cgi?id=84642&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84642&action=review

Seems good. Almost right for landing, but I’d like you to at least rename
Range::getBoundingRect and possibly split this up into more than once patch.

> Source/WebCore/ChangeLog:17
> +	   2. Moved all Mac-only manuel test into manual-tests/platform/mac
directory.

Typo here: "manuel".

I know that our automated tests use the path platforms/mac, but I’m not sure
it’s great for the manual tests. I guess it’s OK, but the extra level of
hierarchy seems annoying. These already were in a directory named
autocorrection. I’m not sure we needed to move them at all.

> Source/WebCore/ChangeLog:18
> +	   3. Cleaned up code in
Editor::removeSpellAndCorrectionMarkersFromWordsToBeEdited().

Could you land this in a separate patch? Is there some reason to combine these?


> Source/WebCore/ChangeLog:28
> +	   *
manual-tests/platforms/mac/autocorrection/autocorreciton-in-iframe.html: Added.


Typo in filename here: autocorreciton

> Source/WebCore/dom/Range.cpp:1913
> +FloatRect Range::getBoundingRect() const

This kind of function does not get the word “get” in its name in the WebKit
project. The getBoundingClientRect function is different, because it is exposed
to JavaScript and so its name is set by a cross-browser standard. But since
this new function is for internal use inside WebKit it should just be named
boundingRect not getBoundingRect.

> Source/WebCore/dom/Range.cpp:1916
> +	   return FloatRect(0, 0, 0, 0);

This should just be FloatRect(). No reason to explicitly write out four zeroes.


> Source/WebCore/editing/Editor.cpp:2407
> +		       IntRect boundingBoxInWindow =
frame()->view()->contentsToWindow(IntRect(boundingBox));

Can frame() or view() ever be 0 here? Same question for the other places this
appears.

I think you should make a helper function in this class that returns the
boundingBoxInWindow for a Range because this idiom is repeated three different
times in this file.