[webkit-reviews] review denied: [Bug 132200] Add a selection overlay : [Attachment 230194] Patch v2

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Brady Eidson <beidson at apple.com>'s
request for review:
Bug 132200: Add a selection overlay
https://bugs.webkit.org/show_bug.cgi?id=132200

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

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


r-

> Source/WebCore/rendering/RenderView.cpp:784
> +#if ENABLE(SERVICE_CONTROLS)
> +    m_currentSelectionRects.clear();
> +
> +    // Always notify the EditorClient of the new selection rects, no matter
what they are.
> +    struct SelectionRectNotifier {
> +	   SelectionRectNotifier(RenderView& view)
> +	       : m_view(view)
> +	   { }
> +
> +	   ~SelectionRectNotifier()
> +	   {
> +	       if (EditorClient* client = 
m_view.view().frame().editor().client())
> +		  
client->selectionRectsDidChange(m_view.m_currentSelectionRects);
> +	   }
> +
> +	   RenderView& m_view;
> +    };
> +
> +    SelectionRectNotifier notifier(*this);
> +#endif // ENABLE(SERVICE_CONTROLS)

It's gross to have this big block of code right in RenderView::setSelection. I
would put this in a helper. I'd even define SelectionRectNotifier in a header
and get it out of here too.

> Source/WebCore/rendering/RenderView.cpp:967
> +#if ENABLE(SERVICE_CONTROLS)
> +	       LayoutRect rect = selectionInfo->rect();
> +	       if (!rect.isEmpty())
> +		   m_currentSelectionRects.append(rect);
> +#endif
> +
> +	       newSelectedObjects.set(o, std::move(selectionInfo));

Beef up SelectionRectNotifier into some kind of controller class and move the
currentSelectionRects there. Then this becomes one call on the new class.

> Source/WebCore/rendering/RenderView.cpp:985
> +#if ENABLE(SERVICE_CONTROLS)
> +		   GapRects rects = blockInfo->rects();
> +		   if (!rects.left().isEmpty())
> +		       m_currentSelectionRects.append(rects.left());
> +		   if (!rects.right().isEmpty())
> +		       m_currentSelectionRects.append(rects.right());
> +		   if (!rects.center().isEmpty())
> +		       m_currentSelectionRects.append(rects.center());
> +#endif

Same here. You could make this a single function call on a separate object.

> Source/WebCore/rendering/RenderView.h:360
> +#if ENABLE(SERVICE_CONTROLS)
> +    Vector<LayoutRect> m_currentSelectionRects;
> +#endif

Have a separate class here instead (similar to how we keep flow thread
controller logic in a separate class).