Bug 112426

Created attachment 193277 [details] Patch

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.

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

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.

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 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?

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

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

Created attachment 202059 [details] Patch Uploaded new patch without the changes in TestExpectations file that will be done later in a separated patch.

Please unskip tests as appropriate.

Comment on attachment 202059 [details] Patch Clearing flags on attachment: 202059 Committed r150289: <http://trac.webkit.org/changeset/150289>

All reviewed patches have been landed. Closing bug.