[webkit-reviews] review granted: [Bug 201549] Incorrect selection rect revealed after pasting images in a contenteditable element : [Attachment 378216] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 201549: Incorrect selection rect revealed after pasting images in a
contenteditable element
https://bugs.webkit.org/show_bug.cgi?id=201549

Attachment 378216: Patch

https://bugs.webkit.org/attachment.cgi?id=378216&action=review




--- Comment #2 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 378216
  --> https://bugs.webkit.org/attachment.cgi?id=378216
Patch

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

> Source/WebCore/editing/Editing.cpp:1313
> +    for (TextIterator iterator(&range); !iterator.atEnd();
iterator.advance()) {

Is TextIterator the most efficient way to find image elements?

> Source/WebCore/editing/Editing.cpp:1319
> +	   if (cachedImage && cachedImage->image() &&
cachedImage->image()->isNull())

Does isNull() guarantee that you'll get a load happening later?

> Source/WebCore/editing/Editor.cpp:1593
> +    document().updateLayout();
> +    revealSelectionAfterEditingOperation();

This is the kind of thing that should be queued and happen at "update
rendering" time.

> Source/WebCore/page/FrameView.cpp:2452
> +void FrameView::renderLayerDidScroll(const RenderLayer& layer)
> +{
> +    if (!m_frame->isWaitingToRevealSelection())
> +	   return;
> +
> +    auto startContainer =
makeRefPtr(m_frame->selection().selection().start().containerNode());
> +    if (!startContainer)
> +	   return;
> +
> +    auto* startContainerRenderer = startContainer->renderer();
> +    if (!startContainerRenderer)
> +	   return;
> +
> +    // FIXME: Ideally, this would also cancel deferred selection revealing
if the selection is inside a subframe and a parent frame is scrolled.
> +    for (auto* enclosingLayer = startContainerRenderer->enclosingLayer();
enclosingLayer; enclosingLayer = enclosingLayer->parent()) {
> +	   if (enclosingLayer == &layer) {
> +	       m_frame->cancelWaitingToRevealSelection();
> +	       break;
> +	   }
> +    }

Not sure this is the best place for this. Long term, FrameView needs to not
know about RenderLayers. The RenderLayer tree walk here seems like the wrong
place for the code too. Why not just tell Editor that the scroll happened?

> Source/WebCore/page/Page.cpp:2987
>  void Page::didFinishLoadingImageForElement(HTMLImageElement& element)

Does this get called for failed loads?