Since RenderedPosition has been added by r92286, we should encapsulate the use of getInlineBoxAndOffset by RenderedPosition.
Created attachment 102839 [details] cleanup
Created attachment 102841 [details] cleanup; fixed a bug
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
Created attachment 102855 [details] cleanup; fixed another bug
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?
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.
Created attachment 103022 [details] Fixed per comment
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?
(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.
Created attachment 103023 [details] Merged two absoluteRects
Comment on attachment 103023 [details] Merged two absoluteRects Clearing flags on attachment: 103023 Committed r92451: <http://trac.webkit.org/changeset/92451>
All reviewed patches have been landed. Closing bug.