Add a wrapper for setSelection that also fires selectionchange event. We intend to migrate most of calls to setSelection to this new version.
The feature probably needs to be under an ifdef during development, so that no one accidentally ships selectionchange that _sometimes_ works.
(In reply to comment #1) > The feature probably needs to be under an ifdef during development, so that no one accidentally ships selectionchange that _sometimes_ works. Yes, that is the plan. I'm currently working on that. I have wrapped all the code I've added so far by ENABLE(SELECTIONCHANGE_EVENT) but I can't figure out how to enable it locally. Is there a command-line option to build-webkit? Or do I define the constant in XCode?
The default is in WebCore/Configurations/FeatureDefines.xcconfig. Command line options can be added to build-webkit script.
Created attachment 71785 [details] Patch
Created attachment 73159 [details] implements setSelectionFiringEvent with a test
Comment on attachment 73159 [details] implements setSelectionFiringEvent with a test +void SelectionController::setSelectionFiringEvent(const VisibleSelection& s, bool closeTyping, bool shouldClearTypingStyle, bool userTriggered, CursorAlignOnScroll align, TextGranularity granularity, DirectionalityPolicy directionalityPolicy) I've been reading this as "set SelectionFiring event" until I saw the implementation. Maybe setSelectionWithEvent()? Or combine the options into a structure, and add that to it? +#if ENABLE(SELECTIONCHANGE_EVENT) + m_frame->document()->dispatchEvent(Event::create(eventNames().selectionchangeEvent, false, false)); +#endif Can m_frame be null? SelectionController::setSelection() has a null check for it. - m_frame->selection()->setSelection(newSelection, granularity, MakeNonDirectionalSelection); + m_frame->selection()->setSelectionFiringEvent(newSelection, granularity, MakeNonDirectionalSelection); I don't know how to review such changes for security impact. +FAIL: double clicking first word of "<a href="javascript:false">hello</a>" yielded 0 selectionchange events but expected 2 events The test is not enabled on any platforms, why check in a FAIL result? That should be a at least explained in ChangeLog.
Comment on attachment 73159 [details] implements setSelectionFiringEvent with a test The naming here is not great. The normal version of the function is the one that sends events. The current version of the function which does not send events is the “buggy” one. I think first step here should be renaming the current one to include the word “deprecated” or “without sending events” or both. We don’t want to leave a rename as the last step if we can help it.
(In reply to comment #6) > (From update of attachment 73159 [details]) > I've been reading this as "set SelectionFiring event" until I saw the implementation. Maybe setSelectionWithEvent()? Or combine the options into a structure, and add that to it? Oh yeah, that's a good point now that you pointed out. I can't step reading it that way. Will fix. > +#if ENABLE(SELECTIONCHANGE_EVENT) > + m_frame->document()->dispatchEvent(Event::create(eventNames().selectionchangeEvent, false, false)); > +#endif > > Can m_frame be null? SelectionController::setSelection() has a null check for it. Will fix. > - m_frame->selection()->setSelection(newSelection, granularity, MakeNonDirectionalSelection); > + m_frame->selection()->setSelectionFiringEvent(newSelection, granularity, MakeNonDirectionalSelection); > > I don't know how to review such changes for security impact. I added a comment to each test case indicating which case corresponds to which. The only function I haven't been able to test is selectClosestWordOrLinkFromMouseEvent. I ran all layout tests with ASSERT_NOT_REACHED but it never hit the assert. It's quite possible that it's dead code. > +FAIL: double clicking first word of "<a href="javascript:false">hello</a>" yielded 0 selectionchange events but expected 2 events > > The test is not enabled on any platforms, why check in a FAIL result? That should be a at least explained in ChangeLog. I'll just remove this test. (In reply to comment #7) > (From update of attachment 73159 [details]) > The naming here is not great. The normal version of the function is the one that sends events. The current version of the function which does not send events is the “buggy” one. I think first step here should be renaming the current one to include the word “deprecated” or “without sending events” or both. Will rename setSelection to deprecatedSetSelection. I'd like to keep the name setSelectionWithEvent for this very first patch. Because if I had used setSelection in this patch, reviewers need to go through every single file to make sure I caught them all. Calling it setSelectionWithEvent will avoid this because it'll show up as a compilation error if I had forgotten any.
Created attachment 73295 [details] fixed per comments
Attachment 73295 [details] did not build on chromium: Build output: http://queues.webkit.org/results/5431038