Bug 109404 - Add selectTrailingWhitespaceEnabled setting to WebCore::Page
Summary: Add selectTrailingWhitespaceEnabled setting to WebCore::Page
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: 107840
Blocks: 108528
  Show dependency treegraph
 
Reported: 2013-02-11 00:06 PST by Manuel Rego Casasnovas
Modified: 2013-03-18 11:58 PDT (History)
25 users (show)

See Also:


Attachments
Patch (19.13 KB, patch)
2013-02-11 00:27 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (19.18 KB, patch)
2013-02-27 13:38 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (81.75 KB, patch)
2013-03-05 01:32 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (81.76 KB, patch)
2013-03-05 01:58 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (81.90 KB, patch)
2013-03-05 04:09 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (81.90 KB, patch)
2013-03-05 05:55 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (82.10 KB, patch)
2013-03-05 08:02 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (82.10 KB, patch)
2013-03-05 10:55 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (85.12 KB, patch)
2013-03-06 02:03 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (82.83 KB, patch)
2013-03-07 01:19 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (82.78 KB, patch)
2013-03-07 12:15 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (95.20 KB, patch)
2013-03-07 23:47 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (83.86 KB, patch)
2013-03-08 13:28 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (87.76 KB, patch)
2013-03-08 14:29 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (87.79 KB, patch)
2013-03-13 14:53 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (87.30 KB, patch)
2013-03-13 15:27 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (86.93 KB, patch)
2013-03-13 15:49 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (87.20 KB, patch)
2013-03-13 23:44 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Fix BlackBerry build. (1.85 KB, patch)
2013-03-18 02:17 PDT, Alberto Garcia
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-02-11 00:06:51 PST
Some tests are skipped in WK2 due to missing layoutTestController.setSelectTrailingWhitespaceEnabled
Comment 1 Manuel Rego Casasnovas 2013-02-11 00:27:32 PST
Created attachment 187520 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2013-02-19 02:33:05 PST
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.
Comment 3 Manuel Rego Casasnovas 2013-02-27 13:38:43 PST
Created attachment 190594 [details]
Patch
Comment 4 WebKit Review Bot 2013-02-27 14:31:03 PST
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
Comment 5 Build Bot 2013-02-28 03:02:07 PST
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
Comment 6 Build Bot 2013-02-28 03:18:45 PST
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
Comment 7 Ryosuke Niwa 2013-02-28 03:21:19 PST
(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?
Comment 8 Ryosuke Niwa 2013-02-28 03:21:43 PST
Comment on attachment 190594 [details]
Patch

I'm gonna say r- due to failing tests.
Comment 9 Manuel Rego Casasnovas 2013-02-28 05:33:12 PST
(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?
Comment 10 Ryosuke Niwa 2013-02-28 11:44:31 PST
(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.
Comment 11 Manuel Rego Casasnovas 2013-02-28 14:17:49 PST
(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 :-)
Comment 12 Grzegorz Czajkowski 2013-03-01 07:51:38 PST
(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.
Comment 13 Manuel Rego Casasnovas 2013-03-05 01:32:13 PST
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.
Comment 14 WebKit Review Bot 2013-03-05 01:33:54 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 15 Early Warning System Bot 2013-03-05 01:46:34 PST
Comment on attachment 191435 [details]
Patch

Attachment 191435 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17050030
Comment 16 Manuel Rego Casasnovas 2013-03-05 01:58:09 PST
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.
Comment 17 Build Bot 2013-03-05 03:38:57 PST
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
Comment 18 Manuel Rego Casasnovas 2013-03-05 04:09:06 PST
Created attachment 191460 [details]
Patch

Try to fix compilation issues for mac port.
Comment 19 Build Bot 2013-03-05 05:18:02 PST
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
Comment 20 Manuel Rego Casasnovas 2013-03-05 05:55:32 PST
Created attachment 191478 [details]
Patch

Init setting in mac from NSUserDefaults in _commonInitializationWithFrameName method.
Comment 21 Build Bot 2013-03-05 07:18:58 PST
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
Comment 22 Manuel Rego Casasnovas 2013-03-05 08:02:28 PST
Created attachment 191497 [details]
Patch

Let's see if mac compilation is fixed now.
Comment 23 Build Bot 2013-03-05 10:08:00 PST
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
Comment 24 Manuel Rego Casasnovas 2013-03-05 10:55:48 PST
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.
Comment 25 Adam Barth 2013-03-05 13:11:13 PST
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?
Comment 26 Adam Barth 2013-03-05 13:12:15 PST
Comment on attachment 191512 [details]
Patch

r- pending explanation about the change to Chromium's public API.
Comment 27 Manuel Rego Casasnovas 2013-03-06 01:46:50 PST
(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.
Comment 28 Manuel Rego Casasnovas 2013-03-06 02:03:59 PST
Created attachment 191692 [details]
Patch

New patch trying to implement option a) described in comment #27.
Comment 29 Tony Chang 2013-03-06 11:26:21 PST
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).
Comment 30 Manuel Rego Casasnovas 2013-03-07 01:11:24 PST
(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.
Comment 31 Manuel Rego Casasnovas 2013-03-07 01:19:10 PST
Created attachment 191944 [details]
Patch
Comment 32 Tony Chang 2013-03-07 10:54:10 PST
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.
Comment 33 Ryosuke Niwa 2013-03-07 11:36:24 PST
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.
Comment 34 Manuel Rego Casasnovas 2013-03-07 12:15:47 PST
Created attachment 192064 [details]
Patch

Fixed issues explained in comment #32 and comment #33. Thanks for the review.
Comment 35 Manuel Rego Casasnovas 2013-03-07 12:16:47 PST
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.
Comment 36 WebKit Review Bot 2013-03-07 19:55:48 PST
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
Comment 37 Manuel Rego Casasnovas 2013-03-07 23:47:33 PST
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.
Comment 38 Tony Chang 2013-03-08 10:50:38 PST
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.
Comment 39 Manuel Rego Casasnovas 2013-03-08 13:26:23 PST
(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.
Comment 40 Manuel Rego Casasnovas 2013-03-08 13:28:05 PST
Created attachment 192278 [details]
Patch
Comment 41 WebKit Review Bot 2013-03-08 13:55:03 PST
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
Comment 42 WebKit Review Bot 2013-03-08 13:55:23 PST
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
Comment 43 Peter Beverloo (cr-android ews) 2013-03-08 14:26:16 PST
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
Comment 44 Manuel Rego Casasnovas 2013-03-08 14:29:06 PST
Created attachment 192283 [details]
Patch

Adding WebSettings API required to set the DRT default values.
Comment 45 Build Bot 2013-03-11 01:27:00 PDT
Comment on attachment 192283 [details]
Patch

Attachment 192283 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17004222
Comment 46 Manuel Rego Casasnovas 2013-03-11 02:40:10 PDT
(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.
Comment 47 Tony Chang 2013-03-11 11:00:30 PDT
Comment on attachment 192283 [details]
Patch

Chromium LGTM.
Comment 48 Manuel Rego Casasnovas 2013-03-11 13:09:46 PDT
(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 :-)
Comment 49 Manuel Rego Casasnovas 2013-03-12 01:33:14 PDT
Comment on attachment 192283 [details]
Patch

Reset commit‑queue flag as the patch is unrelated with the crashes in win EWS.
Comment 50 Manuel Rego Casasnovas 2013-03-13 14:49:47 PDT
(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.
Comment 51 Manuel Rego Casasnovas 2013-03-13 14:53:31 PDT
Created attachment 192999 [details]
Patch

New patch only changing "return true;" by "return false;" in smartInsertDeleteEnabled() method of each EditorClient.
Comment 52 Ryosuke Niwa 2013-03-13 15:04:29 PDT
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.
Comment 53 Manuel Rego Casasnovas 2013-03-13 15:26:28 PDT
(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.
Comment 54 Manuel Rego Casasnovas 2013-03-13 15:27:53 PDT
Created attachment 193009 [details]
Patch
Comment 55 Manuel Rego Casasnovas 2013-03-13 15:49:33 PDT
Created attachment 193013 [details]
Patch

Rebased patch against trunk as previous patch didn't apply.
Comment 56 Early Warning System Bot 2013-03-13 16:09:06 PDT
Comment on attachment 193013 [details]
Patch

Attachment 193013 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17149563
Comment 57 Early Warning System Bot 2013-03-13 16:19:19 PDT
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
Comment 58 Manuel Rego Casasnovas 2013-03-13 23:44:00 PDT
Created attachment 193076 [details]
Patch

Fixed compilation issues in Qt port after rebasing.
Comment 59 WebKit Review Bot 2013-03-14 14:56:34 PDT
Comment on attachment 193076 [details]
Patch

Clearing flags on attachment: 193076

Committed r145849: <http://trac.webkit.org/changeset/145849>
Comment 60 WebKit Review Bot 2013-03-14 14:56:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 61 Alberto Garcia 2013-03-18 02:17:51 PDT
Created attachment 193507 [details]
Fix BlackBerry build.

This patch breaks the BlackBerry build. Here's the fix.
Comment 62 Alberto Garcia 2013-03-18 04:09:41 PDT
> This patch breaks the BlackBerry build. Here's the fix.

I'll file a new bug for this.
Comment 63 Roger Fong 2013-03-18 11:58:43 PDT
http://trac.webkit.org/changeset/146097
Fix for Windows DRT.