Bug 65647 – Use RenderedPosition instead of getInlineBoxAndOffset in Editor and AccessibilityObject

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Since RenderedPosition has been added by r92286, we should encapsulate the use of getInlineBoxAndOffset by RenderedPosition.

[Ryosuke Niwa]

Created attachment 102839 [details]
cleanup

[Ryosuke Niwa]

Created attachment 102841 [details]
cleanup; fixed a bug

[WebKit Review Bot]

Comment on attachment 102841 [details]
cleanup; fixed a bug

Attachment 102841 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9301237

New failing tests:
editing/inserting/caret-position.html
fast/text/international/thai-cursor-position.html
platform/chromium-mac/editing/input/ime-candidate-window-position.html

[Ryosuke Niwa]

Created attachment 102855 [details]
cleanup; fixed another bug

[Hajime Morrita]

Comment on attachment 102855 [details]
cleanup; fixed another bug

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:295
> +        if (tempPosition.isNull() || tempPosition.isNull())

What is this for?

> Source/WebCore/editing/Editor.cpp:2739
> +    if (startCaretRect == LayoutRect() || endCaretRect == LayoutRect())

One "if" for each of both startCaretRect and endCaretRect looks more obvious - There is no need to compute both if the first one is empty.

> Source/WebCore/editing/RenderedPosition.cpp:40
> +static inline RenderObject* rendererFromPosition(const Position& position)

This might be better to a part of Position class.
... or we can even have Position::getInlineBoxAndOffsetAndRenderer(). 

Another idea is to define VisibilityPosition::toRendered() and Position::toRendered(EAffinity)

> Source/WebCore/editing/RenderedPosition.cpp:104
> +LayoutRect RenderedPosition::absoluteRect(int& extraWidthToEndOfLine)

Code duplication between two absoluteRect()s. Why not just making the out parameter optional? (As you know, pointer would allow that.)

> Source/WebCore/editing/RenderedPosition.h:53
> +    LayoutRect absoluteRect();

const?

> Source/WebCore/editing/RenderedPosition.h:54
> +    LayoutRect absoluteRect(int& extraWidthToEndOfLine);

const?

[Ryosuke Niwa]

Comment on attachment 102855 [details]
cleanup; fixed another bug

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:295
>> +        if (tempPosition.isNull() || tempPosition.isNull())
> 
> What is this for?

It's hard to tell but I'm preserving the original code that checked !tempPosition.deepEquivalent().deprecatedNode() below.

>> Source/WebCore/editing/Editor.cpp:2739

> 
> One "if" for each of both startCaretRect and endCaretRect looks more obvious - There is no need to compute both if the first one is empty.

Good point.  Will do.

>> Source/WebCore/editing/RenderedPosition.cpp:40
>> +static inline RenderObject* rendererFromPosition(const Position& position)
> 
> This might be better to a part of Position class.
> ... or we can even have Position::getInlineBoxAndOffsetAndRenderer(). 
> 
> Another idea is to define VisibilityPosition::toRendered() and Position::toRendered(EAffinity)

Right. In fact, we might be able to replace deprecatedNode with this function.

[Ryosuke Niwa]

Created attachment 103022 [details]
Fixed per comment

[Hajime Morrita]

Comment on attachment 103022 [details]
Fixed per comment

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

> Source/WebCore/editing/RenderedPosition.cpp:40
> +static inline RenderObject* rendererFromPosition(const Position& position)

Why not Position::renderer()?

> Source/WebCore/editing/RenderedPosition.cpp:95
> +LayoutRect RenderedPosition::absoluteRect() const

Could you just call  RenderedPosition::absoluteRect(int& extraWidthToEndOfLine) instead of having almost dupliated code?

[Ryosuke Niwa]

(In reply to comment #8)
> (From update of attachment 103022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103022&action=review
> 
> > Source/WebCore/editing/RenderedPosition.cpp:40
> > +static inline RenderObject* rendererFromPosition(const Position& position)
> 
> Why not Position::renderer()?

Because I don't think Position should be aware of renderer.  Also, any code that calls such a function is likely to be broken with first-letter.  As the whole point of RenderedPosition being isolating such such compelxity and dependency, I don't think I want to add such a method to Position.

> > Source/WebCore/editing/RenderedPosition.cpp:95
> > +LayoutRect RenderedPosition::absoluteRect() const
> 
> Could you just call  RenderedPosition::absoluteRect(int& extraWidthToEndOfLine) instead of having almost dupliated code?

Sure. Will do.

[Ryosuke Niwa]

Created attachment 103023 [details]
Merged two absoluteRects

[WebKit Review Bot]

Comment on attachment 103023 [details]
Merged two absoluteRects

Clearing flags on attachment: 103023

Committed r92451: <http://trac.webkit.org/changeset/92451>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.