Bug 65221 – Add RenderedPosition

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

[Ryosuke Niwa]

As proposed during the contributor's meeting, we should add RenderedPosition.  I wasn't sure if this will be a good refactoring but patches for the bug 60910 and my recent works on hit testing code convinced me that this will be useful.

https://docs.google.com/document/d/1vzzi_wQHO0GtLnu-1IYutUKbcpAYlHqZwQlwbfLuowM/edit?hl=en_US&authkey=CLOa9cgN

[Ryosuke Niwa]

Created attachment 102077 [details]
adds the skeleton

[Ryosuke Niwa]

Comment on attachment 102077 [details]
adds the skeleton

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

> Source/WebCore/editing/RenderedPosition.h:47
> +    RenderedPosition(const Position&, EAffinity);
> +    RenderedPosition(const VisiblePosition&);

I should make these explicit.

[Ryosuke Niwa]

Created attachment 102081 [details]
adds the skeleton; made some constructors explicit

[Ryosuke Niwa]

Ping reviewers

[Hajime Morrita]

Comment on attachment 102081 [details]
adds the skeleton; made some constructors explicit

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

> Source/WebCore/editing/RenderedPosition.h:53
> +    RenderObject* m_renderer;

Do we need m_renderer? It loks we can retrieve this from m_inlineBox.

[Ryosuke Niwa]

Comment on attachment 102081 [details]
adds the skeleton; made some constructors explicit

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

>> Source/WebCore/editing/RenderedPosition.h:53
>> +    RenderObject* m_renderer;
> 
> Do we need m_renderer? It loks we can retrieve this from m_inlineBox.

We don't in the current version but we will in the future where we'll have an empty visible block nodes.  e.g. <div></div><div></div>

[Hajime Morrita]

Comment on attachment 102081 [details]
adds the skeleton; made some constructors explicit

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

> Source/WebCore/editing/RenderedPosition.h:47
> +    explicit RenderedPosition(const VisiblePosition&);

Do we need both? Current code covers only the second version.

>>> Source/WebCore/editing/RenderedPosition.h:53
>>> +    RenderObject* m_renderer;
>> 
>> Do we need m_renderer? It loks we can retrieve this from m_inlineBox.
> 
> We don't in the current version but we will in the future where we'll have an empty visible block nodes.  e.g. <div></div><div></div>

That makes sense.

> Source/WebCore/editing/visible_units.cpp:327
> +static inline RootInlineBox* rootBoxForLine(const VisiblePosition& currentPosition)

It looks we no longer need this function. RenderedPosition(currentPosition).rootBox() looks concise and descriptive enough.

[Ryosuke Niwa]

Created attachment 102535 [details]
Removed unnecessary constructor

[Ryosuke Niwa]

Ping reviewers.

[Ryosuke Niwa]

Thanks for the review!

[WebKit Review Bot]

Comment on attachment 102535 [details]
Removed unnecessary constructor

Clearing flags on attachment: 102535

Committed r92286: <http://trac.webkit.org/changeset/92286>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.