Bug 22056 – Share FrameLoaderClient::transitionToCommittedForNewPage between Win/Gtk+/Qt

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Holger Freyther]

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.

[Holger Freyther]

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.

[Holger Freyther]

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.

[Holger Freyther]

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.

[Darin Adler]

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.

[Darin Adler]

(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

[Holger Freyther]

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.

[Holger Freyther]

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.

[Simon Hausmann]

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.

[Simon Hausmann]

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 :)

[Alexey Proskuryakov]

I think it's a little sad that we now have code (even if a static function) in an interface class, which FrameLoaderClient is.

[Holger Freyther]

Comment on attachment 24869 [details]
Move recursively setting a background color and toggling transparency to the FrameView

Clear review, it got landed.

[Holger Freyther]

Comment on attachment 24870 [details]
Share FrameLoaderClient::transitionToCommittedForNewPage between Win/Gtk+/Qt

Clear review.

[Holger Freyther]

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.

[Holger Freyther]

Landed in r40435.