Bug 22579

Created attachment 25643 [details] A patch as described in the bug details

Comment on attachment 25643 [details] A patch as described in the bug details 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.

Thanks for the review, Darin. One question regarding Vector<Int>: The function document()->renderedRectsForMarkers() returns a Vector<IntRect>. If we want Vector<Int> instead we would have to either convert from one vector type to another (inefficient) or add an implementation of renderedRectsForMarkers that returns the new type (adding complexity). However, IntRect makes more sense to me when I think about it because you might want to make the height of the tickmark on the scrollbar relative to the height of the rendered rect. Does that make sense? I will update the patch to address the review comments (except for this unresolved question). PS. Sorry about the style violations - I still find it hard to switch from one style to another; old habits die hard. :)

Created attachment 25750 [details] New patch, after addressing review comments

Comment on attachment 25750 [details] New patch, after addressing review comments r=me as is. But I have two comments. One is that this should probably be a const member function, not non-const. The other is that maybe we should have a default empty implementation in ScrollbarClient -- it seems unfortunate that all classes that derive from this have to override it.

Created attachment 25757 [details] New and improved! Now with const goodness and default implementation. Order now while supplies last! I like that idea very much; makes things simpler. Here is the new patch. Quick, approve it before it shrinks down so much there is nothing left! ;)

Comment on attachment 25757 [details] New and improved! Now with const goodness and default implementation. Order now while supplies last! > Index: WebCore/page/FrameView.h > =================================================================== > --- WebCore/page/FrameView.h (revision 38997) > +++ WebCore/page/FrameView.h (working copy) > @@ -30,6 +30,7 @@ > #include "ScrollView.h" > #include <wtf/Forward.h> > #include <wtf/OwnPtr.h> > +#include <wtf/Vector.h> This include isn't needed because ScrollbarClient.h already includes Vector.h. r=me

Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/page/FrameView.cpp M WebCore/page/FrameView.h M WebCore/platform/ScrollbarClient.h Committed r39042