Bug 190108 – Add a new variant of serializePreservingVisualAppearance which takes VisibleSelection

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Add a new variant of serializePreservingVisualAppearanceInternal which takes VisibleSelection directly instead of Range.
This fixes a bug exhibited in LayoutTests/editing/pasteboard/paste-table-003.html for example.

[Ryosuke Niwa]

I wanted to do this in a simple patch in the bug 190107 but this happens to also fixes a bug so I split it into its own patch.

[Ryosuke Niwa]

Created attachment 351174 [details]
Adds a variant and fixes a bug

[Ryosuke Niwa]

Note that this patch depends on the patch posted on the bug 190107.

[Ryosuke Niwa]

Created attachment 351226 [details]
Updated for trunk

[Wenson Hsieh]

Comment on attachment 351226 [details]
Updated for trunk

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

> Source/WebCore/platform/win/PasteboardWin.cpp:472
> +        // FIXME: Use ResolveURLs::YesExcludingLocalFileURLsForPrivacy

Nit - period at end of this comment.

> Source/WebKit/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:125
>      pasteboardContent.text = range->text();

It looks like this GTK code still grabs text from the selected Range — perhaps we could just keep the VisibleSelection => Range conversion here to call Range::text(), or add a variant of plainText() in TextIterator.h that takes a VisibleSelection instead of a Range and call that instead.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #5)
> Comment on attachment 351226 [details]
> Updated for trunk
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351226&action=review
> 
> > Source/WebCore/platform/win/PasteboardWin.cpp:472
> > +        // FIXME: Use ResolveURLs::YesExcludingLocalFileURLsForPrivacy
> 
> Nit - period at end of this comment.

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:125
> >      pasteboardContent.text = range->text();
> 
> It looks like this GTK code still grabs text from the selected Range —
> perhaps we could just keep the VisibleSelection => Range conversion here to
> call Range::text(), or add a variant of plainText() in TextIterator.h that
> takes a VisibleSelection instead of a Range and call that instead.

Yeah, I'm gonna keep the range conversion for now.

[Ryosuke Niwa]

Created attachment 351320 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 351320 [details]
Patch for landing

Wait for EWS.

[Radar WebKit Bug Importer]

<rdar://problem/44924992>

[EWS Watchlist]

Comment on attachment 351320 [details]
Patch for landing

Attachment 351320 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9419897

New failing tests:
media/range-extract-contents-crash.html

[EWS Watchlist]

Created attachment 351334 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Ryosuke Niwa]

I don't think the failure in the media test is related to this change:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ???                           	000000000000000000 0 + 0
1   com.apple.WebKit              	0x0000000105860775 WebKit::PlaybackSessionModelContext::externalPlaybackChanged(bool, WebCore::PlaybackSessionModel::ExternalPlaybackTargetType, WTF::String const&) + 185 (Vector.h:1504)
2   com.apple.WebKit              	0x000000010570842e void IPC::callMemberFunctionImpl<WebKit::PlaybackSessionManagerProxy, void (WebKit::PlaybackSessionManagerProxy::*)(unsigned long long, bool, unsigned int, WTF::String), std::__1::tuple<unsigned long long, bool, unsigned int, WTF::String>, 0ul, 1ul, 2ul, 3ul>(WebKit::PlaybackSessionManagerProxy*, void (WebKit::PlaybackSessionManagerProxy::*)(unsigned long long, bool, unsigned int, WTF::String), std::__1::tuple<unsigned long long, bool, unsigned int, WTF::String>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) + 66 (utility:753)
3   com.apple.WebKit              	0x00000001057073c1 void IPC::handleMessage<Messages::PlaybackSessionManagerProxy::ExternalPlaybackPropertiesChanged, WebKit::PlaybackSessionManagerProxy, void (WebKit::PlaybackSessionManagerProxy::*)(unsigned long long, bool, unsigned int, WTF::String)>(IPC::Decoder&, WebKit::PlaybackSessionManagerProxy*, void (WebKit::PlaybackSessionManagerProxy::*)(unsigned long long, bool, unsigned int, WTF::String)) + 73 (utility:753)
4   com.apple.WebKit              	0x00000001056e24c5 IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) + 127 (MessageReceiverMap.cpp:123)
5   com.apple.WebKit              	0x00000001058d9c0c WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 24 (WebProcessProxy.cpp:650)
6   com.apple.WebKit              	0x00000001056d35c6 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 108 (memory:2714)
7   com.apple.WebKit              	0x00000001056d6219 IPC::Connection::dispatchIncomingMessages() + 833 (memory:2734)
8   com.apple.JavaScriptCore      	0x00000001043111f6 WTF::RunLoop::performWork() + 214 (Function.h:56)
9   com.apple.JavaScriptCore      	0x0000000104311512 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
10  com.apple.CoreFoundation      	0x00007fffb22ff3e1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
11  com.apple.CoreFoundation      	0x00007fffb22e065c __CFRunLoopDoSources0 + 556
12  com.apple.CoreFoundation      	0x00007fffb22dfb46 __CFRunLoopRun + 934
13  com.apple.CoreFoundation      	0x00007fffb22df544 CFRunLoopRunSpecific + 420
14  com.apple.Foundation          	0x00007fffb3d10252 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 277
15  WebKitTestRunner              	0x000000010424c261 WTR::TestController::platformRunUntil(bool&, WTF::Seconds) + 191 (TestControllerCocoa.mm:204)
16  WebKitTestRunner              	0x000000010424d321 WTR::TestInvocation::invoke() + 237 (TestInvocation.cpp:173)
17  WebKitTestRunner              	0x0000000104242931 WTR::TestController::runTest(char const*) + 2205 (TestController.cpp:1379)
18  WebKitTestRunner              	0x0000000104242c92 WTR::TestController::runTestingServerLoop() + 132 (TestController.cpp:1408)
19  WebKitTestRunner              	0x000000010423d084 WTR::TestController::TestController(int, char const**) + 310 (TestController.cpp:147)
20  WebKitTestRunner              	0x000000010422cd5c main + 657 (main.mm:68)
21  libdyld.dylib                 	0x00007fffc7eb1235 start + 1

[Ryosuke Niwa]

Committed r236707: <https://trac.webkit.org/changeset/236707>