Bug 17172 – Refactor platform checks in ScrollView.h

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jan Alonzo]

The attached patch refactors the platform checks and removing redundant function declarations in ScrollView.h

[Jan Alonzo]

Created attachment 18902 [details]
refactor ScrollView.h

[Jan Alonzo]

Created attachment 18904 [details]
updated patch

This is a more complete patch than the previous one

[Darin Adler]

Comment on attachment 18904 [details]
updated patch

This patch is OK, but there is a better way to do the contentsToWindow/windowToContents thing. The inline implementation for non-Mac platforms can be outside the ScrollView class. there's no reason we have to muck up the declaration of those functions with #if just because the definition needs to be different. In fact, I also think there's no good reason to make them inline. It could just be in ScrollView.cpp.

 114         // Other platforms can just implement these helper methods using the corresponding point conversion methods.

And this comment really doesn't work when your rearrange the file. It's the second half of a comment that was moved far away.

 118 #if PLATFORM(WIN) || PLATFORM(QT)
 119 
 120         virtual void setParent(ScrollView*);        
 121 
136122         IntRect windowResizerRect();
137123         bool resizerOverlapsContent() const;
138124         void adjustOverlappingScrollbarCount(int overlapDelta);
 125 #endif // end PLATFORM(WIN) || PLATFORM(QT)

Seems ugly to have a space after the #if but not before the #endif.

 151         PlatformScrollbar *horizontalScrollBar() const;
 152         PlatformScrollbar *verticalScrollBar() const;

We put the * next to the type name, not the function name.

 177 #else // PLATFORM(WX)
228178         HashSet<Widget*>* children();

Again, you have a space after the #if and before the #else, but no space after the #else -- unnecessarily messy.

I'd like to see a better version of the patch. So I'm going to say review- for now.

The complaints are just about formatting, but since the patch itself is largely about formatting, I think it's OK for me to do that.

[Jan Alonzo]

Created attachment 18957 [details]
updated patch

Hi Darin

Thanks for the review. I updated the patch to include your suggestions. I also moved the implementation of IntRect contentsToWindow/windowToContents in the respective source files. Let me know if you have more suggestions.

Regards

[Darin Adler]

Comment on attachment 18957 [details]
updated patch

r=me

+IntRect ScrollView::contentsToWindow(const IntRect& rect) const
+{
+    return IntRect(contentsToWindow(rect.location()), rect.size());
+}
+
+IntRect ScrollView::windowToContents(const IntRect& rect) const
+{
+    return IntRect(windowToContents(rect.location()), rect.size());
+}

It's unfortunate we now have 3 copies of these functions. I would have preferred a single copy either in a shared ScrollView.cpp file with a proper ifdef or as an inline in the header file (not in the class definition, though). I feel the same way about other copied and pasted code that's in multiple platform files.

-    return &m_data->children;
+    return &(m_data->children);

I personally don't think this change is an improvement.

[Jan Alonzo]

(In reply to comment #5)
> (From update of attachment 18957 [details] [edit])
> r=me
> It's unfortunate we now have 3 copies of these functions. I would have
> preferred a single copy either in a shared ScrollView.cpp file with a proper
> ifdef or as an inline in the header file (not in the class definition, though).

Hi Darin

Thanks. I agree with you. If you want you can r- it for now and i'll try to come up with an updated patch.

> I feel the same way about other copied and pasted code that's in multiple
> platform files.

I will keep that in mind - thanks.

Regards

[Darin Adler]

(In reply to comment #6)
> Thanks. I agree with you. If you want you can r- it for now and i'll try to
> come up with an updated patch.

I'll leave it r+ for now. Feel free to post an updated patch and obsolete your existing one, though.

[Darin Adler]

Comment on attachment 18957 [details]
updated patch

Darn it.

I went to land this patch, and ran into conflicts. Then I realized these conflicts would be easy to fix if I could just merge back in with the version this patch was originally done against, and remembered that svn-apply-patch has a --merge option that does that quite well.

But this is a git patch, not Subversion, so I can't use that option. Merging is too much of a pain so I'm not going to land this today (otherwise I would have).

[Jan Alonzo]

Hi Darin

I'll update the patch and try to make it SVN-friendly. Also, do you think a shared ScrollView.cpp would be helpful?

[Darin Adler]

(In reply to comment #9)
> I'll update the patch and try to make it SVN-friendly. Also, do you think a
> shared ScrollView.cpp would be helpful?

Sure, if there's at least one function that would go in there.

[Jan Alonzo]

Created attachment 19825 [details]
Updated patch

This is an updated patch (still a git diff). A shared ScrollView.cpp would be ideal but i'll leave that for another bug/patch.

Regards,

[Darin Adler]

Comment on attachment 18957 [details]
updated patch

Clearing the review flag on this obsolete patch.

[Darin Adler]

Committed revision 31098.