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

[bugzilla-daemon at webkit.org]

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
Visio
https://bugs.webkit.org/show_bug.cgi?id=195178

Attachment 363401: WIP

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




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

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) :
nullptr;
    }

Then:

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

Then:

    for (auto* container = &enclosingBox(); container; container =
containingBlockCrossingFrameBoundaries(*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))
	break;