[webkit-reviews] review granted: [Bug 195178] Native text selection UI is incorrectly suppressed in Microsoft Visio : [Attachment 363401] WIP

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Mar 1 22:14:07 PST 2019

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 195178: Native text selection UI is incorrectly suppressed in Microsoft

Attachment 363401: WIP


--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 363401
  --> https://bugs.webkit.org/attachment.cgi?id=363401

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

> Source/WebCore/rendering/RenderObject.cpp:1558
> +    auto* container = &enclosingBox();
> +    while (container) {

Nicer to write as a for loop. If we can’t make the third clause work we can
still write:

    for (auto* container = &enclosingBox(); container; ) {

> Source/WebCore/rendering/RenderObject.cpp:1566
> +	       auto& frame = container->frame();

I think this logic about walking out to the enclosing frame's renderer should
be put in a helper function. Writing it out as a sequence of if statements
makes the thing look too complicated and it’s really a single concept. Not sure
exactly which helper function to write. Here’s one:

    static RenderBox* enclosingFrameRenderBox(RenderBox& container)
	auto* owner = container.frame().ownerElement();
	if (!owner)
	    return nullptr;
	auto* renderer = owner->renderer();
	return is<RenderBox>(renderer) ? &downcast<RenderBox>(*renderer) :


    static RenderBox* containingBlockCrossingFrameBoundaries(RenderBox&
	auto* nextContainer = container.containingBlock();
	if (!nextContainer)
	    nextContainer = enclosingFrameRenderBox(container);
	return nextContainer ? nextContainer :


    for (auto* container = &enclosingBox(); container; container =

But I think there are other interesting factorings too that might allow sharing
even more code with other call sites.

> Source/WebCore/rendering/RenderObject.cpp:1572
> +	       if (frame.isMainFrame())
> +		   break;
> +
> +	       auto* owner = frame.ownerElement();
> +	       if (!owner)
> +		   break;

It’s unnecessary to check both isMainFrame and check ownerElement for null. I
suggest skipping the isMainFrame check.

> Source/WebCore/rendering/RenderObject.cpp:1576
> +	       if (!frameRenderer || !frameRenderer->isBox())
> +		   break;

if (!is<RenderBox>(frameRenderer))

More information about the webkit-reviews mailing list