[Webkit-unassigned] [Bug 55717] On Mac, the bounding box sent to EditorClient::showCorrectionPanel() is incorrect when the correction occurs in an iframe.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=55717


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #84642|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2011-03-03 16:45:54 PST ---
(From update of attachment 84642)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.