Created attachment 195136 [details] patch TextCheckerMac (which lives in WebKit2) should use WebKit2 preference keys.
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.
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.
Created attachment 195410 [details] patch
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?!
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.
Comment on attachment 195410 [details] patch Attachment 195410 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17256499
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
Created attachment 195419 [details] patch v2 patch after Tim’s comments and build fixes
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?
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.
(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?