Bug 107840

Created attachment 184546 [details] Patch

Comment on attachment 184546 [details] Patch Attachment 184546 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16076938

Comment on attachment 184546 [details] Patch Attachment 184546 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16084809

Created attachment 184589 [details] Patch

Why don't we make this a setting? We can then auto-generate code in internals.

Comment on attachment 184589 [details] Patch Attachment 184589 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16097599 New failing tests: svg/batik/text/textLayout2.svg svg/batik/text/smallFonts.svg

Created attachment 184770 [details] Patch

Comment on attachment 184770 [details] Patch Attachment 184770 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16110833

Comment on attachment 184770 [details] Patch Attachment 184770 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16116458

Created attachment 184971 [details] Patch

Comment on attachment 184971 [details] Patch Attachment 184971 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16160549

Created attachment 184973 [details] Patch

Comment on attachment 184973 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184973&action=review r- for not using Settings.in. This seems to give us 2 ways to control SmartInsertDelete since Chromium still has WebViewClient::isSmartInsertDeleteEnabled. Maybe that's OK as long as there's a transition plan. > Source/WebCore/page/Settings.h:307 > + void setSmartInsertDeleteEnabled(bool); > + bool smartInsertDeleteEnabled() { return m_smartInsertDeleteEnabled; } Please use Settings.in, which will generate all the Settings code for you (your code doesn't bit pack properly). > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:929 > +void TestRunner::setSmartInsertDeleteEnabled(bool enabled) > +{ > + WKBundlePageSetSmartInsertDeleteEnabled(InjectedBundle::shared().page()->page(), enabled); > +} If you use Settings.in, it will automatically add this setter to internals.settings.setSmartInsertDeleteEnabled.

Created attachment 185048 [details] Patch

Comment on attachment 185048 [details] Patch r=me but we should probably have some WK2 owner take a look at this before we land it.

Comment on attachment 185048 [details] Patch Looks like this breaks initial value of the setting on Mac. We used to get it from "WebSmartInsertDeleteEnabled" user default in TextChecker::isSmartInsertDeleteEnabled(), and pass on process creation. Now, looks like every client has to call WKPreferencesSetSmartInsertDeleteEnabled(). Even worse, if WKPreferencesSetSmartInsertDeleteEnabled() gets called by the client, it is a no-op because of an early return: void WebPageProxy::setSmartInsertDeleteEnabled(bool isSmartInsertDeleteEnabled) { if (m_isSmartInsertDeleteEnabled == isSmartInsertDeleteEnabled) return; So, this means that WKPreferencesSetSmartInsertDeleteEnabled(false) will have no effect on Mac if WebSmartInsertDeleteEnabled was set to NO. r- because of this. Please feel free to reset the review flag if I misunderstood the code.

(In reply to comment #16) > (From update of attachment 185048 [details]) > Looks like this breaks initial value of the setting on Mac. We used to get it from "WebSmartInsertDeleteEnabled" user default in TextChecker::isSmartInsertDeleteEnabled(), and pass on process creation. Now, looks like every client has to call WKPreferencesSetSmartInsertDeleteEnabled(). > > Even worse, if WKPreferencesSetSmartInsertDeleteEnabled() gets called by the client, it is a no-op because of an early return: > > void WebPageProxy::setSmartInsertDeleteEnabled(bool isSmartInsertDeleteEnabled) > { > if (m_isSmartInsertDeleteEnabled == isSmartInsertDeleteEnabled) > return; > > So, this means that WKPreferencesSetSmartInsertDeleteEnabled(false) will have no effect on Mac if WebSmartInsertDeleteEnabled was set to NO. Yes, you're right thanks for pointing it out. The problem is that in my patch I'm removing isSmartInsertDeleteEnabled from page creation parameters, so the initial value on Mac is lost as you say. I'm going to attach a new patch with a small change in order to set the new setting in WebPageProxy::initializeWebPage using the initial value from TextChecker::isSmartInsertDeleteEnabled().

Created attachment 186570 [details] Patch

Comment on attachment 186570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186570&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:429 > +#if PLATFORM(MAC) > + m_pageGroup->preferences()->setSmartInsertDeleteEnabled(m_isSmartInsertDeleteEnabled); > +#endif > + > m_process->send(Messages::WebProcess::CreateWebPage(m_pageID, creationParameters()), 0); Won't this result in a preferences update message being sent before creation message?

Created attachment 186781 [details] Patch Fix the problem sending a preferences update message before the creation page message.

Ping. What do you think about the last version of the patch?

Comment on attachment 186781 [details] Patch It looks great but perhaps you need a WebKit2 owner's review.

*** Bug 99436 has been marked as a duplicate of this bug. ***

Comment on attachment 186781 [details] Patch Clearing flags on attachment: 186781 Committed r144221: <http://trac.webkit.org/changeset/144221>

All reviewed patches have been landed. Closing bug.