Bug 26278 – Patch that make WebCore have a RenderTheme per page

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kenneth Rohde Christiansen]

In order to fix a bug with Qt and its style support, which can be changed per widget, we need to have access to the page from the RenderTheme.

I discussed this issue with Dave Hyatt on IRC and he gave me some suggestions, and this patch implements these.

The patch has been tested with Gtk+ and Qt on Linux, but I have made changes to all backends.

[Kenneth Rohde Christiansen]

Created attachment 31104 [details]
Patch which creates one RenderTheme per page

[Kenneth Rohde Christiansen]

Please ignore that .../template/en/default/global/message.html.tmpl part from the patch.

[Kenneth Rohde Christiansen]

Created attachment 31105 [details]
Fixed the patch to not include a wrong file

Gustavo Noronha has confirmed that the GTK+ port builds and works withuot problems.

[Dave Hyatt]

Comment on attachment 31105 [details]
Fixed the patch to not include a wrong file

One improveement would be to avoid making new RenderThemes on platforms that don't need per-page themes...

(1) Make RenderTheme refcounted.
(2) Make Page's theme() call a function like 

PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page*).

(3) On platforms that want to have per-page theme support, this function could construct a new theme and hand it back.

On platforms that don't need per-page theme support, they could just hand back the static singleton as before (which should have an initial ref() also or maybe be held in static RefPtr).

I'm not sure every RenderTheme subclass has made its data members and such static, so I'm concerned about moving away from singletons on platforms that don't need per-page support... hence the suggestion of a way to keep the singleton model.

[Kenneth Rohde Christiansen]

Thanks for the comments, I have made the adaptation and will add a patch in a few minutes. 

[Kenneth Rohde Christiansen]

Created attachment 31200 [details]
Now using RefPtr.

[Kenneth Rohde Christiansen]

Created attachment 31201 [details]
Using RefPtr (fixed ChangeLog)

Fixed the ChangeLog

[Dave Hyatt]

Comment on attachment 31201 [details]
Using RefPtr (fixed ChangeLog)

I don't think you ever have to null-check page() in the RenderObject::theme() method... you can just make that method return document()->page()->theme() and not worry about the else case.

You should inline defaultTheme() in the header.

I think it would read better if defaultTheme() was a static member function of RenderTheme also at this point.  You can just put it next to themeForPage().

I'm going to go ahead and give this r=me though.  If you fix those things, great.  If not, can be fixed in followup patch also.

[Adam Roben (:aroben)]

Comment on attachment 31201 [details]
Using RefPtr (fixed ChangeLog)

> +class RenderTheme : public RefCounted<RenderTheme> {
>  public:
>      RenderTheme();
>      virtual ~RenderTheme() { }

Normally classes that derive from RefCounted have private constructors and a public create() function that returns a PassRefPtr. This helps avoid leaks/crashes due to incorrect reference count management. I think you should follow this pattern with RenderTheme.

> +PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page* page)
>  {
> -    static RenderThemeChromiumWin winTheme;
> -    return &winTheme;
> +    static RefPtr<RenderTheme> rt = new RenderThemeChromiumWin();
> +    return rt; // keep the reference of one.
>  }

This code causes a destructor to be run when the process exits, which will slow down quit time. A better idiom is to write this like so:

static RenderTheme* rt = RenderThemeChromiumWin::create().releaseRef();
return rt;

[Kenneth Rohde Christiansen]

As classes are inheriting from RenderTheme, I cannot make the ctor and dtor private in RenderTheme.h, but I can do it in the implementations such as RenderThemeQt.h.

Another question:

Is this right? or do I need to do a releaseRef or so here.

+    static inline PassRefPtr<RenderTheme> defaultTheme()
+    {
+        return RenderTheme::themeForPage(0);
+    };

Also is this change right?

 PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page* page)
 {
     if (page)
-        return adoptRef(new RenderThemeQt(page));
+        return RenderThemeQt::create(page);
 
-    static RefPtr<RenderTheme> fallback = new RenderThemeQt(0);
-    return fallback; // keep the reference of one.
+    static RefPtr<RenderTheme> fallback = RenderThemeQt::create(0).releaseRef();
+    return fallback;
 }

Could you explain me why the code causes a destructor to be run when the process exits? I would really like to understand how to use the RefPtr's correctly.

Thanks for your comments!

[Eric Seidel (no email)]

Comment on attachment 31201 [details]
Using RefPtr (fixed ChangeLog)

Marking this r- since Kenneth is not a committer and can't fix Hyatt's comments when landing.  Kenneth: this is a great patch, but you'll have to post another copy so that someone else can land the fixed version for you.

[Adam Roben (:aroben)]

(In reply to comment #10)
> As classes are inheriting from RenderTheme, I cannot make the ctor and dtor
> private in RenderTheme.h, but I can do it in the implementations such as
> RenderThemeQt.h.

In that case, you should make them protected in RenderTheme and private in the derived classes (like RenderThemeQt).

The point is to avoid errors like this one (in your new code);

 114     static RefPtr<RenderTheme> fallback = new RenderThemeQt();
 115     return fallback; // keep the reference of one.

"fallback" will actually have a reference count of 2 here: it is created with a reference count of 1, then assigned into a RefPtr which increments the refcount to 2. Obviously this is OK in this case, since we want to keep this RenderTheme around forever, but this illustrates the reason why we use the create() functions and keep the constructors private/protected.

> Another question:
> 
> Is this right? or do I need to do a releaseRef or so here.
> 
> +    static inline PassRefPtr<RenderTheme> defaultTheme()
> +    {
> +        return RenderTheme::themeForPage(0);
> +    };

This is correct as written.

> Also is this change right?
> 
>  PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page* page)
>  {
>      if (page)
> -        return adoptRef(new RenderThemeQt(page));
> +        return RenderThemeQt::create(page);

This is a good change.

> -    static RefPtr<RenderTheme> fallback = new RenderThemeQt(0);
> -    return fallback; // keep the reference of one.
> +    static RefPtr<RenderTheme> fallback =
> RenderThemeQt::create(0).releaseRef();
> +    return fallback;
>  }

This is not correct.

> Could you explain me why the code causes a destructor to be run when the
> process exits? I would really like to understand how to use the RefPtr's
> correctly.

Static variables and global variables on the stack have their destructors run when the process exits. This is part of the way the C++ language works. We avoid having the destructor run by not keeping a RefPtr on the stack. That's why the code I wrote in comment 9 stores the RenderTheme as a bare pointer.

[Kenneth Rohde Christiansen]

Created attachment 31352 [details]
Follow up patch for Qt

[Adam Roben (:aroben)]

Comment on attachment 31352 [details]
Follow up patch for Qt

This patch has the same reference count problems as the previous one.

[Kenneth Rohde Christiansen]

Yes, that is known. I'm fixing all of this. Simon just wanted to let you guys know that the rest is OK, so that I can get someone else to check this in today, when I'm finished fixing it.

[Kenneth Rohde Christiansen]

Created attachment 31357 [details]
RenderTheme patch incorporating all comments

[Kenneth Rohde Christiansen]

Created attachment 31358 [details]
Qt specific RenderTheme patch, accepted by Simon and with the RefPtr fix.

[Kenneth Rohde Christiansen]

Created attachment 31371 [details]
RenderTheme patch incorporating all comments (fix problem due to git rebase)

[Adam Roben (:aroben)]

Comment on attachment 31357 [details]
RenderTheme patch incorporating all comments

> From 204628441494884d406f8d75ab90ef9589bfc1ae Mon Sep 17 00:00:00 2001
> From: Kenneth Rohde Christiansen <[e-mail]>
> Date: Tue, 16 Jun 2009 15:20:27 -0300
> Subject: [PATCH] 2009-06-16  Kenneth Rohde Christiansen  <[e-mail]>
> 
>         Reviewed by Dave Hyatt and Adam Roben.

We normally leave this line as "Reviewed by NOBODY (OOPS!)." since a different reviewer might review a subsequent iteration of the patch.

>  class RenderThemeGtk : public RenderTheme {
> -public:
> +private:
>      RenderThemeGtk();
>      virtual ~RenderThemeGtk();
>  
> +public:
> +    static PassRefPtr<RenderTheme> create();
> +

Our style is to list public members before private members. This can be fixed by a committer before landing.

> +    // When the theme is needed in non-page dependent code, the defaultTheme() is used as fallback.
> +    static inline PassRefPtr<RenderTheme> defaultTheme()
> +    {
> +        return RenderTheme::themeForPage(0);
> +    };

No need for "RenderTheme::" here.

I'll fix these issues before committing.

r=me

[Adam Roben (:aroben)]

Comment on attachment 31358 [details]
Qt specific RenderTheme patch, accepted by Simon and with the RefPtr fix.

It turns out that this breaks the Windows build, due to calls to theme() that were not corrected in this patch. It probably breaks other builds, too. Can you grep over the WebCore/WebKit directories to find all calls to theme() and update them?

[Kenneth Rohde Christiansen]

Created attachment 31391 [details]
Fixed as many theme() as I could find

Also moved the private sections and removed the RenderTheme:: from the RenderTheme.h as Adam suggested.

[Adam Roben (:aroben)]

Comment on attachment 31391 [details]
Fixed as many theme() as I could find

> +++ b/WebKit/win/WebView.cpp
> @@ -1854,7 +1854,7 @@ static LRESULT CALLBACK WebViewWndProc(HWND hWnd, UINT message, WPARAM wParam, L
>          case WM_XP_THEMECHANGED:
>              if (Frame* coreFrame = core(mainFrameImpl)) {
>                  webView->deleteBackingStore();
> -                theme()->themeChanged();
> +                RenderTheme::defaultTheme()->themeChanged();

Should this use coreFrame->page()->theme() instead?

Thanks for fixing this up!

r=me

[Kenneth Rohde Christiansen]

Right, I can indeed get it from the page here! Patch coming up.

[Kenneth Rohde Christiansen]

Created attachment 31392 [details]
Fix remaining comment.

[Adam Roben (:aroben)]

Comment on attachment 31392 [details]
Fix remaining comment.

r=me

[Adam Roben (:aroben)]

Landed as r44758 and r44759