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

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

The bounding box is expected to be in window's coordinate system. In the case of iframe, it is actually in the frame coordinate system.

<rdar://problem/9018127>

[Jia Pu]

Created attachment 84642 [details]
Proposed patch (v1)

[Darin Adler]

Comment on attachment 84642 [details]
Proposed patch (v1)

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.

[Jia Pu]

(In reply to comment #2)
> (From update of attachment 84642 [details])
> 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.

I just want to be clear that they're all mac-only tests. If you think it's OK as it is, I will pull them out.

> 
> > 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?

I can certainly put this in separate patch.

[Jia Pu]

(In reply to comment #2)
> (From update of attachment 84642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84642&action=review
> 
> > Source/WebCore/ChangeLog:28
> > +        * manual-tests/platforms/mac/autocorrection/autocorreciton-in-iframe.html: Added.
> 
> Typo in filename here: autocorreciton
> 
> > 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.

Judging by the fact that there are many calls to frame() without checking. I assume it's safe. We could put an assertion.  I will check the return value of view(). My thought is not to litter the code with unnecessary check of this sort.
> 
> 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.

[Jia Pu]

Created attachment 84767 [details]
Proposed patch (v2)

[Darin Adler]

Comment on attachment 84767 [details]
Proposed patch (v2)

View in context: https://bugs.webkit.org/attachment.cgi?id=84767&action=review

> Source/WebCore/dom/Range.cpp:1907
>      if (!m_start.container())
>          return 0;

This code isn’t needed any more.

> Source/WebCore/dom/Range.cpp:1979
> +    for (size_t i = 0; i< quads.size(); ++i)

Missing space here before the "<".

> Source/WebCore/editing/Editor.cpp:3609
> +FloatRect Editor::rectToWindow(const FloatRect& rect) const

The function I was suggested we factor out was actually windowRectForRange, ranking a Range*.

> Source/WebCore/editing/Editor.h:434
> +    // Convert abolustion (page) coordinate to window space

abolustion?

[Jia Pu]

Created attachment 84786 [details]
Patch (v3)

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 84786 [details]:

inspector/audits/audits-panel-functional.html bug 55776 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 84786 [details]
Patch (v3)

Clearing flags on attachment: 84786

Committed r80408: <http://trac.webkit.org/changeset/80408>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.