Bug 112362

Created attachment 193148 [details] Patch

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

Comment on attachment 193148 [details] Patch Let's wait for EWS

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.

Created attachment 193159 [details] Patch

Comment on attachment 193159 [details] Patch Removed unnecessary teardown and setup from InternalSettings.{h,cpp}. Removed unnecessary teardowns from layout tests.

Comment on attachment 193159 [details] Patch Deleting code is awesome. Let's let the cq run the tests.

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

Comment on attachment 193159 [details] Patch Yes, thanks for reminding me.

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

Created attachment 193344 [details] Patch for landing

Comment on attachment 193344 [details] Patch for landing Attempting to merge, please do not flip the cq bit yet.

Created attachment 193347 [details] Patch for landing

Comment on attachment 193347 [details] Patch for landing This patch is merged correctly and should be okay to land.

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

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.

Created attachment 193356 [details] Patch for landing

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

Comment on attachment 193356 [details] Patch for landing Clearing flags on attachment: 193356 Committed r145940: <http://trac.webkit.org/changeset/145940>

All reviewed patches have been landed. Closing bug.

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

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