[webkit-reviews] review denied: [Bug 132200] Add a selection overlay : [Attachment 230194] Patch v2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 25 13:11:44 PDT 2014
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).
More information about the webkit-reviews
mailing list