Bug 86591 - [Refactoring] Move platform-specific code in Editor::respondToChangedSelection to the WebKit layer
Summary: [Refactoring] Move platform-specific code in Editor::respondToChangedSelectio...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-16 01:00 PDT by Hironori Bono
Modified: 2012-05-17 11:44 PDT (History)
9 users (show)

See Also:


Attachments
The proposed solution 1 (28.40 KB, patch)
2012-05-16 01:27 PDT, Hironori Bono
rniwa: review+
Details | Formatted Diff | Diff
The proposed solution 2 (copied a comment) (28.57 KB, patch)
2012-05-16 22:23 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hironori Bono 2012-05-16 01:00:22 PDT
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
Comment 1 Hironori Bono 2012-05-16 01:27:55 PDT
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
Comment 2 Hironori Bono 2012-05-16 22:23:56 PDT
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
Comment 3 Ryosuke Niwa 2012-05-16 23:16:40 PDT
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?
Comment 4 Ryosuke Niwa 2012-05-16 23:17:39 PDT
+ap, +darin, +enrica for they might know ways to share code between WebKit/mac and WebKit2/mac.
Comment 5 Alexey Proskuryakov 2012-05-17 11:01:17 PDT
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.
Comment 6 Ryosuke Niwa 2012-05-17 11:07:43 PDT
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.
Comment 7 WebKit Review Bot 2012-05-17 11:43:55 PDT
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>
Comment 8 WebKit Review Bot 2012-05-17 11:44:03 PDT
All reviewed patches have been landed.  Closing bug.