Bug 190108 - Add a new variant of serializePreservingVisualAppearance which takes VisibleSelection
Summary: Add a new variant of serializePreservingVisualAppearance which takes VisibleS...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on: 190107
Blocks: 157443
  Show dependency treegraph
 
Reported: 2018-09-28 23:23 PDT by Ryosuke Niwa
Modified: 2018-10-01 18:59 PDT (History)
6 users (show)

See Also:


Attachments
Adds a variant and fixes a bug (14.95 KB, patch)
2018-09-28 23:35 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated for trunk (14.41 KB, patch)
2018-09-30 22:46 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (14.24 KB, patch)
2018-10-01 16:16 PDT, Ryosuke Niwa
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.90 MB, application/zip)
2018-10-01 18:51 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-09-28 23:23:15 PDT
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.
Comment 1 Ryosuke Niwa 2018-09-28 23:34:48 PDT
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.
Comment 2 Ryosuke Niwa 2018-09-28 23:35:02 PDT
Created attachment 351174 [details]
Adds a variant and fixes a bug
Comment 3 Ryosuke Niwa 2018-09-28 23:35:40 PDT
Note that this patch depends on the patch posted on the bug 190107.
Comment 4 Ryosuke Niwa 2018-09-30 22:46:56 PDT
Created attachment 351226 [details]
Updated for trunk
Comment 5 Wenson Hsieh 2018-10-01 07:18:59 PDT
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.
Comment 6 Ryosuke Niwa 2018-10-01 16:15:30 PDT
(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.
Comment 7 Ryosuke Niwa 2018-10-01 16:16:08 PDT
Created attachment 351320 [details]
Patch for landing
Comment 8 Ryosuke Niwa 2018-10-01 16:16:29 PDT
Comment on attachment 351320 [details]
Patch for landing

Wait for EWS.
Comment 9 Radar WebKit Bug Importer 2018-10-01 16:21:01 PDT
<rdar://problem/44924992>
Comment 10 Build Bot 2018-10-01 18:50:58 PDT
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
Comment 11 Build Bot 2018-10-01 18:51:00 PDT
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
Comment 12 Ryosuke Niwa 2018-10-01 18:58:55 PDT
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
Comment 13 Ryosuke Niwa 2018-10-01 18:59:54 PDT
Committed r236707: <https://trac.webkit.org/changeset/236707>