Summary: | Patch that make WebCore have a RenderTheme per page | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Rohde Christiansen <kenneth> | ||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Enhancement | CC: | aroben, hausmann, hyatt | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||
Attachments: |
|
Description
Kenneth Rohde Christiansen
2009-06-09 13:15:15 PDT
Created attachment 31104 [details]
Patch which creates one RenderTheme per page
Please ignore that .../template/en/default/global/message.html.tmpl part from the patch. 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 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.
Thanks for the comments, I have made the adaptation and will add a patch in a few minutes. Created attachment 31200 [details]
Now using RefPtr.
Created attachment 31201 [details]
Using RefPtr (fixed ChangeLog)
Fixed the ChangeLog
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 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; 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 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.
(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. Created attachment 31352 [details]
Follow up patch for Qt
Comment on attachment 31352 [details]
Follow up patch for Qt
This patch has the same reference count problems as the previous one.
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. Created attachment 31357 [details]
RenderTheme patch incorporating all comments
Created attachment 31358 [details]
Qt specific RenderTheme patch, accepted by Simon and with the RefPtr fix.
Created attachment 31371 [details]
RenderTheme patch incorporating all comments (fix problem due to git rebase)
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 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?
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 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 Right, I can indeed get it from the page here! Patch coming up. Created attachment 31392 [details]
Fix remaining comment.
Comment on attachment 31392 [details]
Fix remaining comment.
r=me
|