Bug 55717 - On Mac, the bounding box sent to EditorClient::showCorrectionPanel() is incorrect when the correction occurs in an iframe.
Summary: On Mac, the bounding box sent to EditorClient::showCorrectionPanel() is incor...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-03-03 15:27 PST by Jia Pu
Modified: 2011-03-04 22:12 PST (History)
1 user (show)

See Also:


Attachments
Proposed patch (v1) (18.25 KB, patch)
2011-03-03 15:57 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Proposed patch (v2) (14.04 KB, patch)
2011-03-04 09:58 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v3) (13.98 KB, patch)
2011-03-04 11:34 PST, Jia Pu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jia Pu 2011-03-03 15:27:36 PST
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>
Comment 1 Jia Pu 2011-03-03 15:57:36 PST
Created attachment 84642 [details]
Proposed patch (v1)
Comment 2 Darin Adler 2011-03-03 16:45:54 PST
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.
Comment 3 Jia Pu 2011-03-03 17:05:36 PST
(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.
Comment 4 Jia Pu 2011-03-03 19:33:19 PST
(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.
Comment 5 Jia Pu 2011-03-04 09:58:46 PST
Created attachment 84767 [details]
Proposed patch (v2)
Comment 6 Darin Adler 2011-03-04 10:43:09 PST
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?
Comment 7 Jia Pu 2011-03-04 11:34:57 PST
Created attachment 84786 [details]
Patch (v3)
Comment 8 WebKit Commit Bot 2011-03-04 22:10:42 PST
The commit-queue encountered the following flaky tests while processing attachment 84786 [details]:

inspector/audits/audits-panel-functional.html bug 55776 (authors: apavlov@chromium.org, pfeldman@chromium.org, and rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 9 WebKit Commit Bot 2011-03-04 22:12:51 PST
Comment on attachment 84786 [details]
Patch (v3)

Clearing flags on attachment: 84786

Committed r80408: <http://trac.webkit.org/changeset/80408>
Comment 10 WebKit Commit Bot 2011-03-04 22:12:57 PST
All reviewed patches have been landed.  Closing bug.