Bug 22056 - Share FrameLoaderClient::transitionToCommittedForNewPage between Win/Gtk+/Qt
Summary: Share FrameLoaderClient::transitionToCommittedForNewPage between Win/Gtk+/Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, Qt
Depends on:
Blocks:
 
Reported: 2008-11-03 14:47 PST by Holger Freyther
Modified: 2009-01-30 17:57 PST (History)
0 users

See Also:


Attachments
Define the Color::transparent in the cpp file to fix link errors (1.96 KB, patch)
2008-11-03 14:50 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
Move recursively setting a background color and toggling transparency to the FrameView (2.72 KB, patch)
2008-11-03 14:51 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
Share FrameLoaderClient::transitionToCommittedForNewPage between Win/Gtk+/Qt (14.39 KB, patch)
2008-11-03 14:54 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
Kill FrameLoaderClient.cpp and move the code to Frame (13.30 KB, patch)
2008-11-18 08:08 PST, Holger Freyther
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.