Bug 88238

Created attachment 145600 [details] Patch Implementation of global selection in editor, and move Chromium and QtWebkit(1) to the new API. Note that for Qt WebKit2 still doesn't respond to middle mouse-button, but now does set the selection so that pasting out of QtWebKit2 is possible.

Ryosuke can you have a look at this?

Comment on attachment 145600 [details] Patch Not for review yet.

Comment on attachment 145600 [details] Patch Attachment 145600 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12906035

Comment on attachment 145600 [details] Patch Attachment 145600 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12901232

Comment on attachment 145600 [details] Patch Attachment 145600 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12903234

Comment on attachment 145600 [details] Patch Attachment 145600 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12893383 New failing tests: editing/pasteboard/paste-text-009.html editing/pasteboard/pasting-empty-html-falls-back-to-text.html editing/pasteboard/copy-backslash-with-euc.html editing/pasteboard/paste-table-cells.html editing/pasteboard/smart-paste-008.html editing/pasteboard/paste-text-005.html editing/pasteboard/paste-table-003.html editing/execCommand/find-after-replace.html editing/pasteboard/paste-text-002.html editing/style/apply-through-end-of-document.html editing/pasteboard/paste-text-003.html editing/pasteboard/paste-text-008.html

Created attachment 145627 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 145600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145600&action=review > Source/WebCore/ChangeLog:10 > + Implement a new EditorCommand for pasting the global selection. > + Update the global the selection whenever a selection of the right > + type is set. What is "the global selection"? Could you elaborate on that? > Source/WebCore/editing/EditorCommand.cpp:939 > +#if PLATFORM(QT) > + bool oldSelectionMode = Pasteboard::generalPasteboard()->isSelectionMode(); > + Pasteboard::generalPasteboard()->setSelectionMode(true); > + if (source == CommandFromMenuOrKeyBinding) { > + UserTypingGestureIndicator typingGestureIndicator(frame); > + frame->editor()->paste(); > + } else > + frame->editor()->paste(); > + Pasteboard::generalPasteboard()->setSelectionMode(oldSelectionMode); > + return true; > +#else > + return false; > +#endif We also need this in Chromium, right? > Source/WebCore/editing/EditorCommand.cpp:1210 > +#if PLATFORM(QT) > + return qApp->clipboard()->supportsSelection(); > +#elif PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN) > + return true; > +#else > + return false; > +#endif Can we use editing behavior here? > Source/WebCore/editing/FrameSelection.cpp:317 > + if (newSelection.isRange()) > + updateGlobalSelection(); You should do this in EditorClient. > Source/WebCore/editing/FrameSelection.cpp:1817 > +#if PLATFORM(QT) || (PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN)) > +void FrameSelection::updateGlobalSelection() > +{ > +#if PLATFORM(QT) > + if (qApp->clipboard()->supportsSelection()) > +#endif > + { > + bool oldSelectionMode = Pasteboard::generalPasteboard()->isSelectionMode(); > + Pasteboard::generalPasteboard()->setSelectionMode(true); > + Pasteboard::generalPasteboard()->writeSelection(toNormalizedRange().get(), m_frame->editor()->canSmartCopyOrDelete(), m_frame); > + Pasteboard::generalPasteboard()->setSelectionMode(oldSelectionMode); > + } > +} > +#endif Please move this to Qt's EditorClient.

Comment on attachment 145600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145600&action=review > Source/WebCore/editing/EditorCommand.cpp:1215 > + return supportedGlobalSelection(frame) && supportedPaste(frame); We should probably not expose it to scripts.

Comment on attachment 145600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145600&action=review > Source/WebCore/editing/EditorCommand.cpp:927 > +#if PLATFORM(QT) Did you mean to allow Chromium Linux here too? Maybe that's why the tests failed on the cr-linux bot?

(In reply to comment #11) > (From update of attachment 145600 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145600&action=review > > > Source/WebCore/editing/EditorCommand.cpp:927 > > +#if PLATFORM(QT) > > Did you mean to allow Chromium Linux here too? Maybe that's why the tests failed on the cr-linux bot? Yes, that was an error. I didn't mean to mark the patch for review yet, I still need to test how it works in chromium.

(In reply to comment #9) > (From update of attachment 145600 [details]) > > Source/WebCore/editing/EditorCommand.cpp:1210 > > +#if PLATFORM(QT) > > + return qApp->clipboard()->supportsSelection(); > > +#elif PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN) > > + return true; > > +#else > > + return false; > > +#endif > > Can we use editing behavior here? > What do you mean by that? > > Source/WebCore/editing/FrameSelection.cpp:317 > > + if (newSelection.isRange()) > > + updateGlobalSelection(); > > You should do this in EditorClient. > Any reason why? One the goals with this cleanup is the share this code between platforms (in this case QtWebKit1, QtWebKit2 and Chromium), this means getting it out of EditorClient. It lacks an a define protection which is why we get all the compile errors, but other than that I think this is a much better place to do the update. > > Source/WebCore/editing/FrameSelection.cpp:1817 > > +#if PLATFORM(QT) || (PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN)) > > +void FrameSelection::updateGlobalSelection() > > +{ > > +#if PLATFORM(QT) > > + if (qApp->clipboard()->supportsSelection()) > > +#endif > > + { > > + bool oldSelectionMode = Pasteboard::generalPasteboard()->isSelectionMode(); > > + Pasteboard::generalPasteboard()->setSelectionMode(true); > > + Pasteboard::generalPasteboard()->writeSelection(toNormalizedRange().get(), m_frame->editor()->canSmartCopyOrDelete(), m_frame); > > + Pasteboard::generalPasteboard()->setSelectionMode(oldSelectionMode); > > + } > > +} > > +#endif > > Please move this to Qt's EditorClient. Again this is to be shared by QtWebKit1, QtWebKit2 and Chromium. This exact sequence of commands currently exists in both QtWebKit1 and Chromium.

Created attachment 145747 [details] Patch

Comment on attachment 145747 [details] Patch Attachment 145747 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12898510

(In reply to comment #13) > (In reply to comment #9) > > (From update of attachment 145600 [details] [details]) > > > Source/WebCore/editing/EditorCommand.cpp:1210 > > > +#if PLATFORM(QT) > > > + return qApp->clipboard()->supportsSelection(); > > > +#elif PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN) > > > + return true; > > > +#else > > > + return false; > > > +#endif > > > > Can we use editing behavior here? > > > What do you mean by that? > EditingBehavior.h

(In reply to comment #16) > (In reply to comment #13) > > (In reply to comment #9) > > > (From update of attachment 145600 [details] [details] [details]) > > > > Source/WebCore/editing/EditorCommand.cpp:1210 > > > > +#if PLATFORM(QT) > > > > + return qApp->clipboard()->supportsSelection(); > > > > +#elif PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN) > > > > + return true; > > > > +#else > > > > + return false; > > > > +#endif > > > > > > Can we use editing behavior here? > > > > > What do you mean by that? > > > > EditingBehavior.h Ah right. I am not sure how good a fit that is. It is not really editing behaviour, it is something a platform supports or not (it should probably be behind an ENABLE flag). Especially on Linux applications can have very different editing behaviour, but as long as they are on X11 they all support global selection.

Comment on attachment 145747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145747&action=review > Source/WebCore/editing/EditorCommand.cpp:1588 > + { "PasteGlobalSelection", { executePasteGlobalSelection, supportedPasteGlobalSelection, enabledPaste, stateNone, valueNull, notTextInsertion, allowExecutionWhenDisabled } }, This comment should NOT be exposed to scripts. So use supportedFromMenuOrKeyBinding instead of supportedPasteGlobalSelection. r- because of this. > Source/WebCore/editing/FrameSelection.cpp:74 > +#if PLATFORM(QT) > +#include <QClipboard> > +#include <qguiapplication.h> > +#endif I don't want to see these if-defs. > Source/WebCore/editing/FrameSelection.cpp:319 > +#if PLATFORM(QT) || (PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN)) > + if (newSelection.isRange()) > + updateGlobalSelection(); > +#endif Ditto. > Source/WebCore/editing/FrameSelection.cpp:1819 > +#if PLATFORM(QT) || (PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN)) > +void FrameSelection::updateGlobalSelection() > +{ > +#if PLATFORM(QT) > + if (qApp->clipboard()->supportsSelection()) > +#endif > + { > + bool oldSelectionMode = Pasteboard::generalPasteboard()->isSelectionMode(); > + Pasteboard::generalPasteboard()->setSelectionMode(true); > + Pasteboard::generalPasteboard()->writeSelection(toNormalizedRange().get(), m_frame->editor()->canSmartCopyOrDelete(), m_frame); > + Pasteboard::generalPasteboard()->setSelectionMode(oldSelectionMode); > + } > +} > +#endif A better way to do this is by adding new preference/setting or editor client callback like supportsGlobalSelection on all ports. Also, we need a test for this because this code didn't exist at least for chromium port. This is another reason for r-.

(In reply to comment #18) > (From update of attachment 145747 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145747&action=review > > > Source/WebCore/editing/EditorCommand.cpp:1588 > > + { "PasteGlobalSelection", { executePasteGlobalSelection, supportedPasteGlobalSelection, enabledPaste, stateNone, valueNull, notTextInsertion, allowExecutionWhenDisabled } }, > > This comment should NOT be exposed to scripts. So use supportedFromMenuOrKeyBinding instead of supportedPasteGlobalSelection. s/comment/command/

(In reply to comment #18) > (From update of attachment 145747 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145747&action=review > > > Source/WebCore/editing/EditorCommand.cpp:1588 > > + { "PasteGlobalSelection", { executePasteGlobalSelection, supportedPasteGlobalSelection, enabledPaste, stateNone, valueNull, notTextInsertion, allowExecutionWhenDisabled } }, > > This command should NOT be exposed to scripts. So use supportedFromMenuOrKeyBinding instead of supportedPasteGlobalSelection. > r- because of this. > Ahh, of course. > > Source/WebCore/editing/FrameSelection.cpp:74 > > +#if PLATFORM(QT) > > +#include <QClipboard> > > +#include <qguiapplication.h> > > +#endif > > I don't want to see these if-defs. > > > Source/WebCore/editing/FrameSelection.cpp:319 > > +#if PLATFORM(QT) || (PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN)) > > + if (newSelection.isRange()) > > + updateGlobalSelection(); > > +#endif > > Ditto. > > > Source/WebCore/editing/FrameSelection.cpp:1819 > > +#if PLATFORM(QT) || (PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN)) > > +void FrameSelection::updateGlobalSelection() > > +{ > > +#if PLATFORM(QT) > > + if (qApp->clipboard()->supportsSelection()) > > +#endif > > + { > > + bool oldSelectionMode = Pasteboard::generalPasteboard()->isSelectionMode(); > > + Pasteboard::generalPasteboard()->setSelectionMode(true); > > + Pasteboard::generalPasteboard()->writeSelection(toNormalizedRange().get(), m_frame->editor()->canSmartCopyOrDelete(), m_frame); > > + Pasteboard::generalPasteboard()->setSelectionMode(oldSelectionMode); > > + } > > +} > > +#endif > > A better way to do this is by adding new preference/setting or editor client callback like supportsGlobalSelection on all ports. That is good suggestion. > Also, we need a test for this because this code didn't exist at least for chromium port. This is another reason for r-. I will see what I can do.

Created attachment 162036 [details] Patch Rebased, implemented full QtWebKit2 support, and changed implementation according to reviewer feedback

(In reply to comment #21) > Created an attachment (id=162036) [details] > Patch > > Rebased, implemented full QtWebKit2 support, and changed implementation according to reviewer feedback There are a number of steps this patch does not take, but could be done in the future to further unify the implementations on this feature. 1. The patch only merges the paste global-selection code of Chromium and Qt, it does not yet merge the similar code from GTK. GTK has implemented it very differently from Chome and Qt and I would require more work than what I am willing to do without a better GTK test setup. 2. Also it leaves the event-handling and triggering of the new editor command in platform WebPage handleMouseRelease (or handleMousePress for GTK). This could be unified in the future by moving it to WebCore's EventHandler.

Comment on attachment 162036 [details] Patch Attachment 162036 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13741533

Comment on attachment 162036 [details] Patch Attachment 162036 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13744519

Comment on attachment 162036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162036&action=review Causes build issues. > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h:130 > + void updateGlobalSelection(WebCore::Frame*) OVERRIDE; Wrong OVERRIDE here.

Comment on attachment 162036 [details] Patch Attachment 162036 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13743525

nit: even though this only seems to introduce behavioral changes to Qt, it also touches non-Qt code. Therefore it may be best not to have the "[Qt] " prefix to your bug title.

Comment on attachment 162036 [details] Patch Attachment 162036 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13745514

Created attachment 162192 [details] Patch Fix build issues on other platforms

Comment on attachment 162192 [details] Patch Attachment 162192 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13761454

(In reply to comment #30) > (From update of attachment 162192 [details]) > Attachment 162192 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/13761454 Can somebody help me get the actual error message here? The output only contains the file not building but as far as I can tell, no error message, which makes it hard to figure out what could be wrong.

(In reply to comment #31) > (In reply to comment #30) > > (From update of attachment 162192 [details] [details]) > > Attachment 162192 [details] [details] did not pass mac-ews (mac): > > Output: http://queues.webkit.org/results/13761454 > > Can somebody help me get the actual error message here? The output only contains the file not building but as far as I can tell, no error message, which makes it hard to figure out what could be wrong. WebEditorClient.o: /Volumes/Data/WebKit/Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:47:32: error: WebCore/Pasteboard.h: No such file or directory The second error was truncated mid-command, convenient..

Created attachment 162203 [details] Patch Fix Mac build by only trying to include Pasteboard.h in Qt builds

Comment on attachment 162203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162203&action=review Thanks for doing this cleanup. Seems fine in general, just some questions. r- for lack of a test. > Source/WebCore/ChangeLog:13 > + This patch moves the implementations of global selection from the > + separate implementations in Qt WebKit, Chromium and GTK to WebCore, > + by implementing a new EditorCommand for pasting the global selection. Do we have any layout tests that cover this? If so, can you name them in the ChangeLog? If not, can you write one? > Source/WebCore/editing/EditorCommand.cpp:931 > + } else > + frame->editor()->paste(); If it's only supportedFromMenuOrKeyBinding, when do we hit this else case? > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:207 > setSelectionPrimaryClipboardIfNeeded(frame); > +#elif PLATFORM(QT) > + updateGlobalSelection(frame); It looks like setSelectionPrimaryClipboardIfNeeded and updateGlobalSelection both do the same thing (take the selection and write it to the selection clipboard). Can they share the same function name?

(In reply to comment #34) > Do we have any layout tests that cover this? If so, can you name them in the ChangeLog? If not, can you write one? > No, I am also not sure if it would be possible without adding new features to TestRunner. > > Source/WebCore/editing/EditorCommand.cpp:931 > > + } else > > + frame->editor()->paste(); > > If it's only supportedFromMenuOrKeyBinding, when do we hit this else case? > You are right. It shouldn't get hit. I just like completeness. > > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:207 > > setSelectionPrimaryClipboardIfNeeded(frame); > > +#elif PLATFORM(QT) > > + updateGlobalSelection(frame); > > It looks like setSelectionPrimaryClipboardIfNeeded and updateGlobalSelection both do the same thing (take the selection and write it to the selection clipboard). Can they share the same function name? They should eventually, but I am hoping I can later change GTK's implementation of global selection to act more like Chromiums and Qt, and then they would share not just function-name but the exact same function.

(In reply to comment #35) > (In reply to comment #34) > > Do we have any layout tests that cover this? If so, can you name them in the ChangeLog? If not, can you write one? > > > No, I am also not sure if it would be possible without adding new features to TestRunner. > I found testRunner.execCommand which means I can execute the pasteGlobalSelection command. Now I just have two problems: I just need a way to programmatically set a selection so that it looks like a user initiated mouse-selection, I guess it could be done with synthetic mouse-events, but is there an easier way. More complicated though, I need to set the selection and copy it without overriding the existing selection. Normally that requires two windows. Default is that pasted content overrides the current selection, but that would make it impossible to tell if command actually did anything.

Created attachment 162463 [details] Patch

Created attachment 162482 [details] Patch with test Now with an attempt at a test-case, though the test may still need to be deactivated on more ports and platforms.

Comment on attachment 162463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162463&action=review I think it's fine to land this patch, but let's make a follow up bug for adding a layout test. For a layout test, I would do the following: - Add a method to internals that sets the global selection. - Write a test that calls internals.setGlobalSelection then calls testRunner.execCommand. Using internals to set the selection clipboard avoids the possibility of the user interacting with the system messing up the test results. > Source/WebKit/chromium/src/EditorClientImpl.h:116 > + virtual bool supportsGlobalSelection(); Nit: OVERRIDE > Source/WebKit/chromium/src/WebViewImpl.cpp:647 > + editor->command(AtomicString("PasteGlobalSelection")).execute(); Nit: I think it will automatically convert to AtomicString (the constructor doesn't have explicit). > Source/WebKit/qt/WebCoreSupport/EditorClientQt.h:108 > + virtual bool supportsGlobalSelection(); Nit: OVERRIDE

(In reply to comment #39) > (From update of attachment 162463 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162463&action=review > > I think it's fine to land this patch, but let's make a follow up bug for adding a layout test. > > For a layout test, I would do the following: > - Add a method to internals that sets the global selection. > - Write a test that calls internals.setGlobalSelection then calls testRunner.execCommand. > > Using internals to set the selection clipboard avoids the possibility of the user interacting with the system messing up the test results. > Well, the second patch already has a working test that sets the selection clipboard using the SelectAll command. I am not sure SelectAll really should set the global selection, but if it is changed we can always use SelectWord instead since that definitely should set the global selection if activated using double-click. The test work as far as I know, the biggest problem is more where to put it, or how to make sure it is skipped everywhere it needs to be skipped.

(In reply to comment #40) > Well, the second patch already has a working test that sets the selection clipboard using the SelectAll command. I am not sure SelectAll really should set the global selection, but if it is changed we can always use SelectWord instead since that definitely should set the global selection if activated using double-click. > > The test work as far as I know, the biggest problem is more where to put it, or how to make sure it is skipped everywhere it needs to be skipped. It might be flaky if another test is running at the same time that sets a selection. I think this is fine on GTK+, which uses a separate Xvfb for each test. Maybe it's fine for Qt as well? You could add the test to editing/pasteboard/globalselection and skip that directory like you've done in the TestExpectations files. Just skipping the file seems fine too. View in context: https://bugs.webkit.org/attachment.cgi?id=162482&action=review > LayoutTests/platform/chromium/TestExpectations:893 > +WONTFIX MAC WIN : edition/pasteboard/paste-global-selection.html = TEXT Typo here and in the other TestExpectations files: edition -> editing

Created attachment 162724 [details] Patch for landing

Created attachment 162725 [details] Patch for landing Forgot one typo in gtk/TestExpections

Comment on attachment 162725 [details] Patch for landing Rejecting attachment 162725 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: file LayoutTests/platform/efl/TestExpectations Hunk #1 FAILED at 1109. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/efl/TestExpectations.rej patching file LayoutTests/platform/gtk/TestExpectations patching file LayoutTests/platform/mac/Skipped patching file LayoutTests/platform/qt-mac/Skipped patching file LayoutTests/platform/win/Skipped Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13768765

Committed r127862: <http://trac.webkit.org/changeset/127862>

Re-opened since this is blocked by 96104

This appears to be broken on Chromium Linux.

This appears to be broken on Chromium Linux.

(In reply to comment #48) > This appears to be broken on Chromium Linux. I was afraid of that. There is no code in WebKit that updates the global selection for Chromium. The chromium code that does that the update is outside WebKit, which is why the automated test will not work in the chromium port.

(In reply to comment #49) > (In reply to comment #48) > > This appears to be broken on Chromium Linux. > > I was afraid of that. There is no code in WebKit that updates the global selection for Chromium. The chromium code that does that the update is outside WebKit, which is why the automated test will not work in the chromium port. We should file a bug and skip the test on Chromium Linux.

(In reply to comment #50) > (In reply to comment #49) > > (In reply to comment #48) > > > This appears to be broken on Chromium Linux. > > > > I was afraid of that. There is no code in WebKit that updates the global selection for Chromium. The chromium code that does that the update is outside WebKit, which is why the automated test will not work in the chromium port. > > We should file a bug and skip the test on Chromium Linux. Oh, that's bug 96109.

(In reply to comment #45) > Committed r127862: <http://trac.webkit.org/changeset/127862> It caused regressions on Qt-WK2 - https://bugs.webkit.org/show_bug.cgi?id=96243 Could you check it, please?