Bug 26278 - Patch that make WebCore have a RenderTheme per page
Summary: Patch that make WebCore have a RenderTheme per page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-09 13:15 PDT by Kenneth Rohde Christiansen
Modified: 2009-06-17 07:53 PDT (History)
3 users (show)

See Also:


Attachments
Patch which creates one RenderTheme per page (31.81 KB, patch)
2009-06-09 13:16 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Fixed the patch to not include a wrong file (30.25 KB, patch)
2009-06-09 13:28 PDT, Kenneth Rohde Christiansen
hyatt: review-
Details | Formatted Diff | Diff
Now using RefPtr. (34.69 KB, patch)
2009-06-12 10:23 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Using RefPtr (fixed ChangeLog) (31.12 KB, patch)
2009-06-12 10:26 PDT, Kenneth Rohde Christiansen
eric: review-
Details | Formatted Diff | Diff
Follow up patch for Qt (14.55 KB, patch)
2009-06-16 09:37 PDT, Kenneth Rohde Christiansen
aroben: review-
Details | Formatted Diff | Diff
RenderTheme patch incorporating all comments (42.31 KB, patch)
2009-06-16 11:30 PDT, Kenneth Rohde Christiansen
aroben: review+
Details | Formatted Diff | Diff
Qt specific RenderTheme patch, accepted by Simon and with the RefPtr fix. (14.20 KB, patch)
2009-06-16 11:31 PDT, Kenneth Rohde Christiansen
aroben: review+
Details | Formatted Diff | Diff
RenderTheme patch incorporating all comments (fix problem due to git rebase) (42.03 KB, patch)
2009-06-16 14:18 PDT, Kenneth Rohde Christiansen
aroben: review-
Details | Formatted Diff | Diff
Fixed as many theme() as I could find (49.13 KB, patch)
2009-06-16 16:09 PDT, Kenneth Rohde Christiansen
aroben: review+
Details | Formatted Diff | Diff
Fix remaining comment. (49.13 KB, patch)
2009-06-16 16:21 PDT, Kenneth Rohde Christiansen
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2009-06-09 13:15:15 PDT
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.
Comment 1 Kenneth Rohde Christiansen 2009-06-09 13:16:07 PDT
Created attachment 31104 [details]
Patch which creates one RenderTheme per page
Comment 2 Kenneth Rohde Christiansen 2009-06-09 13:19:24 PDT
Please ignore that .../template/en/default/global/message.html.tmpl part from the patch.
Comment 3 Kenneth Rohde Christiansen 2009-06-09 13:28:43 PDT
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.
Comment 4 Dave Hyatt 2009-06-09 21:14:19 PDT
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.
Comment 5 Kenneth Rohde Christiansen 2009-06-12 10:22:25 PDT
Thanks for the comments, I have made the adaptation and will add a patch in a few minutes. 
Comment 6 Kenneth Rohde Christiansen 2009-06-12 10:23:43 PDT
Created attachment 31200 [details]
Now using RefPtr.
Comment 7 Kenneth Rohde Christiansen 2009-06-12 10:26:55 PDT
Created attachment 31201 [details]
Using RefPtr (fixed ChangeLog)

Fixed the ChangeLog
Comment 8 Dave Hyatt 2009-06-15 09:29:40 PDT
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.
Comment 9 Adam Roben (:aroben) 2009-06-15 09:34:24 PDT
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;
Comment 10 Kenneth Rohde Christiansen 2009-06-15 16:00:53 PDT
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!
Comment 11 Eric Seidel (no email) 2009-06-15 19:25:23 PDT
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.
Comment 12 Adam Roben (:aroben) 2009-06-16 08:42:51 PDT
(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.
Comment 13 Kenneth Rohde Christiansen 2009-06-16 09:37:00 PDT
Created attachment 31352 [details]
Follow up patch for Qt
Comment 14 Adam Roben (:aroben) 2009-06-16 09:44:53 PDT
Comment on attachment 31352 [details]
Follow up patch for Qt

This patch has the same reference count problems as the previous one.
Comment 15 Kenneth Rohde Christiansen 2009-06-16 10:57:40 PDT
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.
Comment 16 Kenneth Rohde Christiansen 2009-06-16 11:30:29 PDT
Created attachment 31357 [details]
RenderTheme patch incorporating all comments
Comment 17 Kenneth Rohde Christiansen 2009-06-16 11:31:14 PDT
Created attachment 31358 [details]
Qt specific RenderTheme patch, accepted by Simon and with the RefPtr fix.
Comment 18 Kenneth Rohde Christiansen 2009-06-16 14:18:08 PDT
Created attachment 31371 [details]
RenderTheme patch incorporating all comments (fix problem due to git rebase)
Comment 19 Adam Roben (:aroben) 2009-06-16 14:27:15 PDT
Comment on attachment 31357 [details]
RenderTheme patch incorporating all comments

> From 204628441494884d406f8d75ab90ef9589bfc1ae Mon Sep 17 00:00:00 2001
> From: Kenneth Rohde Christiansen <kenneth@spisnigel.osmtc.indt.org.br>
> Date: Tue, 16 Jun 2009 15:20:27 -0300
> Subject: [PATCH] 2009-06-16  Kenneth Rohde Christiansen  <kenneth.christiansen@openbossa.org>
> 
>         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
Comment 20 Adam Roben (:aroben) 2009-06-16 15:30:16 PDT
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?
Comment 21 Kenneth Rohde Christiansen 2009-06-16 16:09:15 PDT
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.
Comment 22 Adam Roben (:aroben) 2009-06-16 16:13:57 PDT
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
Comment 23 Kenneth Rohde Christiansen 2009-06-16 16:20:07 PDT
Right, I can indeed get it from the page here! Patch coming up.
Comment 24 Kenneth Rohde Christiansen 2009-06-16 16:21:03 PDT
Created attachment 31392 [details]
Fix remaining comment.
Comment 25 Adam Roben (:aroben) 2009-06-16 16:35:52 PDT
Comment on attachment 31392 [details]
Fix remaining comment.

r=me
Comment 26 Adam Roben (:aroben) 2009-06-17 07:53:01 PDT
Landed as r44758 and r44759