Some low hanging fruits here.
Created attachment 103538 [details] refactoring
Comment on attachment 103538 [details] refactoring Attachment 103538 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9344306
Comment on attachment 103538 [details] refactoring Attachment 103538 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9344307
Created attachment 103539 [details] fixed build failure
Comment on attachment 103539 [details] fixed build failure View in context: https://bugs.webkit.org/attachment.cgi?id=103539&action=review > Source/WebCore/dom/Position.h:131 > + Element* rootEditableElement() const? > Source/WebCore/dom/Range.h:106 > + void setStart(const Position&, ExceptionCode&); You can give ASSERT_NO_EXCEPTION for the default value of the second parameter.
Created attachment 104112 [details] Made Position::rootEditableElement() const
Comment on attachment 104112 [details] Made Position::rootEditableElement() const OK once all bots pass.
Thanks for the review!
Comment on attachment 104112 [details] Made Position::rootEditableElement() const Clearing flags on attachment: 104112 Committed r93199: <http://trac.webkit.org/changeset/93199>
All reviewed patches have been landed. Closing bug.
Comment on attachment 104112 [details] Made Position::rootEditableElement() const View in context: https://bugs.webkit.org/attachment.cgi?id=104112&action=review > Source/WebCore/dom/Range.cpp:287 > +void Range::setStart(const Position& start, ExceptionCode& ec) > +{ > + Position parentAnchored = start.parentAnchoredEquivalent(); > + setStart(parentAnchored.containerNode(), parentAnchored.offsetInContainerNode(), ec); > +} > + > +void Range::setEnd(const Position& end, ExceptionCode& ec) > +{ > + Position parentAnchored = end.parentAnchoredEquivalent(); > + setStart(parentAnchored.containerNode(), parentAnchored.offsetInContainerNode(), ec); > +} Do callers really need the exception capability? Instead could we just have these fail silently or assert?
Comment on attachment 104112 [details] Made Position::rootEditableElement() const View in context: https://bugs.webkit.org/attachment.cgi?id=104112&action=review >> Source/WebCore/dom/Range.cpp:287 >> +} > > Do callers really need the exception capability? Instead could we just have these fail silently or assert? We can do that. Or pass ASSERT_NO_EXCEPTION by default as morrita suggested.
Generally, the ExceptionCode& thing should only be used for actual DOM binding since it’s how the DOM cis specified. Not internal use, because it’s an awkward way to report errors in C++. ASSERT_NO_EXCEPTION is a fine way to make a function that’s intended for DOM binding easier to use internally. But a function intended for user internally probably should not even use ExceptionCode&.