<rdar://problem/31544320>
Created attachment 308888 [details] First pass
Created attachment 308889 [details] Fix OpenSource iOS build
Created attachment 308926 [details] Handle multiple dragged content ranges in RenderText
Comment on attachment 308926 [details] Handle multiple dragged content ranges in RenderText View in context: https://bugs.webkit.org/attachment.cgi?id=308926&action=review r=me, though it would be good to get Simon, Myles, Dean, or Alan to take a look at some point. > Source/WebCore/dom/DocumentMarkerController.cpp:142 > + DocumentMarker::DraggedContentData draggedContentData { markedText.node() }; Could textPiece ever be null? If not, this is okay. If so, you should add a null check.
(In reply to Beth Dakin from comment #4) > Comment on attachment 308926 [details] > Handle multiple dragged content ranges in RenderText > > View in context: > https://bugs.webkit.org/attachment.cgi?id=308926&action=review > > r=me, though it would be good to get Simon, Myles, Dean, or Alan to take a > look at some point. Wenson and I had a quick chat about textEffectsModifier logic.
(In reply to zalan from comment #5) > (In reply to Beth Dakin from comment #4) > > Comment on attachment 308926 [details] > > Handle multiple dragged content ranges in RenderText > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=308926&action=review > > > > r=me, though it would be good to get Simon, Myles, Dean, or Alan to take a > > look at some point. > Wenson and I had a quick chat about textEffectsModifier logic. ๐
Created attachment 309010 [details] Remove TextEffectsRange, move relevant logic to InlineTextBox.
Created attachment 309011 [details] Remove TextEffectsRange, move relevant logic to InlineTextBox.
Created attachment 309012 [details] Rebased on master
(In reply to Beth Dakin from comment #4) > Comment on attachment 308926 [details] > Handle multiple dragged content ranges in RenderText > > View in context: > https://bugs.webkit.org/attachment.cgi?id=308926&action=review > > r=me, though it would be good to get Simon, Myles, Dean, or Alan to take a > look at some point. > > > Source/WebCore/dom/DocumentMarkerController.cpp:142 > > + DocumentMarker::DraggedContentData draggedContentData { markedText.node() }; > > Could textPiece ever be null? If not, this is okay. If so, you should add a > null check. I think this will be ok. Even if the target node is null, this will just initialize the DraggedContentData to have a null targetNode (a RefPtr). In the two places where we use this, (shouldInsertAsSeparateMarker and draggedContentContainsReplacedElement), we handle the !targetNode case gracefully, and won't crash trying to dereference null.
Created attachment 309014 [details] Rebased on master (again)
Comment on attachment 309014 [details] Rebased on master (again) Attachment 309014 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3670184 New failing tests: media/modern-media-controls/media-documents/background-color-and-centering.html
Created attachment 309024 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 309095 [details] Uploading for EWS pass
Comment on attachment 309095 [details] Uploading for EWS pass Clearing flags on attachment: 309095 Committed r216212: <http://trac.webkit.org/changeset/216212>
Comment on attachment 308926 [details] Handle multiple dragged content ranges in RenderText View in context: https://bugs.webkit.org/attachment.cgi?id=308926&action=review > Source/WebCore/rendering/InlineTextBox.cpp:555 > + auto draggedContentRanges = renderer().draggedContentRangesBetweenOffsets(m_start, m_start + m_len); It's a shame that we're slowing down the paint code for something that almost never happens. draggedContentRangesBetweenOffsets() shows up on profiles. It's small, but still, is there a better way to avoid this work when the user isn't dragging?
File a new bug! Thereโs surely a way.