Bug 86591 – [Refactoring] Move platform-specific code in Editor::respondToChangedSelection to the WebKit layer

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hironori Bono]

Greetings,

Unfortunately, Editor::respondToChangedSelection() has platform-specific code that is hard to understand it as listed below. We should move this code to the WebKit layer for better readability.

  #if (!PLATFORM(MAC) && !PLATFORM(CHROMIUM)) || (PLATFORM(MAC) && (defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD)))
  #if PLATFORM(CHROMIUM)
          if (!m_frame->settings() || !m_frame->settings()->asynchronousSpellCheckingEnabled()) {
              if (RefPtr<Range> wordRange = newAdjacentWords.toNormalizedRange())
                  m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::Spelling);
          }
  #else
          // This only erases markers that are in the first unit (word or sentence) of the selection.
          // Perhaps peculiar, but it matches AppKit on these Mac OS X versions.
          if (RefPtr<Range> wordRange = newAdjacentWords.toNormalizedRange())
              m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::Spelling);
  #endif
  #endif

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 142188 [details]
The proposed solution 1

Greetings,

I have written a change that adds a virtual function TextCheckerClient::shouldEraseMarkersAfterChangeSelection to remove these #if,..,#endif blocks. I would like to set r? to try this change on EWS bots to check if they can compile this change. (I have verified I can compile this change on Chromium, Mac, Win, and Qt, though.)

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 142413 [details]
The proposed solution 2 (copied a comment)

Greetings Niwa-san,

Thanks for your review. I would like to update this change to add a simplified version of the comment in Editor::respondToChangedSelection() to WebKit/mac and WebKit2. Is it OK to land this change when EWS bots compile it?

Regards,

Hironori Bono

[Ryosuke Niwa]

Comment on attachment 142413 [details]
The proposed solution 2 (copied a comment)

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

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:704
> +    // This prevents erasing spelling markers on OS X Lion or later to match AppKit on these Mac OS X versions.
> +#if !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> +    return type != TextCheckingTypeSpelling;
> +#else
> +    return true;
> +#endif

On my second thought, it's not great that this code is duplicated in WebKit and WebKit2. Is there any way to share this code?

[Ryosuke Niwa]

+ap, +darin, +enrica for they might know ways to share code between WebKit/mac and WebKit2/mac.

[Alexey Proskuryakov]

I don't think that there is a great way to do this. Generally, pushing down to WebCore works best, however is seems appropriate to have policy decisions like this implemented in WebKit.

[Ryosuke Niwa]

Comment on attachment 142413 [details]
The proposed solution 2 (copied a comment)

(In reply to comment #5)
> I don't think that there is a great way to do this. Generally, pushing down to WebCore works best, however is seems appropriate to have policy decisions like this implemented in WebKit.

Okay. I guess we'll have to duplicate the code then.

[WebKit Review Bot]

Comment on attachment 142413 [details]
The proposed solution 2 (copied a comment)

Clearing flags on attachment: 142413

Committed r117470: <http://trac.webkit.org/changeset/117470>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.