Bug 112426 – [WK2] Add support for selectTrailingWhitespaceEnabled setting

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Manuel Rego Casasnovas]

Once bug #109404 has landed and as we agreed there this will include the WK2 part of the original patch.

The idea is to add support for the new WebCore::Page setting selectTrailingWhitespaceEnabled. Something similar to what we do in but #107840 for smartInsertDeleteEnabled.

[Manuel Rego Casasnovas]

Created attachment 193277 [details]
Patch

[Tim Horton]

Comment on attachment 193277 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3819
> +        setSelectTrailingWhitespaceEnabled(!enabled);

Is a strict 'not' right here?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3832
> +        setSmartInsertDeleteEnabled(!enabled);

Ditto.

[Manuel Rego Casasnovas]

(In reply to comment #2)
> (From update of attachment 193277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193277&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3819
> > +        setSelectTrailingWhitespaceEnabled(!enabled);
> 
> Is a strict 'not' right here?
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3832
> > +        setSmartInsertDeleteEnabled(!enabled);
> 
> Ditto.

The reason to do that is to keep the behavior of the different ports in WK1 where these 2 settings are mutually exclusive. The only exception is Chromium port, where in some situations these settings are not exclusive, however as Chromium didn't use WK2 I keep a similar approach to WK1 here.

Originally this comes from a comment in WebCore::Editor:
    // smartInsertDeleteEnabled and selectTrailingWhitespaceEnabled are 
    // mutually exclusive, meaning that enabling one will disable the other.
    bool smartInsertDeleteEnabled();
    bool isSelectTrailingWhitespaceEnabled();

Anyway maybe this is not accurate anymore, and we could remove the lines establishing the other setting and everyone using it should be responsible to update it (or not) accordingly.

Let me know what do you think and I can prepare a patch without these 2 lines if needed.

Thanks for the review.

[Manuel Rego Casasnovas]

Created attachment 195954 [details]
Patch

Uploading a new version of the patch without the lines establishing the related setting to the opposite value.\nI think that now it's a matter of deciding what's the preferred option: if we want to keep a similar behavior to WK1 (old patch) or not (new patch).\nI'm not marking as obsolete previous patch so we can decide what's the best approach and choose among them.

[Manuel Rego Casasnovas]

Ping. These patches have been ready for a while we just need to chose what's the preferred behavior for WK2 forcing smartInsertDeleteEnabled and selectTrailingWhitespaceEnabled to be mutually exclusive (first patch) or not (second patch).

[Benjamin Poulain]

Comment on attachment 195954 [details]
Patch

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

I am confused. How does added two preferences, but not use them anywhere, fixes the tests?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3842
> +bool WebPage::isSelectTrailingWhitespaceEnabled()
> +{
> +    return m_page->settings()->selectTrailingWhitespaceEnabled();
> +}
> +
> +void WebPage::setSelectTrailingWhitespaceEnabled(bool enabled)
> +{
> +    m_page->settings()->setSelectTrailingWhitespaceEnabled(enabled);
> +}
> +

This code looks unused, am I missing something?

[Manuel Rego Casasnovas]

(In reply to comment #6)
> (From update of attachment 195954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195954&action=review
> 
> I am confused. How does added two preferences, but not use them anywhere, fixes the tests?

The tests were already fixed in r144221 and r145849, where you can see changes like:

-    testRunner.setSmartInsertDeleteEnabled(false); 
-    internals.settings.setSmartInsertDeleteEnabled(false); 

or:

-     testRunner.setSelectTrailingWhitespaceEnabled(true);
+     internals.settings.setSelectTrailingWhitespaceEnabled(true);

This patch just adds support for selectTrailingWhitespaceEnabled in WK2 as smartInsertDeleteEnabled is already supported. In WK1 both are already supported.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3842
> > +bool WebPage::isSelectTrailingWhitespaceEnabled()
> > +{
> > +    return m_page->settings()->selectTrailingWhitespaceEnabled();
> > +}
> > +
> > +void WebPage::setSelectTrailingWhitespaceEnabled(bool enabled)
> > +{
> > +    m_page->settings()->setSelectTrailingWhitespaceEnabled(enabled);
> > +}
> > +
> 
> This code looks unused, am I missing something?

As I explained before it's used from the tests modified in r145849.

[Benjamin Poulain]

If the tests are unrelated changes, they should not be in the same patch.

[Manuel Rego Casasnovas]

Created attachment 202059 [details]
Patch

Uploaded new patch without the changes in TestExpectations file that will be done later in a separated patch.

[Ryosuke Niwa]

Please unskip tests as appropriate.

[WebKit Commit Bot]

Comment on attachment 202059 [details]
Patch

Clearing flags on attachment: 202059

Committed r150289: <http://trac.webkit.org/changeset/150289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.