Bug 107840 - Add smartInsertDeleteEnabled setting to WebCore::Page
Summary: Add smartInsertDeleteEnabled setting to WebCore::Page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 99436 (view as bug list)
Depends on:
Blocks: 108528 109404
  Show dependency treegraph
 
Reported: 2013-01-24 09:50 PST by Manuel Rego Casasnovas (Holidays - back 1st Sep)
Modified: 2013-02-27 12:22 PST (History)
12 users (show)

See Also:


Attachments
Patch (11.49 KB, patch)
2013-01-24 11:55 PST, Manuel Rego Casasnovas (Holidays - back 1st Sep)
no flags Details | Formatted Diff | Diff
Patch (12.56 KB, patch)
2013-01-24 15:11 PST, Manuel Rego Casasnovas (Holidays - back 1st Sep)
no flags Details | Formatted Diff | Diff
Patch (17.76 KB, patch)
2013-01-25 09:56 PST, Manuel Rego Casasnovas (Holidays - back 1st Sep)
no flags Details | Formatted Diff | Diff
Patch (20.40 KB, patch)
2013-01-28 04:51 PST, Manuel Rego Casasnovas (Holidays - back 1st Sep)
no flags Details | Formatted Diff | Diff
Patch (19.49 KB, patch)
2013-01-28 05:51 PST, Manuel Rego Casasnovas (Holidays - back 1st Sep)
no flags Details | Formatted Diff | Diff
Patch (16.42 KB, patch)
2013-01-28 13:25 PST, Manuel Rego Casasnovas (Holidays - back 1st Sep)
no flags Details | Formatted Diff | Diff
Patch (16.82 KB, patch)
2013-02-05 01:01 PST, Manuel Rego Casasnovas (Holidays - back 1st Sep)
no flags Details | Formatted Diff | Diff
Patch (16.97 KB, patch)
2013-02-06 01:15 PST, Manuel Rego Casasnovas (Holidays - back 1st Sep)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-01-24 09:50:46 PST
editing/deleting/smart-editing-disabled.html is skipped due to this reason.
Comment 1 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-01-24 11:55:36 PST
Created attachment 184546 [details]
Patch
Comment 2 Build Bot 2013-01-24 12:17:23 PST
Comment on attachment 184546 [details]
Patch

Attachment 184546 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16076938
Comment 3 Build Bot 2013-01-24 12:25:12 PST
Comment on attachment 184546 [details]
Patch

Attachment 184546 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16084809
Comment 4 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-01-24 15:11:21 PST
Created attachment 184589 [details]
Patch
Comment 5 Ryosuke Niwa 2013-01-24 15:15:28 PST
Why don't we make this a setting? We can then auto-generate code in internals.
Comment 6 WebKit Review Bot 2013-01-24 22:29:28 PST
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
Comment 7 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-01-25 09:56:31 PST
Created attachment 184770 [details]
Patch
Comment 8 Build Bot 2013-01-25 10:55:55 PST
Comment on attachment 184770 [details]
Patch

Attachment 184770 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16110833
Comment 9 Build Bot 2013-01-25 11:02:02 PST
Comment on attachment 184770 [details]
Patch

Attachment 184770 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16116458
Comment 10 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-01-28 04:51:04 PST
Created attachment 184971 [details]
Patch
Comment 11 Build Bot 2013-01-28 05:28:43 PST
Comment on attachment 184971 [details]
Patch

Attachment 184971 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16160549
Comment 12 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-01-28 05:51:24 PST
Created attachment 184973 [details]
Patch
Comment 13 Tony Chang 2013-01-28 10:42:44 PST
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.
Comment 14 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-01-28 13:25:54 PST
Created attachment 185048 [details]
Patch
Comment 15 Ryosuke Niwa 2013-02-01 00:26:29 PST
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 16 Alexey Proskuryakov 2013-02-01 09:59:19 PST
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.
Comment 17 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-02-05 00:59:41 PST
(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().
Comment 18 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-02-05 01:01:04 PST
Created attachment 186570 [details]
Patch
Comment 19 Alexey Proskuryakov 2013-02-05 10:03:46 PST
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?
Comment 20 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-02-06 01:15:23 PST
Created attachment 186781 [details]
Patch

Fix the problem sending a preferences update message before the creation page message.
Comment 21 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2013-02-18 10:16:46 PST
Ping. What do you think about the last version of the patch?
Comment 22 Ryosuke Niwa 2013-02-18 10:32:48 PST
Comment on attachment 186781 [details]
Patch

It looks great but perhaps you need a WebKit2 owner's review.
Comment 23 Grzegorz Czajkowski 2013-02-27 05:41:06 PST
*** Bug 99436 has been marked as a duplicate of this bug. ***
Comment 24 WebKit Review Bot 2013-02-27 12:22:33 PST
Comment on attachment 186781 [details]
Patch

Clearing flags on attachment: 186781

Committed r144221: <http://trac.webkit.org/changeset/144221>
Comment 25 WebKit Review Bot 2013-02-27 12:22:39 PST
All reviewed patches have been landed.  Closing bug.