Bug 109404 – Add selectTrailingWhitespaceEnabled setting to WebCore::Page

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

[Manuel Rego Casasnovas]

Some tests are skipped in WK2 due to missing layoutTestController.setSelectTrailingWhitespaceEnabled

[Manuel Rego Casasnovas]

Created attachment 187520 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 187520 [details]
Patch

It depends on bug #107840 but it can be already reviewed. Anyway, it needs to wait for #107840 before landing.

[Manuel Rego Casasnovas]

Created attachment 190594 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190594 [details]
Patch

Attachment 190594 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16795141

New failing tests:
editing/spelling/spelling-double-clicked-word-with-underscores.html
editing/selection/doubleclick-beside-cr-span.html
editing/spelling/spelling-double-clicked-word.html
editing/selection/doubleclick-whitespace.html

[Build Bot]

Comment on attachment 190594 [details]
Patch

Attachment 190594 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16822343

New failing tests:
compositing/checkerboard.html
accessibility/anchor-linked-anonymous-block-crash.html
compositing/absolute-inside-out-of-view-fixed.html
animations/3d/matrix-transform-type-animation.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/cache/history-only-cached-subresource-loads.html
compositing/bounds-in-flipped-writing-mode.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/iframe-304-crash.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
http/tests/cache/cached-main-resource.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Build Bot]

Comment on attachment 190594 [details]
Patch

Attachment 190594 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16772546

New failing tests:
editing/selection/doubleclick-beside-cr-span.html
editing/selection/doubleclick-whitespace.html

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 190594 [details])
> Attachment 190594 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/16772546
> 
> New failing tests:
> editing/selection/doubleclick-beside-cr-span.html
> editing/selection/doubleclick-whitespace.html

Did you forget to update these tests or DumpRenderTree?

[Ryosuke Niwa]

Comment on attachment 190594 [details]
Patch

I'm gonna say r- due to failing tests.

[Manuel Rego Casasnovas]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 190594 [details] [details])
> > Attachment 190594 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-commit-queue.appspot.com/results/16772546
> > 
> > New failing tests:
> > editing/selection/doubleclick-beside-cr-span.html
> > editing/selection/doubleclick-whitespace.html
> 
> Did you forget to update these tests or DumpRenderTree?

Yes I forgot to change DumpRenderTree. I was just testing it in WK2.

Anway if I'm not wrong we have another prolbem as now tests are using internals.settings.setSelectTrailingWhitespaceEnabled.

This means that DRT code related to selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled is not used at all. As we're directly establishing the Page setting with internals.

So, in WK2 everything works due to my patches, however in WK1 the implementations of selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled are not using the new setting in Page yet, so that's the reason it's faling.

I guess that the solution should be:
1) Update WK1 code to use Page settings for selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled. I guess it should be done in a generic way and remove the specific  ports code related to selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled.
2) Remove code related to selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled from DRT.

What do you think?

[Ryosuke Niwa]

(In reply to comment #9)
> I guess that the solution should be:
> 1) Update WK1 code to use Page settings for selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled. I guess it should be done in a generic way and remove the specific  ports code related to selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled.
> 2) Remove code related to selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled from DRT.

We should probably do both of these in one patch. It might make sense to do this prior to introducing WK2 bits.

[Manuel Rego Casasnovas]

(In reply to comment #10)
> (In reply to comment #9)
> > I guess that the solution should be:
> > 1) Update WK1 code to use Page settings for selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled. I guess it should be done in a generic way and remove the specific  ports code related to selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled.
> > 2) Remove code related to selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled from DRT.
> 
> We should probably do both of these in one patch. It might make sense to do this prior to introducing WK2 bits.

Yes I agree, so first I'd try to provide a patch doing:
* Include the new setting "selectTrailingWhitespaceEnabled" in WebCore::Page.
* Change WK1 code to use the new settings selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled for all the ports that have this already implemented.
* Remove code related to these settings in DRT (reviewing the implementation of the different ports too).
* Change tests to use internals.

Then in a separated patch, I'd add the WK2 bits that were already included in the attached patch.

Thanks for the feedback, I hope to come with something in this line the sooner the better :-)

[Grzegorz Czajkowski]

(In reply to comment #10)
> (In reply to comment #9)
> > I guess that the solution should be:
> > 1) Update WK1 code to use Page settings for selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled. I guess it should be done in a generic way and remove the specific  ports code related to selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled.
> > 2) Remove code related to selectTrailingWhitespaceEnabled and smartInsertDeleteEnabled from DRT.
> 
> We should probably do both of these in one patch. It might make sense to do this prior to introducing WK2 bits.

This change's likely to enable spell checking tests added in r141354 for wk2 platforms.

[Manuel Rego Casasnovas]

Created attachment 191435 [details]
Patch

In this patch I'm trying to do all that was described in comment #11 but the WK2 part. As I cannot test all the platforms, I'll use the EWSs to check if I'm doing it right or not. Not marking for review yet, till I'm not sure it doesn't break any platform.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Early Warning System Bot]

Comment on attachment 191435 [details]
Patch

Attachment 191435 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17050030

[Manuel Rego Casasnovas]

Created attachment 191439 [details]
Patch

I forgot some parenthesis in qt and mac ports in previous patch. I guess that's the reason why qt EWS failed.

[Build Bot]

Comment on attachment 191439 [details]
Patch

Attachment 191439 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17050074

[Manuel Rego Casasnovas]

Created attachment 191460 [details]
Patch

Try to fix compilation issues for mac port.

[Build Bot]

Comment on attachment 191460 [details]
Patch

Attachment 191460 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17050111

[Manuel Rego Casasnovas]

Created attachment 191478 [details]
Patch

Init setting in mac from NSUserDefaults in _commonInitializationWithFrameName method.

[Build Bot]

Comment on attachment 191478 [details]
Patch

Attachment 191478 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17048137

[Manuel Rego Casasnovas]

Created attachment 191497 [details]
Patch

Let's see if mac compilation is fixed now.

[Build Bot]

Comment on attachment 191497 [details]
Patch

Attachment 191497 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16985131

[Manuel Rego Casasnovas]

Created attachment 191512 [details]
Patch

So this time I forgot an 's' in mac code, let's see if now it works properly and sorry for the noise.

[Adam Barth]

Comment on attachment 191512 [details]
Patch

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

> Source/WebKit/chromium/public/WebViewClient.h:-190
> -    virtual bool isSmartInsertDeleteEnabled() { return true; }
> -    virtual bool isSelectTrailingWhitespaceEnabled() { return true; }

Is it ok to remove these public APIs from the Chromium port?

[Adam Barth]

Comment on attachment 191512 [details]
Patch

r- pending explanation about the change to Chromium's public API.

[Manuel Rego Casasnovas]

(In reply to comment #25)
> (From update of attachment 191512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191512&action=review
> 
> > Source/WebKit/chromium/public/WebViewClient.h:-190
> > -    virtual bool isSmartInsertDeleteEnabled() { return true; }
> > -    virtual bool isSelectTrailingWhitespaceEnabled() { return true; }
> 
> Is it ok to remove these public APIs from the Chromium port?

I agree that remove the API here and not add it in WebSettings.h doesn't seem right. I'll try to explain the issue deeply in the next paragraphs.

Now smartInsertDeleteEnabled and selectTrailingWhitespacesEnabled are going to be settings of WebCore::Page, this implies that in chromium there'll be 2 ways to control them since there will be yet WebViewClient::isSmartInsertDeleteEnabled and WebViewClient::isSelectTrailingWhitespaceEnabled.

So I guess that we have 2 options for chromium port:
a) Use smartInsertDeleteEnabled and selectTrailingWhitespacesEnabled from page settings:
  * This will imply changing the public API:
    * Removing WebViewClient::isSmartInsertDeleteEnabled and WebViewClient::isSelectTrailingWhitespaceEnabled.
    * Adding WebSettings::setSmartInsertDeleteEnabled and WebSettings::setSelectTrailingWhitespaceEnabled.
  * Anyway, I'm not sure if this should be here or in a separate patch. Maybe, chromium port prefers to plan the migration and keep working with WebViewClient API for a while. But I guess that eventually this change should be done.
b) Keep using WebViewClient API and try to fix DRT to use the page settings:
  * So, no changes in Source/WebKit/chromium/ would be needed.
  * DRT should still keep some code related to smartInsertDeleteEnabled and selectTrailingWhitespacesEnabled trying to use directly the page settings. If this is not possible, the related tests would fail in chromium port, as now they'll use internals.settings.setSmartInsertDeleteEnabled (which change directly the page setting) instead of testRunner.setSmartInsertDeleteEnabled and the page setting won't be used at all under Source/WebKit/chromium/.
  * Plan the migration for the future. I could provide a patch doing the changes described in option a) in a different bug and I guess it would be eventually integrated.

I'll upload a patch with the first solution (option a)), however I'm open to change whatever is needed following what chromium port people decide regarding to this issue. Moreover, I can be completely wrong in my assumptions so please let me know.

[Manuel Rego Casasnovas]

Created attachment 191692 [details]
Patch

New patch trying to implement option a) described in comment #27.

[Tony Chang]

Comment on attachment 191692 [details]
Patch

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

> Source/WebCore/page/Settings.in:199
> +# smartInsertDeleteEnabled and selectTrailingWhitespaceEnabled are mutually
> +# exclusive, meaning that enabling one will disable the other.

This seems like a new behavior and I'm not sure how you would enforce this for all ports.  For example, in Chromium, the current behavior is:

bool RenderViewImpl::isSmartInsertDeleteEnabled() {
#if defined(OS_MACOSX)
  return true;
#else
  return false;
#endif
}

bool RenderViewImpl::isSelectTrailingWhitespaceEnabled() {
#if defined(OS_WIN)
  return true;
#else
  return false;
#endif
}

Which means on Chromium Linux, both are false.

> Source/WebKit/chromium/public/WebSettings.h:150
> +    virtual void setSelectTrailingWhitespaceEnabled(bool) = 0;

I don't think we need to add an API for this since I don't think we plan on changing these during runtime.  The exception is for DRT, but that can use the WebCore Internals API.

> Source/WebKit/chromium/public/WebViewClient.h:-190
> -    virtual bool isSmartInsertDeleteEnabled() { return true; }
> -    virtual bool isSelectTrailingWhitespaceEnabled() { return true; }

The problem with removing this is that we'll no longer get the correct behavior in Chromium, which overrides these virtual methods-- see the RenderViewImpl snippet above.  As long as we retain the correct behavior, it's OK to remove this.

There are a couple ways you could do this, (1) Find a good time to set this (maybe WebViewImpl's constructor?) or (2) set the right default value in Settings.cpp (e.g., see defaultUnifiedTextCheckerEnabled in Settings.cpp and Settings.in).

[Manuel Rego Casasnovas]

(In reply to comment #29)
> (From update of attachment 191692 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191692&action=review
> 
> > Source/WebCore/page/Settings.in:199
> > +# smartInsertDeleteEnabled and selectTrailingWhitespaceEnabled are mutually
> > +# exclusive, meaning that enabling one will disable the other.
> 
> This seems like a new behavior and I'm not sure how you would enforce this for all ports.  For example, in Chromium, the current behavior is:
> 
> bool RenderViewImpl::isSmartInsertDeleteEnabled() {
> #if defined(OS_MACOSX)
>   return true;
> #else
>   return false;
> #endif
> }
> 
> bool RenderViewImpl::isSelectTrailingWhitespaceEnabled() {
> #if defined(OS_WIN)
>   return true;
> #else
>   return false;
> #endif
> }
> 
> Which means on Chromium Linux, both are false.

Maybe in Chromium is different, but I included the comment because of it's what is explained in WebCore::Editor:
    // smartInsertDeleteEnabled and selectTrailingWhitespaceEnabled are 
    // mutually exclusive, meaning that enabling one will disable the other.
    bool smartInsertDeleteEnabled();
    bool isSelectTrailingWhitespaceEnabled();

Besides, the ports allowing to change these settings (like mac or win) where changing the other setting to the opposite value when a setting was established. Anyway I can remove the comment here if this is not true for all the ports.
 
> > Source/WebKit/chromium/public/WebSettings.h:150
> > +    virtual void setSelectTrailingWhitespaceEnabled(bool) = 0;
> 
> I don't think we need to add an API for this since I don't think we plan on changing these during runtime.  The exception is for DRT, but that can use the WebCore Internals API.

Yes for DRT I'm already using the internals.

> > Source/WebKit/chromium/public/WebViewClient.h:-190
> > -    virtual bool isSmartInsertDeleteEnabled() { return true; }
> > -    virtual bool isSelectTrailingWhitespaceEnabled() { return true; }
> 
> The problem with removing this is that we'll no longer get the correct behavior in Chromium, which overrides these virtual methods-- see the RenderViewImpl snippet above.  As long as we retain the correct behavior, it's OK to remove this.
> 
> There are a couple ways you could do this, (1) Find a good time to set this (maybe WebViewImpl's constructor?) or (2) set the right default value in Settings.cpp (e.g., see defaultUnifiedTextCheckerEnabled in Settings.cpp and Settings.in).

I'll provide a patch following the 2nd approach. Thanks for the feedback.

[Manuel Rego Casasnovas]

Created attachment 191944 [details]
Patch

[Tony Chang]

Comment on attachment 191944 [details]
Patch

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

This looks OK for the Chromium port.  I'm not sure if it's OK for all the other ports, although it doesn't look like you're removing any APIs.  Hmm, you might want someone to verify the changes to the mac port to verify that the NSUserDefaults behavior is being retained.

> Source/WebCore/page/Settings.in:199
> +# smartInsertDeleteEnabled and selectTrailingWhitespaceEnabled are mutually
> +# exclusive, meaning that enabling one will disable the other.

I would remove this comment since it doesn't seem to be accurate anymore.

[Ryosuke Niwa]

Comment on attachment 191944 [details]
Patch

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

> Source/WebKit/chromium/src/EditorClientImpl.cpp:110
>  #if OS(WINDOWS)
>      return true;
>  #else

I would remove this if-def and return false if I were you since it shouldn't matter what we return in that case.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:248
> +        return true;

This should return false.

[Manuel Rego Casasnovas]

Created attachment 192064 [details]
Patch

Fixed issues explained in comment #32 and comment #33. Thanks for the review.

[Manuel Rego Casasnovas]

Adding people from other ports to CC to verify that they're ok with the patch. Please, let me know if you find any issue or have any comment. Thanks.

[WebKit Review Bot]

Comment on attachment 192064 [details]
Patch

Attachment 192064 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17068240

New failing tests:
editing/pasteboard/smart-paste-001.html
editing/pasteboard/subframe-dragndrop-1.html
editing/pasteboard/4944770-2.html
editing/pasteboard/smart-paste-005.html
editing/pasteboard/smart-paste-007.html
editing/pasteboard/smart-drag-drop.html
editing/pasteboard/smart-paste-008.html
fast/lists/drag-into-marker.html
editing/pasteboard/smart-paste-002.html
editing/pasteboard/smart-paste-003.html
editing/pasteboard/smart-paste-004.html
fast/events/ondragenter.html
editing/pasteboard/drag-drop-list.html
editing/pasteboard/drag-drop-modifies-page.html
editing/deleting/smart-delete-004.html
editing/deleting/smart-delete-003.html

[Manuel Rego Casasnovas]

Created attachment 192163 [details]
Patch

Trying to fix patches in Chromium port, it seems that with the new default values for Chromium they're not passing anymore. I've set smartInsertDeleteEnabled and selectTrailingWhitespaceEnabled manually in the failing tests to check if this fix the problem.

[Tony Chang]

Comment on attachment 192163 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp:-417
> -    m_smartInsertDeleteEnabled = true;
> -#ifdef WIN32
> -    m_selectTrailingWhitespaceEnabled = true;
> -#else
> -    m_selectTrailingWhitespaceEnabled = false;
> -#endif

The Chromium test failures are because Chromium DRT has these default values, which are different from the Chromium browser defaults.  Calling the setter methods in the tests that failed will cause Chromium Win to probably fail some tests.  E.g., editing/pasteboard/smart-paste-00[78].html have different platform results (these tests are from before the testrunner methods setSmartInsertDeleteEnabled and setSelectTrailingWhitespaceEnabled when we had different baselines per platform).

I am sorry this is so complicated.

I think the right thing to do for chromium is in Tools/DumpRenderTree/chromium/TestRunner/src/WebPreferences.cpp, add these defaults in applyTo().

In the long term, we probably want to convert these tests to test both behavior and to have a shared expected result, but that's probably better as a separate patch.

[Manuel Rego Casasnovas]

(In reply to comment #38)
> (From update of attachment 192163 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192163&action=review
> 
> > Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp:-417
> > -    m_smartInsertDeleteEnabled = true;
> > -#ifdef WIN32
> > -    m_selectTrailingWhitespaceEnabled = true;
> > -#else
> > -    m_selectTrailingWhitespaceEnabled = false;
> > -#endif
> 
> The Chromium test failures are because Chromium DRT has these default values, which are different from the Chromium browser defaults.  Calling the setter methods in the tests that failed will cause Chromium Win to probably fail some tests.  E.g., editing/pasteboard/smart-paste-00[78].html have different platform results (these tests are from before the testrunner methods setSmartInsertDeleteEnabled and setSelectTrailingWhitespaceEnabled when we had different baselines per platform).

Yes, I understand it. I didn't realized that the defaults for DRT and browser are different.

> I am sorry this is so complicated.

Don't worry and thank you very much for taking care of reviewing it. I understand that as Chromium port doesn't use the default values, things gets more complicated.

> I think the right thing to do for chromium is in Tools/DumpRenderTree/chromium/TestRunner/src/WebPreferences.cpp, add these defaults in applyTo().

Ok, I'll provide a patch reverting the changes in the tests my last patch and setting the defaults in WebPreferences.cpp.

[Manuel Rego Casasnovas]

Created attachment 192278 [details]
Patch

[WebKit Review Bot]

Comment on attachment 192278 [details]
Patch

Attachment 192278 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17014321

[WebKit Review Bot]

Comment on attachment 192278 [details]
Patch

Attachment 192278 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17016318

[Peter Beverloo (cr-android ews)]

Comment on attachment 192278 [details]
Patch

Attachment 192278 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17109219

[Manuel Rego Casasnovas]

Created attachment 192283 [details]
Patch

Adding WebSettings API required to set the DRT default values.

[Build Bot]

Comment on attachment 192283 [details]
Patch

Attachment 192283 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17004222

[Manuel Rego Casasnovas]

(In reply to comment #45)
> (From update of attachment 192283 [details])
> Attachment 192283 [details] did not pass win-ews (win):
> Output: http://webkit-commit-queue.appspot.com/results/17004222

The crashes don't seem related to this patch.

[Tony Chang]

Comment on attachment 192283 [details]
Patch

Chromium LGTM.

[Manuel Rego Casasnovas]

(In reply to comment #47)
> (From update of attachment 192283 [details])
> Chromium LGTM.

Great. Thanks for the review. Let's see what Mac people thought and maybe it could land soon :-)

[Manuel Rego Casasnovas]

Comment on attachment 192283 [details]
Patch

Reset commit‑queue flag as the patch is unrelated with the crashes in win EWS.

[Manuel Rego Casasnovas]

(In reply to comment #33)
> (From update of attachment 191944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191944&action=review
> 
> > Source/WebKit/chromium/src/EditorClientImpl.cpp:110
> >  #if OS(WINDOWS)
> >      return true;
> >  #else
> 
> I would remove this if-def and return false if I were you since it shouldn't matter what we return in that case.
> 
> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:248
> > +        return true;
> 
> This should return false.

Ok, this 2 explicit issues were already fixed in the current patches.

However, as we talked by IRC you were also asking about the "return true;" in all the smartInsertDeleteEnabled() methods in the EditorClients of every port.

I'm going to change it in a new patch for "return false;" as I agree with you that it doesn't matter and returning true has not too much sense. I just did it initially because of they were the default values, but that's already defined in Settings.

[Manuel Rego Casasnovas]

Created attachment 192999 [details]
Patch

New patch only changing "return true;" by "return false;" in smartInsertDeleteEnabled() method of each EditorClient.

[Ryosuke Niwa]

Comment on attachment 192999 [details]
Patch

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

r=me provided the following comments are addressed.

> Source/WebKit/mac/WebView/WebView.mm:789
> +        _private->page->settings()->setSmartInsertDeleteEnabled(smartInsertDeleteEnabled);
> +        _private->page->settings()->setSelectTrailingWhitespaceEnabled(!smartInsertDeleteEnabled);

Why don't we call [self setSelectTrailingWhitespaceEnabled] here.

> Source/WebKit/mac/WebView/WebView.mm:2481
> +        // Set smartInsertDeleteEnabled as they are mutually exclusive.

I don't think this and similar comments are useful as the code clearly indicates they're mutually exclusive already. Please remove them.

[Manuel Rego Casasnovas]

(In reply to comment #52)
> (From update of attachment 192999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192999&action=review
> 
> r=me provided the following comments are addressed.
> 
> > Source/WebKit/mac/WebView/WebView.mm:789
> > +        _private->page->settings()->setSmartInsertDeleteEnabled(smartInsertDeleteEnabled);
> > +        _private->page->settings()->setSelectTrailingWhitespaceEnabled(!smartInsertDeleteEnabled);
> 
> Why don't we call [self setSelectTrailingWhitespaceEnabled] here.

I guess that as the key is called "WebSmartInsertDeleteEnabled" it's clearer if we use setSmartInsertDeleteEnabled:
    if ([[NSUserDefaults standardUserDefaults] objectForKey:WebSmartInsertDeleteEnabled])                
        [self setSmartInsertDeleteEnabled:[[NSUserDefaults standardUserDefaults] boolForKey:WebSmartInsertDeleteEnabled]];

> > Source/WebKit/mac/WebView/WebView.mm:2481
> > +        // Set smartInsertDeleteEnabled as they are mutually exclusive.
> 
> I don't think this and similar comments are useful as the code clearly indicates they're mutually exclusive already. Please remove them.

Yep. I'll provide a new patch with this 2 changes. Thanks for the review.

[Manuel Rego Casasnovas]

Created attachment 193009 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 193013 [details]
Patch

Rebased patch against trunk as previous patch didn't apply.

[Early Warning System Bot]

Comment on attachment 193013 [details]
Patch

Attachment 193013 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17149563

[Early Warning System Bot]

Comment on attachment 193013 [details]
Patch

Attachment 193013 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17147225

[Manuel Rego Casasnovas]

Created attachment 193076 [details]
Patch

Fixed compilation issues in Qt port after rebasing.

[WebKit Review Bot]

Comment on attachment 193076 [details]
Patch

Clearing flags on attachment: 193076

Committed r145849: <http://trac.webkit.org/changeset/145849>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alberto Garcia]

Created attachment 193507 [details]
Fix BlackBerry build.

This patch breaks the BlackBerry build. Here's the fix.

[Alberto Garcia]

> This patch breaks the BlackBerry build. Here's the fix.

I'll file a new bug for this.

[Roger Fong]

http://trac.webkit.org/changeset/146097
Fix for Windows DRT.