Bug 22579 - Avoid high-level WebCore types under platform
Summary: Avoid high-level WebCore types under platform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-01 16:28 PST by Finnur Thorarinsson
Modified: 2008-12-05 14:10 PST (History)
0 users

See Also:


Attachments
A patch as described in the bug details (6.45 KB, patch)
2008-12-01 16:34 PST, Finnur Thorarinsson
darin: review-
Details | Formatted Diff | Diff
New patch, after addressing review comments (6.12 KB, patch)
2008-12-04 13:27 PST, Finnur Thorarinsson
darin: review+
Details | Formatted Diff | Diff
New and improved! Now with const goodness and default implementation. Order now while supplies last! (2.49 KB, patch)
2008-12-04 16:49 PST, Finnur Thorarinsson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Finnur Thorarinsson 2008-12-01 16:28:17 PST
Dave Hyatt requested that people refrain from using high-level WebCore types in files under WebCore/platform/. My last patch for scrollbar tickmarks doesn't provide a way to get at the highlighted TextMatches (except through the Frame and FrameView types).

This changelist is an attempt to break that dependency by implementing a function on ScrollbarClient that the scrollbar code for the ports can call to get at the data.

I will be uploading a patch shortly.
Comment 1 Finnur Thorarinsson 2008-12-01 16:34:31 PST
Created attachment 25643 [details]
A patch as described in the bug details
Comment 2 Darin Adler 2008-12-04 09:29:05 PST
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.
Comment 3 Finnur Thorarinsson 2008-12-04 13:26:46 PST
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. :)
Comment 4 Finnur Thorarinsson 2008-12-04 13:27:27 PST
Created attachment 25750 [details]
New patch, after addressing review comments
Comment 5 Darin Adler 2008-12-04 14:37:42 PST
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.
Comment 6 Finnur Thorarinsson 2008-12-04 16:49:20 PST
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 7 Darin Adler 2008-12-04 17:07:31 PST
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
Comment 8 Eric Seidel (no email) 2008-12-05 14:10:46 PST
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