Bug 17172 - Refactor platform checks in ScrollView.h
Summary: Refactor platform checks in ScrollView.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-02-04 04:24 PST by Jan Alonzo
Modified: 2008-03-17 09:57 PDT (History)
1 user (show)

See Also:


Attachments
refactor ScrollView.h (6.53 KB, patch)
2008-02-04 04:25 PST, Jan Alonzo
no flags Details | Formatted Diff | Diff
updated patch (7.10 KB, patch)
2008-02-04 05:19 PST, Jan Alonzo
darin: review-
Details | Formatted Diff | Diff
updated patch (11.08 KB, patch)
2008-02-06 03:57 PST, Jan Alonzo
no flags Details | Formatted Diff | Diff
Updated patch (5.87 KB, patch)
2008-03-16 22:49 PDT, Jan Alonzo
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Alonzo 2008-02-04 04:24:50 PST
The attached patch refactors the platform checks and removing redundant function declarations in ScrollView.h
Comment 1 Jan Alonzo 2008-02-04 04:25:32 PST
Created attachment 18902 [details]
refactor ScrollView.h
Comment 2 Jan Alonzo 2008-02-04 05:19:19 PST
Created attachment 18904 [details]
updated patch

This is a more complete patch than the previous one
Comment 3 Darin Adler 2008-02-04 09:51:29 PST
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.
Comment 4 Jan Alonzo 2008-02-06 03:57:58 PST
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
Comment 5 Darin Adler 2008-02-06 06:44:19 PST
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.
Comment 6 Jan Alonzo 2008-02-07 02:29:07 PST
(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
Comment 7 Darin Adler 2008-02-07 09:21:34 PST
(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.
Comment 8 Darin Adler 2008-03-16 14:53:28 PDT
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).
Comment 9 Jan Alonzo 2008-03-16 17:16:17 PDT
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?
Comment 10 Darin Adler 2008-03-16 20:48:07 PDT
(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.
Comment 11 Jan Alonzo 2008-03-16 22:49:58 PDT
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,
Comment 12 Darin Adler 2008-03-17 09:48:43 PDT
Comment on attachment 18957 [details]
updated patch

Clearing the review flag on this obsolete patch.
Comment 13 Darin Adler 2008-03-17 09:57:51 PDT
Committed revision 31098.