Bug 22056

Summary: Share FrameLoaderClient::transitionToCommittedForNewPage between Win/Gtk+/Qt
Product: WebKit Reporter: Holger Freyther <zecke>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal Keywords: Gtk, Qt
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Define the Color::transparent in the cpp file to fix link errors
none
Move recursively setting a background color and toggling transparency to the FrameView
none
Share FrameLoaderClient::transitionToCommittedForNewPage between Win/Gtk+/Qt
none
Kill FrameLoaderClient.cpp and move the code to Frame hausmann: review+

Description Holger Freyther 2008-11-03 14:47:19 PST
Currently we need to copy and paste code from the windows port to get fixes, sometimes a port lags behind... Change it by sharing the implementation.
Comment 1 Holger Freyther 2008-11-03 14:50:45 PST
Created attachment 24868 [details]
Define the Color::transparent in the cpp file to fix link errors

Link fix required to use Color::transparent from within WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp. Some how my gcc only inlines that or such, any comment to find the real cause to resolve my link error would be welcome.
Comment 2 Holger Freyther 2008-11-03 14:51:58 PST
Created attachment 24869 [details]
Move recursively setting a background color and toggling transparency to the FrameView

Move the code from WebFrame to WebCore::FrameView to be able to use from the Gtk+ and the Qt build.
Comment 3 Holger Freyther 2008-11-03 14:54:10 PST
Created attachment 24870 [details]
Share FrameLoaderClient::transitionToCommittedForNewPage between Win/Gtk+/Qt

Create a FrameLoaderClient.cpp and provide a helper function to implement transitionToCommittedForNewPage in a cross port way. The mac port can not be switched to this implementation but this copy reduction is helping both the Qt and Gtk+ port.
Comment 4 Darin Adler 2008-11-09 10:15:35 PST
Comment on attachment 24868 [details]
Define the Color::transparent in the cpp file to fix link errors

This change isn't right.

We do need to define these in the .cpp file; it was an error to have definitions in the header only.

But the values should stay in the .h file. This is how C++ static const data members work. I'll land a corrected version.
Comment 5 Darin Adler 2008-11-09 10:22:12 PST
(In reply to comment #1)
> Define the Color::transparent in the cpp file to fix link errors

Landed in http://trac.webkit.org/changeset/38242
Comment 6 Holger Freyther 2008-11-15 16:17:06 PST
Comment on attachment 24868 [details]
Define the Color::transparent in the cpp file to fix link errors

Clearing review as Darin landed a patch which is using static const as of the various C++ FAQs.
Comment 7 Holger Freyther 2008-11-15 16:18:33 PST
The Share FrameLoaderClient::tra... patch has two build failures on Qt. One in qwebpage.cpp (calling a removed method) one in FrameLoaderClientQt.cpp (not declaring a variable name). I would fix it before landing the patch, the spirit/substance is still the same.
Comment 8 Simon Hausmann 2008-11-17 07:27:56 PST
Comment on attachment 24869 [details]
Move recursively setting a background color and toggling transparency to the FrameView

> +++ b/WebCore/page/FrameView.h
> @@ -101,6 +101,7 @@ public:
>  
>      Color baseBackgroundColor() const;
>      void setBaseBackgroundColor(Color);
> +    void updateBackgroundRecursively(const Color&, bool);

My feeling is that the second argument should have a name. But that's a minor detail. Otherwise the patch looks good.
Comment 9 Simon Hausmann 2008-11-17 07:53:42 PST
Comment on attachment 24870 [details]
Share FrameLoaderClient::transitionToCommittedForNewPage between Win/Gtk+/Qt

Nice cleanup!

> +    FrameView* frameView;
> +    if (isMainFrame) {
> +        frameView = new FrameView(frame, size);
> +    } else

Minor coding style issue with the braces there.
r+ and please fix the braces when landing :)
Comment 10 Alexey Proskuryakov 2008-11-18 04:59:44 PST
I think it's a little sad that we now have code (even if a static function) in an interface class, which FrameLoaderClient is.
Comment 11 Holger Freyther 2008-11-18 08:06:29 PST
Comment on attachment 24869 [details]
Move recursively setting a background color and toggling transparency to the FrameView

Clear review, it got landed.
Comment 12 Holger Freyther 2008-11-18 08:06:47 PST
Comment on attachment 24870 [details]
Share FrameLoaderClient::transitionToCommittedForNewPage between Win/Gtk+/Qt

Clear review.
Comment 13 Holger Freyther 2008-11-18 08:08:46 PST
Created attachment 25236 [details]
Kill FrameLoaderClient.cpp and move the code to Frame

As suggested by Maciej, move the code to Frame. I picked the name createView.
Comment 14 Holger Freyther 2009-01-30 17:57:54 PST
Landed in r40435.