Bug 112362 – Move setAsynchronousSpellCheckingEnabled to internals.settings

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Rouslan Solomakhin]

Move setAsynchronousSpellCheckingEnabled to internals.settings

[Rouslan Solomakhin]

Created attachment 193148 [details]
Patch

[Rouslan Solomakhin]

Tony: This is the move of setAsynchronousSpellCheckingEnabled to internals.settings that you requested in https://bugs.webkit.org/show_bug.cgi?id=108370#c15.

[Ryosuke Niwa]

Comment on attachment 193148 [details]
Patch

Let's wait for EWS

[Tony Chang]

Comment on attachment 193148 [details]
Patch

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

> Source/WebCore/testing/InternalSettings.cpp:78
> +    , m_originalUnifiedSpellCheckerEnabled(settings->unifiedTextCheckerEnabled())
> +    , m_originalAsynchronousSpellCheckingEnabled(settings->asynchronousSpellCheckingEnabled())

You don't need to add this.  The generated code from Settings.in already does the backup/restore for you.

> Source/WebCore/testing/InternalSettings.cpp:117
> +    settings->setUnifiedTextCheckerEnabled(m_originalUnifiedSpellCheckerEnabled);
> +    settings->setAsynchronousSpellCheckingEnabled(m_originalAsynchronousSpellCheckingEnabled);

You should be able to delete this too.

> Source/WebCore/testing/InternalSettings.h:64
>          bool m_originalUnifiedSpellCheckerEnabled;
> +        bool m_originalAsynchronousSpellCheckingEnabled;

And be able to delete these too.

> LayoutTests/editing/spelling/grammar-markers-hidpi.html:30
> +        if (window.internals)
> +            internals.settings.setAsynchronousSpellCheckingEnabled(false);

Is this trying to reset the value?  The test harness should do that automatically and we shouldn't need this code.  Can you try removing this and seeing if tests that run afterwards fail?

> LayoutTests/editing/spelling/grammar-markers.html:27
> +        if (window.internals)
> +            internals.settings.setAsynchronousSpellCheckingEnabled(false);

Ditto.

> LayoutTests/editing/spelling/grammar-paste.html:52
> +    internals.settings.setAsynchronousSpellCheckingEnabled(false);

Ditto.

> LayoutTests/editing/spelling/script-tests/spellcheck-paste.js:43
> +    internals.settings.setAsynchronousSpellCheckingEnabled(false);

Ditto.

> LayoutTests/editing/spelling/spellcheck-async-mutation.html:82
> +    if (window.internals) {
>          internals.settings.setUnifiedTextCheckerEnabled(false);
> +        internals.settings.setAsynchronousSpellCheckingEnabled(false);
> +    }

You should be able to remove this whole block.

> LayoutTests/editing/spelling/spellcheck-async.html:63
> +    if (window.internals) {
>          internals.settings.setUnifiedTextCheckerEnabled(false);
> +        internals.settings.setAsynchronousSpellCheckingEnabled(false);
> +    }

You should be able to remove this whole block.

> LayoutTests/editing/spelling/spellcheck-paste-disabled.html:54
> +    internals.settings.setAsynchronousSpellCheckingEnabled(false);

Remove.

> LayoutTests/editing/spelling/spellcheck-queue.html:84
> +    if (window.internals)
> +        internals.settings.setAsynchronousSpellCheckingEnabled(false);

Remove.

> LayoutTests/editing/spelling/spellcheck-sequencenum.html:81
> +    if (window.internals) {
> +        internals.settings.setAsynchronousSpellCheckingEnabled(false);

Remove.

> LayoutTests/editing/spelling/spelling-marker-description.html:15
> +    internals.settings.setAsynchronousSpellCheckingEnabled(false);

Remove.

> LayoutTests/platform/chromium/editing/spelling/delete-misspelled-word.html:40
> +    internals.settings.setAsynchronousSpellCheckingEnabled(false);

Remove.

> LayoutTests/platform/chromium/editing/spelling/move-cursor-to-misspelled-word.html:21
> +internals.settings.setAsynchronousSpellCheckingEnabled(false);

Remove.

[Rouslan Solomakhin]

Created attachment 193159 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 193159 [details]
Patch

Removed unnecessary teardown and setup from InternalSettings.{h,cpp}. Removed unnecessary teardowns from layout tests.

[Tony Chang]

Comment on attachment 193159 [details]
Patch

Deleting code is awesome.  Let's let the cq run the tests.

[Rouslan Solomakhin]

(In reply to comment #7)
> (From update of attachment 193159 [details])
> Deleting code is awesome.  Let's let the cq run the tests.

EWS bots are all green. Should we flip the CQ bit?

[Tony Chang]

Comment on attachment 193159 [details]
Patch

Yes, thanks for reminding me.

[WebKit Review Bot]

Comment on attachment 193159 [details]
Patch

Rejecting attachment 193159 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 193159, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
.html
patching file LayoutTests/editing/spelling/spellcheck-sequencenum.html
patching file LayoutTests/editing/spelling/spelling-marker-description.html
patching file LayoutTests/platform/chromium/editing/spelling/delete-misspelled-word.html
patching file LayoutTests/platform/chromium/editing/spelling/move-cursor-to-misspelled-word.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Tony Chang']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/17186271

[Rouslan Solomakhin]

Created attachment 193344 [details]
Patch for landing

[Rouslan Solomakhin]

Comment on attachment 193344 [details]
Patch for landing

Attempting to merge, please do not flip the cq bit yet.

[Rouslan Solomakhin]

Created attachment 193347 [details]
Patch for landing

[Rouslan Solomakhin]

Comment on attachment 193347 [details]
Patch for landing

This patch is merged correctly and should be okay to land.

[WebKit Review Bot]

Comment on attachment 193347 [details]
Patch for landing

Rejecting attachment 193347 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'validate-changelog', '--non-interactive', 193347, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17217240

[Ryosuke Niwa]

Comment on attachment 193347 [details]
Patch for landing

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

> LayoutTests/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

You need to update this line.

[Rouslan Solomakhin]

Created attachment 193356 [details]
Patch for landing

[Rouslan Solomakhin]

Good catch, thank you. I've updated the reviewer line.

[WebKit Review Bot]

Comment on attachment 193356 [details]
Patch for landing

Clearing flags on attachment: 193356

Committed r145940: <http://trac.webkit.org/changeset/145940>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

This caused three editing/spellchecker failures on windows:
http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r146097%20(33689)/results.html

[Rouslan Solomakhin]

(In reply to comment #21)
> This caused three editing/spellchecker failures on windows:
> http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r146097%20(33689)/results.html

Would we need to rollout this change?