Bug 56324 – EventHandler calls shouldChangeSelection needlessly

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

[Ryosuke Niwa]

Currently, EventHandler::updateSelectionForMouseDrag calls shouldChangeSelection regardless of whether current selection is different from new selection or not.  We should not call shouldChangeSelection when no change is needed.

[Ryosuke Niwa]

Created attachment 85746 [details]
fixes the bug

[Darin Adler]

Comment on attachment 85746 [details]
fixes the bug

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

> Source/WebCore/page/EventHandler.cpp:662
> -    if (m_frame->selection()->shouldChangeSelection(newSelection)) {
> -        m_frame->selection()->setIsDirectional(false);
> -        m_frame->selection()->setSelection(newSelection, m_frame->selection()->granularity(), MakeNonDirectionalSelection);
> -    }
> +    setNonDirectionalSelectionIfNeeded(m_frame->selection(), newSelection, m_frame->selection()->granularity());

What does the setIsDirectional(false) work in the new code?

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 85746 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85746&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:662
> > -    if (m_frame->selection()->shouldChangeSelection(newSelection)) {
> > -        m_frame->selection()->setIsDirectional(false);
> > -        m_frame->selection()->setSelection(newSelection, m_frame->selection()->granularity(), MakeNonDirectionalSelection);
> > -    }
> > +    setNonDirectionalSelectionIfNeeded(m_frame->selection(), newSelection, m_frame->selection()->granularity());
> 
> What does the setIsDirectional(false) work in the new code?

The first thing SelectionController::setSelection does is to call setIsDirectional(directionalityPolicy == MakeDirectionalSelection) so call to setIsDirectional here was completely redundant.

[Ryosuke Niwa]

Having said that, there's a behavioral change that after this patch WebKit won't change previously directional selection to non-directional in EventHandler but that's rarely an issue because I don't think users nor scripts can make such changes.

[Ryosuke Niwa]

(In reply to comment #4)
>that's rarely
should read "that shouldn't be"

[Ryosuke Niwa]

Committed r81095: <http://trac.webkit.org/changeset/81095>