Bug 113332 – Have TextCheckerMac use WebKit2 preference keys for TextChecker state

• Component: WebKit2
• Status: NEW

[Roger Fong]

Created attachment 195136 [details]
patch

TextCheckerMac (which lives in WebKit2) should use WebKit2 preference keys.

[Tim Horton]

Comment on attachment 195136 [details]
patch

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

> Source/WebKit2/UIProcess/mac/TextCheckerMac.mm:40
>  static NSString* const WebSmartInsertDeleteEnabled = @"WebSmartInsertDeleteEnabled";

It is weird:

a) that we had these preferences with their weird WK1 names. Is anything expecting them to be usable?
b) that we aren't updating all of them at once.

[WebKit Review Bot]

Attachment 195136 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/mac/TextCheckerMac.mm']" exit_code: 1
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Roger Fong]

Created attachment 195410 [details]
patch

[Tim Horton]

Comment on attachment 195410 [details]
patch

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

> Source/WebKit2/ChangeLog:10
> +		WebKit2 TextChecker was not using WebKit2 preference keys when determining TextChecker states.
> +		This patch adds new web preferences and has TextChecker use those in initializing and setting state.
> +		The old keys were also being queried directly from NSUserDefaults instead of going through WebPreferences.

Why is this indented.

> Source/WebKit2/ChangeLog:45
> +        (WebKit):
> +        * UIProcess/API/mac/WKView.mm:
> +        (-[WKView toggleContinuousSpellChecking:]):
> +        (-[WKView setGrammarCheckingEnabled:]):
> +        (-[WKView toggleGrammarChecking:]):
> +        (-[WKView toggleAutomaticSpellingCorrection:]):
> +        (-[WKView toggleSmartInsertDelete:]):
> +        (-[WKView setAutomaticQuoteSubstitutionEnabled:]):
> +        (-[WKView toggleAutomaticQuoteSubstitution:]):
> +        (-[WKView setAutomaticDashSubstitutionEnabled:]):
> +        (-[WKView toggleAutomaticDashSubstitution:]):
> +        (-[WKView setAutomaticLinkDetectionEnabled:]):
> +        (-[WKView toggleAutomaticLinkDetection:]):
> +        (-[WKView setAutomaticTextReplacementEnabled:]):
> +        (-[WKView toggleAutomaticTextReplacement:]):
> +        * UIProcess/WebPageProxy.cpp:
> +        (WebKit::WebPageProxy::initializeWebPage):
> +        (WebKit::WebPageProxy::initializeTextCheckerState):
> +        (WebKit):
> +        * UIProcess/WebPageProxy.h:
> +        (WebPageProxy):
> +        * UIProcess/mac/TextCheckerMac.mm:
> +        (WebKit):
> +        (WebKit::TextChecker::state):
> +        (WebKit::TextChecker::setContinuousSpellCheckingEnabled):
> +        (WebKit::TextChecker::setGrammarCheckingEnabled):
> +        (WebKit::TextChecker::setAutomaticSpellingCorrectionEnabled):
> +        (WebKit::TextChecker::setAutomaticQuoteSubstitutionEnabled):
> +        (WebKit::TextChecker::setAutomaticDashSubstitutionEnabled):
> +        (WebKit::TextChecker::setAutomaticLinkDetectionEnabled):
> +        (WebKit::TextChecker::setAutomaticTextReplacementEnabled):
> +        (WebKit::TextChecker::isSmartInsertDeleteEnabled):
> +        (WebKit::TextChecker::setSmartInsertDeleteEnabled):

You should make sue of this area!

> Source/WebKit2/UIProcess/API/mac/WKView.mm:66
> +#import "WebPageGroup.h"

Please reorder these.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:886
> +    bool automaticSpellingCorrection = !TextChecker::state().isAutomaticSpellingCorrectionEnabled;

Please add an Enabled suffix to all of these (smartInsertDelete etc also)

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3427
> +void WebPageProxy::initializeTextCheckerState()

Maybe update instead of initialize? It gets called more than once (if preferences change).

Or at least it's supposed to... where did that code go?!

[WebKit Review Bot]

Attachment 195410 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/WebPreferencesStore.h', u'Source/WebKit2/UIProcess/API/mac/WKView.mm', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/UIProcess/mac/TextCheckerMac.mm']" exit_code: 1
Source/WebKit2/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 195410 [details]
patch

Attachment 195410 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17256499

[Early Warning System Bot]

Comment on attachment 195410 [details]
patch

Attachment 195410 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17305287

[Roger Fong]

Created attachment 195419 [details]
patch v2

patch after Tim’s comments and build fixes

[Sam Weinig]

It seems like there is a mismatch here.  TextChecker is a global, but we are setting it based on non-global settings.  Maybe we need to make TextChecker per-page?

[Roger Fong]

I think I'm going to stick with just having everything being set globally.
I'm not sure anymore if having a TextChecker state as a WebPreference is the right thing to do anymore.

I was initially confused about TextChecker directly using the NSUserDefaults and thought that I had to make the default into a WebPreference but perhaps it's better to keep things the way they are and use the NSUserDefault for now.

It might be nice to be able to have different TextCheckerStates/TextCheckers per WebPage but I feel like that would require a lot of architectural changes that would affect lots of other platforms as well.

[Sam Weinig]

(In reply to comment #10)
> I think I'm going to stick with just having everything being set globally.
> I'm not sure anymore if having a TextChecker state as a WebPreference is the right thing to do anymore.
> 
> I was initially confused about TextChecker directly using the NSUserDefaults and thought that I had to make the default into a WebPreference but perhaps it's better to keep things the way they are and use the NSUserDefault for now.
> 
> It might be nice to be able to have different TextCheckerStates/TextCheckers per WebPage but I feel like that would require a lot of architectural changes that would affect lots of other platforms as well.

I am unclear what bug you are trying to fix then?  Can you explain?