Bug 107840 – Add smartInsertDeleteEnabled setting to WebCore::Page

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Manuel Rego Casasnovas]

editing/deleting/smart-editing-disabled.html is skipped due to this reason.

[Manuel Rego Casasnovas]

Created attachment 184546 [details]
Patch

[Build Bot]

Comment on attachment 184546 [details]
Patch

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

[Build Bot]

Comment on attachment 184546 [details]
Patch

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

[Manuel Rego Casasnovas]

Created attachment 184589 [details]
Patch

[Ryosuke Niwa]

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

[WebKit Review Bot]

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

[Manuel Rego Casasnovas]

Created attachment 184770 [details]
Patch

[Build Bot]

Comment on attachment 184770 [details]
Patch

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

[Build Bot]

Comment on attachment 184770 [details]
Patch

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

[Manuel Rego Casasnovas]

Created attachment 184971 [details]
Patch

[Build Bot]

Comment on attachment 184971 [details]
Patch

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

[Manuel Rego Casasnovas]

Created attachment 184973 [details]
Patch

[Tony Chang]

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.

[Manuel Rego Casasnovas]

Created attachment 185048 [details]
Patch

[Ryosuke Niwa]

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.

[Alexey Proskuryakov]

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.

[Manuel Rego Casasnovas]

(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().

[Manuel Rego Casasnovas]

Created attachment 186570 [details]
Patch

[Alexey Proskuryakov]

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?

[Manuel Rego Casasnovas]

Created attachment 186781 [details]
Patch

Fix the problem sending a preferences update message before the creation page message.

[Manuel Rego Casasnovas]

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

[Ryosuke Niwa]

Comment on attachment 186781 [details]
Patch

It looks great but perhaps you need a WebKit2 owner's review.

[Grzegorz Czajkowski]

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

[WebKit Review Bot]

Comment on attachment 186781 [details]
Patch

Clearing flags on attachment: 186781

Committed r144221: <http://trac.webkit.org/changeset/144221>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.