<rdar://problem/35205373>
Created attachment 370645 [details] Patch
Created attachment 370646 [details] Patch
Comment on attachment 370646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370646&action=review > Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:200 > + UITargetedDragPreview *preview = createTargetedDragPreview(textIndicatorImage.get(), contentView, previewContainer, indicator.textBoundingRectInRootViewCoordinates, indicator.textRectsInBoundingRectCoordinates, [UIColor colorWithCGColor:cachedCGColor(indicator.estimatedBackgroundColor)], nil); A -create method that doesn't return a smart pointer? Weird. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:844 > + for (TextIterator iterator(range.get()); !iterator.atEnd(); iterator.advance()) { Why TextIterator? I think just one of the nice DOM child traversal things would be fine, no?
Comment on attachment 370646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370646&action=review Thanks for taking a look! It seems this patch breaks the windows build, so I'll fix that as well. >> Source/WebKit/UIProcess/ios/DragDropInteractionState.mm:200 >> + UITargetedDragPreview *preview = createTargetedDragPreview(textIndicatorImage.get(), contentView, previewContainer, indicator.textBoundingRectInRootViewCoordinates, indicator.textRectsInBoundingRectCoordinates, [UIColor colorWithCGColor:cachedCGColor(indicator.estimatedBackgroundColor)], nil); > > A -create method that doesn't return a smart pointer? Weird. Weird indeed! Changed to return a RetainPtr. >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:844 >> + for (TextIterator iterator(range.get()); !iterator.atEnd(); iterator.advance()) { > > Why TextIterator? I think just one of the nice DOM child traversal things would be fine, no? Ah, I chose TextIterator so that it would (1) skip image elements that are hidden/not displayed, and (2) it was easier to extract just elements inside the range, whereas the child traversal helpers (AFAIK) only operate on whole nodes, rather than start/end positions.
Created attachment 370647 [details] Fix Win build/review comment
Comment on attachment 370647 [details] Fix Win build/review comment Oops, this breaks watchOS builds.
Created attachment 370648 [details] Fix watchOS too
Comment on attachment 370648 [details] Fix watchOS too Clearing flags on attachment: 370648 Committed r245778: <https://trac.webkit.org/changeset/245778>
All reviewed patches have been landed. Closing bug.
Comment on attachment 370648 [details] Fix watchOS too Seems unfortunate to have special casing for the image elements. It’s possible for other elements to be used entirely for the purpose of displaying an image, for example with CSS as a background image or content of an image. Typically we want to have code that waits for images to be loaded to use some more abstract question rather than "is this an HTMLImageElement".
(In reply to Darin Adler from comment #10) > Comment on attachment 370648 [details] > Fix watchOS too > > Seems unfortunate to have special casing for the image elements. It’s > possible for other elements to be used entirely for the purpose of > displaying an image, for example with CSS as a background image or content > of an image. Typically we want to have code that waits for images to be > loaded to use some more abstract question rather than "is this an > HTMLImageElement". Yes, that's true. What we could do instead is make WebPage a CachedImageClient (or have WebPage hold onto a CachedImageClient), and add the CachedImageClient to all cached images in the dropped content range that should load before we try and take a snapshot. I filed <https://bugs.webkit.org/show_bug.cgi?id=198297> to track this.