[webkit-reviews] review denied: [Bug 131511] [CSS Regions] Selection highlight doesn't match DOM selection : [Attachment 229126] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 131511: [CSS Regions] Selection highlight doesn't match DOM selection
https://bugs.webkit.org/show_bug.cgi?id=131511

Attachment 229126: Patch
https://bugs.webkit.org/attachment.cgi?id=229126&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229126&action=review


This looks good. I just have two issues:

(1) I think you want RenderNamedFlowThread to inherit from
SelectionSubtreeRoot, since I don't think in-flow RenderFlowThreads (e.g., the
new multi-column code) need to have a separate selection.

(2) Since selectionRoot() can never be null, make sure to use a & rather than a
* everywhere.

Other than that, the approach looks fine.

> Source/WebCore/rendering/RenderFlowThread.h:60
> +class RenderFlowThread: public RenderBlockFlow, public SelectionSubtreeRoot
{

I don't think all flow threads should be selection subtree roots. I think only
named RenderFlowThreads should be selection subtree roots. In-flow flow threads
have no reason to maintain a separate selection subtree.

> Source/WebCore/rendering/RenderObject.cpp:1550
> +SelectionSubtreeRoot* RenderObject::selectionRoot() const
> +{
> +    RenderFlowThread* flowThread = flowThreadContainingBlock();
> +    if (flowThread)
> +	   return flowThread;
> +
> +    return &view();
> +}

This is going to actually cause in-flow RenderFlowThreads to be selection
roots. I don't think we want this, since a single subtree is capable of
crossing into and out of in-flow RenderFlowThreads. You should limit this to
only out-of-flow RenderFlowThreads (i.e., named RenderFlowThreads).

Also, the selectionRoot() can never be null, so I think this should return a
reference rather than a pointer.

> Source/WebCore/rendering/RenderView.cpp:795
> +    // Initializate map for RenderView and all RenderFlowThreads

Typo: "Initialize"

> Source/WebCore/rendering/RenderView.cpp:857
> +void RenderView::setSubtreeSelection(SelectionSubtreeRoot* root,
RenderObject* start, int startPos, RenderObject* end, int endPos,
SelectionRepaintMode blockRepaintMode)
> +{

SelectionSubtreeRoot& would be better (since it can't be null).