[webkit-reviews] review denied: [Bug 22579] Avoid high-level WebCore types under platform : [Attachment 25643] A patch as described in the bug details

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Finnur Thorarinsson
<finnur.webkit at gmail.com>'s request for review:
Bug 22579: Avoid high-level WebCore types under platform
https://bugs.webkit.org/show_bug.cgi?id=22579

Attachment 25643: A patch as described in the bug details
https://bugs.webkit.org/attachment.cgi?id=25643&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Patch generally looks good. Definitely a step in the right direction.

The name of this function needs to talk about tick marks, not text-match
markers. Not only do we want to avoid types from the rest of WebCore, we want
to avoid hard-coding in the concepts too. So the function should be one that
gives the tick mark implementation what it needs in a form that's about "tick
marks" not about "text match markers", since the latter concept is one that
doesn't make any sense in the context of a scroll view.

I'm thinking that instead of Vector<IntRect> you might want this to be a
Vector<int> of tick mark positions.

> +void FrameView::renderedRectsForTextMatchMarkers(Vector<IntRect>* rects) {
> +    *rects =
frame()->document()->renderedRectsForMarkers(DocumentMarker::TextMatch);
> +}

The brace that starts a new function needs to be on a separate line as
described in the WebKit coding style document.

> +    virtual void renderedRectsForTextMatchMarkers(Vector<IntRect>* rects);

We'd normally use a reference, not a pointer, for an "out" parameter like
rects. Also, in declarations we normally omit the name of the argument if the
type speaks for itself. For functions that use an "out" parameter for a return
value rather than the function result we use the word get, so this would be
getRendereredRectsForTextMatchMarkers.

> +    virtual void renderedRectsForTextMatchMarkers(Vector<IntRect>* rects) {
}

Important to omit the argument here to avoid unused argument warnings, which we
probably want to turn on at some point.