Bug 112426 - [WK2] Add support for selectTrailingWhitespaceEnabled setting
Summary: [WK2] Add support for selectTrailingWhitespaceEnabled setting
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 108528
  Show dependency treegraph
 
Reported: 2013-03-15 04:16 PDT by Manuel Rego Casasnovas
Modified: 2013-05-17 12:50 PDT (History)
6 users (show)

See Also:


Attachments
Patch (10.04 KB, patch)
2013-03-15 04:24 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (9.60 KB, patch)
2013-04-01 04:21 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2013-05-17 04:20 PDT, Manuel Rego Casasnovas
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 2013-03-15 04:16:53 PDT
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.
Comment 1 Manuel Rego Casasnovas 2013-03-15 04:24:18 PDT
Created attachment 193277 [details]
Patch
Comment 2 Tim Horton 2013-03-22 18:49:23 PDT
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.
Comment 3 Manuel Rego Casasnovas 2013-03-25 02:25:49 PDT
(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.
Comment 4 Manuel Rego Casasnovas 2013-04-01 04:21:12 PDT
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.
Comment 5 Manuel Rego Casasnovas 2013-05-15 22:45:39 PDT
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).
Comment 6 Benjamin Poulain 2013-05-15 23:05:39 PDT
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?
Comment 7 Manuel Rego Casasnovas 2013-05-16 00:50:47 PDT
(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.
Comment 8 Benjamin Poulain 2013-05-17 00:50:14 PDT
If the tests are unrelated changes, they should not be in the same patch.
Comment 9 Manuel Rego Casasnovas 2013-05-17 04:20:20 PDT
Created attachment 202059 [details]
Patch

Uploaded new patch without the changes in TestExpectations file that will be done later in a separated patch.
Comment 10 Ryosuke Niwa 2013-05-17 12:29:13 PDT
Please unskip tests as appropriate.
Comment 11 WebKit Commit Bot 2013-05-17 12:50:13 PDT
Comment on attachment 202059 [details]
Patch

Clearing flags on attachment: 202059

Committed r150289: <http://trac.webkit.org/changeset/150289>
Comment 12 WebKit Commit Bot 2013-05-17 12:50:16 PDT
All reviewed patches have been landed.  Closing bug.